Add an overload to UseSerilog() for inline initialization during program startup

[hexawyz]

As discussed here:
serilog/serilog-settings-configuration#66

I propose to add an overload to the UseSerilog extension method:

/// <summary>Sets Serilog as the logging provider.</summary>
/// <param name="builder">The web host builder to configure.</param>
/// <param name="configureSerilog">The delegate for configuring the <see cref="LoggerConfiguration" /> that will be used to construct a <see cref="Logger" />.</param>
/// <returns>The web host builder.</returns>
public static IWebHostBuilder UseSerilog(this IWebHostBuilder builder, Action<WebHostBuilderContext, LoggerConfiguration> configureSerilog)

The goal of this new method is to be able to initialize Serilog during the call to IWebHostBuilder.Build(), after configuration and environment have been built. (see at the end for an example)
For that, the creation of the LoggerConfiguration instance and the call to LoggerConfiguration.CreateLogger() are handled by the method itself, similarly to what is done by IWebHostBuilder.ConfigureAppConfiguration().

Of course, as it is always the case whith choices, this one has its advantages and its drawbacks.

Drawbacks:

• Serilog being initialized inside the IWebHostBuilder.Build() method means that it is initialized a bit later compared to when the ILogger instance is set up earlier.
• It does not allow to log anything before Serilog's ILoggerFactory has been added to the service collection. More specifically, logging can happen inside the call IWebHost.Build() but cannot happen before.
• The built ILogger is not exposed outside of ASP.NET Core DI. That behavior can easily be modified if desired.

Advantages:

• This stays close to the usual pattern of ASP.NET Core, where everything is setup by the chained method calls on WebHostBuilder.
• Serilog can be configured when the IHostingEnvironment is already available: Access to EnvironmentName, ContentRootPath or other properties of the hosting environment is possible.
• Serilog can be configured when the final IConfiguration instance is available: This overload of UseSerilog() integrates more nicely with ConfigureAppConfiguration, although one can still decide to use only UseConfiguration if desired.

At the end of the day, this does not solve the chicken/egg problem of what to initialize first: Configuration, Logging, or DI… It is just an alternative way of setting things up.

A typical usage would be as below:

var host = new WebHostBuilder()
    .UseKestrel()
    .UseContentRoot(Directory.GetCurrentDirectory())
    .ConfigureAppConfiguration
    (
        (hostingContext, configurationBuilder) =>
        {
            configurationBuilder
                .SetBasePath(hostingContext.HostingEnvironment.ContentRootPath)
                .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
                .AddJsonFile($"appsettings.{hostingContext.HostingEnvironment.EnvironmentName}.json", optional: true, reloadOnChange: true)
                .AddEnvironmentVariables();
        }
    )
    .UseSerilog
    (
        (hostingContext, loggerConfiguration) => loggerConfiguration.ReadFrom.Configuration(hostingContext.Configuration)
            .Enrich.FromLogContext()
            .WriteTo.Console()
    )
    .UseIISIntegration()
    .UseStartup<Startup>()
    .Build();

[nblumhardt]

Thanks Fabian! The PR looks great.

I need to give some thought to how the logger could be exposed - I don't think it's a huge deal, but it is a bit hard to offer the full Serilog experience if there's no way for the program to get access to a Serilog logger.

Guessing there has to be a solution, but registering ILogger in DI without full integration seems inelegant. If we did that, I'd want to try configuring the logger with its source "type", without resorting to some kind of generic ILogger<T>. Most containers can do this, will have to see what options there might be via the dependency injection abstractions available.

Also wondering whether we'd allow Log.Logger to be assigned somehow. Having the lambda call CreateLogger(), i.e. typing it as Func<IHostingContext, Logger> or Func<IHostingContext, LoggerConfiguration, Logger> would do it, because the body of the function would have access to the logger instance and could stash it in the static property. Returning ILogger rather than Logger may be more flexible, although it doesn't communicate the ownership transfer as neatly.

Will give it a little more thought... Thanks for kicking this off!

[hexawyz]

You're right, that was totally an oversight on my side. 😞
I knew the ILogger was not directly exposed but did not realise that it was thus totally inaccessible to the rest of the application.

I wonder, wouldn't adding the Serilog.ILogger to the services collection still be a good-enough solution ? (Maybe make that optional by adding a bool parameter to UseSerilog ?)

That way, it could be injected in the Startup class constructor or in any of the calls to Startup class methods, and be accessible when configuring an custom DI provider.

Let's say I'm using Autofac together with https://github.com/nblumhardt/autofac-serilog-integration, I could simply do something like this:

public class Startup
{
    private IConfiguration Configuration { get; }
    private Serilog.ILogger Logger { get; }

    public Startup(IHostingEnvironment environment, IConfiguration configuration, Serilog.ILogger logger)
    {
        Configuration = configuration;
        Logger = logger;
    }

    public void ConfigureContainer(ContainerBuilder builder)
    {
        builder.RegisterLogger(Logger);
    }

    public void Configure(IApplicationBuilder app, IHostingEnvironment env)
    {
        app.Run(async (context) =>
        {
            await context.Response.WriteAsync("Hello World!");
        });
    }
}

As for the case where one wants to use Serilog without a custom DI provider, then maybe a Serilog.ILogger<T> is needed ?
After all, that is what Microsoft.Extensions.Logging is offering.

Besides having some kind of ILogger<T>, another possibility that could work, although less straightforward, would be to add an extension method GetSerilogLogger() on ILoggerFactory:

// I'm assuming that SerilogLoggerFactory exposes a property named Logger of type Serilog.ILogger.
public static Serilog.ILogger GetSerilogLogger(this ILoggerFactory loggerFactory) => loggerFactory is SerilogLoggerFactory serilogLoggerFactory ? serilogLoggerFactory.Logger : null;
public static Serilog.ILogger GetSerilogLogger<T>(this ILoggerFactory loggerFactory) => loggerFactory is SerilogLoggerFactory serilogLoggerFactory ? serilogLoggerFactory.Logger.ForContext<T>() : null;

The advantage is that it does not pollute the service collection with a raw ILogger, but I'm not sure how I feel about this.

[nblumhardt]

Thanks for the follow-up and suggestions. I need some more time to think through this one.

Registering a Serilog-based ILogger<T> of some sort (easy enough to define one in the integration package) would cover the DI case reasonably well, although setting up Log.Logger would also be pretty handy.

Registering the Serilog ILogger does also sound fine, but then needs an opt-out so that it doesn't conflict with something full-featured like the Autofac/Serilog integration.

Trade-offs everywhere! :-) :-)

[ngbrown]

I ended up adding something very similar to this pull request my application, except that I set the result of loggerConfiguration.CreateLogger() to the static Log.Logger as well.

The problem with the builder.UseSerilog() way (the documented way for the initial release of Serilog.AspNetCore) in ASP.Net Core, is that the normal way of calling ConfigureAppConfiguration is bypassed and WebHostBuilderContext isn't available.

[nblumhardt]

Added some thoughts/comments.

[nblumhardt]

Although it's useful having the sample as documentation, I'm wary of adding samples for all of the various configuration options (it'll become a bit unwieldy to maintain). Perhaps we remove this, but look to add a few explanatory lines in the README instead?

[nblumhardt]

This looks a little spaced-out to my eyes. Can we put the method name and params on the same line? E.g.:

            builder.ConfigureServices( (context, collection) =>
            {
                var loggerConfiguration = new LoggerConfiguration();
                configureSerilog(context, loggerConfiguration);
                collection.AddSingleton<ILoggerFactory>(new SerilogLoggerFactory(loggerConfiguration.CreateLogger(), true));
            });

[nblumhardt]

Since the action applies to a logger configuration, can we call the parameter configureLogger instead of configureSerilog?

[nblumhardt]

May need to include a note on how this differs from the overload above, e.g. Sets Serilog as the logging provider. A <see cref="WebHostBuilderContext"/> is supplied so that configuration and hosting information can be used.

[nblumhardt]

Also should possibly add The logger will be shut down when application services are disposed.

[nblumhardt]

Does AddSingleton guarantee disposal of the logger factory?

[ngbrown]

I think the created ILogger should to be saved to the static Log.Logger here, because at some point in the Startup.Configure() function, the CloseAndFlush() function needs to be setup for calling at application shutdown:

var applicationLifetime = app.ApplicationServices.GetRequiredService<IApplicationLifetime>();
applicationLifetime.ApplicationStopping.Register(() => Log.CloseAndFlush());

[nblumhardt]

Can we call those "sensible defaults" and just start out by wiring all of that up automatically here? The overall perspective would then be: if you supply the logger, you configure everything to your liking; if we supply it (via this new overload), "just trust us", everything should be set up reasonably. Does that make sense?

[hexawyz]

According to the documentation, it seems AddSingleton won't dispose the logger factory if we create the object ourselves. 😟
I think that using the overload with the factory method should do the trick, however.

Regarding the call to CloseAndFlush() method, should I try to wire it in the call to ConfigureServices(), then ?

[nblumhardt]

Hi again @GoldenCrystal - I've had some time to think about this and feel like we should move ahead. Is that your opinion also?

To expose the logger, we've got a few options, but one that might work well is to expose a parameter setStaticLogger on UseSerilog(), e.g. ..., bool setStaticLogger = true). On second thoughts, inverting that to bool preserveStaticLogger = false would better fit the "Boolean parameters default to false" convention used throughout Serilog.

If we do this, it's possible for packages like AutofacSerilogIntegration to consume the logger from there, or at worst, the developer can retrieve it via var logger = Log.Logger; and do something with it from there.

The only wrinkle this would introduce is that we'd need to switch from disposing the logger directly to calling Log.CloseAndFlush() for everything to stay in sync (unless the parameter turns off the static logger). I have a feeling SerilogLoggerFactory might already have machinery in place to enable this behavior.

What do you think?

[nblumhardt]

@GoldenCrystal Hi Fabian - just checking in, are you still keen on these changes? No great rush, but if you need help to get it over the line or want to pass on the baton just let me know :-) Cheers!

[mudrz]

This is a very essential change for real-world logging scenarios, thank you

[hexawyz]

@nblumhardt Sorry, I was out of town for a while. I will take a look at this in a few hours :)

[hexawyz]

@nblumhardt I'm wondering about the setStaticLogger parameter:
Wouldn't an enum such as this one be a bit better ?

[Flags]
enum LoggerRegistrationOptions
{
    /// <summary>Do not expose the <see cref="ILogger" /> directly.</summary>
    None = 0,
    /// <summary>Register the <see cref="ILogger" /> as the global <see cref="Log.Logger" /> instance.</summary>
    RegisterStatic = 1,
    /// <summary>Registers the <see cref="ILogger" /> within ASP.NET dependency injection.</summary>
    RegisterSingleton = 2,
    /// <summary>Registers a generic <see cref="ILogger{T}" /> as singleton within ASP.NET dependency injection.</summary>
    RegisterGenericSingleton = 4,
    /// <summary>Registers the <see cref="ILogger" /> as well as <see cref="ILogger{T}" /> as singletons within ASP.NET dependency injection.</summary>
    RegisterSingletons = 6,
}

This seems to me like it would offer the most choices to developpers. (Perhaps even a bit too much ?)

[nblumhardt]

Hi @GoldenCrystal, thanks for the reply; I had second thoughts about the preserveStaticLogger parameter, too, but came to the conclusion we should just be prescriptive for the first iteration, and add more policies once they're requested.

The RegisterStatic option seems to solve the issue of exposing the logger, and also allows the resulting logger to participate in whatever dependency injection scheme the app uses, so perhaps we start there?

I am not sure how to register Log.CloseAndFlush for application shutdown via the APIs we have at setup time, but I'm guessing there's an option somewhere. I can take a look at this if it turns out to be problematic, let me know what you think.

[nblumhardt]

Had a quick look at the singleton disposal problem; all we should need to do is switch to using the AddSingleton() overload that accepts a Func.

If we want to do it via IApplicationLifetime, we will need to rig up a component that receives it as a dependency somehow; perhaps a bit much effort for a first cut, but probably the right thing to do if it can be made to work.

[nblumhardt]

Thanks Fabian! I made one small tweak. Merging now :-)