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 environment variable #3805

Closed

Conversation

jack-berg
Copy link
Member

Part of incorporating OTEP #225 into the specification.

Resolves #3752.

@jack-berg jack-berg requested review from a team January 5, 2024 19:43
@@ -8,7 +8,39 @@ aliases:

# OpenTelemetry Environment Variable Specification

**Status**: [Mixed](../document-status.md)
**Status**: [Stable](../document-status.md) except where otherwise specified
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't Mixed be the best fit in that case?

Copy link
Member

Choose a reason for hiding this comment

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

+1. Seems like explicitly specifying status for each section is clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

A while back I extended the metrics API with the the ability to specify histogram bucket boundary advice in #3216. At that point, the document was completely stable, except for the newly introduced experimental hint API. I argued that calling the document "mixed" makes the document seem less stable than it deserves:

Adjusted this document to be "mixed" status to reflect one new experimental API seems heavy handed.
Instead, we might consider saying that the document status is "Stable, except where specified". This would allow us to add experimental annotations to only relevant sections. Also, doing so arguably inspires more confidence in the overall stability of the document since the whole thing isn't tainted with the somewhat ominous "mixed" status.

I think this is true in general for documents which are > 50% stable.

However, we should be consistent so however we land on this, we should update docs with the outcome.


| Name | Description | Default | Notes |
|------------------|---------------------------------------------------------------|---------|-----------|
| OTEL_CONFIG_FILE | The path of the configuration file used to configure the SDK. | | See below |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| OTEL_CONFIG_FILE | The path of the configuration file used to configure the SDK. | | See below |
| OTEL_CONFIG_FILE | The path of the configuration file used to configure the SDK. If set, the configuration in this file takes precedence over all other SDK configuration environment variables. | | See below |

To make this very clear to all users.

@jack-berg
Copy link
Member Author

cc @tedsuo and @trask who had comments on #3744 related to this, which I summarized in #3752. There are several approvals on this so its theoretically ready to merge, but want to loop you in in case you missed this PR.

@trask
Copy link
Member

trask commented Jan 22, 2024

cc @tedsuo and @trask who had comments on #3744 related to this, which I summarized in #3752. There are several approvals on this so its theoretically ready to merge, but want to loop you in in case you missed this PR.

thanks @jack-berg. Yeah, I still have reservations about this as we discussed in this morning's Configuration WG. I added an outline of an alternate proposal on the issue: #3752 (comment)

@jack-berg
Copy link
Member Author

Thanks for the comment and concrete proposal @trask 🙏

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

I think there are many questions to clear before we can merge this. Please check this issue: open-telemetry/opentelemetry-java#6170

When `OTEL_CONFIG_FILE` is set, all other environment variables besides those
referenced in the configuration file
for [environment variable substitution](file-configuration.md#environment-variable-substitution)
MUST be ignored. Ignoring the environment variables is necessary because
Copy link
Contributor

Choose a reason for hiding this comment

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

If other env. vars. apart from the file are ignored then the file configuration has precedence over all other configuration methods.

there is no intuitive way to merge the flat environment variable scheme with the
structured file configuration scheme in all cases. Users that require merging
multiple sources of configuration are encouraged to customize the configuration
model returned by `Parse` before `Create` is called. For example, a user may
Copy link
Contributor

@brunobat brunobat Jan 23, 2024

Choose a reason for hiding this comment

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

This seems to supperceed the programatic interface concept defined here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-configuration.md

All configuration methods should be built on top of a common programatic interface that grant similar configuration capabilities across the board, not just for file configurations.

@jack-berg
Copy link
Member Author

See #3840 for an alternative to this which defines merge semantics with the environment variable configuration scheme.

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.

LGTM.

This solution is better compared to the merge semantics PR #3840 in my opinion.

@jack-berg
Copy link
Member Author

Closing this PR, as I now believe that #3850 is the best way to go for the reasons discussed here.

The fact of the matter is that users in our industry have come to expect the ability to be able to override configuration with environment variables. #3840 accommodates this expectation, but in a way that is sure to confuse users and annoy maintainers forever. #3850 accommodates expectations of users while maintaining coherence and matching intuition.

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.

Should file configuration merge environment variable configuration?
10 participants