Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

params arguments should be regarded as optional #441

Open
sreejith-kulamgarath opened this issue Oct 22, 2024 · 7 comments
Open

params arguments should be regarded as optional #441

sreejith-kulamgarath opened this issue Oct 22, 2024 · 7 comments

Comments

@sreejith-kulamgarath
Copy link

sreejith-kulamgarath commented Oct 22, 2024

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
Copy link
Member

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
Copy link
Author

sreejith-kulamgarath commented Oct 22, 2024

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

@sreejith-kulamgarath
Copy link
Author

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

"WriteTo": [
      {
        "Name": "TestCorrelator",
        "Args": {
          "ids": []
        }
      }
    ],

@MitchBodmer
Copy link

@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
Copy link
Member

Thanks @MitchBodmer! 👍

@nblumhardt nblumhardt changed the title TestCorrelator v4 is not working params arguments should be regarded as optional Oct 22, 2024
@nblumhardt nblumhardt reopened this Oct 22, 2024
@nblumhardt
Copy link
Member

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

See: serilog/serilog#2108 (comment) via @ArieGato

@ArieGato
Copy link

ArieGato commented Oct 25, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants