Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
4 of 5 tasks
pichlermarc opened this issue Mar 18, 2024 · 9 comments · Fixed by #4684
Closed
4 of 5 tasks

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

pichlermarc opened this issue Mar 18, 2024 · 9 comments · Fixed by #4684
Labels
needs:code-contribution This feature/bug is ready to implement pkg:instrumentation type:feature-tracking A feature with sub-issues that need to be addressed

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Mar 18, 2024

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 [email protected] here as that version includes a bug (DataDog/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:

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
Copy link
Member Author

pichlermarc commented Apr 30, 2024

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

@okko
Copy link

okko commented May 6, 2024

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
Copy link
Member Author

@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
Copy link

gajus commented May 10, 2024

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

@trentm
Copy link
Contributor

trentm commented May 13, 2024

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
Copy link

gajus commented May 13, 2024

@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
Copy link
Contributor

trentm commented May 13, 2024

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
Copy link
Contributor

trentm commented May 13, 2024

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
Copy link

gajus commented May 13, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-contribution This feature/bug is ready to implement pkg:instrumentation type:feature-tracking A feature with sub-issues that need to be addressed
Projects
None yet
4 participants