feat(core): add more scalable replacements for getEnv(), getEnvWithoutDefaults() #5443

pichlermarc · 2025-02-10T12:27:44Z

Which problem is this PR solving?

Currently we're accepting "environment variable-like" configuration even in the browser. This makes little sense IMO, since the SDK has to be configured in code anyway, so we're just offering duplicate ways to configure an SDK. Removing that feature, however, would be a breaking change - which is why #5217 is scheduled for SDK 2.0.

This PR lays the foundation for that by adding an easy replacement for getEnv() and getEnvWithoutDefaults(). It takes the env var name directly and no-ops in the browser. Once these replacements are used, we can release SDK 2.0 and code-changes to remove usages from the browser versions of the new SDK completely in a refactor.

Further, we currently have a problem with scaling getEnv() and the associated ENVIRONMENT type any further. For any new experimental feature, we have to add something to the stable ENVIRONMENT type, which is introduces friction during development as a stable type is modified for an experimental features.

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.

This PR introduces 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 later. Base functionality can be established with just getting string, number, boolean and string[] from environment variables.

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 package 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

Follow-ups

After this is merged, I will

open PRs for each package to start using the new replacements (the individual commits from [core] make browser-implementation of getEnv() return only defaults, getEnvWithoutDefaults() an empty object #5217)
remove getEnv() et al from @opentelemetry/core

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Unit tests
replaced all usages of getEnv() and getEnvWithoutDefaults() with the new functions in draft feat(core): introduce replacement functions for getEnv, getEnvWithoutDefaults #5429 to show that this works

…Defaults

…WithoutDefaults

codecov · 2025-02-10T12:34:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.79%. Comparing base (9feaee3) to head (5c904ce).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5443      +/-   ##
==========================================
+ Coverage   94.77%   94.79%   +0.02%     
==========================================
  Files         309      309              
  Lines        7967     7998      +31     
  Branches     1678     1687       +9     
==========================================
+ Hits         7551     7582      +31     
  Misses        416      416

Files with missing lines	Coverage Δ
...pentelemetry-core/src/platform/node/environment.ts	`97.29% <100.00%> (+13.96%)`	⬆️

…WithoutDefaults

pichlermarc · 2025-02-10T12:54:02Z

packages/opentelemetry-core/src/platform/node/environment.ts

+export function getNumberFromEnv(key: string): number | undefined {
+  const raw = process.env[key]?.trim();
+  if (raw == null || raw === '') {
+    return undefined;
+  }
+
+  const value = Number(raw);
+  if (isNaN(value)) {
+    diag.warn(
+      `Unknown value ${raw} for ${key}, expected a number, using defaults`
+    );
+    return undefined;
+  }
+
+  return value;
+}


Note for reviewers: I've left out the sub-classifications of integer, duration and timeout from the spec on purpose for now. These are mostly implemented in the consuming packages at the moment.

We can add these additional functions later if we see that we re-implement the same thing over and over again.

packages/opentelemetry-core/src/platform/node/environment.ts

pichlermarc · 2025-02-10T12:59:15Z

packages/opentelemetry-core/src/platform/node/environment.ts

+export function getStringListFromEnv(key: string): string[] | undefined {
+  return getStringFromEnv(key)
+    ?.split(',')
+    .map(v => v.trim())
+    .filter(s => s !== '');
+}


Note for reviewers: this is not in the spec but it has proven useful in quite a few places OTEL_PROPAGATORS, OTEL_SEMCONV_STABILITY_OPT_IN, ...

trentm · 2025-02-10T22:09:21Z

Once these replacements are used, we can release SDK 2.0 and code-changes to remove usages from the browser versions of the new SDK completely in a refactor.

For contrib packages to get a release that uses the replacements, we'd need to backport the new getStringFromEnv et al to the 1.x branch and do a release there, right?

trentm

LGTM with some nits.

packages/opentelemetry-core/src/platform/node/environment.ts

packages/opentelemetry-core/test/node/environment.test.ts

… getEnvWithoutDefaults

pichlermarc · 2025-02-11T10:37:23Z

The longer I thought about trimming all string values the more I became clear to me that it's likely not a good idea.
There may be places where we need it. OTEL_EXPORTER_JAEGER_PASSWORD may be such an example.

I think keeping whitespace is more in line with what the user expects, in line with getEnv() behavior and it's easy to chain ?.trim() where needed.

pichlermarc · 2025-02-11T10:45:16Z

cc @trentm if you want to have another look: bd5ca62 did change the behavior from trimming whitespace when retrieving strings to not trimming, see the comment above for the reasoning behind the change

pichlermarc · 2025-02-11T14:03:30Z

Once these replacements are used, we can release SDK 2.0 and code-changes to remove usages from the browser versions of the new SDK completely in a refactor.

For contrib packages to get a release that uses the replacements, we'd need to backport the new getStringFromEnv et al to the 1.x branch and do a release there, right?

I don't think that'd be necessary - I've had a quick look and I think the changes would be fairly simple to do. The packages there are all intended for Node.js and only get strings. They are also all non-otel env vars (with the exception of OTEL_LOG_LEVEL) so these can be easily switched over to use process.env directly.

I'll open a few PRs to take care of those :)

Edit:

trentm

Stamping, but again with a question.

packages/opentelemetry-core/src/platform/node/environment.ts

pichlermarc added 2 commits February 10, 2025 12:57

feat(core): introduce replacement functions for getEnv, getEnvWithout…

71b5a94

…Defaults

fixup! feat(core): introduce replacement functions for getEnv, getEnv…

50f4100

…WithoutDefaults

fixup! feat(core): introduce replacement functions for getEnv, getEnv…

99242f1

…WithoutDefaults

pichlermarc commented Feb 10, 2025

View reviewed changes

pichlermarc added this to the OpenTelemetry SDK 2.0 milestone Feb 10, 2025

pichlermarc marked this pull request as ready for review February 10, 2025 13:03

pichlermarc requested a review from a team as a code owner February 10, 2025 13:03

pichlermarc added target:next-major-release This PR targets the next major release (`next` branch) pkg:core labels Feb 10, 2025

trentm approved these changes Feb 10, 2025

View reviewed changes

packages/opentelemetry-core/src/platform/node/environment.ts Outdated Show resolved Hide resolved

packages/opentelemetry-core/test/node/environment.test.ts Show resolved Hide resolved

packages/opentelemetry-core/test/node/environment.test.ts Show resolved Hide resolved

pichlermarc added 4 commits February 11, 2025 10:33

Merge branch 'main' into feat/core-getenv-replace

2910491

fixup! fixup! feat(core): introduce replacement functions for getEnv,…

ffe7e1e

… getEnvWithoutDefaults

chore: clean up merge

97e9fb7

feat(core): do not trim string values

bd5ca62

trentm approved these changes Feb 11, 2025

View reviewed changes

packages/opentelemetry-core/src/platform/node/environment.ts Outdated Show resolved Hide resolved

packages/opentelemetry-core/src/platform/node/environment.ts Outdated Show resolved Hide resolved

pichlermarc added 2 commits February 12, 2025 10:46

fix(core): use false as default for getBooleanFromEnv()

466cebc

Merge branch 'main' into feat/core-getenv-replace

5c904ce

pichlermarc added this pull request to the merge queue Feb 12, 2025

Merged via the queue into open-telemetry:main with commit 1ed613c Feb 12, 2025
18 checks passed

pichlermarc deleted the feat/core-getenv-replace branch February 12, 2025 10:31

pichlermarc mentioned this pull request Feb 12, 2025

feat(sdk-trace-base)!: do not read environment variables from window #5455

Open

2 tasks

trentm mentioned this pull request Feb 12, 2025

remove export * from ... usage in sdk-node package #5461

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add more scalable replacements for getEnv(), getEnvWithoutDefaults() #5443

feat(core): add more scalable replacements for getEnv(), getEnvWithoutDefaults() #5443

pichlermarc commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

pichlermarc Feb 10, 2025

pichlermarc Feb 10, 2025 •

edited

Loading

trentm commented Feb 10, 2025

trentm left a comment

pichlermarc commented Feb 11, 2025

pichlermarc commented Feb 11, 2025

pichlermarc commented Feb 11, 2025 •

edited

Loading

trentm left a comment

feat(core): add more scalable replacements for getEnv(), getEnvWithoutDefaults() #5443

feat(core): add more scalable replacements for getEnv(), getEnvWithoutDefaults() #5443

Conversation

pichlermarc commented Feb 10, 2025 • edited Loading

Which problem is this PR solving?

Follow-ups

Type of change

How Has This Been Tested?

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

pichlermarc Feb 10, 2025

Choose a reason for hiding this comment

pichlermarc Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

trentm commented Feb 10, 2025

trentm left a comment

Choose a reason for hiding this comment

pichlermarc commented Feb 11, 2025

pichlermarc commented Feb 11, 2025

pichlermarc commented Feb 11, 2025 • edited Loading

trentm left a comment

Choose a reason for hiding this comment

pichlermarc commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

pichlermarc Feb 10, 2025 •

edited

Loading

pichlermarc commented Feb 11, 2025 •

edited

Loading