Expose IServiceProvider on inline initialization delegate

[ralphhendriks]

This PR adds an overload for UseSerilog that enables access to IServiceProvider in the configuration delegate. This allows users to query the MS.DI container when configuring the logger. See discussion in #169.

An example where this might be useful is for configuring the Application Insights sink. Microsoft have now deprecated the TelemetryConfiguration.Active singleton. Current advice to set up the AI sink is to still use this deprecated singleton. It would be more elegant and more in line with Microsoft's advised use to do something like

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        })
        .UseSerilog((hostingContext, loggerConfiguration, serviceProvider) => loggerConfiguration
            .WriteTo.ApplicationInsights(serviceProvider.GetRequiredService<TelemetryConfiguration>()));

TO DO

• Re-add writeToProviders.
• Address implications of re-adding preserveStaticLogger.
• Discuss whether to have this as a separate overload, or join this with the existing overload for inline initialization (i.e. provide example/doc for passing a dontcare _ for the serviceProvider parameter.
• Add or update demo project.
• Tests? (this area isn't covered by a lot of tests right now)
• Update docs.

[nblumhardt]

👍

[nblumhardt]

Looking great! I think it may actually be better to land the IHostBuilder variant of this in https://github.com/serilog/serilog-extensions-hosting (e.g. alongside https://github.com/serilog/serilog-extensions-hosting/blob/dev/src/Serilog.Extensions.Hosting/SerilogHostBuilderExtensions.cs#L86), since the web host version is essentially "deprecated".

Joining this with the existing overload is tempting, but adding a new one will improve backwards compatibility, so might be the best bet, initially.

Let me know if you need any further info. Thanks for taking this on, Ralph! 👍 👍

[ralphhendriks]

Thank you for your encouragement, Nick.

I must admit that I find it rather verbose and (from a user perspective) difficult to understand that the integration with ASP.NET Core is layered; serilog-aspnetcore is using components from both serilog-extensions-hosting and serilog-extensions-logging. I know this comes from the desire to have a fine-grained support model, but it can be quite confusing nonetheless.

We should also keep in mind that ASP.NET Core 2.1 is on LTS mode and also needs to remain supported.

B.t.w, https://github.com/serilog/serilog-extensions-hosting/blob/dev/README.md explicitly states that

ASP.NET Core applications should consider using Serilog.AspNetCore instead, which bundles this package and includes other ASP.NET Core-specific features.

I agree on adding the additional overload for backward compatibility.

[ralphhendriks]

Ok, I've added back the writeToProviders support. Now looking into adding the preserveStaticLogger functionality.

It would be nice if somebody could provide a bit more context on why this option exists in the first place. The present XML doc is not particularly helpful in this respect

Indicates whether to preserve the value of Log.Logger.

I assume what we mean is that when setting this option true the static instance remains untouched, leading to potentially two instances, the global static instance and the one configured inline scoped to the lifetime of the services collection/the (web)host. On the other hand, setting the option false (default) the static instance gets replaced by the logger configured inline. As soon as the services collection/the (web)host goes out of scope, the static instance gets replaced by a no-op. I guess this option was discussed/introduced for cases where existing code using the global instance had to get wired up into the (web)host composition root.

Implementation-wise, this gets rather tricky, since on the new overload we move the logger creation into the factory delegate for the ILoggerFactory registration. We could push the logic to override the static instance down there, but that would introduce a side effect when the ILoggerFactory gets resolved for the first time.

Any ideas?

[nblumhardt]

Hi @ralphhendriks - RE preserveStaticLogger - it's just as you say; sometimes it's desirable to set up different loggers for different parts of an application.

RE the initialization order issue... 🤔 hmmmmmmm.... I guess it would be normal to expect that ILoggerFactory will be resolved early enough in the app's startup process to push the static logger initialization down there - if there's no other option I think that would be fine.

(This kind of thing is why I always initialized Log.Logger at program startup and not during DI configuration.)

[nblumhardt]

Hi @ralphhendriks! I dug into the upstream Serilog.Extensions.Hosting and managed to expose IServiceProvider to the UseSerilog() extension there:

serilog/serilog-extensions-hosting#20

I am not sure whether the code there would be a good basis for this version, but, I did end up working through the various issues so there could be some inspiration.

Since UseSerilog() on IHostBuilder is now the preferred API, there's also a chance we should consider obsoleting the web host alternatives, to guide people down the successful path. Any thoughts?

[ralphhendriks]

Hi @nblumhardt thanks for the nudge ;)

I looked at serilog/serilog-extensions-hosting#20 and I think that it might be very useful here.

As I wanted to play with the new overload, I looked at the sample project in Serilog.Extensions.Hosting. It then realized that this sample was still targeting netcoreapp2.1. So I upgraded the target to netcoreapp3.1. See also my PR serilog/serilog-extensions-hosting#21.

Overseeing how Microsoft deprecated the WebHostBuilder and moved all logic to the HostBuilder, I think that Serilog should clearly follow that path. I would agree to deprecating all the extensions on IWebHostBuilder in this package and promote the extensions on IHostBuilder from Serilog.Extensions.Hosting. However, we should be wary that .NET Core is LTS and we should probably keep supporting this for a while (without deprecation warnings?).

I'd suggest that we put this PR on ice now, and instead focus on getting the docs and samples in Serilog.Extensions.Hosting improved. If agree I'd like to add specific samples for early/late initialization, as well as adding this to the readme. Please let me know your thoughts.

[nblumhardt]

That sounds great, @ralphhendriks, thanks for all the help 👍

[nblumhardt]

I'll close this for now, we can reopen and revisit in future if our thinking evolves.