feat: Add package for automatic propagator configuration

[martinkuba]

Which problem is this PR solving?

Follow-up to #2232
Related to open-telemetry/opentelemetry-js#4494.

The spec says that the lambda instrumentation should be configurable via the OTEL_PROPAGATORS env variable, which MUST include the xray-lambda propagator.

The spec also says that third-party propagators MUST NOT be maintained as part of the core SDK.

Short description of the changes

This PR introduces new package that combines all known propagators configurable via the OTEL_PROPAGATORS env variable. It exposes a getPropagator() function that returns a propagators instance based on what is provided in the env variable.

[codecov]

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.40%. Comparing base (dfb2dff) to head (23538d2).
Report is 190 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2299      +/-   ##
==========================================
- Coverage   90.97%   90.40%   -0.58%     
==========================================
  Files         146      149       +3     
  Lines        7492     7367     -125     
  Branches     1502     1530      +28     
==========================================
- Hits         6816     6660     -156     
- Misses        676      707      +31     

Files	Coverage Δ
...ckages/auto-configuration-propagators/src/utils.ts	85.29% <85.29%> (ø)

... and 63 files with indirect coverage changes

[pichlermarc]

Thanks for picking this up. Some nits but overall looks good.
You can add me as a code-owner to the component-owners file. 🙂

[pichlermarc]

Ah there's also one other thing we need to to do make auto-labelling of PRs work: we need to add its path to .github/component-label-map.yml.

[pichlermarc]

cc @trentm @JamieDanielson since you reviewed my original PR #2232

[trentm]

Is the intention that auto-instrumentations-node will use this?
And that sdk-trace-base will NOT use this because that would be a dependency from the core repo to the contrib repo?

[pichlermarc]

Is the intention that auto-instrumentations-node will use this?

The intention is to make the register.js setup of auto-instrumentations-node use it eventually.

And that sdk-trace-base will NOT use this because that would be a dependency from the core repo to the contrib repo?

Exactly. People would have to add it manually to either the NodeSDK constructor, TracerProvider#register() or the propgator API, but it will be a one-liner for all of these. The lambda layer can take a dependency on the new package to set up the propagator here.

Having it seperate will allow us to move the thrid-party propagator packages (xray, xray-lambda) back to contrib and will allow the lambda layer to avoid auto-instrumentations-node which pulls in a lot of dependencies that are not needed in that context.

[martinkuba]

Looks to me like the build failure is unrelated to this change.

[trentm]

Looks to me like the build failure is unrelated to this change.

Actually it is related to the (large, possibly accidental) "package-lock.json" update in this PR. Currently it includes this diff hunk:

     "node_modules/@types/scheduler": {
-      "version": "0.16.8",
-      "resolved": "https://registry.npmjs.org/@types/scheduler/-/scheduler-0.16.8.tgz",
-      "integrity": "sha512-WZLiwShhwLRmeV6zH+GkbOFT6Z6VklCItrDioxUnv+u4Ll+8vKeFySoFyK/0ctcRpOmwAicELfmys1sDc/Rw+A==",
+      "version": "0.23.0",
+      "resolved": "https://registry.npmjs.org/@types/scheduler/-/scheduler-0.23.0.tgz",
+      "integrity": "sha512-YIoDCTH3Af6XM5VuwGG/QL/CJqga1Zm3NkU3HZ4ZHK2fRMPYP1VczsTUqtsf43PH/iJNVlPHAo2oWX7BSdB2Hw==",
       "dev": true
     },

That is a transitive dep from here:

% npm ls @types/scheduler
[email protected] /Users/trentm/tmp/opentelemetry-js-contrib
└─┬ @opentelemetry/[email protected] -> ./plugins/web/opentelemetry-plugin-react-load
  └─┬ @types/[email protected]
    └── @types/[email protected]

On main this is currently:

[email protected] /Users/trentm/src/opentelemetry-js-contrib
└─┬ @opentelemetry/[email protected] -> ./plugins/web/opentelemetry-plugin-react-load
  └─┬ @types/[email protected]
    └── @types/[email protected]

@types/[email protected] has a wildcard dep on @types/scheduler: "@types/scheduler": "*",.

So, something about the steps of npm ... commands you took in your working copy resulted in @types/scheduler being updated to version 0.23.0. This PR to @types/scheduler -- DefinitelyTyped/DefinitelyTyped#65812 -- broke @types/[email protected] when it dropped some types that were being used and the * dependency. There is a discussion of that in the DefinitelyTyped PR with the suggestion to move to latest @types/react@^18.

Options

1. Updating to @types/react@18 isn't an option for us because, IIUC, ./plugins/web/opentelemetry-plugin-react-load does not support React 18.
2. I notice that the latest @types/react@17.* does seem to have a fix for this: @types/[email protected] now has this dep: "@types/scheduler": "^0.16". (Aside: I'd point to the code for this, but I do not currently follow how DefinitelyTyped.git does things. No branches, no tags.)
3. I have a change that reduces the package-lock.json diff for this PR down to approximately 200 lines, instead of 23k lines. The diff (compared to "main") is: https://gist.github.com/trentm/9c9c80bd6915e0a33747e09dfd1bdae1

I think option 3 is the most appropriate for this PR. I'll attempt to push that to this feature branch.

[trentm]

Aside: package-lock.json maint/updates are a royal PITA.

[martinkuba]

@trentm Thanks for looking into this. I was just working on it myself and was coming to the same conclusion. I think what happened is the PR had a merge conflict in the package-lock.json file, and I tried resolving it by deleting it and doing npm install.

[trentm]

I tried resolving it by deleting it and doing npm install.

For the record this is what I did in the working copy:

cd ~/tm/opentelemetry-js-contrib8  # my working copy
cp ~/src/opentelemetry-js-contrib/package-lock.json .   # copy in the package-lock file from "main"
npm install

And, IIRC, this generates a much smaller change than doing: rm package-lock.json && npm install.

[trentm]

LGTM

[JamieDanielson]

Since our function is getPropagator(single) and it returns a CompositePropagator or a single propagator, I'm wondering if the package should actually be called auto-configuration-propagator (singular instead of plural)? I also thought about going the other way with this being getPropagators to match getResourceDetectors....

I don't feel strongly about this, just an observation on the second look. Otherwise though this looks great, thanks for working on this.

[trentm]

On the plural/singular:

• The NodeSDK API takes a singular textMapPropagator, and plural resouceDetectors, so I think it "fine" that the exported API here is getPropagator. Perhaps a note in the comment for that function and/or a not in a reference section in the README could clarify that a composite propagator is used to merge multiple independent propagators, but that could be done in a separate PR.
• The "users" of this package are using it to handle the plural OTEL_PROPAGATORS envvar, so I think it is "fine" that the package name is plural.

There is no "all singular" or "all plural" naming option here, so ... I think it is fine as is. And that we should merge and march on. :)

(I wonder if this package will be somewhat short-lived, if a more general auto-configuration handling package is written that handles file-config and/or other OTEL_* SDK envvars.)

[JamieDanielson]

There is no "all singular" or "all plural" naming option here, so ... I think it is fine as is. And that we should merge and march on. :)

Agreed! 🚀 Naming is hard.