serilog / serilog-settings-configuration

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

`Serilog:Using` Check Seems Unnecessarily Restrictive #389

Closed chrisoverzero closed 1 year ago

chrisoverzero commented 1 year ago

For an AOT-published application on .NET 8, I have the following configuration (in the Microsoft.Extensions.Configuration sense):

{
  "Serilog": {
    "MinimumLevel": "Debug"
  }
}

…and the rest of the Serilog configuration is done in C# code. I would like be able to change this log level at runtime. However, Serilog.Settings.Configuration fails at runtime when the only assembly to search is "Serilog". Because of how that is done, the following expected workarounds do not solve the problem:

{
  "Serilog": {
    "Using": [],
    "MinimumLevel": "Debug"
  }
}
{
  "Serilog": {
    "Using": null,
    "MinimumLevel": "Debug"
  }
}
{
  "Serilog": {
    "Using": ["Serilog"],
    "MinimumLevel": "Debug"
  }
}

All of which fail at runtime with the same error message: "No Serilog:Using configuration section is defined and no Serilog assemblies were found. This is most likely because the application is published as single-file." and so on.

In order to continue testing AOT publication, I have to configure like this:

{
  "Serilog": {
    "Using": ["<name of entry assembly>"],
    "MinimumLevel": "Debug"
  }
}

…but this is undesirable since we'd have to put the name of the entry assembly in the configuration for each application. Is there or can there be a way to give Serilog.Settings.Configuration a positive signal that the single assembly it found is OK?

0xced commented 1 year ago

The rest of the error message says this (I spent literally days implementing this feature 😉):

Either add a Serilog:Using section or explicitly specify assemblies that contains sinks and other types through the reader options. For example: var options = new ConfigurationReaderOptions(typeof(ConsoleLoggerConfigurationExtensions).Assembly, typeof(SerilogExpression).Assembly); new LoggerConfiguration().ReadFrom.Configuration(configuration, options);

Have you tried explicitly listing Serilog assemblies that contain sinks and other types as hinted in the error message? Certainly you are referencing at least one sink?

chrisoverzero commented 1 year ago

I am referencing sinks and whatnot, but in C# code. Those assemblies don't need to be loaded by the search Serilog.Settings.Configuration does:

_ = builder.Services.AddSerilog(c => c
    .WriteTo.Console(new RenderedCompactJsonFormatter())
    .Enrich.FromLogContext()
    .Enrich.WithExceptionDetails()
    .ReadFrom.Configuration(builder.Configuration));

I'm not sure why I would list assemblies there except as a workaround to this, so I didn't quote the rest of the error message.

0xced commented 1 year ago

OK I think I get it. You are using Serilog.Settings.Configuration but only for log levels and everything else is configured directly with C# code. This is a scenario that was not envisioned when working on #353. So indeed listing at least one assembly in the reader options is probably your best way forward.

chrisoverzero commented 1 year ago

So indeed listing at least one assembly in the reader options is probably your best way forward.

Do you mean for now or forever? Forever feels unsatisfying to me because I do have a perfectly valid configuration, but Serilog doesn’t believe me.

I’m certainly not dismissing the work put into this feature, but the results are surprising to me. I’m hoping we can work out something that feels like less of a workaround for the future. To me, a positive signal in the configuration saying, “It’s fine, Serilog,” feels like less of a workaround than selecting one or more assemblies arbitrarily and adding them to the configuration’s configuration.

In that spirit, I personally like using the presence of “Serilog:Using”, not only the presence of children, as that kind of a positive signal. It should work with any value other than (using JSON syntax) []. "Using": true or "Using": {} feels wrong, since that value is usually a list, but it’s simple to type and probably, maybe, kind of clear. It’s too bad about [], though.

0xced commented 1 year ago

It would be reasonable to read the presence of the Serilog:Using section as a hint that the users know what they are doing. I think that was my first idea but then have gone a little bit further by ensuring that the section actually contains at least one loadable assembly. Probably we could reconsider this.

Thoughts about it @nblumhardt, @sungam3r ?

It’s too bad about [], though.

Do you mean it's too bad it doesn't work out of the box to tell ˋSerilog.Settings.Configuration` that it should not throw? Because if I remember correctly it could be easily be implemented (but can't currently experiment from my phone).

chrisoverzero commented 1 year ago

Do you mean it's too bad it doesn't work out of the box […]

Definitely do check me on this, but I think I remember that the way MEC handles arrays means the no key is present in configuration for keys with values of empty arrays. Because configuration is a flat key:value store, arrays get added as Key:0, Key:1, and so on. If I misremember, so much the better!

0xced commented 1 year ago

I think you're right but we can probably detect if a Using section exists (even if it's empty) by doing this (section referring to the Serilog configuration section):

var hasUsingSection =
section.GetChildren().Any(e => e.Key == "Using"));
nblumhardt commented 1 year ago

Another option might be to only trigger the check if sections other than MinimumLevel exist?

sungam3r commented 1 year ago

@0xced I modified check according to your suggestions. Indeed, Exists API does not work properly. I tested it manually. I added test but I think it's useless since should be run into single-file app environment. I've never used such deployment and I'm not familiar with tests you've added some time ago. Feel free to commit on my branch.

0xced commented 1 year ago

Actually I prefer Nicholas' proposal to check if sections other than MinimumLevel exist and throw only if that's the case. It also solves this issue but without forcing users to add a Using section in the configuration.

I'll have a look at the tests when I'm back from vacation in a few days.

0xced commented 1 year ago

I submitted pull request #391 that fixes this issue by implementing @nblumhardt solution. I also identified two sections (in addition to MinimumLevel) that don't require any additional assemblies: LevelSwitches and Properties.

The integration tests have been updated to ensure that a configuration only using the MinimumLevel section doesn't throw when published as a single exe.

0xced commented 1 year ago

Serilog.Settings.Configuration 7.0.1, which fixes this issue, is now released on NuGet, enjoy! 🚀