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

Define OTEL_CONFIG_FILE with placeholder for new environment variable override scheme #3850

Closed

Conversation

jack-berg
Copy link
Member

Resolves #3752.

An alternative to #3805 which ignores the existing environment variable scheme, and leaves a placeholder for defining a new environment variable scheme specific to file configuration, with names derived from the configuration model.

This is the proposal I suggested here. To restate the advantages:

  • Avoid the messy merge semantics associated with merging the existing environment variable scheme
  • Provide environment variable overrides, which users have come to expect
  • We can soften the sharp edges associated with a environment variable scheme derived from the model by refactoring the configuration model to be more friendly to the rules (i.e. by avoiding object arrays)

To understand exactly what a refactored configuration model might look like, I've put together open-telemetry/opentelemetry-configuration#69 to review along side of this.

Comment on lines 311 to 313
The implementation SHOULD log a warning if calling Merge Environment results in
changes to
the [configuration model](./file-configuration.md#configuration-model).
Copy link
Member

Choose a reason for hiding this comment

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

I didn't follow why emit a warning for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think its important to draw attention to the fact that the contents of the file don't reflect the configuration of the SDK. I imagine the workflow being something like:

  • Include an overriding environment variable
  • Receive a warning that the configuration file is different (warning doesn't mean a problem in this context - more like a notification): "Environment variables were set that overrode the configuration specified at /path/to/config.yaml"
  • If opted in, log the entire effective configuration after applying overrides so you can deduce what changed

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest limiting this to a debug message. It could be worth looking at Spring for prior art.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd suggest limiting this to a debug message

That's fine. I can relax the language to say "The implementation SHOULD emit a log if calling Merge Environment results in change...". Leave it to implementations to decide the appropriate log level.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

I still have questions around the existing env vars, but I support this PR as a good incremental step forward 👍

marcalff

This comment was marked as outdated.

@tedsuo
Copy link
Contributor

tedsuo commented Feb 6, 2024

@marcalff unfortunately running a separate process is a non-starter for many end user environments that OpenTelemetry needs to support.

Copy link
Contributor

@tedsuo tedsuo left a comment

Choose a reason for hiding this comment

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

I agree that this is the correct approach for moving forwards. Thanks for threading the needle on this one.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Previous suggestion withdrawn - it does not work for all SIG.

The is unfortunate, as a lot of complexity will be pushed to every SIG runtime.


| Name | Description | Default | Notes |
|------------------|------------------------------------------------------------------------------------------------------------------------------------|---------|-----------|
| OTEL_CONFIG_FILE | The path of the configuration file used to configure the SDK. If set other environment variables defined in this file are ignored. | | See below |
Copy link
Contributor

Choose a reason for hiding this comment

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

If set other environment variables defined in this file are ignored.

It needs to be clarified in which file will the environment variables be ignored. The file defined in OTEL_CONFIG_FILE?

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 15, 2024
@jack-berg jack-berg removed the Stale label Feb 16, 2024
Co-authored-by: Marc Alff <marc.alff@free.fr>
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Copy link

github-actions bot commented Mar 7, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

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

Successfully merging this pull request may close these issues.

Should file configuration merge environment variable configuration?
7 participants