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

accept standard OpenTelemetry environmental variables for configuration #12

Closed
loomis opened this issue Jan 3, 2023 · 10 comments · Fixed by #141
Closed

accept standard OpenTelemetry environmental variables for configuration #12

loomis opened this issue Jan 3, 2023 · 10 comments · Fixed by #141
Labels
enhancement New feature or request

Comments

@loomis
Copy link
Contributor

loomis commented Jan 3, 2023

See the OpenTelemetry environmental variable definitions.

@julealgon
Copy link

Just a pointer on this one: ideally, it should be supported via Extensions.Configuration using IOptions, so that the settings are honored no matter how they are passed to the application (either via actual envvars, or via any other config source as long as the setting names are consistent).

The initial implementation of the OTEL libraries were only considering actual env vars and not using the configuration system to read them and that was a mistake (it has since been changed).

@loomis
Copy link
Contributor Author

loomis commented Feb 12, 2023

@nblumhardt Is there another sink that uses the IOptions pattern for configuration? I'm finding it difficult to understand how to integrate this and a concrete example would be helpful.

@nblumhardt
Copy link
Member

Splitting WriteTo.OpenTelemetry(..) might need to be split into the one we have today, and one that accepts an IConfiguration or similar and no additional args.

Trying to meld the two (explicit parameters for endpoints etc. + default ones from configuration) seems fraught with difficulties, so better if the user decides which style they're after.

IIRC the Serilog.Sinks.MSSqlServer sink does some special options support along these lines (not sure if it's the model to follow, but might help).

HTH!

@loomis
Copy link
Contributor Author

loomis commented Feb 14, 2023

Thanks! Will take a look.

@nblumhardt
Copy link
Member

nblumhardt commented Apr 12, 2023

Just adding another note for when we're back around to this; the typical way for Serilog to interact with external configuration is via ReadFrom.X() where X() today is one of Configuration()/Settings()/KeyValuePairs().

Given that the OTel variables are already defined and won't conform to any schema Serilog currently recognizes, we might be best off adding a ReadFrom.OpenTelemetryEnvironment() option that would just do the obvious thing - look for environment variables and (internally) pass these to WriteTo.OpenTelemetry() in strongly-typed args.

There's no restriction preventing e.g. an IConfiguration, or a bunch of defaults, being passed through to the ReadFrom extension, either.

Edit: clarifying - this would be called instead of WriteTo.OpenTelemetry() when using the sink. The sink would still provide WriteTo.OpenTelemetry() for purely programmatic configuration, but where the environment should be consulted, consumers would use ReadFrom.OpenTelemetryEnvironment() instead, potentially passing in default values for when the standard variables are absent.

@nblumhardt nblumhardt added the enhancement New feature or request label Apr 24, 2023
@collinkostichuk-unity3d

The opentelemetry-dotnet project's OtlpExporterOptions class might be a helpful example of merging these environment variables with dotnet's Options pattern.

AlbertoMonteiro pushed a commit to AlbertoMonteiro/serilog-sinks-opentelemetry that referenced this issue Jun 20, 2024
This method configure with default environmental variables definitions

Fixes serilog#12
@AlbertoMonteiro
Copy link

@nblumhardt I've created a PR #141

Can you review please?

@prochnowc
Copy link

prochnowc commented Jul 19, 2024

@AlbertoMonteiro I found another environment variables which seems to be missing in the PR:

OTEL_SERVICE_NAME https://opentelemetry.io/docs/languages/sdk-configuration/general/#otel_service_name

Could you add support for this aswell?

@nblumhardt
Copy link
Member

@prochnowc 4.0.0-dev-00317 now includes this.

@prochnowc
Copy link

@nblumhardt Many thanks. It's really helpfull when using Aspire.

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

Successfully merging a pull request may close this issue.

6 participants