feat(auto-instrumentations-node): add getPropagator to get propagators from environment

[pichlermarc]

Which problem is this PR solving?

According to the spec we MUST NOT maintain the aws propagator be part of the core repo, so we also cannot introduce it as a dependency to sdk-node. However, we've had getResourceDetectors() in @opentelemetry/auto-instrumentations-node which also includes all third-party resource-detectors. This PR adds getPropagator() which allows you to get a propagator based on the environment variable OTEL_PROPAGATORS. It includes third-party ones like xray or xray-lambda without having to add them as a core feature.

Short description of the changes

• adds getPropagator() which builds a propagator based on OTEL_PROPAGATORS
• adds that propagator automatically to the @opentelemetry/auto-instrumentations-node/register export, so it overrides what the SDK would do.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.06%. Comparing base (dfb2dff) to head (1dbc804).
Report is 162 commits behind head on main.

❗ Current head 1dbc804 differs from pull request most recent head 2f635ff

Please upload reports for the commit 2f635ff to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2232      +/-   ##
==========================================
+ Coverage   90.97%   97.06%   +6.08%     
==========================================
  Files         146        8     -138     
  Lines        7492      409    -7083     
  Branches     1502       67    -1435     
==========================================
- Hits         6816      397    -6419     
+ Misses        676       12     -664     

see 140 files with indirect coverage changes

[trentm]

Drive by.

[trentm]

Should empty string be treated as "use the default" as well? (That's what I infer from the language in this section: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#enum-value)

Also the spec mentions handling "none" as a value to say "no propagators": https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration

[pichlermarc]

Should empty string be treated as "use the default" as well? (That's what I infer from the language in this section: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#enum-value)

Yes that's correct, good catch 🙂 I added that to the default case. cf3bb0d

Also the spec mentions handling "none" as a value to say "no propagators": https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#general-sdk-configuration

Also good catch, it would've incorrectly logged that it's unkown before (the Propgator would still have been a no-op CompositePropagator with no sub-propgators, so I think that would've been spec compliant). I added an info log that lets the user know that none. Though I'm not sure what to do if there's none among other valid values so I've opted to still take the rest of the propagators in that case. My guess is that users would be least surpirsed if they set tracecontext, none and the W3CTraceContextPropagator would be the one that gets set. cf3bb0d

[trentm]

Some comparison points on "none" handling:

• auto-instrumentations-node handling of OTEL_NODE_RESOURCE_DETECTORS handles foo,none,bar and "none"

  opentelemetry-js-contrib/metapackages/auto-instrumentations-node/src/utils.ts

  Lines 238 to 244 in caf7cb5

  	if (resourceDetectorsFromEnv.includes('all')) {
  	return [...resourceDetectors.values()].flat();
  	}
  	if (resourceDetectorsFromEnv.includes('none')) {
  	return [];
  	}

• ditto for sdk-node's handling of that same envvar
• sdk-node's handling of OTEL_TRACES_EXPORTER is slightly weird: OTEL_TRACES_EXPORTER=none,otlp results in different behaviour to OTEL_TRACES_EXPORTER=otlp,none :) https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts#L101-L126

Anyway, this is something that could perhaps share some common processing at some point. Doesn't have to be this PR, and perhaps would wait for work related to supporting file-config.

[trentm]

Just the npm run lint:fix to run first.

[JamieDanielson]

Looks great! (just needs lint as per the previous comment)

[JamieDanielson]

also we could note that this updates open-telemetry/opentelemetry-js#4494

[pichlermarc]

Thanks for the reviews. 🙇

I'm moving this back to draft as I talked to @martinkuba about some other options to solve this issue. There will likely be a propagator-autoconfigure package that only contains the spec propagators (all from core and including lambda and lambda-xray) - then we'll be able to move these propagator packages back to contrib. The code used in that package will look very similar to what I proposed here.

[pichlermarc]

closing in favor of #2299