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

feat(core): introduce replacement functions for getEnv, getEnvWithoutDefaults #5429

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Feb 6, 2025

Which problem is this PR solving?

Note

Draft PR to show the whole change. I'll break those up into more digestible pieces.

See #5217

This attempts to solve this by introducing new functions that map to the spec's idea of which types of env vars exist. It leaves some out, but the one's I've left out are mostly convenience functions that we can add added later. Base functionality can be established with just getting string, number, boolean and string[] from environment variables.

Most environment variables are only used once. At the moment, everyone that uses getEnv() has to import and parse all environment variables, even the ones they don't care about. The properties from ENVIRONMENT can also not be minified, nor tree-shaken - similar to what was happening with @opentelemetry/semantic-conventions but at a smaller scale.

With the proposed change here

  • only the functions for parsing need to be imported. Unused functions can be tree-shaken.
  • the env var names are used directly without intermediates steps, so they only occur once on read.
    • if we find that one is used in many times we can still introduce a constant/function later from a place where it makes sense (a function to retrieve OTEL_SEMCONV_STABILITY_OPT_IN could be exported from @opentelemetry/instrumentation, for instance)
    • we don't need to include env vars that the end-user does not need as the name is moved to the package that's using it
    • defaults are also moved to the packages, so they don't need to take up space if they're not needed
    • we don't need to deal with experimental env vars in the stable @opentelemetry/core packages anymore
  • the added functions are all no-op in the browser, which means we can remove all env var related code completely in a refactor without introducing another breaking change (fixes [core] make browser-implementation of getEnv() return only defaults, getEnvWithoutDefaults() an empty object  #5217)
  • allows us to remove getEnv() and getEnvWithoutDefaults(), ENVIRONMENT, DEFAULT_ENVIRONMENT, RAW_ENVIRONMENT, et al. in a follow-up

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Unit tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 97.41379% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.42%. Comparing base (99091be) to head (9a91af9).

Files with missing lines Patch % Lines
...kages/opentelemetry-sdk-trace-base/karma.worker.js 0.00% 2 Missing ⚠️
...ckages/opentelemetry-core/src/utils/environment.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5429      +/-   ##
==========================================
- Coverage   94.79%   90.42%   -4.37%     
==========================================
  Files         310      310              
  Lines        7974     8022      +48     
  Branches     1682     1715      +33     
==========================================
- Hits         7559     7254     -305     
- Misses        415      768     +353     
Files with missing lines Coverage Δ
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 89.06% <100.00%> (+0.17%) ⬆️
...imental/packages/opentelemetry-sdk-node/src/sdk.ts 96.01% <100.00%> (-0.02%) ⬇️
...ental/packages/opentelemetry-sdk-node/src/utils.ts 92.03% <100.00%> (+0.65%) ⬆️
experimental/packages/sdk-logs/src/config.ts 100.00% <100.00%> (ø)
...sdk-logs/src/export/BatchLogRecordProcessorBase.ts 93.61% <100.00%> (+0.67%) ⬆️
...pentelemetry-core/src/platform/node/environment.ts 91.66% <100.00%> (+8.33%) ⬆️
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts 95.40% <100.00%> (+0.34%) ⬆️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 100.00% <100.00%> (ø)
...entelemetry-resources/src/detectors/EnvDetector.ts 93.87% <100.00%> (-0.13%) ⬇️
...ackages/opentelemetry-sdk-trace-base/src/config.ts 93.33% <100.00%> (+4.96%) ⬆️
... and 4 more

... and 14 files with indirect coverage changes

@pichlermarc pichlermarc force-pushed the feat/get-env-replacement branch from 82b06b8 to b6c1ca9 Compare February 10, 2025 09:06
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.

[core] make browser-implementation of getEnv() return only defaults, getEnvWithoutDefaults() an empty object
1 participant