Support collections as constructor arguments

[ChristofferGersen]

Support collections and arrays as constructor arguments.

Closes #314

[nblumhardt]

Thanks for sending this! It'd be great to nail down/discuss how this applies to other Serilog sinks, and what the expected limitations are.

For example, many sinks accept IEnumerable<T> arguments, and Serilog.Sinks.OpenTelemetry has an outstanding issue around specifying resource attributes, which are an IDictionary<string, object>:

https://github.com/serilog/serilog-sinks-opentelemetry/blob/dev/src/Serilog.Sinks.OpenTelemetry/Sinks/OpenTelemetry/OpenTelemetrySinkOptions.cs#L52

Since we'd likely need to do the same extensive testing around this change as around that one, and the code overlaps substantially, would it be possible to consider it here? It wouldn't necessarily need to be in the same PR, but doing that would help to ensure everything lines up.

[ChristofferGersen]

Yeah, I did overlook interfaces. I can try using List<T> or HashSet<T> when the target type is abstract.

Users may also want to explicitly specify an implementation to use. I think most collections accept an IEnumerable<T> inside the constructor to initialize the collection. I think at first this will have to suffice. In this case a superfluous List<T> will be allocated to initialize the actual collection implementation through its constructor. The only collections I can think of that will not work with this pattern are immutable collections. They would require special handling anyway, since they commonly do not have a public constructor.

I had not intended for dictionaries to be included inside this pull request, but I can see how it is similar. For dictionaries I think it would be better to implement a special case, that looks for an Add method with two arguments. This would allow JSON like the following:

"resourceAttributes": {
  "key1": "value1",
  "key2": "value2"
}

I think this will be the most intuitive solution for users. It does assume that the keys are of type string, I think this is probably fine, because System.Text.Json also supports a limited set of types, see docs. For I(ReadOnly)Dictionary I will attempt to use Dictionary<TKey, TValue> as default implementation. If another implementation is required, then it can be constructed through its constructor. Just like with other collection types commonly accepting IEnumerable<T> for initialization, dictionaries commonly accept a IDictionary<TKey, TValue>, for example SortedDictionary<TKey, TValue> does. The only problem with this is that when you have a IDictionary<string, string> with either a "type" or "$type" key, then it should prefer to interpret it as a constructor call. This would mean moving the TryBuildCtorExpression call before TryCreateContainer. Doing that will cause TryCreateContainer never to be used, because then the container code inside TryBuildCtorExpression will always be used. I tried to solve this by having two versions of TryBuildCtorExpression, but more refactoring would probably be better (more on that later).

As an alternative, dictionaries can be viewed as ICollection<KeyValuePair<TKey, TValue>>. In that case dictionaries can be handled similarly to other collections, at the expense of less intuitive syntax. I do not like this approach, but I am listing the possibility here for completeness. The previous example would become the following when taking this alternative approach:

"resourceAttributes": [
  { "key": "key1", "value": "value1" },
  { "key": "key2", "value": "value2" }
]

Another problem that OpenTelemetrySink seems to be having is that they accept a callback to configure OpenTelemetrySinkOptions, instead of directly accepting one as argument. That problem is out-of-scope of this pull request, since it has nothing to do with initializing collections.

Another thing that is bugging me about the current implementation is that most things have to be implemented twice. Once inside ObjectArgumentValue.ConvertTo and another time inside ObjectArgumentValue.TryBuildCtorExpression using Linq expressions. If more extensive testing is added, then it may also be worthwhile to reduce this duplication of functionality. I would suggest having TryBindToCtorArgument call ConvertTo for an IConfigurationSection and having TryBuildCtorExpression produce a value instead of a Linq expression that will produce that value. I did not do this here, because I was trying to minimize the changes I had to make inside this pull request. And such a refactoring can only be done safely by first adding a lot of test cases. I also believe that adding extensive test cases should be a separate pull request, because you want to run them on the current code, while trying to avoid making other changes.

I updated the branch to include support for using default implementations for common collection interfaces and support for dictionaries. I will see what I can do about improving the test cases next.

[nblumhardt]

Hi @ChristofferGersen - just checking in to make sure I understood your message above correctly, and you're not waiting on anything from me -- let me know if that's not the case 😄

Also, @0xced any thoughts on the direction of this? I know you've done some thinking along similar lines, and you seem to always be a step or two ahead of me when it comes to this project 😅

[ChristofferGersen]

No I am not waiting on you. I am just waiting on some more time to work on this. The implementation is more or less what I intended it to be, so if there are improvements you would like to see, let me know.

Next I will work on improving the test cases for ObjectArgumentValue. I will do this as a separate pull request so that tests can run on the existing code first. I will inline the test json, like is done for other test cases. I will try to have all test cases call ConvertTo instead of TryBuildCtorExpression, so that the tests do not rely on Linq expressions. This is important when I later try to refactor ObjectArgumentValue.TryBuildCtorExpression inside this pull request. Unless there was a good reason to use Linq expressions here that I am unaware of.

[prochnowc]

@nblumhardt It would be really nice to have this feature released soon, the OpenTelemetry Sink is more or less unusable without and I dont want to switch to Microsoft.Extensions.Logging ;)

[ChristofferGersen]

@prochnowc More test cases are required, to ensure that my changes do not break existing use cases. I am working on that, but I am doing this in my free time, since I do not require this for my work. As a result, this may not go as fast as you would expect. While working on the test cases last weekend I already found a bug in the dictionary conversion code. I just pushed another commit to fix the bug that I found. If you really want this now, then you can just clone my branch and build a version of this library that includes the changes. That way you can use this feature before it is released. But be aware that I am still testing this code further, so there may still be subtle bugs that I have not discovered yet.

On a side note. While fixing the bug, I also found out that it is not too difficult to also support dictionaries with keys that are not strings, by running them through StringArgumentValue. I only see this being used with primitives and enums, but it could be anything that StringArgumentValue supports.

[HHobeck]

Hi @ChristofferGersen.

Thank you very much taking the time in your free time and implementing this feature. I need this for Serilog.Sinks.OpenTelemetry package and I would like to ask you how far you are? Can I help you somehow?

Regards
Hardy

[ChristofferGersen]

I am currently waiting for #409 to be merged. I believe that is waiting for @0xced to review my intended changes. After that merge my intention is to update this pull request to look like my removed-linq-expressions branch. So I believe there is not much more to be done, unless it turns out that I should implement the features differently.

[ChristofferGersen]

I updated by branch with the required changes, the old branch is still available under support-collection-arguments-old.

The first commit contains the actual changes. The second one places all JSON strings into its own variable first, before passing it on to JsonStringConfigSource.LoadSection. Without doing this I got a bunch of JSON001 analyzer errors in Visual Studio 2022, because it also tries to parse the second argument as JSON, because of the language comments. Moving the JSON string to its own statement fixes this issue.

[nblumhardt]

Awesome! 🎉

While we give the other reviewers a chance to weigh in - is anyone on this thread able to grab the CI packages from https://ci.appveyor.com/project/serilog/serilog-settings-configuration/builds/49262471/job/1lb4arqp3k8jvtut/artifacts, and test this out in a real-world config? 😎

[nblumhardt]

All good to merge this @0xced? I'll send it through in 24h so we can start testing it out in the prerelease packages :)

[0xced]

I'll try to have a look this week.

[nblumhardt]

Hi @0xced, shall I merge this one and start getting some feedback? Cheers!

[nblumhardt]

Hey folks! Assuming everyone has their hands full, I'm hoping to keep things rolling :-)

Serilog.Sinks.OpenTelemetry v2 just shipped, with a couple of key parameters using dictionary types, so it'd be nice to have the whole scenario work end-to-end there.

I have some time aside to make sure everything hangs together, so perhaps we leave this weekend as a window for any further feedback, and otherwise aim to publish this in the dev package early next week.

[MikkelPorse]

Have been running it since the 2.0.0-preview with no issues.

Thank you for your work on this @ChristofferGersen , @0xced, @nblumhardt

[woha]

I'm unable to specify the service.name in my appsettings.json, the service name is still 'unknown_service...' when I check the logs.

This is what I have in my config:

"WriteTo": [
{
        "Name": "OpenTelemetry",
        "Args" : {
            "endpoint" : "http://localhost:4317",
            "protocol": "Grpc",
            "resourceAttributes": {
                "service.name": "Test.Name"
            }
        }
    }
]

Is this the correct way to do it?

[MikkelPorse]

Looks ok to me. I'd double check that Serilog.Settings.Configuration is v8.0.1

[woha]

Looks ok to me. I'd double check that Serilog.Settings.Configuration is v8.0.1

Thank you very much!