Granular configuration using Expression Trees

[tsimbalar]

Does this issue relate to a new feature or an existing bug?

• Bug
• New Feature

This issue proposes an extension on top of the initial proposal described in #1038 .

The objective with this one is to be able to define a LoggerConfiguration using the existing fluent API, but still being able to override some specific parameters with key-value settings from another source.

The proposed implementation would look like this :

// supposedly, appsetting contains 
// <add key="serilog:write-to:RollingFile.pathFormat" value="anotherPath.log" />
// <add key="serilog:minimum-level:override:Microsoft" value="Warning" />
var logger = new LoggerConfiguration()
    .ReadFrom.Combined(x => x
        .AddExpression(lc => lc  
            .MinimumLevel.Debug
            .WriteTo.RollingFile("default-path-{Date}.txt"))
        .AddAppSettings())  
    .CreateLogger();

// AddExpression takes as parameter a : 
// Expression<Func<LoggerConfiguration, LoggerConfiguration>>

that is, a new extension method for ISettingsSourceBuilder that accepts a parameter of type Expression<Func<ISettingsSourceBuilder, ISettingsSourceBuilder>>.

That extension method would analyze the method calls, and generate the correspounding key-value settings that would allow to accomplish such a configuration.

Limitations
Expression-trees have a few limitations (see https://github.com/dotnet/roslyn/issues/2060 ), among others :

• you cannot call methods using named arguments (only using positional arguments work)
• you cannot call methods using default arguments (you have to call the method passing the argument event if it is the default value)

At least in the first version, I think not all configuration methods will / should be handled ...

Supported features

Here is what I think makes sense to implement based on my personal usage of Serilog, or the technical limitations (and the fact that they translate rather well to the stringy key-value settings) :

• the MinimumLevel.Debug(), MinimumLevel.Information() etc family of methods
• the MinimumLevel.Override("System", LogEventLevelWarning)
• Enrich.WithProperty(propName, propValue)
• Enrich.WithExtensionMethod() , that is also work with extension methods from Serilog or external (Enrich.WithThreadId(), Enrich.FromLogContext() etc)
• Sinks : all the WriteTo.Something(...) or AuditTo.SomeThing() should work

Unsupported features

Here is what I think we may not need to implement :

• MinimumLevel.Is(LogEventLevel) : the MinimumLevel.Debug|Information|...() methods look like enough to me
• ReadFrom ! calling ReadFrom.Foo()fromReadFrom.KeyValuePairs` seems pretty redundant !

Other

There are probably other things I didn't think of, so Help Welcome !.
On the top of my head : Filters ? Formatters ?

Also, what is the best way to report to the user that he/she is trying to use something that is not supported ?

Anyways, any idea is welcome :)

[nblumhardt]

Hey, this is looking cool :-)

It's a bit tricky to follow the thread of the many issues/experiements related to this; do you think we could spin up a new repository, do all of the experimentation there (building entirely upon what we can achieve by calling ReadFrom.KeyValuePairs()), and from that point figure out what any future hook points in serilog/serilog might look like?

I don't want to slow this down while the ideas are coming, but I'm also going to have a hard time keeping up as it evolves in the short-term.

On the syntax, we've used a pattern/naming scheme similar to this in other areas:

var logger = new LoggerConfiguration()
    .ReadFrom.Combined(readFrom => readFrom
        .Expression(lc => lc  
            .MinimumLevel.Debug()
            .WriteTo.RollingFile("default-path-{Date}.txt"))
        .AppSettings())
    .CreateLogger();

I.e. the outer lambda argument becomes readFrom, and the methods called on it echo what would be available under ReadFrom normally, such as AppSettings() shown here. What do you think? (Also, trying to come up with an alternative proposal to the Expression method name... Code? :-))

[nblumhardt]

Regarding reporting configuration issues to the user, I think based on discussions we've had in other cases, throwing exceptions is probably the friendliest option.

[tsimbalar]

do you think we could spin up a new repository, do all of the experimentation there (building entirely upon what we can achieve by calling ReadFrom.KeyValuePairs()), and from that point figure out what any future hook points in serilog/serilog might look like?

Sure ! Do I put only the expression tree stuff in a separate repo and leave the core stuff in serilog/serilog or should I take everything out of serilog/serilog for now ?

[nblumhardt]

I'm still digesting #1041, so please excuse the half-formed thinking around this :-)

The thing nagging at me is that we almost have everything we need to build a package like Serilog.Settings.Combined externally, today, except for a way of sending key-value-pairs from methods like ReadFrom.AppSettings() into a collection rather than directly through to the code that interprets them. (I know you've already thought about this).

Ideally, if we could come up with that minimal API to enable this feature in Serilog, we could then iterate on it in an external package without the pressure that comes from bedding down APIs to last a decade.

Straw-man proposal, as an example, if we just made a public constructor on LoggerSettingsConfiguration and exposed some kind of Action<IEnumerable<KeyValuePair<string, string>> applyPairs parameter on it, the code at:

https://github.com/serilog/serilog/blob/dev/src/Serilog/Configuration/LoggerSettingsConfiguration.cs#L55

could check whether such a delegate had been supplied, and call it instead of applying the settings to the logger configuration. We could also add a check to the Settings() method several lines up, and throw if it is called directly.

This would get Serilog.Settings.AppSettings working with Serilog.Settings.Combined, since the latter could just construct a LoggerSettingsConfiguration and provide it as the readFrom parameter.

A bit hard to follow, but does have the very appealing characteristic of making very minimal API commitments in the core while we figure out how Combined will look, and whether it gets enough use/fits enough scenarios for us to pull it into Serilog in a future release.

Does this make sense/sound workable? (I can create a serilog/serilog-settings-combined repository, BTW, so that it's easy for other Serilog contributors to find and contribute.)

[tsimbalar]

A bit hard to follow

yep ! 😵

I think we could do it even simpler :), without touching the constructor of LoggerSettingsConfiguration.

I believe relying on the existing KeyValuePairSettings class is the way to go. After all, we are still reading from key-value pairs :p.
If we can separate the building and the reading of the key-value pairs. For instance :

• add a KeyValuePairSettings constructor (and optionally adapt the existing one to call this new one)

public KeyValuePairSettings(Func<IEnumerable<KeyValuePair<string, string>>> settingsProvider)
{
    if (settingsProvider == null) throw new ArgumentNullException(nameof(settingsProvider));
    _settingsProvider = settingsProvider;
}

• change Configure like so :

replacing

var directives = _settings.Keys
    .Where(k => _supportedDirectives.Any(k.StartsWith))
    .ToDictionary(k => k, k => _settings[k]);

with

var directives = _settingsProvider.ToDictionary().Keys
    .Where(k => _supportedDirectives.Any(k.StartsWith))
    .ToDictionary(k => k, k => _settings[k]);

• And then adding a lazy-ish overload of public LoggerConfiguration KeyValuePairs(IEnumerable<KeyValuePair<string, string>> settings) on LoggerSettingsConfiguration that would look like that :

public LoggerConfiguration KeyValuePairs(Func<IEnumerable<KeyValuePair<string, string>>> settingsSource)

And it should then be doable to implement Combined based on that overload, without adding any new type in Serilog.Core.

On the other hand, I still have doubts about the whole Func<IEnumerable<Stuff>>, and it seems kind of redundant , because in the end, IEnumerable is already lazy, that is, its contents are not consumed until someone starts iterating on it ... so if we really wanted to, we could probably manage to not change anything in the public API, but change KeyValuePairSettings so that it does not consume the key-value pairs enumerable in its constructor, but only store a reference to it and starts consuming it in the implementation of Configure

---

Regarding the new repo :

Does this make sense/sound workable? (I can create a serilog/serilog-settings-combined repository, BTW, so that it's easy for other Serilog contributors to find and contribute.)

Sure, that would be great ! :) ... also it kind of settles it on the name Combined name ?

[nblumhardt]

Thanks for the follow-up. Always takes a while to settle on a design with these ones :-)

I am not as keen on exposing this vial ReadFrom.KeyValuePairs() yet because the added overload would then be a part of the regular consumer API that's easily found and depended on.

I might not have done a good job of explaining how Combined could work using my sketchy proposal, sorry if this is just re-stating the same idea, but I just want to make sure we're on the same page.

public static LoggerConfiguration Combined(
    this loggerSettingsConfiguration lsc,
    Action<LoggerSettingsConfiguration> configureSources)
{
    var settings = new Dictionary<string, string>();

    void ApplyPairs(IEnumerable<KeyValuePair<string, string>> pairs) {
        foreach (var kvp in pairs)
            settings[kvp.Key] = kvp.Value;
    }

    var sourceConfiguration = 
        new LoggerSettingsConfiguration(new LoggerConfiguration(), ApplyPairs);
    configureSources(sourceConfiguration);

    return lsc.KeyValuePairs(settings);
}

The consumer side would look like:

    .ReadFrom.Combined(readFrom => {
        readFrom.AppSettings(...);
        readFrom.Expression(...);
        readFrom.KeyValuePairs(...);
        // `readFrom.Configuration(...)` when we have an impl.
    })

The API ends up being a bit of a compromise - method chaining doesn't work within the configuration delegate, which could be a gotcha. We could solve that in a later iteration once we're comfortable making more changes to LoggerConfiguration and friends to support the feature, but it's definitely a downside.

Perhaps it highlights a bit of a deficiency in the idea of re-using ReadFrom. Hmmmmmm... I'm off for a coffee and a think about this :-D

[tsimbalar]

Thanks for clarifying it ! That is a lot clearer :) ...

Actually, I've been playing around a bit, and it turns out we should be able to do the Combined thing without touching anything on the public Serilog APIs .... so we could just play around in a separate project until the Combined APIs look the way we want, and later on, if necessary, merge some part of it in the Core.

We can rely on the fact that IEnumerable<KeyValuePair<string, string>> is inherently lazy, and actually build an IEnumerable from several ones ....

Here's a hacked together example of what could be done with no changes on the public API :

public class CombinedConfigurationTests
{
	[Fact]
	public void CombinedCombinesSettings()
	{
		LogEvent evt = null;
		var log = new LoggerConfiguration()
			.ReadFrom.Combined(b => b
				.AddKeyValuePairs(new Dictionary<string, string>
				{
					{"minimum-level", "Information" }
				})
				.AddKeyValuePairs(new Dictionary<string, string>
				{
					{"minimum-level", "Warning" }
				})
				)
			.WriteTo.Sink(new DelegatingSink(e => evt = e))
			.CreateLogger();

		log.Write(Some.InformationEvent());
		Assert.Null(evt);
		log.Write(Some.WarningEvent());
		Assert.NotNull(evt);
	}

	[Fact]
	public void ConfigBuilderConsumesEnumerablesAsLateAsPossible()
	{
		var consumeCount = 0;

		IEnumerable<KeyValuePair<string, string>> Enumerable1()
		{
			consumeCount++;
			yield break;
		}

		IEnumerable<KeyValuePair<string, string>> Enumerable2()
		{
			consumeCount++;
			yield break;
		}

		var builder = new ConfigBuilder();
		builder.AddSource(Enumerable1());
		builder.AddSource(Enumerable2());
		Assert.Equal(0, consumeCount);

		var combined = builder.BuildCombinedEnumerable();
		Assert.Equal(0, consumeCount);

		// ReSharper disable once ReturnValueOfPureMethodIsNotUsed
		combined.ToList();

		Assert.Equal(2, consumeCount);
	}
}

public static class LoggerSettingsConfigurationExtensions
{
	public static LoggerConfiguration Combined(this LoggerSettingsConfiguration lsc, Func<IConfigBuilder, IConfigBuilder> build)
	{
		var configBuilder = new ConfigBuilder();
		configBuilder = (ConfigBuilder)build(configBuilder);
		var enumerable = configBuilder.BuildCombinedEnumerable();

		return lsc.KeyValuePairs(enumerable);
	}
}

public interface IConfigBuilder
{
	IConfigBuilder AddSource(IEnumerable<KeyValuePair<string, string>> source);
}

public static class ConfigBuilderExtensions
{
	public static IConfigBuilder AddKeyValuePairs(this IConfigBuilder builder, Dictionary<string, string> keyValuePairs)
	{
		return builder.AddSource(new ReadOnlyDictionary<string, string>(keyValuePairs));
	}
}

public class ConfigBuilder : IConfigBuilder
{
	List<IEnumerable<KeyValuePair<string, string>>> _sources;

	public ConfigBuilder()
	{
		_sources = new List<IEnumerable<KeyValuePair<string, string>>>();
	}

	public IEnumerable<KeyValuePair<string, string>> BuildCombinedEnumerable()
	{
		IEnumerable<KeyValuePair<string, string>> Combined()
		{
			var result = new Dictionary<string, string>();
			foreach (var source in _sources)
			{
				foreach (var kvp in source)
				{
					result[kvp.Key] = kvp.Value;
				}
			}
			return result;
		}

		foreach (var kvp in Combined())
		{
			yield return kvp;
		}
	}

	public IConfigBuilder AddSource(IEnumerable<KeyValuePair<string, string>> source)
	{
		_sources.Add(source);
		return this;
	}
}

[nblumhardt]

Cool, let's create the additional repo and see how it goes.

I don't think we even need to rely on laziness, using this kind of API - the main reason we'd need changes in Serilog itself is to make it possible to call the existing ReadFrom.AppSettings() method without modifying Serilog.Settings.AppSettings. If we're prepared to create new APIs like AddAppSettings() in Serilog.Settings.Combined, these can call the underlying ReadFrom.KeyValuePairs() without jumping through hoops (.... I think :-))

:plus: repo now...

[nblumhardt]

https://github.com/serilog/serilog-settings-combined

Let's use the issue tracker there for discussion (core team are all watchers by default). Cheers! :-)