Populate IConfiguration or IConfigurationSection parameter

[MV10]

Proposal

The package will automatically populate any IConfiguration or IConfigurationSection parameter on the target method.

The Issue

Any library being initialized by this package can only receive literals defined in the WriteTo entries. If the target supports more complex configuration than WriteTo provides (for example, currently arrays are not supported), the target has no means to access any configuration data when the target method is executed.

(This was not an issue under the .NET Framework in packages such as Serilog.Settings.AppSettings because the old ConfigurationManager approach is a static class, which means it is always available. Microsoft.Extensions.Configuration has no equivalent facility, it is the responsibility of the referencing library or application to provide access.)

In theory this issue could be avoided by simply not using this package at all, but I argue this limitation won't be obvious to Serilog users. For example, the MS SQL sink can add custom columns via a separate configuration section, so using this package is mutually exclusive with that feature. Each target package could explain any such limitations, but it seems easier in the long run to just provide a means to fix the problem in a way that is transparent to Serilog users.

If we make this package "smart" enough to populate IConfiguration or IConfigurationSection then, as dependent Serilog packages are updated to full Microsoft.Extensions.Configuration support, the problem basically fixes itself. (Probably the README for this package should provide advice to prefer IConfiguration over IConfigurationSection but that isolates RTFM caveats to the docs in this single repo.)

Implementation

Currently, the log config extension accepts either IConfiguration or IConfigurationSection, but IConfiguration simply retrieves the Serilog section and calls the section-based extension method. The section is passed into ConfigurationReader where all of the main processing occurs when the main Serilog package calls the ILoggerSettings.Configure method.

I propose to create two ctor overloads which also accept IConfiguration. They will extract the Serilog section and pass it along to the existing ctors. All ctors will store a local reference to IConfiguration (if available) and IConfigurationSection.

Finally, CallConfigurationMethods would be modified to check each MethodInfo for an IConfigurationSection parameter and an IConfiguration parameter (when available), then add these to the call list before invoking the method.

A pair of related tests should also be added, and possibly the sample could be updated.

"All ya gotta do is..."

And yes, I'm proposing to do the work myself, although any Reflection Ninjas in the house are more than welcome to handle it (I'm a little rusty on the specifics).

[merbla]

@MV10 do you have an example config (maybe from Azure Functions) that illustrates the problem? Also for the purposes of some nice test cases it would be nice to see the variations you need to cater to.

[MV10]

Fortunately the change is simple. The code is already done in my fork, though I haven't managed to make heads or tails of the unit tests.

My need arises from the problem described at the end of the linked thread. I don't know if applies to any other sink, but the MS SQL sink allows you to create arbitrary additional columns by adding a separate configuration section listing the column names and data types.

Edit: Just noticed somebody posted an issue about this specific problem:
serilog-mssql/serilog-sinks-mssqlserver#109

Technically Azure Functions V2 doesn't support this kind of config in the Functions code, the dependency is in the underlying runtime (which gets the .NET runtime spun up long before our own Functions code is ever loaded), but to use things like Serilog many of us build our own little support libraries which will pull in M.E.Config or whatever else is needed. But I also use the same libs and similar configs for ASP.NET Core web apps, console-based utilities, and so on throughout the infrastructure for the overall system.

Structurally as appsettings.json, a config might look like:

{
  "Serilog": {
    "Using":  ["Serilog.Sinks.MSSqlServer"],
    "MinimumLevel": "Debug",
    "WriteTo": [
      { "Name": "MSSqlServer", 
                "Args": { 
                  "connectionString": "Server...",
                  "tableName": "Logs"
                } 
      }
    ]
  },
  "MSSqlServerSink": {
    "Columns": [
      {
        "ColumnName": "Hostname",
        "DataType": "varchar"
      },
      {
        "ColumnName": "IP",
        "DataType": "varchar"
      }
    ]
  }
}

[MV10]

I'm trying to puzzle my way through the tests... I wish there were comments, and I'm not sure how much sense it makes for the tests to be this much more complex than the code being tested!

But I noticed the DummyRollingFile extension has some config methods that take an IFormatProvider formatProvider parameter, and in the test-config (a dictionary) the Args list contains an entry like this:

{ "formatter", new StringArgumentValue(() => "SomeFormatter, SomeAssembly") }

The README doesn't document this "reference, assembly" syntax, but it has me wondering, if I could divine the correct syntax, could that populate an IConfiguration parameter...? Maybe even from this package if IConfiguration were exposed properly?

(If this package would have to change to make that work, I think I still prefer my proposal -- less ceremony for Serilog users -- in my version, It Just Works ™ ).

[nblumhardt]

Interesting idea!

Since the configuration is per-sink-instance, is there some scheme that looks more like:

{
  "Serilog": {
    "Using":  ["Serilog.Sinks.MSSqlServer"],
    "MinimumLevel": "Debug",
    "WriteTo": [
      { "Name": "MSSqlServer", 
                "Args": { 
                  "connectionString": "Server...",
                  "tableName": "Logs",
                  "config": {
                    "Columns": [
                    {
                        "ColumnName": "Hostname",
                        "DataType": "varchar"
                    },
                    {
                        "ColumnName": "IP",
                        "DataType": "varchar"
                    }
                    ]
                  }
                } 
      }
    ]
  }
}

I.e., allow the config to be injected in just like any other sink argument. This is just a sketch, but in the above, WriteTo.MSSqlServer() would have an additional IConfigurationSection parameter called config.

Regarding the design/code/tests, this library suffers from having been produced via an evolutionary process, beginning way back at the .NET Core pre-1.0 days; it'd be great to take a fresh look at it at some point - there are some syntactic improvements possible as well.

The name, assembly syntax is just the regular qualified type name syntax, though there's a little extension for the benefit of the console sink that allows (IIRC) name::property, assembly to denote static properties on types. HTH!

[MV10]

@nblumhardt I had similar thoughts, but I'm leery of breaking existing code by changing the config format, and (if I understood the comments) currently array parameters aren't supported.

[nblumhardt]

Hi @MV10 - not sure I follow - which array are you referring to?

Regarding breakage, since this package and MSSQL config support aren't currently compatible, won't this be additive only? (Apologies if I'm missing the point here - early Saturday morning :-))

[MV10]

Edit:

Disregard the array comment... Now that I've had lots of coffee (my turn at Saturday!) I see what you're saying about an IConfigurationSection ... I guess I haven't reviewed exactly how the value type mapping works (which I think currently is limited to a string or one of those name, assembly values).

The more I think about where to put the config, the more I like keeping it in the Serilog section, too.

I'll leave the following post as-is ... the change you're suggesting is more complicated but I think it may be better in the long run. I'll have to experiment more to see what works.

This package can configure other parts of the current SQL sink, but I guess the actual compatibility issue (in my case it's Friday afternoon after a 13-hour work-day!) is just that it differs from the app.config or web.config approach (which does use a separate section).

But beyond just addressing the needs of the SQL sink, it seems desirable in the long run to ensure config is generally available, and this is a super-easy way to do it (for sink authors; transparent to Serilog users).

[MV10]

The change to support this is super-simple -- the important part is just four lines of code in ConfigurationReader: two calls to a two-line method that replaces the IConfiguration or IConfigurationSection parameter value if it exists. This is done just before call execution, so it doesn't even affect the tests (though it would be nice to add a dummy sink that uses IConfiguration). The other code changes are as described in the proposal (new ctors and changing the extension to call them).

Pretty good bang-for-the-buck if you ask me.

void CallConfigurationMethods(ILookup<string, Dictionary<string, IConfigurationArgumentValue>> methods, IList<MethodInfo> configurationMethods, object receiver, IReadOnlyDictionary<string, LoggingLevelSwitch> declaredLevelSwitches)
{
    foreach (var method in methods.SelectMany(g => g.Select(x => new { g.Key, Value = x })))
    {
        var methodInfo = SelectConfigurationMethod(configurationMethods, method.Key, method.Value);

        if (methodInfo != null)
        {
            var call = (from p in methodInfo.GetParameters().Skip(1)
                        let directive = method.Value.FirstOrDefault(s => s.Key == p.Name)
                        select directive.Key == null ? p.DefaultValue : directive.Value.ConvertTo(p.ParameterType, declaredLevelSwitches)).ToList();

            // ****************************************************************
            // add these
            if(_configuration != null) ReplaceValueByType(_configuration, methodInfo, call);
            if(_configSection != null) ReplaceValueByType(_configSection, methodInfo, call);

            call.Insert(0, receiver);

            methodInfo.Invoke(null, call.ToArray());
        }
    }

    // ****************************************************************
    // add this
    void ReplaceValueByType<T>(T value, MethodInfo methodInfo, List<object> parameterList)
    {
        var parm = methodInfo.GetParameters().FirstOrDefault(i => i.ParameterType is T);
        if(parm != null) parameterList[parm.Position] = value;
    }
}

[nblumhardt]

(Skipping parent post and commenting on grandparent :-))

Sounds like a plan 👍 - yes, one of the benefits of this sink over the current XML/key-value-pair configuration mechanism is that it cleanly handles multiple instances of the same sink, so going the extra distance to preserve that seems worthwhile. Shouldn't be a hugely different change to make; keen to see how it comes out!

Cheers,
Nick

[MV10]

I'm also fixing several places where ConfigurationSection is null-checked. The docs state that GetSection will never return null. Minor, but right now it runs code that isn't necessarily applicable to the provided configuration.

This if is always true:

var filterDirective = _configuration.GetSection("Filter");
if (filterDirective != null)

The fix is:

var filterDirective = _configuration.GetSection("Filter");
if (filterDirective.GetChildren().Any())

[MV10]

I hate to say it, but after pondering this more, I'm back to arguing in favor of my original proposal to automatically and transparently populate references to the entire IConfiguration object during target method invocation -- and, I promise, not because it's easier and could be a quick-fix for the SQL sink. 😁

One thing I 'd change from my original proposal is to not provide a way to "inject" the Serilog section into a sink -- I think that section should remain the domain of this package. But I think there are plenty of valid use-cases for referencing configuration external to the Serilog section.

One very common scenario where this is important is the use of named connection strings. The SQL sink can accept a literal connection string or the name of a connection string. The named version is expected to be found in a separate ConnectionStrings section. In certain deployment scenarios like Azure, connection strings (not necessarily for databases) are used all over the place.

Furthermore, in Azure, connection strings and other settings are commonly deployed as environment variables alongside XML or JSON files, not to mention more exotic options like Key Vault settings. This means we shouldn't make any assumptions about the source-format of the configuration information. This alludes to another problem with the idea we discussed about putting custom sections into the Serilog section. As we know, config sections are identified by the key of a given config key-value pair, and the content of a section is the value of the key-value pair. Since we can't just assume everything is JSON, we also can't assume anything about the formatting of the value portion, which means we shouldn't just store the value and parse it later -- the correct approach is to use GetSection to convert the value to a ConfigurationSection object.

However, GetSection would have to refer to the key by name, including the section-nesting context, all through a call via a configuration object -- none of which is available by the time the various target methods are invoked.

It gets worse. This package allows nested configuration sections. There is no way for this block to decide whether a given section should be recursively processed as a nested configuration or be processed or otherwise stored to be passed to a parameter on the invocation target. (This is also the reason arrays don't work, or parameters like List<T>...) Edit: See below for a fix.

With all that being said, I still agree as much configuration as possible should live within the Serilog section (and I'm going to look at changing the SQL sink to expect the custom column list there), but I argue it's well within the intent and spirit of the new approach to configuration to simply let every library or class access IConfiguration on demand.

Looking forward to thoughts on this and hopefully getting it wrapped up!

[MV10]

I have a proposed solution (tested, working) for the nested configuration issue.

I figure it's more likely that sinks or other config targets will need complex objects as parameters versus the frequency that nested configurations are needed. Thinking back to the :: syntax for static references, I added a > "hint" character to the end of key names whose values should be interpreted as additional configuration sections. For example, the "Sublogger" entry in this project's sample program would change to this (note the configureLogger> key:

    "WriteTo:Sublogger": {
      "Name": "Logger",
      "Args": {
        "configureLogger>": {
          "WriteTo": [
            {
              "Name": "Console",
              "Args": {
                "outputTemplate": "[{Timestamp:HH:mm:ss} {SourceContext} [{Level}] {Message}{NewLine}{Exception}",
                "theme": "Serilog.Sinks.SystemConsole.Themes.SystemConsoleTheme::Grayscale, Serilog.Sinks.Console"
              }
            }
          ]
        },
        "restrictedToMinimumLevel": "Debug"
      }
    },

The hint is just an EndsWith test and a Replace to remove it from the stored name, so if it needs to be more prominent, that's easy... configureLogger>>>>MOAR_CONFIG_PLZ

Next, I created a BoundArgumentValue class for when the hint is not present. The ctor just stores the config section. When ConvertTo is called, it simply uses the config extension Get<T> option-binding syntax to convert the config data to whatever CLR type is called for by the method parameter.

This still wouldn't work to populate an IConfigurationSection parameter since config uses the builder approach, so I still believe we should add code to recognize an IConfiguration parameter and populate it just before invocation.

But if everyone can live with hinting for nested configuration, it opens up this package to populating all sorts of complex parameters -- anything the config option binder extension can handle.

I already have it working with a List<Column> parameter for custom columns in the SQL sink.

[MV10]

Changes are completed to populate an IConfiguration parameter on the target method. All changes are available on this branch in my fork if anybody wants to check it out or discuss further before I open a PR.

https://github.com/MV10/serilog-settings-configuration/tree/iconfig_parameter

I also added a couple entries to the end of the README, they start here:

https://github.com/MV10/serilog-settings-configuration/tree/iconfig_parameter#nested-configuration-sections

Once I can PR this one, my dependent SQL sink changes are ready to PR, too.

[MV10]

I realized with just one additional line of code, that new BoundArgumentValue class can also populate an IConfigurationSection parameter. (It is now renamed ObjectArgumentValue since this takes it out of the realm of options-style binding; hopefully that name is also a clue about where to look for any future conversions that need extra hand-holding.)

The ability to grab a whole section is handy in the case of very complex configuration (again, the SQL sink is a good example, the ColumnOptions object is big and complicated and not well-suited to bind-friendly direct representation).

PR coming later today, last chance to object / comment / discuss... 3... 2... 1...