Opinion: `UseSerilog` should not set the static `Logger.Log` by default (incompatibility with testing, mainly)

[daiplusplus]

I noticed that when SerilogWebHostBuilderExtensions.UseSerilog( this IWebHostBuilder builder, Action<WebHostBuilderContext, LoggerConfiguration> configureLogger ) is used then by default preserveStaticLogger == false - which causes this behaviour when called:

1. It sets global::Serilog.Log.Logger (as expected) from configureLogger( new LoggerConfiguration ).CreateLogger().
2. Then it registers a SerilogFactory which always uses global::Serilog.Log.Logger to log, instead of caching the logger from the CreateLogger() call (this was unexpected to me and caused problems down the line).

This is incompatible with WebApplicationFactory<TStartup>-based tests in ASP.NET Core testing projects - because xUnit runs tests in parallel by default, which means it creates multiple WebApplicationFactory instances, which will then run through their own UseSerilog call-sites - which then means different WebApplicationFactory instances (each with their own separate Startup and DI container roots) will be sharing the same Serilog.ILogger instances, which is undesirable.

...and generally, using mutable static state is a bad idea anyway (and I think the Serilog project should move the static Logger class to a separate NuGet package so projects don't unintentionally use it either).

I think the UseSerilog call should be "safe" by default and require users to explicitly opt-in to mutating static state. I propose the two UseSerilog methods be replaced with:

• AddSerilog( IWebHostBuilder builder, Serilog.ILogger logger, Boolean dispose = false )
  • This registers Serilog with DI so it can be injected into pre-Startup types but does not set it as the default ASP.NET Core Microsoft.Extensions.Logging logger.
• UseSerilog( IWebHostBuilder builder, Serilog.ILogger logger, Boolean dispose = false )
  • Calls AddSerilog and then sets it as the MEL logger - the same as today.
• UseSerilog( IWebHostBuilder builder, Action< WebHostBuilderContext, Serilog.LoggerConfiguration > configureLogger )
  • This would be the same as UseSerilog( builder, configureLogger, preserveStaticLogger = true );
• UseSerilog( IWebHostBuilder builder, Action< WebHostBuilderContext, Serilog.LoggerConfiguration > configureLogger, Boolean setStaticLogger = false )
  • The same as above, with optionally setting Serilog.Log.Logger but will cache the generated Serilog.ILogger so the logging system in this host-builder won't be affected by future changes to Serilog.Log.Logger.
  • The parameter is also renamed to setStaticLogger to make it clear what it actually does - because a name like preserveStaticLogger suggests that setting it to true will not do something rather than doing something.
• AlwaysUseStaticSerilogLogger( IWebHostBuilder builder, Action< WebHostBuilderContext, Serilog.LoggerConfiguration > configureLogger )
  • Explicitly named extension-method which has the same behaviour as UseSerilog( builder, configureLogger, preserveStaticLogger: false ) does today, and makes it clear that it always uses Serilog.Log.Logger and does not cache the generated Serilog.ILogger.

[nblumhardt]

Thanks for raising this. The default behavior here was chosen because a lot of code generally expects Log.Logger to be assigned; there's a trade-off both ways, unfortunately. I'm usually vehemently against static state, too, and recognize the problems it can cause.

Serilog was the first .NET logging library in wide use that I'm aware of to use a static-state-free configuration system and logging pipeline, and I tried to push that as far as possible. Log.* and the opt-in LogContext were added later in the piece due to ergonomic issues with the "pure" design. I still believe that, in the context of typical application code, it's nicer not to have to pass loggers throughout the codebase.

Making it harder to accidentally reassign Log.Logger might be another option, here 🤔

All food for thought, thanks.

[daiplusplus]

Is it something you could consider as a breaking-change to introduce in version 3 of Serilog.AspNetCore?

I'm not proposing making any changes to Serilog.Log (for now, at least) - just that the IWebHostBuilder extension methods in the Serilog.AspNetCore package be renamed, basically.

[nblumhardt]

Thanks for the follow-up, sorry about the slow reply. I don't think we'd make this a breaking change in Serilog.AspNetCore at this time. I think some protections around accidental reassignment of Log.Logger are worth some more thought - I'll leave it running as a background thread :-) and close this ticket for now. Thanks again!

[daiplusplus]

@nblumhardt Can you apply a "backlog - reopen in future" label to this issue so it won't be lost in the void?

[nblumhardt]

Thanks for the note. Things have a nice natural tendency to bubble back up when the time is right - we consider all closed tickets as fair game for future reopening 😅

[mariusingjer]

You should put this information in the repo read.me. Its such a painstaking bug to figure out. I had the identical problem as the author of the issue, a large integration test suite with multiple hosts. You end up debugging everything from Microsoft.Extensions.Logging to Serilog, this repo, your own and so forth. I spent a day on this and only found this issue after fixing it myself.

[nblumhardt]

Just revamped the README, not sure exactly where it'd fit but something along the lines of "pass preserveStaticLogger: true to avoid setting Log.Logger." might do the trick. Open to a PR if anyone has the time to figure out a nice uncluttered way to get this info in 👍

[warmfire540]

This was confusing for us too and caused some issues..

We used to use the first overload w/ just logger like so (worked great)

var logger = new LoggerConfiguration()
	.ReadFrom.Configuration(config.Build())                  
	.CreateLogger();

webHostBuilder.UseSerilog(logger);

we decided to use the other overload.. but noticed it wasn't logging anything..

.UseSerilog((webHostBuilderContext, loggerConfiguration) =>
{
	// register Serilog to be fully configurable
	loggerConfiguration
		.ReadFrom.Configuration(webHostBuilderContext.Configuration);
})

dug into code and saw that if we don't pass preserveStaticLogger: true then a null logger is passed to SerilogLoggerFactory and subsequently registered w/ DI

We don't use the static logger - so maybe that's the issue - however preserveStaticLogger didn't seem like something we needed to pass to have a proper logger registered w/ DI... so anyway - now we're passing in preserveStaticLogger so that our DI logger is registered