ObjectArgumentValue can fail to bind to proper Constructors when parameter names are identical

[jkrueger-veradigm]

In situations where the Configuration binding matches identically to multiple public constructors via the naming of parameters all matched between themselves, the complex ObjectArgumentValue binding code will only attempt to use the first matched constructor. This will obviously lead to failure in the case where the first constructor has invalid matching types against the provided arguments within a Serilog configuration section. https://github.com/serilog/serilog-settings-configuration/blob/cfe1b523c3354f153a14a93796ec9a8d8ab70ea4/src/Serilog.Settings.Configuration/Settings/Configuration/ObjectArgumentValue.cs#L187

i.e. (rough examples to demonstrate the issue)

public class Test {
   public Constructor1(string a, string b, FirstType c) {}
   public Constructor2(string a, string b, SecondType c) {}
}

// Causes Exception
{
  "Serilog": {
    "WriteTo": {
      "TestSink": {
        "Name": "TestSink",
        "Args": {
          "parameterOfTestType": {
            "$type": "Test",
            "a": "value-for-a",
            "b": "value-for-b",
            "c": {
              "$type": "SecondType",
              "parameterForSecondType": "some-value"
...
// Loads Successfully
{
  "Serilog": {
    "WriteTo": {
      "TestSink": {
        "Name": "TestSink",
        "Args": {
          "parameterOfTestType": {
            "$type": "Test",
            "a": "value-for-a",
            "b": "value-for-b",
            "c": {
              "$type": "FirstType",
              "parameterForFirstType": "some-value"
...

Ideally, this could be solved while still picking the first constructor, by matching parameter names in addition to matching criteria for the Types of each corresponding Parameter, which is currently ignored. https://github.com/serilog/serilog-settings-configuration/blob/d0a1ba01fb92bc92c0ad1c83dacc73e2aaa28c77/src/Serilog.Settings.Configuration/Settings/Configuration/ObjectArgumentValue.cs#L168-L176

[nblumhardt]

Thanks for the suggestion!

Which sink currently has overlapping parameter names of different types? While I agree we could try to succeed in this scenario, it'd also be preferable that we avoid creating sinks that exhibit this behavior - I don't think UX will be great even if we disambiguated by type.

[nblumhardt]

(Also - things get much more murky when possibilities overlap due to e.g. abstract types or interfaces; probably more complexity than we would want to add here.)

[jkrueger-veradigm]

Which sink currently has overlapping parameter names of different types?

LoggerConfigurationAzureEventHubExtensions.AzureEventHub from https://github.com/serilog-contrib/serilog-sinks-azureeventhub (which unfortunately is over 2 years since latest NuGet release) Internally it can build an EventHubProducerClient (https://github.com/Azure/azure-sdk-for-net/blob/c51ca649b1554ff2c38da3507adf97243f4c2cfd/sdk/eventhub/Azure.Messaging.EventHubs/src/Producer/EventHubProducerClient.cs#L288) which has 4 matching Constructor patterns (1 is an abstract for the other 3):

EventHubProducerClient(string fullyQualifiedNamespace,string eventHubName,AzureNamedKeyCredential credential,EventHubProducerClientOptions clientOptions = default)
EventHubProducerClient(string fullyQualifiedNamespace,string eventHubName,AzureSasCredential credential,EventHubProducerClientOptions clientOptions = default)
EventHubProducerClient(string fullyQualifiedNamespace,string eventHubName,TokenCredential credential,EventHubProducerClientOptions clientOptions = default)

Hence the actual constructors come from Azure SDK code and themselves are not controlled by Serilog packages/Sinks.

While I agree we could try to succeed in this scenario, it'd also be preferable that we avoid creating sinks that exhibit this behavior - I don't think UX will be great even if we disambiguated by type.

It's definitely been a struggle trying to work out why it wasn't working for sure, and finding out that the Sink config code literally cannot use the constructor desired via proper config structuring means we've had to fallback to a 'less-than-ideal' solution in that regard.

(Also - things get much more murky when possibilities overlap due to e.g. abstract types or interfaces; probably more complexity than we would want to add here.)

When working with reflection it's definitely a situation of "try our best" since there are so many combinations of what is possible... I just think maybe a touch more than "if the name matches, it's accurate" would help... this clearly isn't a prevalent situation; probably most other sinks and dependencies properly setup unique constructor signatures based solely on parameter names... either by coincidence or intentional design.

[jkrueger-veradigm]

Essentially, I'm under the expectation that this Serilog config JSON should properly configure an AzureEventHub logging Sink by way of matching this particular EventHubProducerClient constructor signature EventHubProducerClient(string fullyQualifiedNamespace,string eventHubName,TokenCredential credential,EventHubProducerClientOptions clientOptions = default) In actuality, it always matches the first constructor EventHubProducerClient(string fullyQualifiedNamespace,string eventHubName,AzureNamedKeyCredential credential,EventHubProducerClientOptions clientOptions = default) and I run into an Exception concerning invalid parameter types (since a ManagedIdentityCredential cannot implicitly cast to an AzureNamedKeyCredential)

{
"Serilog": {
  "WriteTo": {
    "AzureEventHubSink": {
      "Name": "AzureEventHub",
      "Args": {
        "eventHubClient": {
          "$type": "Azure.Messaging.EventHubs.Producer.EventHubProducerClient, Azure.Messaging.EventHubs",
          "fullyQualifiedNamespace": "[NAMESPACE].servicebus.windows.net",
          "eventHubName": "[NAME]",
          "credential": {
            "$type": "Azure.Identity.ManagedIdentityCredential, Azure.Identity",
            "clientId": "[GUID]"
          }
        }
      }
    }
  }
}

[nblumhardt]

Open to exploring what the solution would look like. 1:1 exact type matching might keep expectations (and scope) reasonable. Some careful, minimal refactoring would be needed to avoid spaghettifying some already fairly complex code, I think :)

[jkrueger-veradigm]

In my particular case, an exact type match also wouldn't work since it is creating an overridden type which matches an abstract type on the constructor.

Alternatively, I feel a better exception message would go a long way when the chosen constructor signature fails to be instantiated as to explaining the internal logic which is attempting to match up the parameters. Currently the failure will bubble out from the internal reflection binding specifying that the parameter type is incorrect. And again if the provided config JSON does not properly parse into objects or fully match to a constructor, the code falls out to the base attempt of creating the object from the logging configuration and we see "no parameter-less constructor exists", which I found quite confusing as to why the code was trying to make a parameter-less construction when I was clearly providing parameter values in the JSON.

[jkrueger-veradigm]

As a separate workaround which is actually much cleaner for anyone who runs into this in the future, I found it was simple to create a new LoggerConfiguration extension, receiving a custom override class for the EventHubProducerClient which explicitly uses the exact constructor needed.

i.e. concerning the specific above example:

Copy paste of the original sink source code, but with overridden 'eventHubClient' parameter

public static LoggerConfiguration CustomAzureEventHub(
        this LoggerSinkConfiguration loggerConfiguration,
        CustomEventHubProducerClient eventHubClient,
        string outputTemplate = "{Timestamp:yyyy-MM-dd HH:mm:ss.fff zzz} [{Level}] {Message}{NewLine}{Exception}",
        IFormatProvider formatProvider = null,
        LogEventLevel restrictedToMinimumLevel = LogEventLevel.Verbose,
        bool writeInBatches = false,
        TimeSpan? period = null,
        int? batchPostingLimit = null)
    {
        if (loggerConfiguration == null) { throw new ArgumentNullException(nameof(loggerConfiguration)); }

        if (eventHubClient == null) { throw new ArgumentNullException(nameof(eventHubClient)); }

        MessageTemplateTextFormatter formatter = outputTemplate != null
            ? new MessageTemplateTextFormatter(outputTemplate, formatProvider)
            : throw new ArgumentNullException(nameof(outputTemplate));
        return loggerConfiguration.AzureEventHub(
            formatter,
            eventHubClient,
            restrictedToMinimumLevel,
            writeInBatches,
            period,
            batchPostingLimit);
    }

Overridden EventHubProducerClient

public class CustomEventHubProducerClient : EventHubProducerClient
{
    public CustomEventHubProducerClient(
        string fullyQualifiedNamespace,
        string eventHubName,
        TokenCredential credential,
        EventHubProducerClientOptions clientOptions = default) : base(
        fullyQualifiedNamespace,
        eventHubName,
        credential,
        clientOptions)
    { }
}

And make sure the current project assembly for the CustomEventHubProducerClient is defined in the Serilog:Using JSON array of assembly names as well as expected custom types for the parameters, and it should load up the Serilog Sink directly from the config reader!