open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.74k stars 803 forks source link

use `module.register(...)` in recommended bootstrap code for ESM support #4933

Open trentm opened 2 months ago

trentm commented 2 months ago

The current advice for limited ESM support is to use --experimental-loader=.... Ultimately we should move towards recommending module.register(...) usage instead, because it is on the path to being non-experimental in Node.js core (https://nodejs.org/api/module.html#moduleregisterspecifier-parenturl-options).

To use this, bootstrap code will move to something like:

telemetry.mjs

import { register } from 'module';
import { Hook, createAddHookMessageChannel } from 'import-in-the-middle';

// Yes, createAddHookMessageChannel is new. See below.
const { registerOptions, waitForAllMessagesAcknowledged } = createAddHookMessageChannel();
register('import-in-the-middle/hook.mjs', import.meta.url, registerOptions);

// Init the SDK. This is all the same bootstrap code that we are showing in
// various examples.
// ...

await waitForAllMessagesAcknowledged();

And it would be run like one of the following.

# Some config common to all options.
export OTEL_EXPORTER_OTLP_ENDPOINT=https://{your-otlp-endpoint.example.com}
export OTEL_EXPORTER_OTLP_HEADERS="Authorization={authorization-information}"
export OTEL_SERVICE_NAME=my-service
# ...

# 1.
node --import=./telemetry.mjs app.js

# 2.
export NODE_OPTIONS='--import=./telemetry.mjs'
node app.js

# 3. Using ts-node to directly transpile and run. Note that, IIRC, ts-node
#    supports .ts files used for --import (and --require), which is another
#    opportunity for some user confusion. See note below.
ts-node --import=./telemetry.ts app.ts

# 4. Eventually some auto-instrumentations-node support for `module.register`.
node --import @opentelemetry/auto-instrumentations-node/register app.js

Notes:

Originally posted by @trentm in https://github.com/open-telemetry/opentelemetry-js/issues/4845#issuecomment-2253556217

trentm commented 2 months ago

Regarding usage with CommonJS (CJS) code

It would be nice to have one recommended way to bootstrap OTel, to eliminate user confusion.

Currently, setting up OTel when using ESM (often it isn't clear if a given user is using ESM or CJS) requires the extra --experimental-loader=... argument. That's (a) experimental, (b) long/labourious, and (c) requires a separate dependency on @opentelemetry/instrumentation. That means we aren't going to recommend it for all users.

Moving to module.register(...) usage in standard bootstrap boilerplate (or in @opentelemetry/auto-instrumentations-node/register) almost gets us to a single recommendation that we can make to all users. The main blocking limitation is that the full usage of import-in-the-middle's new createAddHookMessageChannel() -- which we definitely want to use -- uses top-level await (see the await waitForAllMessagesAcknowledged(); above). Implications of this:

So... until those are our base support Node.js versions, we will still have to talk about / document a bootstrap module that uses CJS and --require and doesn't support ESM.

github-actions[bot] commented 1 week ago

This issue 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.

acidoxee commented 1 week ago

Hi, since Node v22 is now the official LTS (well, it will be tomorrow), and since it now supports require(esm), might you consider switching to native ESM? I'm guessing it would be way easier for you to maintain and build/ship, and we could finally have a single way of bootstrapping, right?

jpmunz commented 3 days ago

Just noting a couple things based on having looked at https://github.com/open-telemetry/opentelemetry-js/issues/5097 for loading typescript files specifically:

trentm commented 3 days ago

@acidoxee Currently OTel JS supports back to node v14, so until the base supported Node.js version is v22 we cannot rely on require(esm) being supported. However, I'm not sure we need that -- I may be missing something. When we bump the min supported Node.js versions to ^18.19.0 || ^20.6.0 || >=22 then I think we'll be able to just recommend "the one way" of bootstrapping: use --import some-bootstrap-file-name.mjs which calls module.register().

@jpmunz I'm pretty ignorant to what typescript path aliases are for. Correct me if I'm wrong: supporting them in OTel bootstrap code is independent of whether we use module.register(".../hook.mjs") vs. --experimental-import .../hook.mjs, I think.

trentm commented 3 days ago

FYI: The autoinstrumentation.{js,mjs} file used by the OTel Operator might soon be using module.register() -- as an opt-in -- for ESM hooking support: https://github.com/open-telemetry/opentelemetry-operator/pull/3416

jpmunz commented 3 days ago

@trentm correct I was just mentioning in regards to some of the bullets related to .ts files, e.g.

The bootstrap file should be ESM: i.e. "telemetry.mjs" and probably not a ".ts" file which is dependent on tsconfig.json settings.

douglasg14b commented 2 days ago

This method works for me unlike the experimental loader step!

However, this does generate a few typescript errors:

image

image

// This isn't a function according to the types, just a promise
await waitForAllMessagesAcknowledged();

// Should be something like
await Promise.all([waitForAllMessagesAcknowledged]);
raphael-theriault-swi commented 2 days ago

Quick note re .ts file : If we start using .mjs in examples why not just also use .mts and avoid any confusion ? ts-node supports it by default on any Node.js version that supports register.

trentm commented 2 days ago

Yup using .mts would be a good idea to suggest to eliminate the ambiguity, especially if the recommended bootstrap code will be using top-level await.

zacharyblasczyk commented 1 day ago

Hope this is the right place,

I am facing a similar issue here with a next build and a instrumentation.ts, instrumentation-node.tspattern of instrumentation.

Eventually I landed on https://github.com/open-telemetry/opentelemetry-js/issues/4437 and https://github.com/open-telemetry/opentelemetry-js/blob/main/doc/esm-support.md#instrumentation-hook-required-for-esm

Is there any recommendation to get this to work with a next app building with "module": "esnext"?

trentm commented 1 day ago

@zacharyblasczyk Let's please need debugging Next.js instrumentation on a separate issue. While module.register(...) usage is related to getting ESM instrumentation working, its basic usage is functionally the same as using the --experimental-loader @opentelemetry/instrumentation/hook.mjs Node.js option that is currently recommended at https://github.com/open-telemetry/opentelemetry-js/blob/main/doc/esm-support.md#instrumentation-hook-required-for-esm

Issues with instrumenting Next.js servers could also be unrelated to the ESM hook.