[instrumentation] support ESM instrumentation for Node.js v18.19, v20, v21

[pichlermarc]

Currently we use an older version of import-in-the-middle that does not work with

• Node.js v18.19+
• Node.js v20+
• Node.js v21+

We're blocked on updating to import-in-the-middle@1.7.3 here as that version includes a bug (nodejs/import-in-the-middle#57) which breaks our way of detecting ESM packages. We do this ESM detection in multiple packages in contrib (see open-telemetry/opentelemetry-js-contrib#2021).

To resolve this issue there are two options:

• Option 1 (preferred): contribute a fix for @@toStringTag property not present on modules passed to hookFn  nodejs/import-in-the-middle#57 upstream
  • contribute a fix (thanks @trentm 🎉 - fix: ensure the hooked module exports has @@toStringTag property nodejs/import-in-the-middle#66)
  • wait for the release of import-in-the-middle
• Option 2: implement a workaround in the contrib repo (going for option 1)
  • see [all] support ESM instrumentation for Node.js v18.19, v20, v21 opentelemetry-js-contrib#2021

Regardless of the option chosen above, we then need to:

• bump the import-in-the-middle version in core
• remove import-in-the-middle from the ignore list in renovate.json
• release a new version of @opentelemetry/instrumentation (done by a maintainer)

[pichlermarc]

Will release @opentelemetry/instrumentation with an updated version early next week 🙂
Release PR to follow on Friday.

[okko]

I found the release PR at #4677, installed it from there and built it with "npm run build". Then I referred to them with yarn "resolutions" in my project's package.json to give it a test run.

The built code in build/esm gives this warning with esbuild:

▲ [WARNING] Constructing "ImportInTheMiddle" will crash at run-time because it's an import namespace object, not a constructor [call-import-namespace]

    node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js:287:30:
      287 │             var esmHook = new ImportInTheMiddle([module_2.name], { internals: false }, hookFn);
          ╵                               ~~~~~~~~~~~~~~~~~

  Consider changing "ImportInTheMiddle" to a default import instead:

    node_modules/@opentelemetry/instrumentation/build/esm/platform/node/instrumentation.js:48:7:
      48 │ import * as ImportInTheMiddle from 'import-in-the-middle';
         │        ~~~~~~~~~~~~~~~~~~~~~~
         ╵        ImportInTheMiddle 

There is a related open issue at #3954 when consuming the CJS version, but afaik here we're consuming the ESM version. Is this expected?

[pichlermarc]

@okko this is a different issue affecting esbuild and bundlers for Node.js apps, unrelated to what's being tracked here. It's somewhat related to what's being discussed over at #3954. Please open a new issue so we can triage it properly.

[gajus]

What's the best place to track work towards full ESM support?

[trentm]

What's the best place to track work towards full ESM support?

@gajus I think perhaps open-telemetry/opentelemetry-js-contrib#1942 for now.
Not much has been done on that yet because we've sort of been waiting for the import-in-the-middle (IITM) issue here to be resolved. Once there is a new release of @opentelemetry/instrumentation with that IITM fix, then it is (more) practical for us to start working through the instrumentations fixing/testing ESM support.

[gajus]

@gajus I think perhaps open-telemetry/opentelemetry-js-contrib#1942 for now.
Not much has been done on that yet because we've sort of been waiting for the import-in-the-middle (IITM) issue here to be resolved. Once there is a new release of @opentelemetry/instrumentation with that IITM fix, then it is (more) practical for us to start working through the instrumentations fixing/testing ESM support.

Does this mean that import-in-the-middle is seen as the long term solution?

[trentm]

Does this mean that import-in-the-middle is seen as the long term solution?

For hooking import ... to do automatic instrumentation? Yes, definitely. I'm not aware of an alternative.

(One current exception is the use of diagnostic_channels for instrumentation the undici package. However, that depends on the package being instrumentation supporting sufficient hooks, so it is not a general solution. Another possibility is for packages adding "native" OTel instrumentation themselves -- i.e. they take a dep on @opentelemetry/api and create spans as appropriate for their internal operation. I'm not sure many/all popular packages will go this route -- it is up to the maintainers of those packages. One example is Next.js. I believe the Azure JS Client libraries also include some native instrumentation.)

[trentm]

Once there is a new release of @opentelemetry/instrumentation with that IITM fix

I was mistaken: There is a release of @opentelemetry/instrumentation with the import-in-the-middle fix: v0.51.1. I was confused because the tags for that release are not on the "main" git branch.

[gajus]

they take a dep on @opentelemetry/api and create spans as appropriate for their internal operation. I'm not sure many/all popular packages will go this route -- it is up to the maintainers of those packages. One example is Next.js. I believe the Azure JS Client libraries also include some native instrumentation.

Regarding this point, I think this would accelerate a lot more with more end to end documentation.

A while back, I tried adding native instrumentation to https://github.com/gajus/slonik, but was confused by the lack of end to end examples of how to instrument libraries (as opposed to apps).