Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a ConfigurationMethod to Populate ResourceAttributes-dictionary from appsettings.json #107

Closed
MikkelPorse opened this issue Sep 20, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@MikkelPorse
Copy link

MikkelPorse commented Sep 20, 2023

When using new LoggerConfiguration().ReadFrom.Configuration(config) I would like to be able to assign values to ResourceAttributes: I'm thinking something like

"WriteTo": [
{
        "Name": "OpenTelemetry",
        "Args" : {
            "endpoint" : "http://localhost:4317",
            "protocol": "Grpc",
            "resourceAttributes": {
                "service.name": "my choice of ServiceName in stead of 'unknown service: executable.exe'."
            }
        }
    }
]

Of course that doesn't work without a little help. I pulled the source and built it into the existing ConfigurationExtension like so

public static LoggerConfiguration OpenTelemetry(
    this LoggerSinkConfiguration loggerSinkConfiguration,
    string endpoint = OpenTelemetrySinkOptions.DefaultEndpoint,
    OtlpProtocol protocol = OpenTelemetrySinkOptions.DefaultProtocol,
    IDictionary<string,object>? resourceAttributes = null)
{
    if (loggerSinkConfiguration == null) throw new ArgumentNullException(nameof(loggerSinkConfiguration));

    return loggerSinkConfiguration.OpenTelemetry(options =>
    {
        options.Endpoint = endpoint;
        options.Protocol = protocol;
        if (resourceAttributes is not null)
            options.ResourceAttributes = resourceAttributes!;
    });
}

It works but is sort of hard to unittest without resorting to reflection. Could the above be integrated into the project? Or is there a better way to do what I propose?

@nblumhardt
Copy link
Member

Hi!

I think the intended path for this is serilog/serilog-settings-configuration#398 - and some help over there would be most welcome if you have the opportunity.

I can see the place for resourceAttributes in a top-level configuration method, though, given their prominence in the OTel world. We initially dropped this but might need to reconsider - any thoughts @loomis ?

@MikkelPorse
Copy link
Author

Ah, I didn't think to look there, as I assumed it would be Otel-specific. :-)

I think maybe 0xced's suggestion is a bit ... ambitious ... compared to say FileLoggerConfigurationExtensions, where all (?) the possible configuration combinations have been brute-forced into extension methods, which are easily understood and require a fair bit less testing - and would not have to deal with options that refer to object instances like the LevelSwitch.

@nblumhardt
Copy link
Member

See also #123, just merged, which will support the changes in Serilog.Settings.Configuration.

@MikkelPorse
Copy link
Author

Solved by Serilog config PR #405. Horray 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants