Configuration with decimal-point numbers

[tmjorg]

When reading configuration for sinks that include decimal-point numbers, the configuration reads the value as a string and then parses it using whatever number format the current culture supports.

Example of configuring Sentry as a sink, where SampleRate should be set to 10%::

      {
        "Name": "Sentry",
        "Args": {
          "Dsn": "https://<some value>.ingest.sentry.io/<some value>",
          "MinimumEventLevel": "Warning",
          "Environment": "Prod",
          "Release": "<some value>",
          "SampleRate": 0.1
        }
      }

The value is read / parsed as a string here:
argumentValue = new StringArgumentValue(argumentSection.Value);

While applying the configuration the rSampleRate value then depends on whether the current context supports point or comma as decimal point, giving either 0.1 (10%) or 1 (100%) as result.

The desired behavior would be for the number value to be parsed as a number and not a string, when the value is supplied as a number.

Package versions used:

• Serilog: 2.10.0
• Serilog.Settings.Configuration: 3.3.0
• Sentry.Serilog: 3.17.1

[nblumhardt]

👍

Here's where the conversion currently happens:

https://github.com/serilog/serilog-settings-configuration/blob/dev/src/Serilog.Settings.Configuration/Settings/Configuration/StringArgumentValue.cs#L124

Unfortunately, Microsoft.Extensions.Configuration doesn't internally support numbers - all configuration values are essentially strings by the time they reach any Serilog code.

I think a fix for this will require opt-in to avoid pernicious breakages in existing apps; e.g. ReadFrom.Configuration() might be able to accept a CultureInfo or similar, and use that when converting strings to numeric types, defaulting to CurrentCulture as Convert will be doing in the current code.

[tmjorg]

Adding an optional CultureInfo to ReadFrom.Configuration() sounds like a good solution.

[0xced]

I have started working on this issue on my FormatProvider branch.

But I also have pull request #347 which is currently in progress. This pull request already adds a parameter to the ReadFrom.Configuration() methods. So adding another parameter in another branch would be a recipe for merge conflicts.

Also, adding parameters to the ReadFrom.Configuration() methods in a non-breaking way is exponential. Pull request #347 goes from 6 methods to 12 methods (ignoring obsolete methods). Adding another optional IFormatProvider formatProvider parameter on top of it would go from 12 methods to 24 methods. 😕

@nblumhardt: Would you consider adding an optional IFormatProvider? formatProvider = null parameter to all the ReadFrom.Configuration() methods in a major (v4) version? This would be a binary breaking change but not a source breaking change.

[0xced]

Also, while we are talking about potential breaking changes, I'd argue that the default format provider should be changed from CultureInfo.CurrentCulture to CultureInfo.InvariantCulture.

Why? Because the Microsoft.Extensions.Configuration.Json package reads numbers as raw strings. E.g. in the following JSON data, the number 0.1 is read as "0.1" without any intermediate conversion to any kind of number. The raw 0.1 bytes are simply read as a string.

Full sample code with stack trace

var json = "{ \"number\": 0.1 }"u8;
using var jsonStream = new MemoryStream(json.ToArray());
var configuration = new ConfigurationBuilder().AddJsonStream(jsonStream).Build();

System.Text.Json.JsonDocument.GetRawValue(Int32 index, Boolean includeQuotes)
System.Text.Json.JsonDocument.GetRawValueAsString(Int32 index)
System.Text.Json.JsonElement.ToString()
Microsoft.Extensions.Configuration.Json.JsonConfigurationFileParser.VisitValue(JsonElement value)
Microsoft.Extensions.Configuration.Json.JsonConfigurationFileParser.VisitObjectElement(JsonElement element)
Microsoft.Extensions.Configuration.Json.JsonConfigurationFileParser.ParseStream(Stream input)
Microsoft.Extensions.Configuration.Json.JsonConfigurationFileParser.Parse(Stream input)
Microsoft.Extensions.Configuration.Json.JsonStreamConfigurationProvider.Load(Stream stream)
Microsoft.Extensions.Configuration.StreamConfigurationProvider.Load()
Microsoft.Extensions.Configuration.ConfigurationRoot..ctor(IList`1 providers)
Microsoft.Extensions.Configuration.ConfigurationBuilder.Build()

{ "number": 0.1 }

And since JSON numbers are written with a decimal point (.) the invariant culture is the best fit to parse those numbers.

Note that the same argument also holds if using TOML instead of JSON, for example through the Tomlyn.Extensions.Configuration NuGet package.

[nblumhardt]

Hi @0xced! I agree that InvariantCulture is/would have been a better default. I think this could be changed with a major version bump.

Regarding added optional args, I think given the role of this package, it'd be reasonable to break binary compatibility (it's rare for sinks to depend on it, and they're by far the biggest concern with respect to binary compat).

One of my hopes for the Serilog 3.0 wave is that the various Serilog.Extensions.* packages switch their major/minor versioning strategy to match 1:1 with the target Microsoft.Extensions.* packages; this would make the new version 7.0.x I guess 🤔

[sungam3r]

If we are talking about break binary compatibility, then I propose to group the entire set of additional parameters in the settings class. This will allow to protect code from further breaking changes when adding new settings in the future:

public static LoggerConfiguration Configuration(
        this LoggerSettingsConfiguration settingConfiguration,
        IConfiguration configuration,
        LoggerConfigurationOptions options = null)
    {
        options ??= new();
        // go on with values from options instance
    }

    public class LoggerConfigurationOptions
    {
        public string SectionName { get; set; } = DefaultSectionName;
        public CultureInfo Culture { get; set; } = CultureInfo.InvariantCulture;
        // and so on
    }

So only one method exists, no combinations now and in the future, no breaking changes also (in ideal).