Fix: OtlpExporterOptions is not picking up env variables

[lyra95]

Fixes #6558

Changes

Update the constructor to augment the provided IConfiguration with environment variables before applying it.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: lyra95 / name: 조태혁 (b5f8bce, 81b6155, c2e3e2d, 20d3c38, ed479a3, 7590730, 2899815)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.62%. Comparing base (5f66728) to head (dec8d51).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6559      +/-   ##
==========================================
- Coverage   86.63%   86.62%   -0.01%     
==========================================
  Files         258      258              
  Lines       11888    11892       +4     
==========================================
+ Hits        10299    10302       +3     
- Misses       1589     1590       +1     

Flag	Coverage Δ
unittests-Project-Experimental	86.61% <100.00%> (+0.27%)	⬆️
unittests-Project-Stable	86.38% <100.00%> (-0.18%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs	99.12% <100.00%> (+0.03%)	⬆️

... and 1 file with indirect coverage changes

[martincostello]

The changes look reasonable.

Can you add a test that would fail without this change to protect against regressions in the future, and also update the CHANGELOG please?

[rajkumar-rangaraj]

Please take a look at #5586.. We should avoid adding AddEnvironmentVariables to the IConfiguration provider, since it's intended to be provided by the application.

[lyra95]

The changes look reasonable.

Can you add a test that would fail without this change to protect against regressions in the future, and also update the CHANGELOG please?

I’ve added tests and updated the CHANGELOG. I just noticed that this change also fixes #5586, so I included a test case for it as well.

[lyra95]

Please take a look at #5586.. We should avoid adding AddEnvironmentVariables to the IConfiguration provider, since it's intended to be provided by the application.

Instead of adding AddEnvironmentVariables, do you prefer OtlpExporterOptions to read them directly via Environment.GetEnvironmentVariable?

[martincostello]

Maybe another way to achieve the same end result is to make the IConfiguration field nullable and do something like this:

configuration ??= new ConfigurationBuilder().AddEnvironmentVariables().Build();
this.ApplyConfiguration(configuration, configurationType);

[lyra95]

Maybe another way to achieve the same end result is to make the IConfiguration field nullable and do something like this:

configuration ??= new ConfigurationBuilder().AddEnvironmentVariables().Build();
this.ApplyConfiguration(configuration, configurationType);

The issue is that if Host doesn’t load environment variables into IConfiguration (e.g. with Host.CreateEmptyApplicationBuilder), or if environment variables are set after the Host has already built IConfiguration, then OtlpExporterOptions won’t pick them up. This PR addresses that scenario, so the suggested change wouldn’t resolve the problem.

[martincostello]

Does it not move the decision later in a way that is equivalent to before ("no IConfiguration was provided at all, so we create one with only environment variables in it")?

[lyra95]

The problematic case is "IConfiguration is provided, but no environment variables are in it".

Also, when docs says it is configurable via environment variables, I think the implementation should do so. (not "if environment variables are set and if Host is configured appropriately")

It would be ideal if OtlpExporterOptions reads directly from environment variables rather than from IConfigrutaion, but I think AddEnvironmentVariables to existing IConfiguration is an acceptable solution.