serilog-contrib / serilog-sinks-splunk

A Serilog sink that writes to Splunk
https://splunk.com
Apache License 2.0
46 stars 47 forks source link

Allow Splunk Sink to reuse HttpClient #102

Closed Christoba closed 2 years ago

Christoba commented 5 years ago

The sink currently allows reuse of the HttpMessageHandler (https://github.com/serilog/serilog-sinks-splunk/blob/dev/src/Serilog.Sinks.Splunk/Sinks/Splunk/EventCollectorSink.cs#L92)

Would it be possible to also allow reuse of the full HttpClient? The EventCollectorClient is a thin implementation of HttpClient and either the sink or the user would be on the hook to handle the authentication header.

The request stems from the usage of the Sink within the execution context of an Azure Function, where best practice includes reuse of the HttpClient.

Currently, I'm seeing occasional SocketExceptions when using the Splunk Sink within an Azure Function that I believe are stemming from the construction of a new HttpClient by the Sink during every Function invocation.

merbla commented 5 years ago

Hi @Christoba,

Thanks for the note. Just wondering what is the lifetime of your overall Serilog logger? The sink (and the underlying HTTP Client) should be related to that life cycle. I am guessing the logger is initialized every Azure Function lifecycle?

In terms of the handler there is an overload already for HttpMessageHandler here.

HTH, however not sure this is what you are after.

Christoba commented 5 years ago

You're getting at the next workaround I was planning to try, and that is reusing the entire logger. The logger is currently constructed on every function invocation, which will end up creating new HttpClient instances. I'll note that its less desirable to persist the logger, as the Splunk-specific configuration is appsettings-driven, so reusing the logger makes it more difficult to make changes to the settings (one would then need to cycle the app service).

I am already using the HttpMessageHandler overload, but reusing the handler has not prevented occurrences of the socket exceptions.

Christoba commented 5 years ago

A couple of things that are downsides with reusing the whole Serilog ILogger to work around this issue

var logger = new LoggerConfiguration()
                .MinimumLevel.Information()
                .WriteTo.ILogger(log)
                .WriteTo.EventCollector(
                    splunkHost: splunkHost,
                    eventCollectorToken: splunkToken,
                    **host: context.InvocationId.ToString()**,
                    source: source,
                    sourceType: SourceType,
                    index: splunkIndex,
                    messageHandler: client)
                .CreateLogger();

EDIT: I think I can work around these issues using Serilog's

https://github.com/serilog/serilog/wiki/Enrichment#the-logcontext https://github.com/serilog/serilog/wiki/Configuration-Basics#sub-loggers

merbla commented 5 years ago

@Christoba when you mention..

We're also using the Function's (MS's) ILogger

Are you using MEL? e.g.

Christoba commented 5 years ago

MEL: yes With this sink: https://www.nuget.org/packages/Serilog.Sinks.ILogger/

merbla commented 5 years ago

Thanks @Christoba I have not used that sink before, I will have to dig in and research it.

joacar commented 5 years ago

This would be a feature enhancement for high load scenarios. For release 2.x of ASP.NET Core they introduced HttpClientFactory (docs) to mitigate problems concerned with opening of, and tearing down, socket connections as well as draining the number of connections.

The EventCollectorClient could be removed and its functionality for setting authentication header moved to the inner/outer HttpMessageHandler in the chain.

Christoba commented 5 years ago

@joacar, that's a good link and I'll keep it in mind when working with httpClient. It seems like you're advocating a similar approach in not explicitly managing the httpClient lifecycle within the sink.

@merbla, I've found stability by persisting (singleton) and reusing the EventCollector ILogger as well as enriching and pushing properties for each function invocation.

merbla commented 2 years ago

Closing as a part of larger Serilog contrib reorg

Checkout https://github.com/serilog/serilog/issues/1627