[next] Deprecate old runtimes #4295

dyladan · 2023-11-15T16:14:51Z

codecov · 2023-11-15T16:17:46Z

Codecov Report

Merging #4295 (543f0b4) into next (401062e) will decrease coverage by 0.07%.
Report is 1 commits behind head on next.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #4295      +/-   ##
==========================================
- Coverage   92.30%   92.24%   -0.07%     
==========================================
  Files         330      332       +2     
  Lines        9424     9437      +13     
  Branches     1998     1999       +1     
==========================================
+ Hits         8699     8705       +6     
- Misses        725      732       +7

Files	Coverage Δ
...entelemetry-instrumentation/src/instrumentation.ts	`80.00% <100.00%> (ø)`
...ntation/src/instrumentationNodeModuleDefinition.ts	`14.28% <ø> (ø)`
...strumentation/src/instrumentationNodeModuleFile.ts	`33.33% <ø> (ø)`

... and 2 files with indirect coverage changes

dyladan · 2023-11-15T16:30:55Z

Test failures on node 20:

Instrumentation `test:esm` - fixed

Fixed. See explanation

> @opentelemetry/instrumentation@0.45.1 test:esm
> nyc node --experimental-loader=./hook.mjs ../../../node_modules/mocha/bin/mocha 'test/node/*.test.mjs' test/node/*.test.mjs

(node:6135) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
--import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("./hook.mjs", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)

file:///home/runner/work/opentelemetry-js/opentelemetry-js/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs:19
  InstrumentationBase,
  ^^^^^^^^^^^^^^^^^^^
SyntaxError: The requested module '../../build/src/index.js' does not provide an export named 'InstrumentationBase'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:131:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:213:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async importModuleDynamicallyWrapper (node:internal/vm/module:431:15)
    at async formattedImport (/home/runner/work/opentelemetry-js/opentelemetry-js/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
    at async exports.loadFilesAsync (/home/runner/work/opentelemetry-js/opentelemetry-js/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
    at async singleRun (/home/runner/work/opentelemetry-js/opentelemetry-js/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at async exports.handler (/home/runner/work/opentelemetry-js/opentelemetry-js/node_modules/mocha/lib/cli/run.js:370:5)

Prometheus Exporter fixed by using `err.code`

Error fixed. See details

  1) PrometheusExporter
       export
         should export multiple attributes on manual shutdown:
     Uncaught AssertionError [ERR_ASSERTION]: null == true
      at ClientRequest.<anonymous> (test/PrometheusExporter.test.ts:364:13)
      at ClientRequest.emit (node:events:514:28)
      at Socket.socketErrorListener (node:_http_client:495:9)
      at Socket.emit (node:events:514:28)
      at emitErrorNT (node:internal/streams/destroy:151:8)
      at emitErrorCloseNT (node:internal/streams/destroy:116:3)
      at processTicksAndRejections (node:internal/process/task_queues:82:21)

dyladan · 2023-11-15T20:37:18Z

@legendecas I was looking at the prometheus test failure and the test doesn't seem to do what it says. it looks like you modified the test in https://github.com/open-telemetry/opentelemetry-js/pull/2563/files#diff-7ae372e0e2c48cb428c513beb5afdcbf243d7221760f05c67a06a060e12b6f47R368 What is this test actually testing now? Should it be renamed, fixed, or removed?

dyladan · 2023-11-15T21:01:53Z

Looks like now we're running into microsoft/TypeScript#26362

dyladan · 2023-11-15T21:19:06Z

It seems like the loader API changed somewhat drastically in 20. They want us to now use --import to import a file that uses the register() method of the node:module module instead of --experimental-loaders. Unfortunately, this was only introduced in 20, so there isn't a solution that is backwards compatible with node 18.

edit: seems this is not actually what caused the issue. using --import and register() as described above results in the same error.

dyladan · 2023-11-15T21:25:41Z

@Qard I see you were active on that loaders issue. Do you know if that's what is affecting us here or have a suggestion for how we can resolve this?

Qard · 2023-11-15T21:34:08Z

Not sure what this issue is. Probably worth reporting on the IITM repo to get some attention on it.

trentm · 2023-11-15T21:49:11Z

@dyladan The @opentelemetry/instrumentation@0.45.1 test:esm test failure is hitting nodejs/import-in-the-middle#29

Node.js v20 included changes in the ESM loader so that a module is loaded in a separate thread. That impacted import-in-the-middle so that it had to manually parse ESM files itself to get a list of exports for hooking. That handling doesn't currently handle "re-exports", i.e.:

export * from './platform/index';

which is this in the compiled JS:

var __exportStar = (this && this.__exportStar) || function(m, exports) {
    for (var p in m) if (p !== "default" && !Object.prototype.hasOwnProperty.call(exports, p)) __createBinding(exports, m, p);
};
__exportStar(require("./platform/index"), exports);

works without re-exports

So if you import the deeper "./build/src/platform/node/instrumentation.js" which doesn't have any re-exports, then it works:

% cd opentelemetry-js/experimental/packages/opentelemetry-instrumentation
% cat foo1.mjs
import { InstrumentationBase } from './build/src/platform/node/instrumentation.js'; // works
console.log('XXX InstrumentationBase: ', InstrumentationBase);

% node foo1.mjs
XXX InstrumentationBase:  [class InstrumentationBase extends InstrumentationAbstract]

% NODE_NO_WARNINGS=1 node --experimental-loader=import-in-the-middle/hook.mjs foo1.mjs
XXX InstrumentationBase:  [class InstrumentationBase extends InstrumentationAbstract]

fails with re-exports

But if you use any of the index.js files that use re-exports:

% cat foo2.mjs
import { InstrumentationBase } from './build/src/index.js'; // fails with IITM due to https://github.com/DataDog/import-in-the-middle/issues/29
console.log('XXX InstrumentationBase: ', InstrumentationBase);

% node foo2.mjs
XXX InstrumentationBase:  [class InstrumentationBase extends InstrumentationAbstract]

% NODE_NO_WARNINGS=1 node --experimental-loader=import-in-the-middle/hook.mjs foo2.mjs
file:///Users/trentm/tm/opentelemetry-js3/experimental/packages/opentelemetry-instrumentation/foo2.mjs:1
import { InstrumentationBase } from './build/src/index.js'; // fails with IITM due to https://github.com/DataDog/import-in-the-middle/issues/29
         ^^^^^^^^^^^^^^^^^^^
SyntaxError: The requested module './build/src/index.js' does not provide an export named 'InstrumentationBase'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:131:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:213:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:66:12)

Node.js v20.9.0

a workaround

A workaround that allows at least the npm run test:esm to pass is to avoid named imports:

diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs
index f09097cd..0163a519 100644
--- a/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs
+++ b/experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs
@@ -15,10 +15,10 @@
  */

 import * as assert from 'assert';
-import {
-  InstrumentationBase,
-  InstrumentationNodeModuleDefinition,
-} from '../../build/src/index.js';
+// Avoid named imports to workaround https://github.com/DataDog/import-in-the-middle/issues/29
+import mod from '../../build/src/index.js';
+const InstrumentationBase = mod.InstrumentationBase;
+const InstrumentationNodeModuleDefinition = mod.InstrumentationNodeModuleDefinition
 import * as exported from 'test-esm-module';

 class TestInstrumentationWrapFn extends InstrumentationBase {

Note that ESM support, in general, is still going to be busted for any cases where user code (including their dependencies) uses named ESM imports that depend on re-exports.

trentm · 2023-11-15T21:51:11Z

There is a PR open for fixing this IITM issue. It has perhaps languished. I haven't had a chance to look at it yet.

(I also indirectly noticed that this "import off-thread" handling in Node v20 is being considered for backport to Node v18 in nodejs/node#50669. So there may come a v18 minor release that breaks similarly.)

experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs

dyladan · 2023-11-17T13:37:36Z

This is now ready for review. I've decided to disable node 20 testing because larger changes will need to be made in a separate PR to fix the http instrumentation tests

.github/workflows/benchmark.yml

.github/workflows/unit-test.yml

trentm · 2023-11-17T18:42:30Z

Also need a npm run lint:fix.

dyladan · 2023-11-17T19:26:47Z

@trentm the browser tests actually require node 16 runtime in order to use the version of webpack we have installed. That will have to be done separately.

old webpack version requires old node version. old node version prevents webpack upgrades 🔁

pichlermarc · 2023-11-20T09:48:56Z

@trentm the browser tests actually require node 16 runtime in order to use the version of webpack we have installed. That will have to be done separately.

old webpack version requires old node version. old node version prevents webpack upgrades 🔁

There's an interesting new PR over at the contrib repo (open-telemetry/opentelemetry-js-contrib#1816, I did not have time to review that one yet) that looks promising - looks like karma was deprecated back in April, which means we may be able to ditch it in favor of @web/test-runner, which in turn would mean we'd be able to drop webpack (@web/test-runner uses rollup/esbuild) as the only reason we use it is to run tests AFAIK.

dyladan · 2023-11-20T15:07:04Z

experimental/packages/opentelemetry-instrumentation/src/types.ts

@@ -77,3 +77,47 @@ export interface ShimWrapped extends Function {
  // eslint-disable-next-line @typescript-eslint/ban-types
  __original: Function;
 }
+
+export interface InstrumentationModuleFile<T> {


Moved out of the platform dir because there was no web-specific version

dyladan · 2023-11-20T15:07:10Z

experimental/packages/opentelemetry-instrumentation/src/types.ts

+  unpatch(moduleExports?: T, moduleVersion?: string): void;
+}
+
+export interface InstrumentationModuleDefinition<T> {


Moved out of the platform dir because there was no web-specific version

dyladan · 2023-11-20T15:10:32Z

experimental/packages/opentelemetry-instrumentation/src/instrumentationNodeModuleFile.ts

I'm thinking we might want to just remove this entirely? It doesn't seem to be used anywhere in the main repo. I haven't yet looked in contrib.

…etry-js into deprecate-old-runtimes

.github/workflows/benchmark.yml

experimental/backwards-compatibility/node14/package.json

experimental/backwards-compatibility/node16/package.json

pichlermarc

looks good, thanks for taking care of this 🙂

dyladan requested a review from a team November 15, 2023 16:14

dyladan force-pushed the deprecate-old-runtimes branch from d602748 to 7b32468 Compare November 15, 2023 16:17

dyladan force-pushed the deprecate-old-runtimes branch from efd20a7 to 98ff89e Compare November 15, 2023 21:04

dyladan force-pushed the deprecate-old-runtimes branch from 08570e8 to f6671a2 Compare November 16, 2023 13:55

dyladan commented Nov 16, 2023

View reviewed changes

experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs Outdated Show resolved Hide resolved

dyladan force-pushed the deprecate-old-runtimes branch 2 times, most recently from b8cd126 to e1f814f Compare November 16, 2023 19:39

dyladan marked this pull request as draft November 16, 2023 19:45

dyladan force-pushed the deprecate-old-runtimes branch from e1f814f to 837153a Compare November 16, 2023 19:48

dyladan marked this pull request as ready for review November 17, 2023 13:36

trentm approved these changes Nov 17, 2023

View reviewed changes

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved

.github/workflows/unit-test.yml Show resolved Hide resolved

dyladan added 5 commits November 17, 2023 14:29

[next] deprecate 14,16 and support 20

ab52a84

Fix prom test for node 20

9d62673

Name exports to fix ESM tests

7cf46c7

Temporarily disable node 20

d229b3a

lint

343ca78

dyladan force-pushed the deprecate-old-runtimes branch 2 times, most recently from d308ff9 to 343ca78 Compare November 17, 2023 20:06

dyladan force-pushed the deprecate-old-runtimes branch 3 times, most recently from aa142dd to 934cfa1 Compare November 20, 2023 14:53

Do not export node-specific names

bf30779

dyladan force-pushed the deprecate-old-runtimes branch from 934cfa1 to bf30779 Compare November 20, 2023 14:57

dyladan commented Nov 20, 2023

View reviewed changes

Move node module file to root

4cbb635

dyladan commented Nov 20, 2023

View reviewed changes

dyladan mentioned this pull request Nov 20, 2023

Consider removing NodeModuleFile #4312

Closed

dyladan added 4 commits November 20, 2023 10:20

Export node module file

21da78f

Merge branch 'next' into deprecate-old-runtimes

5be08f4

Bump to 18 on other workflows

dc10b81

Merge branch 'deprecate-old-runtimes' of github.com:dyladan/opentelem…

3b8f0af

…etry-js into deprecate-old-runtimes

pichlermarc reviewed Nov 22, 2023

View reviewed changes

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved

experimental/backwards-compatibility/node14/package.json Show resolved Hide resolved

experimental/backwards-compatibility/node16/package.json Show resolved Hide resolved

Benchmark a tested version

543f0b4

pichlermarc approved these changes Nov 22, 2023

View reviewed changes

dyladan merged commit 35be022 into open-telemetry:next Nov 22, 2023
17 checks passed

dyladan deleted the deprecate-old-runtimes branch November 22, 2023 15:39

dyladan mentioned this pull request Nov 30, 2023

Add node 20 to test matrix #4336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[next] Deprecate old runtimes #4295

[next] Deprecate old runtimes #4295

dyladan commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

dyladan commented Nov 15, 2023 •

edited

Loading

dyladan commented Nov 15, 2023

dyladan commented Nov 15, 2023

dyladan commented Nov 15, 2023 •

edited

Loading

dyladan commented Nov 15, 2023

Qard commented Nov 15, 2023

trentm commented Nov 15, 2023

trentm commented Nov 15, 2023

dyladan commented Nov 17, 2023

trentm commented Nov 17, 2023

dyladan commented Nov 17, 2023

pichlermarc commented Nov 20, 2023

dyladan Nov 20, 2023

dyladan Nov 20, 2023

dyladan Nov 20, 2023

pichlermarc left a comment

[next] Deprecate old runtimes #4295

[next] Deprecate old runtimes #4295

Conversation

dyladan commented Nov 15, 2023 • edited Loading

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

dyladan commented Nov 15, 2023 • edited Loading

Instrumentation test:esm - fixed

Prometheus Exporter fixed by using err.code

dyladan commented Nov 15, 2023

dyladan commented Nov 15, 2023

dyladan commented Nov 15, 2023 • edited Loading

dyladan commented Nov 15, 2023

Qard commented Nov 15, 2023

trentm commented Nov 15, 2023

works without re-exports

fails with re-exports

a workaround

trentm commented Nov 15, 2023

dyladan commented Nov 17, 2023

trentm commented Nov 17, 2023

dyladan commented Nov 17, 2023

pichlermarc commented Nov 20, 2023

dyladan Nov 20, 2023

Choose a reason for hiding this comment

dyladan Nov 20, 2023

Choose a reason for hiding this comment

dyladan Nov 20, 2023

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

dyladan commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

dyladan commented Nov 15, 2023 •

edited

Loading

Instrumentation `test:esm` - fixed

Prometheus Exporter fixed by using `err.code`

dyladan commented Nov 15, 2023 •

edited

Loading