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

[lizthegrey]

Revised version of #2640 for review now that types/tests pass. Originally authored by @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.

Checklist:

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

[codecov]

Codecov Report

Merging #2763 (548bc65) into main (0213d82) will increase coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2763      +/-   ##
==========================================
+ Coverage   93.25%   93.40%   +0.15%     
==========================================
  Files         152      164      +12     
  Lines        4904     5580     +676     
  Branches     1039     1176     +137     
==========================================
+ Hits         4573     5212     +639     
- Misses        331      368      +37     

Impacted Files	Coverage Δ
...strumentation/src/platform/node/instrumentation.ts	59.25% <0.00%> (-1.00%)	⬇️
packages/exporter-trace-otlp-http/src/util.ts	100.00% <0.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	96.53% <0.00%> (ø)
...ation-xml-http-request/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...metrics-otlp-http/src/OTLPMetricExporterOptions.ts	100.00% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts	97.63% <0.00%> (ø)
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts	57.14% <0.00%> (ø)
...s/exporter-trace-otlp-http/src/OTLPExporterBase.ts	100.00% <0.00%> (ø)
...mentation-xml-http-request/src/enums/EventNames.ts	100.00% <0.00%> (ø)
... and 3 more

[lizthegrey]

Oh, I see the problem, Typescript expects:

declare class Hook {
  constructor (modules: Array<string>, options: Options, hookFn: HookFn)

but the javascript defines it as

function Hook(modules, options, hookFn) {
  if ((this instanceof Hook) === false) return new Hook(modules, options, hookFn)

so TS complains if it's called as IncludeInTheMiddle() but JS complains if it's called as new IncludeInTheMiddle(). Raagh.

Oh, I see, but it's... sometimes a constructor! and sometimes an object, depending upon how it's called? hilarious.

functionality is copied from https://github.com/elastic/require-in-the-middle/pull/12/files but I have no idea why they did it that way.

[pretzelmaker]

I appreciate all of the work done to get to this point. Is this something that will be supported in the near term?

[JamieDanielson]

It seems the ideal solution is to enable esModuleInterop in tsconfig. This would require some changes throughout the codebase for many imports, e.g. no longer doing import * as foo from 'foo';. The error we're getting here is TypeError: import_in_the_middle_1.default is not a constructor. The missing esModuleInterop setting is causing the mismatch issue after transpiling to JavaScript. Without esModuleInterop, the compile step results in import_in_the_middle_1.default.

It looks like the esModuleInterop setting was shot down with a concern about it breaking downstream customers unless they also enable this, though it seems the only customers that would be affected are those that export or reference types that come from non-esm friendly packages with d.ts files, that are not already using this setting. Either way, it seems likely that this setting will be needed at some point. It's a default setting when initializing tsconfig, and included as a recommended base config for most use cases.

As it's set up today though, without enabling esModuleInterop, it looks like there is still a workaround. You can do something like PR 2846 that is aiming to achieve the same thing:

// current import:
import ImportInTheMiddle, { HookFn } from 'import-in-the-middle';

// proposed change:
import { HookFn } from 'import-in-the-middle';
import * as ImportInTheMiddle from 'import-in-the-middle';

...

// current usage:
 new ImportInTheMiddle([module.name], { internals: true }, <HookFn>hookFn);

// proposed change:
new (ImportInTheMiddle as unknown as typeof ImportInTheMiddle.default) ([module.name], { internals: true }, <HookFn>hookFn);

Build output in /experimental/packages/opentelemetry-instrumentation/build/src/platform/node/instrumentation.js:

// current, doesn't work
new import_in_the_middle_1.default([module.name], { internals: true }, hookFn);

// new, after change
new ImportInTheMiddle([module.name], { internals: true }, hookFn);

[lizthegrey]

Thanks for the help, @JamieDanielson. @vmarchaud, given tests are now passing over here, shall we proceed with officially doing review over here?

[vmarchaud]

I don't know about other but i think we need to have a test in place to be sure that works (thats something i've looked into for my PR over the last couple weekends)

[dyladan]

It seems the ideal solution is to enable esModuleInterop in tsconfig.
It looks like the esModuleInterop setting was shot down with a concern about it breaking downstream customers unless they also enable this

Yeah this is exactly what happened.

Either way, it seems likely that this setting will be needed at some point. It's a default setting when initializing tsconfig, and included as a recommended base config for most use cases.

Very interesting, I hadn't realized that.

In any case, ESM support is something we will definitely need at some point. I agree with @vmarchaud that we need some testing to ensure that we're useable both by ESM and by require users. I'm not an ESM export and @vmarchaud has been doing much more work than I have on that front so I'll let him continue to lead on this topic.

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

[pichlermarc]

Completed in #3698, huge thanks to everyone involved 🙂