serilog / serilog-settings-configuration

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

Add support for overrides from environment variables #385

Open sungam3r opened 1 year ago

sungam3r commented 1 year ago

One of my colleagues was faced with the problem when trying to set the value of the level override from environment variables. This PR helps to do that. @skomis-mm @nblumhardt I have a suspicion that there is another (simpler) solution and I do not believe that all this time no one has come across this problem. However, fix seems to be obvious and non-breaking affecting only cases when level overrides come from environment variables. Configuration from json files works well - it's safe to use dots there.

sungam3r commented 1 year ago

If you prefer all new code can be moved under flag like public bool ConfigurationReaderOptions.AllowNestedLevelOverrides { get; init; }.

skomis-mm commented 1 year ago

@sungam3r there is OOTB solution is tracking on the MEC side. Would the official workaround or custom serilog configuration extension be sufficient for now? Since we don't know what MEC fix exactly will look like (three underscores or something else)

sungam3r commented 1 year ago

Thank you very much for links. I was sure that this problem should already be described somewhere. Both workarounds should work, yes. The problem is that both require changes in code and configuration. I mean in format of level overrides - either using ___ as indication of special case or swithing to ApplyCustomOverrides and splitting key/value pair into different properties. For existing infrastructures deployed in kubernetes with hundreds of applications using some shared configuration coming from key vault or something else it will be a real nightmare to change format. Of course the best option for me is fix from MS on their side but as you see изображение and изображение

I would really like to see working and non-intrusive solution for any target platform (personally I need net5/net7), not waiting for NET 8 (or 9). As you can see Serilog can deal with it with rather trivial changes. PR fixes such env usages and effectively is no-op for other cases (json config files generally). The only thing I may be afraid of is backward compatibility for unforseen cases so I suggested to introduce option to provide ability to quickly disable new behaviour if someone need it. New design of ConfigurationReaderOptions is perfect for this.

skomis-mm commented 1 year ago

@sungam3r I have no strong opinion on this except general desire fix non-serilog issues outside serilog if possible. Let's give this some time, may get a few other opinions on this

0xced commented 1 year ago

I don't think it's a good idea to handle this at the Serilog level. This should definitely be handled upstream.

Also it seems there's a misunderstanding in your code/test. If I add your test (TestMinimumLevelOverridesForChildContext_With_Environment_Variables) and change "My_Serilog__MinimumLevel__Override__System__Threading" to "My_Serilog__MinimumLevel__Override__System.Threading" then the test passes without any changes in the code. It seems you have mixed configuration hierarchy which is delimited with : (or __ in environment variables) and log level names which are typically delimited with the . character.

Actually, the problem seems very specific to Azure (not the environment variables). See Werner Mairl's post who reports that the . character is somehow replaced with a _ character.

I was not able to reproduce this in the unit test. Calling builder.Build().GetDebugView() yields the following (correct) debug view:

Serilog:
  MinimumLevel:
    Default=Warning (EnvironmentVariablesConfigurationProvider Prefix: 'My_')
    Override:
      System=Warning (EnvironmentVariablesConfigurationProvider Prefix: 'My_')
      System.Threading=Debug (EnvironmentVariablesConfigurationProvider Prefix: 'My_')