Define OTEL_CONFIG_FILE with placeholder for new environment variable override scheme#3850
Define OTEL_CONFIG_FILE with placeholder for new environment variable override scheme#3850jack-berg wants to merge 11 commits intoopen-telemetry:mainfrom
Conversation
…y-specification into define-otel-config-file
…y-specification into otel-config-file-env-vars
| The implementation SHOULD log a warning if calling Merge Environment results in | ||
| changes to | ||
| the [configuration model](./file-configuration.md#configuration-model). |
There was a problem hiding this comment.
I didn't follow why emit a warning for this?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'd suggest limiting this to a debug message. It could be worth looking at Spring for prior art.
There was a problem hiding this comment.
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.
trask
left a comment
trask
left a comment
There was a problem hiding this comment.
I still have questions around the existing env vars, but I support this PR as a good incremental step forward 👍
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@marcalff unfortunately running a separate process is a non-starter for many end user environments that OpenTelemetry needs to support. |
tedsuo
left a comment
tedsuo
left a comment
There was a problem hiding this comment.
I agree that this is the correct approach for moving forwards. Thanks for threading the needle on this one.
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
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 | |
There was a problem hiding this comment.
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?
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Co-authored-by: Marc Alff <marc.alff@free.fr>
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
7 participants
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:
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.