feat: support node 20

[dyladan]

22 not supported because of a bad engine failure on npm ci

[dyladan]

22 fails with bad engine error on a dependency

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.37%. Comparing base (dfb2dff) to head (a39e225). Report is 121 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2169      +/-   ##
==========================================
- Coverage   90.97%   90.37%   -0.61%     
==========================================
  Files         146      147       +1     
  Lines        7492     7500       +8     
  Branches     1502     1569      +67     
==========================================
- Hits         6816     6778      -38     
- Misses        676      722      +46     

see 42 files with indirect coverage changes

[blumamir]

added lables for all the instrumentation to verify test-all-versions pass with new node version

[blumamir]

I think we should also add node 20 here

[trentm]

See also:

• https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1872
• https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2021

[pichlermarc]

Hmm, I'm surprised this works. :thinking: Does anyone know why we're not running into the original problem what we had with import-in-the-middle on Node.js 18.19+? #1872

If I run koa tests on Node.js 20 locally they fail.

[pichlermarc]

Aaah, I see... The tests are only running the changed packages (which is nothing as only the workflow changed). Tests here are unfortunately not an indicator that it works. We should add some logic to the workflow that it runs the full tests if a workflow file changes.

My guess is we'll see failures when we add Node.js 20 to the TAV workflow

[trentm]

My guess is we'll see failures when we add Node.js 20 to the TAV workflow

Indeed:

For example:

  1) ioredis
       should work with ESM usage:
     Uncaught AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Command failed: /opt/hostedtoolcache/node/20.12.2/x64/bin/node fixtures/use-ioredis.mjs redis://localhost:6379
file:///home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-ioredis/test/fixtures/use-ioredis.mjs:23
import { IORedisInstrumentation } from '../../build/src/index.js';
         ^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: The requested module '../../build/src/index.js' does not provide an export named 'IORedisInstrumentation'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:134:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:217:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12)

Node.js v20.12.2

[trentm]

22 fails with bad engine error on a dependency

Was that the failure to build the 'grpc' dependency? It was for me, at least. This gist shows the npm ci failure for me using Node.js v22.0.0: https://gist.github.com/trentm/ebd332620e3cb3018b9ee79b745af174

This is a dep of this one package:

% npm ls grpc
[email protected] /Users/trentm/src/opentelemetry-js-contrib
└─┬ @opentelemetry/[email protected] -> ./propagators/opentelemetry-propagator-grpc-census-binary
  └── [email protected]

Some notes on the propagator-grpc-census-binary package (it was before my time, so I'm ignorant):

• It has no owner.
• It hasn't had a change (other than meta things like updating OTel core APIs and dep versions) since its original addition 4y ago.
• It dep, grpc, is deprecated and only supports Node.js <=14 (per https://github.com/grpc/grpc-node/blob/master/PACKAGE-COMPARISON.md). Also from the README: "It works on versions of Node.js up to 14 on most platforms that Node.js runs on."

grpc is a binary Node.js package that uses node-pre-gyp install --fallback-to-build --library=static_library to build/install. In the "happy" path, it downloads a prebuilt binary from https://node-precompiled-binaries.grpc.io/src/node/extension_binary/{node_abi}-{platform}-{arch}-{libc} (an HTTP frontend to a Google Compute Storage bucket). Otherwise if falls back to attempting to build from source, because of the --fallback-to-build option.

For example attempting npm install grpc using Node.js 16 on macOS/arm64 fails to get a prebuilt binary with:

npm ERR! node-pre-gyp ERR! install response status 404 Not Found on https://node-precompiled-binaries.grpc.io/grpc/v1.24.11/node-v93-darwin-arm64-unknown.tar.gz

It then fails to build from source (at least on my machine) due to now out-of-date assumptions about Python versions.

On combinations of Node.js version and platform where it does succeed in building, it takes a LONG time. I've developed the habit of always installing opentelemetry-js-contrib with npm ci --ignore-scripts to skip this install-time penalty.

Basically, I'm saying that the grpc transitive dep is a problem for the opentelemetry-js-contrib repo already for some existing supported Node.js versions and platforms (OS and arch).

Would it be possible to consider moving the propagator-grpc-census-binary out of the workspace? E.g. move it to the "archive/..." dir and have it not be part of the usual build? Effectively that means EOL'ing it. I'm not sure if that is problematic. If we stopped publishing new versions of propagator-grpc-census-binary, I wonder if users would be fine. It has no deps.

An alternative might be to not install the grpc devDep -- it is only used in a few tests -- and have the tests skip if grpc is not installed. (Then we could consider a "pretest" script in package.json that installs grpc, but only on Node.js v14 or somethign.) Thoughts?

(I can move this to a separate issue if you feel that would be better.)

[pichlermarc]

Would it be possible to consider moving the propagator-grpc-census-binary out of the workspace? E.g. move it to the "archive/..." dir and have it not be part of the usual build? Effectively that means EOL'ing it. I'm not sure if that is problematic. If we stopped publishing new versions of propagator-grpc-census-binary, I wonder if users would be fine. It has no deps.

An alternative might be to not install the grpc devDep -- it is only used in a few tests -- and have the tests skip if grpc is not installed. (Then we could consider a "pretest" script in package.json that installs grpc, but only on Node.js v14 or somethign.) Thoughts?

Let's move it to the archive/ and unlink it. The spec says that we MAY maintain a propagator like this, so we don't have to keep maintaining it. I think with the grpc package being EOL stopping the development of the propagator that goes with it should be fairly uncontroversial.

[pichlermarc]

Would it be possible to consider moving the propagator-grpc-census-binary out of the workspace? E.g. move it to the "archive/..." dir and have it not be part of the usual build? Effectively that means EOL'ing it. I'm not sure if that is problematic. If we stopped publishing new versions of propagator-grpc-census-binary, I wonder if users would be fine. It has no deps. An alternative might be to not install the grpc devDep -- it is only used in a few tests -- and have the tests skip if grpc is not installed. (Then we could consider a "pretest" script in package.json that installs grpc, but only on Node.js v14 or somethign.) Thoughts?

Let's move it to the archive/ and unlink it. The spec says that we MAY maintain a propagator like this, so we don't have to keep maintaining it. I think with the grpc package being EOL stopping the development of the propagator that goes with it should be fairly uncontroversial.

I opened a PR #2201 to take care of this. @dyladan once #2201 is merged a rebased version of this PR should pass tests, we've updated the transitive IITM dependency via #2180