Skip to content

feat(configuration): refactoring config loader to print warning message#6524

Merged
pichlermarc merged 8 commits intoopen-telemetry:mainfrom
maxday:maxday/fix-config-loader
Apr 8, 2026
Merged

feat(configuration): refactoring config loader to print warning message#6524
pichlermarc merged 8 commits intoopen-telemetry:mainfrom
maxday:maxday/fix-config-loader

Conversation

@maxday
Copy link
Copy Markdown
Member

@maxday maxday commented Mar 24, 2026

Part Of #6107

While I wanted to add some simple debug logs, I noticed that it might require a bigger change.
I started small with only two env variables OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG to make sure we all agree on the approach.

The new approach for loading the config (and adding warning logs) is the following:

  1. Have a map of env variable + possible values
  2. The env reading happens only one time (it will auto-loop over all defined env variables)
  3. Since we now know the list of allowed values, it can print generic warning message without any additional work

Note that everything is strongly typed, I just did String for now, but it can be super easily extended to Bool, Number and StringList. I also added unit test around new functions.

Very open to any feedback, my plan was to start small to validate the approach, and if you like it, I can do follow-up PRs to adopt this mechanism in all the EnvironmentConfigFactory.ts

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • [N/A] Documentation has been updated

Output of local unit test running

  EnvReader
    readEnvVar
      ✔ should return undefined when env var is not set
      ✔ should return the value when env var is set
      ✔ should return defaultValue when env var is not set and defaultValue is provided
      ✔ should return the value when it matches allowedValues
      ✔ should warn and return undefined for invalid value with no defaultValue
      ✔ should warn and return defaultValue for invalid value with defaultValue
      ✔ should not warn when no allowedValues is defined
    readAllEnvVars
      ✔ should read OTEL_TRACES_SAMPLER when set to a valid value

@maxday maxday requested a review from a team as a code owner March 24, 2026 18:51
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.73%. Comparing base (e9831e7) to head (06c5d63).
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6524      +/-   ##
==========================================
- Coverage   95.75%   95.73%   -0.02%     
==========================================
  Files         364      366       +2     
  Lines       12096    12138      +42     
  Branches     2885     2893       +8     
==========================================
+ Hits        11582    11620      +38     
- Misses        514      518       +4     
Files with missing lines Coverage Δ
...mental/packages/configuration/src/EnvDefinition.ts 100.00% <100.00%> (ø)
...perimental/packages/configuration/src/EnvReader.ts 100.00% <100.00%> (ø)
...ages/configuration/src/EnvironmentConfigFactory.ts 99.12% <100.00%> (-0.44%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@maxday maxday changed the title feat: refactoring config loader to print warning message feat(configuration): refactoring config loader to print warning message Mar 24, 2026
Copy link
Copy Markdown
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Thank you for working on this
LGTM

@maryliag
Copy link
Copy Markdown
Contributor

maryliag commented Apr 8, 2026

Btw, I updated the PR description to say "Part Of" instead of "Fixes", since this PR is just the start and not the complete solution for that issue

@pichlermarc pichlermarc added this pull request to the merge queue Apr 8, 2026
Merged via the queue into open-telemetry:main with commit 87d0112 Apr 8, 2026
27 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.

3 participants