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

[pichlermarc]

Description

Important

Only actionable for next branch. Any PRs opened for this MUST be based on and targeted at the next branch.

The @opentelemetry/core package contains a browser implementation of getEnv() that attempts to get the configuration from the _globalThis (similar to getting env vars from process.env in Node.js). This behvaior

• duplicates ways to configure components that have to be set up via code anyway
• increases bundle size
• significantly increases test-complexity
• significantly increases code-complexity
• confuses users who are looking for the "right" way to do things

Since removing getEnv() completely would be quite an undertaking and would result in a large, difficult to review PR, this issue focuses on:

• disabling the functionality of getEnv() and getEnvWithoutDefaults() so that it does not extract config from _globalThis
• removing any browser tests (run via the npm run test:browser script) that tested this behavior.

Since the behavior is then not present anymore when we release 2.0, removing getEnv() usage for browser code can be a non-breaking change and can be done in a feature-release. We can remove usages of getEnv() and getEnvWithoutDefaults() step-by-step throughout the code base as a clean-up in feature releases. This should yield bundle-size improvements and an eventual further reduction of code-complexity.

This issue is considered done when:

• getEnv() has been changed returns a copy of DEFAULT_ENVIRONMENT for the browser implementation
• getEnvWithoutDefaults() has been changed to return a new empty object for the browser implementation
• any tests that rely on this behavior have been removed or adapted
  • note: please do not blindly remove all tests. some tests may be used both in Node.js and Browser (usually in common directories) - you may have to adapt some tests and move them to different directories so that the only run on Node.js
• a follow-up issue to track removing getEnv() and getEnvWithoutDefaults() in a feature release has been created

Code location

• opentelemetry-js/packages/opentelemetry-core/src/platform/browser/environment.ts

  Lines 28 to 37 in 141b457

  	export function getEnv(): Required<ENVIRONMENT> {
  	const globalEnv = parseEnvironment(
  	_globalThis as typeof globalThis & RAW_ENVIRONMENT
  	);
  	return Object.assign({}, DEFAULT_ENVIRONMENT, globalEnv);
  	}
  	export function getEnvWithoutDefaults(): ENVIRONMENT {
  	return parseEnvironment(_globalThis as typeof globalThis & RAW_ENVIRONMENT);
  	}

[chancancode]

duplicates ways to configure components that have to be set up via code anyway

@pichlermarc do you feel confident that all the configs currently settable via globalThis.OTLP_* variables have obvious programmatic equivalents? I ask because I looked into implementing this, but having to update some of the existing tests revealed that some of the migration (for users) can be less than straight-forward, if possible at all.

If you currently set one of the OTEL_* variables, you'd often have to look deeply into the implementation source code to see how they are being used to infer what the equivalent is. Sometimes it is used deep inside the construction of nested/related objects that you won't otherwise have to construct yourself today when using these variables, making the migration non-trivial, if possible at all.

Some variables have no obvious replacement (that I could find). For example, #4897 plan on using the OTEL_SEMCONV_STABILITY_OPT_IN in the migration plan, which you can't otherwise set.

So if we are going ahead with this, I think we'd have to:

1. Audit the existing variable, triage them into "not supported on the web", "have programmatic replacement API", "need new programatic API"
2. Write up a migration guide to document the replacement for each of the supported variables

That seems like a pretty significant lift to me, but perhaps it feels more tractable to someone with more context.

Alternatively, here is a counterproposal that would capture most of the same benefits, but less aggressive:

1. Introduce a new entrypoint @opentelemetry/core/browser-env, or somewhat equivalently, a new package @opentelemetry/browser-env
2. Export a setEnv({ ... }) function from @opentelemetry/core/browser-env as a direct replacement for setting the variables on globalThis
3. Ensure all the internal validation and parsing code (e.g. "true" -> true) are only required within @opentelemetry/core/browser-env
4. setEnv({ ... }) will validate and parse the variables, and privately set the parsed result as module state in core or something like globalThis._PRIVATE_OTEL_PARSED_ENV
5. getEnvWithoutDefaults() in core can simply return the parsed result, if set, and getEnv() is simply { ...DEFAULTS, ...PARSED_ENV }

Benefits:

1. Existing code that already uses those variables have a straightforward migration path, they can migrate the the programmatic APIs over time and won't be blocked from upgrading to 2.0 just because of this issue
2. We can start prioritizing documenting the programmatic APIs and make clear that those are the preferred way
3. In a bundler that supports tree-shaking, this should realize the same bundle size benefits for code that doesn't use the new entrypoint/package
4. If we do this as a separate package, the README for the package itself can document that the programmatic APIs are preferable, and can serve as the migration guide; we can deprecate the package itself when we feel confident about the coverage and that the community had enough time to absorb the changes

As far as reducing the test and code complexity, so far I am not sure that would be the case anyway. As you pointed out, most of the code consuming getEnv() are shared between both environments. So long as the node version is still required to support customizing the behavior based on those ENV variables (to maintain parity/consistency with other language SDKs), the getEnv() abstraction (even if it does ~nothing in the browser) seems to make it easier to share the code and test the customizations.

Part of the issue I ran into when implementing this is precisely that – the current tests are written to both 1. test that setting the ENV variable has the desired effect, and 2. test the functionality of said "desired effect". Since even under the original proposal, this should continue to work in Node, we still need the tests for (1) in Node, but it's hard to test that without actually test the functionality, so it ends up requiring the tests to be duplicated (plus a lot of adaptation to the existing test that resulted in more elaborate setup).

[pichlermarc]

@chancancode thanks for the detailed write-up - yes you're right it's a pretty significant change with lots of moving parts, I think this needs some more thought so I'll apply the needs:refinement label and we can go back to the drawing board on this.

The long-term goal is to remove "automatic configuration" via environment variables (or env-var like config in the case of the web implementations) from the plain SDK packages @opentelemetry/sdk-trace*, @opentelemetry/sdk-metrics* (it's already gone there), @opentelemetry/sdk-logs (that's experimental and we may introduce breaking changes later, so it's out of scope for now) and move them to a package like @opentelemetry/sdk-node, since there's additional specification on the way that will provide an alternative way of configuring that will override env-var config - but I think that needs more discussion with the other maintainers before we can make a move on this.

I'll put this on ice right now since there are some other changes I'm thinking of (and that I did not write down yet) which may make this change here significantly easier.

[pichlermarc]

I'll update the issue once we've come up with a more comprehensive plan.