Add a way to get the configured log level switches #347
Conversation
src/Serilog.Settings.Configuration/Settings/Configuration/LoadedConfiguration.cs
Outdated
Show resolved
Hide resolved
| /// The log level switches that were loaded from the <c>LevelSwitches</c> section of the configuration. | ||
| /// </summary> | ||
| /// <remarks>The keys of the dictionary are the name of the switches without the leading <c>$</c> character.</remarks> | ||
| public IReadOnlyDictionary<string, LoggingLevelSwitch> LogLevelSwitches => _logLevelSwitches.ToDictionary(e => e.Key.TrimStart('$'), e => e.Value); |
There was a problem hiding this comment.
Property allocates Dictionary and N strings each call to it.
There was a problem hiding this comment.
I don't know if it is worth worrying about the initial dollar sign in the name. In fact, this is part of the switch name. What is the goal in removal of the dollar sign?
There was a problem hiding this comment.
In fact, this is part of the switch name.
Well, this is debatable: the LevelSwitches section does not need to contain the $ prefix. As found in the README and the appsettings.json file of the Sample project.
{
"Serilog": {
"Using": [ "Serilog.Sinks.Console" ],
"LevelSwitches": { "controlSwitch": "Verbose" },Property allocates
Dictionaryand N strings each call to it.
I wouldn't worry too much about that, one call is made per call to .ReadFrom.Configuration(configuration). So for most apps that would be exactly once at startup I guess.
Also, IDictionary does not implement IReadOnlyDictionary, sigh. But we could store Dictionary<string, LoggingLevelSwitch> instead of IDictionary<string, LoggingLevelSwitch> everywhere if the consensus is that this allocation must really be avoided.
There was a problem hiding this comment.
the LevelSwitches section does not need to contain the $ prefix.
Then it makes no sense in cutting off the dollar sign if it is not a special symbol. Why to do this? Just return _logLevelSwitches as is.
There was a problem hiding this comment.
I have addressed this concern in 2ecbe0a.
There was a problem hiding this comment.
👍 $ should be appended in tests.
There was a problem hiding this comment.
Of course! Fixed in 3f459e1.
Co-authored-by: Ivan Maximov <[email protected]>
| /// <summary> | ||
| /// The log level switches that were loaded from the <c>LevelSwitches</c> section of the configuration. | ||
| /// </summary> | ||
| /// <remarks>The keys of the dictionary are the name of the switches, including the leading <c>$</c> character.</remarks> |
There was a problem hiding this comment.
| /// <remarks>The keys of the dictionary are the name of the switches, including the leading <c>$</c> character.</remarks> | |
| /// <remarks>The keys of the dictionary are the names of the switches, including the leading <c>$</c> character if any.</remarks> |
There was a problem hiding this comment.
Fixed in c58ef69, but not the if any part because the switch always have a leading $ in the _declaredLevelSwitches dictionary:
public void AddLevelSwitch(string levelSwitchName, LoggingLevelSwitch levelSwitch)
{
_declaredLevelSwitches[ToSwitchReference(levelSwitchName)] = levelSwitch;
}
string ToSwitchReference(string switchName)
{
return switchName.StartsWith("$") ? switchName : $"${switchName}";
}
|
Closing this pull request because it will be 200% better to work on top of #348 to provide access to the configured log level switches. 😎 |
I have created this pull request as a draft because a few things should be discussed before merging.
LoadedConfigurationfor the class holding the loaded log level switches, as proposed by @tsimbalar in How to configure from JSON, but then get a reference to the switch? #206 (comment) I'm not convinced it's the best name and I'm happy to change it to something better.LoadedConfigurationclass could also expose the log filter switches, although not the internalLoggingFilterSwitchProxyclass directly. Maybe through a publicILoggingFilterSwitchinterface with a singleExpression { get; set; }property?I am looking forward to your feedback.
Fixes #206