Add Node.js support for --import flag

[raphael-theriault-swi]

Description:

This adds a new useImport option for Node.js autoinstrumentation that injects and --import flag to NODE_OPTIONS instead of the default --require flag. The behaviour with the provided image is the same as what --require does with the added feature of also registering hooks to instrument ESM code.

This also opens the door for custom images to run asynchronous code in their instrumentation code, which isn't possible with --require.

Link to tracking Issue(s):

• Resolves: Support --import flag for Node.js instrumentation #3414

Testing:

Added both unit tests and a new e2e test that exercise the feature.

Documentation:

The new option is documented and was added to the generated docs.

[swiatekm]

This looks good to me overall, but I'd feel more confident if one of @open-telemetry/javascript-maintainers could have a look at the JS specific parts.

[trentm]

One subtlety that might impact the choice of the config var name.
The feature for the user is whether a module import hook is added (via module.register()) to support instrumentating ESM code.

That is actually independent of whether --import something.mjs or --require something.js is used to run the OTel setup when Node.js is starting up. It is definitely possible to call module.register("@opentelemetry/instrumentation/hooks.mjs"); from the CommonJS autoinstrumentation.js (loaded via --require).

So I wonder if a config name something like instrumentEsm or something like that would be clearer.

---

There are a few subtleties discussed at open-telemetry/opentelemetry-js#4933, but I don't think you need to read and grok all that. Eventually we'd like it so that there is just the one obvious way to setup OTel -- and that would be via --import some-esm-file-that-calls-module.register.mjs. However, for now we need to make the ESM support opt-in because:

• it requires newer versions of node
• it is possible the user explicitly does not want ESM instrumentation enabled because it is still new and can have overhead and bugs

[raphael-theriault-swi]

I think instrumentEsm here would be a bit of a misnomer, given that functionality could be added without an import flag, and a custom image could also require the use of an import flag for top level await support (which is actually our usecase for this addition) without providing ESM instrumentation

[trentm]

a custom image could also require the use of an import flag for top level await support (which is actually our usecase for this addition)

Ah, I misunderstood the use case then. Using import in the var name makes sense then. The docs for this feature should mention the two things then: (1) --import ... is used to load "autoinstrumentation.mjs" (so custom versions can use ESM and top-level await), (2) the default "autoinstrumentation.mjs" differs from "autoinstrumentation.js" in that it registers the hook for ESM instrumentation.

[trentm]

The important Node.js feature here is whether module.register() is supported and that version range is ^18.19.0 || ^20.6.0 || >=22. It would be good to be specific here, so some user of an earlier minor version of v18 or v20 doesn't expect this to work.

[raphael-theriault-swi]

Good point

[trentm]

The current code will crash if used on a verison of node that doesn't yet have module.register:

% cat foo.mjs
import { register } from "node:module";
console.log('reg', register);

% /Users/trentm/.nvm/versions/node/v18.0.0/bin/node foo.mjs
file:///Users/trentm/.../foo.mjs:1
import { register } from "node:module";
         ^^^^^^^^
SyntaxError: The requested module 'node:module' does not provide an export named 'register'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:128:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:194:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:409:24)
    at async loadESM (node:internal/process/esm_loader:85:5)
    at async handleMainPromise (node:internal/modules/run_main:61:12)

Node.js v18.0.0

An option might be to gracefully warn (untested code):

Suggested change

	import { register } from "node:module";
	register("@opentelemetry/instrumentation/hooks.mjs");
	import { diag } from '@opentelemetry/api';
	import * as module from 'node:module';
	if (typeof module.register === 'function') {
	module.register('@opentelemetry/instrumentation/hooks.mjs');
	} else {
	diag.warn(`OpenTelemetry Operator auto-instrumentation could not instrument ESM code: Node.js ${process.version} does not support 'module.register()'`);
	}

[raphael-theriault-swi]

Does the SDK setup a logger for the diag API by default ? If not I'm thinking this should probably be a console.warn

[trentm]

The NodeSDK sets a DiagConsoleLogger if the OTEL_LOG_LEVEL env var is set. (Per open-telemetry/opentelemetry-js#3693 it does not set a diag logger at all if the env var isn't set at all. So by default logging is effectively off.) node -r @opentelemetry/auto-instrumentations-node/register ... does turn it on by default (https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/metapackages/auto-instrumentations-node/src/register.ts#L23-L26).

Using the diag logger is the intended mechanism for logging by the OTel JS packages, but that doesn't necessarily have to mean that the OTel Operator uses it. I don't have a strong opinion whether the diag logger or console.* is used here.

[raphael-theriault-swi]

@trentm Thoughts on the updated wording ?

[trentm]

I think the JS parts of this look good now. (I haven't reviewed the test code at all.)

[swiatekm]

@raphael-theriault-swi could you regenerate the manifests? You have some conflicts there.

[swiatekm]

@raphael-theriault-swi looks like you have a slightly malformed changelog entry. Can you have a look? It's complaining about tabs vs spaces.

[raphael-theriault-swi]

This should do it

[swiatekm]

@raphael-theriault-swi it looks like the new e2e test you added is consistently failing. Can you have a look?

My suspicion is that it's because we don't actually build the autoinstrumentation images for e2e tests, and your new test depends on the image having the mjs file.

[raphael-theriault-swi]

Yeah that was also my assumption, I'm not sure what could be done on my side to fix that as I'm far from familiar with the test machinery.

[pavolloffay]

@raphael-theriault-swi how does the --import flag with with the upstream opentelemetry/auto-instrumentations-node ? We are trying to minimize the nodejs code and rely only on that package.

Related ticket: #3465

[pavolloffay]

@trentm could you please take a look here? cc) @JamieDanielson

[trentm]

@pavolloffay Are you asking if node --import @opentelemetry/auto-instrumentations-node/register ... works? If so, I think yes it should work fine because node --import something ... works with something being either ESM or CommonJS code. I haven't tried it, though.

However, whether to move the operator to using @opentelemetry/auto-instrumentations-node instead of the bootstrap code in "autoinstrumentation.js" doesn't need to be part of this PR, right?

[raphael-theriault-swi]

I would also add that the instrumentation.js file needs to exist. Adding a --require @opentelemetry/auto-instrumentations-node/register option alone will not work, since this requires the package to be present in the node_modules search path of the application itself, which would require the application to have the package installed and defeat the entire purpose of the operator autoinstrumentation.

The contents of the file could be replaced by require("@opentelemetry/auto-instrumentations-node/register") instead, which would not conflict with this PR whatsoever.

[trentm]

I think this documentation should be expanded a bit. It is relevant that useImport: true also changes to using the autoinstrumentation.mjs file (rather than autoinstrumentation.js). Anyone providing a custom image for nodejs injection should be aware of that.