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

See original GitHub issue

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:

It sets global::Serilog.Log.Logger (as expected) from configureLogger( new LoggerConfiguration ).CreateLogger().
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.

Issue Analytics

State:
Created 4 years ago
Reactions:2
Comments:8 (4 by maintainers)

Top GitHub Comments

1reaction

warmfire540commented, Sep 17, 2021

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

1reaction

nblumhardtcommented, Feb 17, 2021

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 👍