feat(instrumentation): support ESM in node via import-in-the-middle

[bengl]

Which problem is this PR solving?

Node.js ESM support.

Short description of the changes

Adds import-in-the-middle alongside require-in-the-middle. Also avoids hanging on to the hooks, because they're unnecessary (not used in disable).

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

The existing tests pass. The part of code that uses require-in-the-middle does not appear to be tested by the test files in the package. Are there other tests elsewhere that cover it? If so, happy to add appropriate tests there. I'll call this a draft until there's something better here.

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[lizthegrey]

this should be IITM not RITM right?

[bengl]

Whoops! Fixing this now.

[bengl]

Fixed, I think. 😅

[lizthegrey]

To answer the question of tests: I know there's full integration tests in https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-pg/test/pg.test.ts#L223 etc. -- that verify spans are created when instrumentation is hooked, you'd just need to run under ESM in order to force the import to happen with ESM rather than it being converted to a require.

Likewise https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/http.ts#L86 is exercised by https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/test/functionals/https-package.test.ts

[vmarchaud]

i think we can remove this altogether and only use _enabled ?

[vmarchaud]

I've launched the tests suite for the PR and it seems there are issue with our typescript config, do you think you could look into it ?

[codecov]

Codecov Report

Merging #2640 (14f5b90) into main (5e9c7e1) will increase coverage by 0.91%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2640      +/-   ##
==========================================
+ Coverage   93.10%   94.02%   +0.91%     
==========================================
  Files         123       76      -47     
  Lines        4758     2409    -2349     
  Branches     1061      549     -512     
==========================================
- Hits         4430     2265    -2165     
+ Misses        328      144     -184     

Impacted Files	Coverage Δ
...pentelemetry-exporter-trace-otlp-http/src/types.ts
...ry-instrumentation-grpc/src/grpc-js/clientUtils.ts
...-instrumentation-fetch/src/enums/AttributeNames.ts
...exporter-metrics-otlp-http/src/transformMetrics.ts
...elemetry-instrumentation-grpc/src/grpc-js/index.ts
...emetry-instrumentation-xml-http-request/src/xhr.ts
...es/opentelemetry-instrumentation-http/src/utils.ts
...mentation-xml-http-request/src/enums/EventNames.ts
...etry-exporter-prometheus/src/PrometheusExporter.ts
...ry-sdk-metrics-base/src/view/InstrumentSelector.ts
... and 37 more

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[lizthegrey]

Please keep this open, I think this feature should be still considered for adoption.

[lizthegrey]

I've rebased this as #2763

[itssumitrai]

Any Update on this ? we would also like to have ESM support

[Kampe]

Would LOVE to see this get pulled in, this is massive!

[pichlermarc]

Completed in #3698, huge thanks to everyone involved 🙂