[docs/rfc] RFC about environment variables

[mx-psi]

Description: Adds an RFC about how environment variable resolution should work
Link to tracking Issue: Fixes #9515, relates to:

• Password containing $ doesn't work when passed via ${env:DB_PASSWORD} #8215
• [confmap] Strings containing numbers in nonstandard notation are normalized to decimal notation #8565
• Provide warning when $ is used in config for env var expansion #9162
• [confmap] Restrict characters in environment variable URIs even more! #9531
• [confmap] Stricter types on configuration resolution #9532

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.70%. Comparing base (65cdb18) to head (90db1c0).
Report is 37 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9854      +/-   ##
==========================================
- Coverage   91.95%   91.70%   -0.25%     
==========================================
  Files         357      358       +1     
  Lines       16501    16543      +42     
==========================================
- Hits        15173    15171       -2     
- Misses       1000     1045      +45     
+ Partials      328      327       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mx-psi]

Self-reviewing to start some threads about topics I want to discuss

[mx-psi]

I am undecided about this, I feel like we should fail in some cases but I am not sure about which. open-telemetry/opentelemetry-specification/issues/3981 has some more discussion about this

[TylerHelmuth]

I agree with aligning with the configuration spec and I hope that ends up being failing. I think failing fast is a clearer outcome for the user.

For users that mis-typed the env var name, keeping the string verbatim could mean an unintuitive error caused by the unwanted string. If we fail immediately we can log a clear error message that will be easier to troubleshoot.

If the user needs an illegal string like ${env:this-is-illegal} they can use the escape to make it exact: $${env:this-is-illegal}.

To summarize: using the env var syntax is a request to parse an env var - if the provided env var cannot be parsed we should fail.

[mx-psi]

@TylerHelmuth The one edge case here is ${1} from open-telemetry/opentelemetry-collector-contrib/issues/9984. Should we fail in that case and force uses to use escaping syntax?

[mx-psi]

Since that is what we currently do and it seems impossible to detect all kinds of syntaxes past or future that would overlap with this, I have changed this to an error on 3f05467

[TylerHelmuth]

@mx-psi I feel like since ${1} corresponds with the regex it should be expanded. If users need the literal ${1} can they do $${1}?

[mx-psi]

I feel like since ${1} corresponds with the regex it should be expanded

I think we are on the same page, but to be clear 1 is not a valid identifier per the 'desired behavior' section (valid identifiers need to start with underscore or ASCII alphabetic characters), so it should not be expanded, but rather raise an error.

[TylerHelmuth]

Ah ok, then we are on the same page, I just got the outcome wrong. My opinion is the ${1} should fail and if users need that literal string they should escape: $${1}.

This prevents us from having any edge cases with our unwrap syntax while keeping all functionality available.

[TylerHelmuth]

@mx-psi where in the spec do we need to raise this issue?

[mx-psi]

Tyler and I discussed this offline but for the benefit of anybody else keeping track of this: this is being discussed on open-telemetry/opentelemetry-specification/pull/4002

[mx-psi]

For 'Desired behavior, inline string field' here, the behavior of the File Configuration SDK would be to set !!str 0123. I think this is a rare edge case, but as written it would set 0123 since the AsString method would not return the original representation

[evan-bradley]

For 'Desired behavior, inline string field' here, the behavior of the File Configuration SDK would be to set !!str 0123.

Can you link to where this is defined? I can't see where this is addressed this outside of declaring a suggested minimum version of the YAML spec.

[mx-psi]

I think the closest one would be the combo_string_key from https://opentelemetry.io/docs/specs/otel/configuration/file-configuration/#environment-variable-substitution

I take this to mean that the original value passed by the user on the environment variable should be preserved as is when you use expansion inline

[TylerHelmuth]

I vote we stick with whatever the configuration sig is doing

[TylerHelmuth]

I agree with aligning with the configuration spec and I hope that ends up being failing. I think failing fast is a clearer outcome for the user.

For users that mis-typed the env var name, keeping the string verbatim could mean an unintuitive error caused by the unwanted string. If we fail immediately we can log a clear error message that will be easier to troubleshoot.

If the user needs an illegal string like ${env:this-is-illegal} they can use the escape to make it exact: $${env:this-is-illegal}.

To summarize: using the env var syntax is a request to parse an env var - if the provided env var cannot be parsed we should fail.

[songy23]

LGTM

[mx-psi]

Remaining work

• Decide on [docs/rfc] RFC about environment variables #9854 (comment) by raising this on the spec and seeing if we can align on that
  • Filed [configuration] Fail on unsupported identifiers opentelemetry-specification#4002
• Decide on [docs/rfc] RFC about environment variables #9854 (comment). I am going to try and do a PoC of the confmap behavior to double check what makes more sense

[TylerHelmuth]

Based on our discussion in this thread this sentence should be updated to clarify that ${1} is no supported.

[TylerHelmuth]

@mx-psi where in the spec do we need to raise this issue?

[codeboten]

Remaining work

* [ ]  Decide on [[docs/rfc] RFC about environment variables #9854 (comment)](https://github.com/open-telemetry/opentelemetry-collector/pull/9854#discussion_r1561181980) by raising this on the spec and seeing if we can align on that
  
  * Filed [[configuration] Fail on unsupported identifiers opentelemetry-specification#4002](https://github.com/open-telemetry/opentelemetry-specification/pull/4002)

* [ ]  Decide on [[docs/rfc] RFC about environment variables #9854 (comment)](https://github.com/open-telemetry/opentelemetry-collector/pull/9854#discussion_r1549449041). I am going to try and do a PoC of the confmap behavior to double check what makes more sense

@mx-psi it looks like the spec issue will merge within the next 2 days, is the second issue still blocking this work?

[mx-psi]

@codeboten I haven't written a fully working PoC yet. I am fine merging this without the PoC if everybody else is okay with it (it may mean minor changes while we work on implementing this)

[codeboten]

Let's merge this and change it as needed, the spec change was merged 👍🏻