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

Discussion: priority on environment variable vs compiling time value #1225

Open
TommyCpp opened this issue Aug 23, 2023 · 6 comments · Fixed by #1323
Open

Discussion: priority on environment variable vs compiling time value #1225

TommyCpp opened this issue Aug 23, 2023 · 6 comments · Fixed by #1323
Assignees
Labels
A-common Area:common issues that not related to specific pillar release:required-for-stable Must be resolved before GA release, or nice to have before GA.

Comments

@TommyCpp
Copy link
Contributor

TommyCpp commented Aug 23, 2023

Most of our creates generally allow two ways to configure values:

  • Use environment variable fetched during runtime
  • User provided value during compiling time(with_xxx)

However, this is unclear when both of the configurations present which one should take priority. Regardless we should align on this and use it as a tenet across all our creates.

cc @open-telemetry/rust-approvers

@TommyCpp TommyCpp added A-common Area:common issues that not related to specific pillar release:required-for-stable Must be resolved before GA release, or nice to have before GA. labels Aug 23, 2023
@TommyCpp
Copy link
Contributor Author

TommyCpp commented Aug 23, 2023

We discussed it in weekly meetings. Some notes

  • Compiling time value
    • @TommyCpp @cijothomas agree with this
    • @shaun-cox is not objecting if we advise users to not configure anything in code and leave it to ENV.
    • NET, Go, Python, follow this.
  • Environment variables

@djc
Copy link
Contributor

djc commented Aug 24, 2023

I find it very confusing to make the compiled-in value take priority over the run-time value (from an environment variable). Code is usually written (and compiled) to function across many environments, whereas the runtime environment is specific to a particular invocation. Clearly the runtime environment, with its better specificity, should be trusted more than the code.

Maybe we should just not provide any way to configure these values in code? Or clarify with function names etc that the values provided in code are just fallbacks if the environment doesn't provide the required information?

See also this recent discussion in Cargo on a related problem:

rust-lang/cargo#12515

In this case, there is no value provided in code, but there's a clear hierarchy of specificity.

@TommyCpp
Copy link
Contributor Author

I think most of people favors the environment variables. I think we can favor env vars here and make sure it's consistent.

Please leave comments if you disgree @open-telemetry/rust-approvers

@hdost
Copy link
Contributor

hdost commented Oct 14, 2023

I think most of people favors the environment variables. I think we can favor env vars here and make sure it's consistent.

Please leave comments if you disgree @open-telemetry/rust-approvers

So while this is not a deviation (more of a clarification), it might also be useful to document. #1297

@q3k
Copy link

q3k commented May 29, 2024

FWIW I've just gotten confused by OTEL_EXPORTER_OTLP_ENDPOINT taking precedence over .with_endpoint. This was in a scenario where code was written without the expectation of this variable being set, but run in a different environment where (junk) env vars took over and broke export with counter-intuitive transport errors.

I would've instead expected having to specify some kind of (imaginary) .with_env_vars() to enable environment variables being consulted. Otherwise it's not clear from glancing at the code that any environment variables are even considered (at least to people not familiar with the OpenTelemetry specs).

I'm not necessarily advocating for changing this behaviour, just providing a data point from a library user.

@cijothomas
Copy link
Member

Reopening to further discuss given above.

@cijothomas cijothomas reopened this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar release:required-for-stable Must be resolved before GA release, or nice to have before GA.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants