Skip to content

Conversation

srogovtsev
Copy link
Contributor

This fixes #151 by exposing already existing configuration delegate.

I thought about adding some tests, but there's no good seam to verify the behavior and the code is as simple as it could be.

@nblumhardt
Copy link
Member

Thanks for the PR @srogovtsev!

In this sink, the top-level parameters on WriteTo.OpenTelemetry() are the simple/happy path. The rest go onto OpenTelemetrySinkOptions, which I think would be the best place for this one. Adding to the options type is usually not a breaking API change, and over time it's easier to manage these than a variety of overloads.

Could the setting instead be exposed as OpenTelemetrySinkOptions.GetEnvironmentVariable perhaps? Since the accepted values are the ones documented as environment variables in the OTel docs, I think this name would be reasonable (though GetOTelConfigurationVariable or similar could also work).

@srogovtsev
Copy link
Contributor Author

Could the setting instead be exposed as OpenTelemetrySinkOptions.GetEnvironmentVariable perhaps?

I wouldn't mind that at all, but wouldn't it be somewhat roundabout: we get a setting from OpenTelemetrySinkOptions to then modify other settings in the same object? I personally have no objections to that, just somewhat curious.

Since the accepted values are the ones documented as environment variables in the OTel docs, I think this name would be reasonable (though GetOTelConfigurationVariable or similar could also work).

I'd prefer it to say "configuration", because the whole idea of this setting is to allow reading them from places other than environment, so I would find it confusing when the option GetEnvironmentVariable is set to, let's say, AppSettings.GetString.

@nblumhardt
Copy link
Member

wouldn't it be somewhat roundabout

Looking again, I think your way is better, since it sets up a nice clear relationship between the new option and ignoreEnvironment 👍

I'd prefer it to say "configuration"

getConfigurationVariable might be the winner? 👍

@srogovtsev srogovtsev force-pushed the configuration-delegate branch from 21051be to 1d018e4 Compare August 19, 2024 19:26
@srogovtsev
Copy link
Contributor Author

I have renamed the delegate parameter to getConfigurationVariable, as discussed.

Copy link
Contributor Author

@srogovtsev srogovtsev Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file are purely cosmetic (renaming getEnvironmentVariable to getConfigurationVariable) and can be discarded if they seem redundant

Copy link
Member

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nitpicks but otherwise LGTM!

@nblumhardt nblumhardt merged commit 2c9ebbc into serilog:dev Aug 20, 2024
@nblumhardt
Copy link
Member

Thanks @srogovtsev!

@nblumhardt nblumhardt mentioned this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow configuring OTEL_* settings from anywhere, not only environment

2 participants