serilog / serilog-settings-configuration

A Serilog configuration provider that reads from Microsoft.Extensions.Configuration
Apache License 2.0
456 stars 129 forks source link

Aspnetcore 7 WebApplicationFactory doesn't get default minimum configuration correctly #332

Closed wzchua closed 1 month ago

wzchua commented 2 years ago

Checking for the minimum section returns "" instead of null, so the logic doesn't get the value from Default

nblumhardt commented 2 years ago

Hi! Thanks for the report - any chance of a code sample/failing test that demonstrates the issue?

wzchua commented 2 years ago

Hmm, I made the incorrect diagnosis .

I hit this using https://learn.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-7.0 with net7.0.

What happens is that the WebApplicationFactory forwards a bunch of configuration as arguments for the program.

for an appsetting of

{
    "Serilog": {
      "MinimumLevel": {
          "Default": "Information",
      }
    }
  }

It sets the below as arguments

var configuration = new ConfigurationBuilder()
    .AddCommandLine(new []
    {
        "--Serilog=",
        "--Serilog:MinimumLevel=",
        "--Serilog:MinimumLevel:Default=Information"
    })
    .Build();

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

which the logger configuration cannot handle.

Something changed in how they set the configuration resulted in this new behavior.

wzchua commented 2 years ago

repro https://github.com/wzchua/WebApplicationFactoryBugWithSerilogConfiguration


public class UnitTest1  :
    IClassFixture<CustomWebApplicationFactory<Program>>
{
    private readonly HttpClient _client;
    private readonly CustomWebApplicationFactory<Program>
        _factory;

    public UnitTest1(CustomWebApplicationFactory<Program> factory)
    {
        _factory = factory;
        _client = factory.CreateClient(new WebApplicationFactoryClientOptions
        {
            AllowAutoRedirect = false
        });
    }

    [Fact]
    public async Task Test1()
    {
        var message = await _client.GetAsync("/");
    }
}

public class CustomWebApplicationFactory<TProgram>
    : WebApplicationFactory<TProgram> where TProgram : class
{
    protected override IHost CreateHost(IHostBuilder builder)
    {
        builder.ConfigureHostConfiguration(configurationBuilder =>
        {
            configurationBuilder.AddInMemoryCollection(new Dictionary<string, string?>
            {
                {"Serilog:MinimumLevel:Default", "Debug"},
            });
        });

    builder.UseSerilog((context, configuration) =>
        {
            configuration.ReadFrom.Configuration(context.Configuration);
            configuration.WriteTo.Debug();
        });
        return base.CreateHost(builder);
    }
}
wzchua commented 2 years ago

Okay I can workaround this by using ConfigureAppConfiguration instead of ConfigureHostConfiguration.

I am not sure if this is an issue for this library. You can close this if it's not actionable.

nblumhardt commented 2 years ago

Thanks for all the details, sounds like it needs some investigation :+1:

0xced commented 1 year ago

I think using ConfigureAppConfiguration is the right thing to do:

Sets up the configuration for the remainder of the build process and application. This can be called multiple times and the results will be additive. The results will be available at Configuration for subsequent operations, as well as in Services.

Using ConfigureHostConfiguration is not appropriate to configure Serilog:

Set up the configuration for the builder itself. This will be used to initialize the IHostEnvironment for use later in the build process. This can be called multiple times and the results will be additive.

peabnuts123 commented 1 year ago

I'm pretty sure there is a bug here though I am not entirely sure where (e.g. Serilog, Microsoft.AspNetCore.Mvc.Testing, etc.)

Basically when using WebApplicationFactory (i.e. Integration Tests) the following Serilog config will not work:

// appsettings.json
…
  "Serilog": {
    "MinimumLevel": {
      "Default": "Information"
    }
  }
…

This throws an exception like the following:

Error Message:
   System.InvalidOperationException : The value  is not a valid Serilog level.
  Stack Trace:
     at Serilog.Settings.Configuration.ConfigurationReader.ParseLogEventLevel(String value)
…

Note the double space in there: The value␣␣is not a valid Serilog level for (presumably) the empty string to be inserted into (i.e. $"The value {level} is not a valid Serilog level").

But the following works fine:

// appsettings.json
…
  "Serilog": {
    "MinimumLevel": "Information"
  }
…

I am using code like the following to set this up but I don't think it matters:

public class ComponentTestsFixture : WebApplicationFactory<Program>
{
    protected override void ConfigureWebHost(IWebHostBuilder builder)
    {
        var config = new ConfigurationBuilder()
                .SetBasePath(Directory.GetCurrentDirectory())
                .AddJsonFile("appsettings.json", optional: false);
        builder.UseConfiguration(config.Build());
    }
}

I've also tried using ConfigureAppConfiguration() but this doesn't appear to be called until after my Program.cs has run which does all the setup for Serilog and stuff so it isn't really useful.

peabnuts123 commented 1 year ago

@nblumhardt

nblumhardt commented 1 year ago

@peabnuts123 a fix would be welcome if you can track this down, I don't have anything to add.

wzchua commented 8 months ago

The min repro is

var configuration = new ConfigurationBuilder()
    .AddCommandLine(new []
    {
        "--Serilog=",
        "--Serilog:MinimumLevel=",
        "--Serilog:MinimumLevel:Default=Information"
    })
    .Build();

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

the fix likely would involve ignoring the key Serilog:MinimumLevel when Serilog:MinimumLevel:Default exists

Pastafarian commented 6 months ago

Just run into this myself, be great if we could get a release with the fix in it!

Cheers