Support for LevelSwith in KeyValuePairSettings #1020

tsimbalar · 2017-09-01T08:40:10Z

In order to support serilog/serilog-settings-appsettings#7

key="level-switch:foo" value="Information" will create an instance of LoggingLevelSwitch named foo with initial level Information

…trol MinimumLevel key="minimum-level:controlled-by" value="foo" will set MinimumLevel.ControlledBy with the previously declared named level-switch

KeyValuePairSettings.ParseVariableDeclarationDirectives and KeyValuePairSettings.LookUpVariable

Passing a parameter directive the name of a declared Switch will pass it to the configuration method

… level=information

tsimbalar · 2017-09-01T08:45:39Z

test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs

+                .WriteTo.Sink(new DelegatingSink(e => evt = e))
+                .CreateLogger();
+
+            // TODO : maybe add Logger.LevelSwitch as an internal readonly property to avoid reflection here ? 


I'm using reflection to access a private field _levelSwitch of class Logger ....
I'm not super happy about that, but

I believe accessing that member is the only way to test that the Level switch is really applied (I have no "pointer" to the actual object because it was defined from config....)

I was not sure whether it was ok to make the field internal to make it visible from the test (or insite an internal get-only property)

Could we just infer the level by testing with a couple of different events? May be a bit clunky, but should stay stable over the long term via the public API.

yes, that would prove that the initial value for the "level switch" was indeed passed, but not that it was assigned .... but I guess it is covered by the full test-case (passing the switch also to a sink)

tsimbalar · 2017-09-01T08:47:50Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

+            if (directives.TryGetValue(MinimumLevelControlledByDirective, out minimumLevelControlledByLevelSwitchName))
+            {
+                var levelSwitch = LookUpVariable(minimumLevelControlledByLevelSwitchName, typeof(LoggingLevelSwitch), declaredVariables) as LoggingLevelSwitch;
+                // TODO : maybe throw an exception to notify missing variable declaration ?


I'm not sure what is the policy around exception in Serilog.
I know the idea is that a logging library shouldn't crash the app, but is it ok to throw exceptions during configuration, to help the developer fix the incorrect config ?
I believe it would be more user friendly to throw an exception that states what is exactly wrong (undeclared variable / wrong name etc), but I was not sure :p

https://github.com/serilog/serilog/wiki/Reliability should have some clues around this (it's a little vague, but sets out the general idea).

thanks ! I knew I had read something along those lines somewhere, I just couldn't remember :)
I'll change the code to throw a more explicit exception

tsimbalar · 2017-09-01T08:51:44Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

@@ -185,6 +252,36 @@ internal static IEnumerable<Assembly> LoadConfigurationAssemblies(Dictionary<str
            return configurationAssemblies.Distinct();
        }

+        internal static bool TryInstantiate(Type type, string constructorParamAsString, out object createdInstance)


I'm wondering if I did not over-engineer that part ... Seeing how the "callable directives" relied on reflection to find the proper method to call, I did the same to try and find a constructor for the type we are trying to build .... but considering the only type we need to build is always LoggingLevelSwitch, that might be overkill ...
If you think this should be simplified in order not to rely on reflection, that's fine, I'll do it :p

I'm just doing a quick first-pass over to try to provide some feedback, so haven't give it a huge amount of thought yet, but my instinct says just building something explicit to support LevelSwitch would be reasonable right now.

We're likely to address general object construction through the requirement that sinks be able to receive complex parameters to their configuration methods - ColumnOptions for the MSSQL sink, for example. My bet is we'll revisit general construction rules then.

ok, go for YAGNI then :)

tsimbalar · 2017-09-01T08:53:18Z

I added some comments in places where I was unsure about the proper way of doing things to be consistent with the rest of the code base ...

nblumhardt

Hi Thibaud! I had a few minutes spare so tried to get some feedback down, apologies if it's a bit of a shallow skim. Had a few queries/suggestions, let me know what you think.

nblumhardt · 2017-09-03T23:02:22Z

test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs

+            var settings = new Dictionary<string, string>
+            {
+                ["level-switch:Switch1"] = "Information",
+                ["minimum-level:controlled-by"] = "Switch1",


<add key="serilog:level-switch:Switch1" value="Information" /> contains the same information but also enforces that if a switch is declared, we set its level. Separating the two means we can declare a switch without using it, or try to use a nonexistent switch. Do we gain anything by splitting the declaration and usage across two directives?

Sorry, I'm not sure I understand your comment U_U

I just based my implementation on your proposal from here : serilog/serilog-settings-appsettings#7 (comment)

The switch needs to be used twice, both for the :

.MinimumLevel.ControlledBy(LoggingLevelSwitch ls)

.WriteTo.Seq( LoggingLevelSwitch controlLevelSwitch)
... so we need to find a way to express that both parameters refer to the same instance of a LoggingSwitch , or did I miss something ?

Sorry, yes, you're entirely correct, I misread the code in my attempt to skim through quickly.

nblumhardt · 2017-09-03T23:03:32Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

+            if (directives.TryGetValue(MinimumLevelControlledByDirective, out minimumLevelControlledByLevelSwitchName))
+            {
+                var levelSwitch = LookUpVariable(minimumLevelControlledByLevelSwitchName, typeof(LoggingLevelSwitch), declaredVariables) as LoggingLevelSwitch;
+                // TODO : maybe throw an exception to notify missing variable declaration ?


https://github.com/serilog/serilog/wiki/Reliability should have some clues around this (it's a little vague, but sets out the general idea).

nblumhardt · 2017-09-03T23:06:43Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

@@ -185,6 +252,36 @@ internal static IEnumerable<Assembly> LoadConfigurationAssemblies(Dictionary<str
            return configurationAssemblies.Distinct();
        }

+        internal static bool TryInstantiate(Type type, string constructorParamAsString, out object createdInstance)


I'm just doing a quick first-pass over to try to provide some feedback, so haven't give it a huge amount of thought yet, but my instinct says just building something explicit to support LevelSwitch would be reasonable right now.

We're likely to address general object construction through the requirement that sinks be able to receive complex parameters to their configuration methods - ColumnOptions for the MSSQL sink, for example. My bet is we'll revisit general construction rules then.

nblumhardt · 2017-09-03T23:08:46Z

test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs

+                .WriteTo.Sink(new DelegatingSink(e => evt = e))
+                .CreateLogger();
+
+            // TODO : maybe add Logger.LevelSwitch as an internal readonly property to avoid reflection here ? 


Could we just infer the level by testing with a couple of different events? May be a bit clunky, but should stay stable over the long term via the public API.

…tle private field

+ throw a "nicer" error message when referencing an undeclared LoggingLevelSwitch

tsimbalar · 2017-09-06T17:29:06Z

I think I'm done with this PR.
Please tell me if anything needs changing !

… override key="serilog:minimum-level:override:System" value ="switchName" will bind the override to a switch that may be controlled through the sink

tsimbalar · 2017-09-11T19:13:35Z

I have added code to handle referencing a LoggingLevelSwitch also from a MinimumLevel.Override

nblumhardt · 2017-09-13T01:46:04Z

Nice catch on the Overrides 👍

This does have me thinking, when I see:

["minimum-level:override:System"] = "specificSwitch"

Alongside:

["minimum-level:override:System"] = "Warning"

I'm reminded again of the possibility of disambiguating by forcing some kind of 'variable reference'-style syntax:

["minimum-level:override:System"] = "$specificSwitch"

I don't want to take the discussion around in circles :-), but what are your thoughts about this case?

tsimbalar · 2017-09-13T04:35:49Z

I do like that there is an indication that it is a reference to a variable :) . This also help explicitly look up a value by name instead of first trying to look up a value by name and then fall back to conversion. For consistence, I would also add a specific symbol when declaring the variable (phpanyone ? :p) , that would make it easier to visually recognize occurrences of the variable in the config file.

If we do not want to go too deep into variables and stuff like that and really just support LoggingLevelSwitches for now , maybe an option is to :

change the "regex" for switch declaration to enforce that the switch name must start with a $ in level-switch directives (thus forbidding to declare a switch named Information or Error)

<add key="serilog:level-switch:$systemSwitch" value="Warning" />

when calling configuration methods and trying to find a value to pass for each argument, keep on looking for a switch declared with name $switch ... and fall back to conversion .... without risk of interpreting it as a LoggingLevel .

(I think we still need to fallback to conversion because of possible Sink configuration methods that would accept a string, and in which the string "$this is my string" is valid (for instance file paths on file loggers ?) ... )

To support this, the only change needed is to change the regex for switch-level declarations, the rest would work as is, I believe.

This allows to support the use case for now, without closing too many doors for a possible more generic implementation in the future.

The final syntax would then be :

<!-- declare the switch -->
<add key="serilog:level-switch:$Switch1" value="Debug" />
<!-- use it to control the root logger -->
<add key="serilog:minimum-level:controlled-by" value="$Switch1" />
<add key="serilog:using:Seq" value="Serilog.Sinks.Seq" />
<add key="serilog:write-to:Seq.serverUrl" value="http://localhost:5341" />
<!-- give the sink access to the switch -->
<add key="serilog:write-to:Seq.controlLevelSwitch" value="$Switch1" />

and for the overrides

<add key="serilog:level-switch:$systemSwitch" value="Warning" />
<add key="serilog:minimum-level:override:System" value="$systemSwitch" />
<add key="serilog:using:Seq" value="Serilog.Sinks.Seq" />
<add key="serilog:write-to:Seq.serverUrl" value="http://localhost:5341" />
<!-- give the sink access to the switch -->
<add key="serilog:write-to:Seq.controlLevelSwitch" value="$systemSwitch" />

nblumhardt · 2017-09-19T00:02:45Z

@tsimbalar that looks good to me 👍 (sorry about the slow reply).

tsimbalar · 2017-09-19T04:16:24Z

@nblumhardt no problem ;) OK, I'll change the regex to accept only switches in the format $switchName ... and if I find the time, I'll start also issuing pull requests for documentation updates

... and use $ also when referencing the declared swith This helps disambiguate between a variable and the minimum level for a switch ... and makes sure noone is goind to declared a switch with name "Information"

nblumhardt

I think this is the way to go 👍

I took the liberty of nitpicking a bit, since I think the design is solid - keen to have your thoughts on these minor points but otherwise ready to

nblumhardt · 2017-09-20T03:22:29Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

@@ -81,6 +88,9 @@ public void Configure(LoggerConfiguration loggerConfiguration)
                .Where(k => _supportedDirectives.Any(k.StartsWith))
                .ToDictionary(k => k, k => _settings[k]);

+            var declaredLevelSwitches = ParseNamedLevelSwitchDeclarationDirectives(directives);
+
+


Nit: extra whitespace

your OCD might be worse than mine ! ;)

nblumhardt · 2017-09-20T03:24:55Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

@@ -37,13 +41,16 @@ class KeyValuePairSettings : ILoggerSettings
        const string MinimumLevelOverrideDirectivePrefix = "minimum-level:override:";

        const string CallableDirectiveRegex = @"^(?<directive>audit-to|write-to|enrich|filter):(?<method>[A-Za-z0-9]*)(\.(?<argument>[A-Za-z0-9]*)){0,1}$";
+        const string LevelSwitchDeclarationDirectiveRegex = @"^level-switch:(?<switchName>\$[A-Za-z0-9]+)$";


Instead of enforcing the presence of $ here, would it be friendlier to accept anything, but emit a warning or throw when the directive is found but the name is invalid?

Yes that would be friendlier indeed :)
I would say "throw" in here, the same way that right now for instance setting an invalid url in the Seq Sink throws a FormatException somewhere down the line.

I think throwing a FormatException with a detailed message would make sense. I don't think creating a custom exception type would add any value.

Agreed 👍

nblumhardt · 2017-09-20T03:26:14Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

+                    var initialLevel = (LogEventLevel)SettingValueConversions.ConvertToType(switchDeclarationDirective.InitialSwitchLevel, typeof(LogEventLevel));
+                    newSwitch = new LoggingLevelSwitch(initialLevel);
+                }
+                namedSwitches[switchDeclarationDirective.SwitchName] = newSwitch;


Can we assume keys are unique, and so use namedSwitches.Add() here to express/enforce that expectation?

makes sense

nblumhardt · 2017-09-20T03:29:30Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

+
+        static LoggingLevelSwitch LookUpSwitchByNameOrThrow(string minimumLevelControlledByLevelSwitchName, IReadOnlyDictionary<string, LoggingLevelSwitch> declaredLevelSwitches)
+        {
+            if (!declaredLevelSwitches.ContainsKey(minimumLevelControlledByLevelSwitchName))


How about:

if (declaredLevelSwitches.TryGetValue(minimumLevelControlledByLevelSwitchName, out var levelSwitch) return levelSwitch; throw new InvalidOperationException($"No `LoggingLevelSwitch` has been declared with name \"{minimumLevelControlledByLevelSwitchName}\". You might be missing a key \"{LevelSwitchDirective}:{minimumLevelControlledByLevelSwitchName}\"");

nblumhardt · 2017-09-20T03:30:44Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

+            return new ReadOnlyDictionary<string, LoggingLevelSwitch>(namedSwitches);
+        }
+
+        static LoggingLevelSwitch LookUpSwitchByNameOrThrow(string minimumLevelControlledByLevelSwitchName, IReadOnlyDictionary<string, LoggingLevelSwitch> declaredLevelSwitches)


Could we just call the argument switchName? The result might not always be used in a minimum-level:controlled-by directive, so the name might be a bit too specific.

yep :) That's a leftover of R# Extract Method ;)

nblumhardt · 2017-09-20T03:34:03Z

src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs

+        {
+            if (type == typeof(LoggingLevelSwitch))
+            {
+                return LookUpSwitchByNameOrThrow(valueOrSwitchName, declaredSwitches);


The naming scheme here isn't consistent - since ConvertOrLookupByName() could throw, I'd expect it to also carry the OrThrow() suffix if we're using that convention. Perhaps we could drop the OrThrow() from LookUpSwitchByNameOrThrow(), with the intention that we'd provide a TryXxxx() variant for the non-throwing version?

tsimbalar · 2017-09-20T04:29:41Z

Thanks for the review, I'll fix'em when I get a moment.

…witch with a switch name not starting with a $ sign

use Dictionary.Add() instead of Dictionary[] to make it mor explicit that uniqueness is implied

… for consistency

tsimbalar · 2017-09-20T11:27:16Z

I've updated my PR to include the suggested changes. Please tell me if I need to do anything else (for instance, should I squash / rebase my commits, or is it ok to see the sausage-making ?)

nblumhardt · 2017-09-20T21:32:23Z

Squashed and merged, thanks!

* add support for declaring a named LoggingLevelSwitch key="level-switch:foo" value="Information" will create an instance of LoggingLevelSwitch named foo with initial level Information * add support for using a previously declared LoggingLevelSwitch to control MinimumLevel key="minimum-level:controlled-by" value="foo" will set MinimumLevel.ControlledBy with the previously declared named level-switch * [WIP] unit test for passing controlLevelSwitch to a sink * [Refactoring] extracted methods to make it clearer KeyValuePairSettings.ParseVariableDeclarationDirectives and KeyValuePairSettings.LookUpVariable * add the possibility to pass a declared LoggingLevelSwitch to a sink Passing a parameter directive the name of a declared Switch will pass it to the configuration method * add support for level-switch:Switch1 - value = "" and interpret it as level=information * Minor edits/typos in the unit tests * Change brittle tests in order not to rely on reflection to acces brittle private field * Simplify the code around "creating a new LoggingLevelSwitch" + throw a "nicer" error message when referencing an undeclared LoggingLevelSwitch * Add support for referencing a LoggingLevelSwitch from a minimul level override key="serilog:minimum-level:override:System" value ="switchName" will bind the override to a switch that may be controlled through the sink * Enforce declaration of switch as level-switch:$switchName ... and use $ also when referencing the declared swith This helps disambiguate between a variable and the minimum level for a switch ... and makes sure noone is goind to declared a switch with name "Information" * Minor tweaks after review * Explicitly throw a helpful FormatException when specifiying a level-switch with a switch name not starting with a $ sign * Minor refactoring of ParseNamedLevelSwitchDeclarationDirectives use Dictionary.Add() instead of Dictionary[] to make it mor explicit that uniqueness is implied * Refactoring : use TryGetValue instead of ContainsKey * Refactoring : renamed LookUpSwitchByNameOrThrow to LookUpSwitchByName for consistency

tsimbalar added 6 commits August 30, 2017 20:46

add support for declaring a named LoggingLevelSwitch

07150b3

key="level-switch:foo" value="Information" will create an instance of LoggingLevelSwitch named foo with initial level Information

add support for using a previously declared LoggingLevelSwitch to con…

ec07c48

…trol MinimumLevel key="minimum-level:controlled-by" value="foo" will set MinimumLevel.ControlledBy with the previously declared named level-switch

[WIP] unit test for passing controlLevelSwitch to a sink

ab8f232

[Refactoring] extracted methods to make it clearer

178cfed

KeyValuePairSettings.ParseVariableDeclarationDirectives and KeyValuePairSettings.LookUpVariable

add the possibility to pass a declared LoggingLevelSwitch to a sink

c511056

Passing a parameter directive the name of a declared Switch will pass it to the configuration method

add support for level-switch:Switch1 - value = "" and interpret it as…

fa0b450

… level=information

tsimbalar commented Sep 1, 2017

View reviewed changes

Minor edits/typos in the unit tests

4440198

nblumhardt reviewed Sep 3, 2017

View reviewed changes

tsimbalar added 2 commits September 4, 2017 15:04

Change brittle tests in order not to rely on reflection to acces brit…

e56acc9

…tle private field

Simplify the code around "creating a new LoggingLevelSwitch"

aaf378e

+ throw a "nicer" error message when referencing an undeclared LoggingLevelSwitch

Add support for referencing a LoggingLevelSwitch from a minimul level…

ab1b463

… override key="serilog:minimum-level:override:System" value ="switchName" will bind the override to a switch that may be controlled through the sink

tsimbalar mentioned this pull request Sep 11, 2017

Deeper LoggingLevelSwitch support serilog/serilog-settings-appsettings#7

Closed

nblumhardt mentioned this pull request Sep 18, 2017

How to support Level control through Appsettings? datalust/serilog-sinks-seq#86

Closed

Enforce declaration of switch as level-switch:$switchName

4d92ab2

... and use $ also when referencing the declared swith This helps disambiguate between a variable and the minimum level for a switch ... and makes sure noone is goind to declared a switch with name "Information"

nblumhardt reviewed Sep 20, 2017

View reviewed changes

tsimbalar added 4 commits September 20, 2017 07:19

Minor tweaks after review

8d9631f

Explicitly throw a helpful FormatException when specifiying a level-s…

5488f42

…witch with a switch name not starting with a $ sign

Minor refactoring of ParseNamedLevelSwitchDeclarationDirectives

6d3f43d

use Dictionary.Add() instead of Dictionary[] to make it mor explicit that uniqueness is implied

Refactoring : use TryGetValue instead of ContainsKey

93dc232

Refactoring : renamed LookUpSwitchByNameOrThrow to LookUpSwitchByName…

84e9e90

… for consistency

nblumhardt merged commit 3bb8523 into serilog:dev Sep 20, 2017

tsimbalar deleted the level-switch-appsettings branch September 21, 2017 04:18

This was referenced Sep 21, 2017

Documents serilog/serilog-settings-appsettings#7 tsimbalar/serilog-sinks-seq#1

Closed

Documents serilog/serilog-settings-appsettings#7 datalust/serilog-sinks-seq#89

Merged

tsimbalar mentioned this pull request Oct 9, 2017

Allow logging level configuration per source in AppSettings configuration serilog/serilog-settings-appsettings#18

Closed

nblumhardt mentioned this pull request Nov 23, 2017

2.6.0 Release #1066

Merged

rsabirov mentioned this pull request Dec 11, 2017

Support for LevelSwith serilog-contrib/serilog-settings-xml#4

Open

tsimbalar mentioned this pull request Dec 22, 2017

LoggingLevelSwitch support serilog/serilog-settings-configuration#69

Closed

Support for LevelSwith in KeyValuePairSettings #1020

Support for LevelSwith in KeyValuePairSettings #1020

Conversation

tsimbalar commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsimbalar commented Sep 1, 2017

nblumhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsimbalar commented Sep 6, 2017

tsimbalar commented Sep 11, 2017 • edited Loading

nblumhardt commented Sep 13, 2017

tsimbalar commented Sep 13, 2017 • edited Loading

nblumhardt commented Sep 19, 2017

tsimbalar commented Sep 19, 2017

nblumhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsimbalar commented Sep 20, 2017

tsimbalar commented Sep 20, 2017

nblumhardt commented Sep 20, 2017

tsimbalar commented Sep 11, 2017 •

edited

Loading

tsimbalar commented Sep 13, 2017 •

edited

Loading