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

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

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.

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.

Good point

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

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

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

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

@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?

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.

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?

The issue is that, if we keep adding new features here it will be harder to migrate them to auto-instrumentations-node (or other node packages)

Like I mentioned above switching to auto-instrumentations-node would be not require any change to the code added by this PR now or in the future.

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.

It's documented in the Dockerfile which is where the related info about autoinstrumentation.js was, I don't feel strongly about it either way but I wasn't sure whether documenting the internal filenames made sense in the user-facing documentation.