serilog / serilog-settings-configuration

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

`params` arguments should be regarded as optional #441

Open sreejith-kulamgarath opened 3 days ago

sreejith-kulamgarath commented 3 days ago

The following code was working with the previous version of TestCorrelator (3.2.0), but it is failing with v4.

Here is the code.

var configuration = new ConfigurationBuilder()
            .SetBasePath(Directory.GetCurrentDirectory())
            .AddJsonFile(path: "appsettings.Test.json", optional: false, reloadOnChange: true)
            .Build();

const string message = "An event.";
using var context = TestCorrelator.CreateContext();

Log = new LoggerConfiguration().ReadFrom.Configuration(configuration).CreateLogger();

// Act
Log.Information(message);

// Assert
var logEvent = TestCorrelator.GetLogEventsFromCurrentContext().Single();
Assert.Equal(message, logEvent.MessageTemplate.Text);

If I replace ReadFrom.Configuration with this line, the code above works.

Log = new LoggerConfiguration().WriteTo.TestCorrelator().CreateLogger();

Here is my appsettings.Test.json

{
  "Serilog": {
    "Using": [
      "Serilog.Sinks.TestCorrelator"
    ],
    "MinimumLevel": "Debug",
    "WriteTo": [
      {
        "Name": "TestCorrelator",
      }
    ],
    "Enrich": [
      "FromLogContext"
    ],
    "Properties": {
      "Application": "Logging.Tests"
    }
  }
}
nblumhardt commented 3 days ago

Thanks for your message. If the problem is a change is between versions of TestCorrelator, this is best asked over in that project: https://github.com/MitchBodmer/serilog-sinks-testcorrelator. Thanks!

sreejith-kulamgarath commented 3 days ago

I have posted it there now. Let me see if they push it back here :)

sreejith-kulamgarath commented 3 days ago

For the record, changing the appsettings file as below worked.

"WriteTo": [
      {
        "Name": "TestCorrelator",
        "Args": {
          "ids": []
        }
      }
    ],
MitchBodmer commented 3 days ago

@nblumhardt I think this is probably a "bug" in the configuration library, or more specifically a missing feature. It seems (from my quick browsing) that this is happening because the config library doesn't determine that if the parameter is a params array it shouldn't require a value.

I think the fix is to add a check for the ParamArrayAttribute and then provide an empty array of the correct type if one isn't provided in this section.

nblumhardt commented 3 days ago

Thanks @MitchBodmer! 👍

nblumhardt commented 20 hours ago

This is also the likely cause of Serilog 4.1's WriteTo.Fallback not being callable via configuration.

See: https://github.com/serilog/serilog/pull/2108#issuecomment-2428268999 via @ariegato

ArieGato commented 13 hours ago

@nblumhardt I think it has to do with the fact that FallbackChain is not an extension method e.g. static.

I guess that the WriteTo FallbackChain acts as a sort of Sink, but is not configured the same way as normal sinks. The solution could be to add the extension method in the main library. Not sure that this will fix the params issue.