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

[configuration] Allow ${env:ENV} notation for environment variable substitution #3974

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Apr 2, 2024

Fixes #3961

Changes

Allows for the usage of the ${env:ENV} syntax in SDK file configuration. This syntax is used in the OpenTelemetry Collector, see https://opentelemetry.io/docs/collector/configuration/#environment-variables for more information.

This does not mean that other providers need to be supported; if we want to allow this we would need to reserve that syntax by rejecting ${<provider>:URI} as invalid, since right now it would be left as-is.

@mx-psi mx-psi requested review from a team as code owners April 2, 2024 16:07
@mx-psi
Copy link
Member Author

mx-psi commented Apr 2, 2024

cc @open-telemetry/configuration-maintainers

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

I'm in favor of this for an improved compatibility story with the collector env vars substitution syntax. I'm not in favor of expanding the set of prefixes to account for other sources like those supported in the collector since doing so adds complexity to implementations which needs to be justified with user demand / use cases.

specification/configuration/file-configuration.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

Do we want ${env:ENV} syntax in addition to ${ENV}? Did we consider it to be instead so that there is only one syntax?

@mx-psi
Copy link
Member Author

mx-psi commented Apr 2, 2024

Do we want ${env:ENV} syntax in addition to ${ENV}? Did we consider it to be instead so that there is only one syntax?

I would be in favor of only supporting ${env:ENV}, as I think most/all of @open-telemetry/collector-triagers would be. I am not sure what the opinion of the Configuration WG is, this was explicitly discussed in the past IIRC.

@dmitryax
Copy link
Member

dmitryax commented Apr 2, 2024

Right, the collector SIG decided in favor of ${env:ENV} to stay consistent with other config sources

@jack-berg
Copy link
Member

Do we want ${env:ENV} syntax in addition to ${ENV}? Did we consider it to be instead so that there is only one syntax?

There was pushback on the env: prefix because most users will be more familiar with the bash syntax. More details about the conversation in this comment: #3961 (comment)

This was discussed in the spec SIG in addition to the file config SIG.

Copy link
Contributor

@codeboten codeboten 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 it makes sense to support env: prefix. Since not having the prefix was the preference of the config working group, i'm ok with moving forward supporting both (as we do today in the collector)

@jack-berg jack-berg merged commit d76625f into open-telemetry:main Apr 8, 2024
7 checks passed
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.

Support for ${env:ENV} syntax