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

Serilog:Using Check Seems Unnecessarily Restrictive #389

Closed
chrisoverzero opened this issue Jul 27, 2023 · 12 comments · Fixed by #391
Closed

Serilog:Using Check Seems Unnecessarily Restrictive #389

chrisoverzero opened this issue Jul 27, 2023 · 12 comments · Fixed by #391

Comments

@chrisoverzero
Copy link

chrisoverzero commented Jul 27, 2023

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

0xced commented Jul 27, 2023

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

chrisoverzero commented Jul 27, 2023

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

0xced commented Jul 27, 2023

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

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

0xced commented Jul 27, 2023

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

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

0xced commented Jul 27, 2023

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

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

@sungam3r
Copy link
Contributor

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

0xced commented Jul 30, 2023

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 added a commit to 0xced/serilog-settings-configuration that referenced this issue Aug 1, 2023
0xced added a commit to 0xced/serilog-settings-configuration that referenced this issue Aug 4, 2023
@0xced
Copy link
Member

0xced commented Aug 4, 2023

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

0xced commented Aug 24, 2023

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

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