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

Add support for overrides from environment variables #385

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

sungam3r
Copy link
Contributor

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

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

@skomis-mm
Copy link
Contributor

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

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

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

0xced commented Aug 4, 2023

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_')

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

Successfully merging this pull request may close these issues.

3 participants