Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
trentm opened this issue Aug 20, 2024 · 12 comments
Open

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

trentm opened this issue Aug 20, 2024 · 12 comments

Comments

@trentm
Copy link
Contributor

trentm commented Aug 20, 2024

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:

  • All of this recommended future requires module.register() and --import, which requires ^18.19.0 || ^20.6.0 || >=22. One option would be document this as a minimum requirement at the top of our "experimental ESM support" and then mention that for some earlier versions of node there is the "unsupported, forever-experimental, unrecommended" (or some working) option using --experimental-loader=. Then we can shunt all that complexity to a separate section.
  • In the bootstrap code I'm referencing a new createAddHookMessageChannel thing (feat: Optionally only wrap modules hooked in --import nodejs/import-in-the-middle#146) from import-in-the-middle that will almost certainly be coming soon, and that we'll want to use.
  • Mini-debate: "./instrumentation.mjs" or "./instrument.mjs" or "./telemetry.mjs"? I know "./instrumentation.mjs" is the current state (with outdated vestiges of "tracer.js" kicking around), but I'd be happy with a shorter one. :) Any non-binding opinions? Of course, this is minor.
  • I'm using the .mjs ext for the bootstrap file. There are some subtleties here.
    • Explicit .mjs makes it clear the file is ESM code, so that it doesn't depend on a "type" entry in the package.json file.
    • We may be tempted to also show a .ts option (see run option 3 above). This may be fine. --import does support getting a CJS file, so even if "tsconfig.json" emits CJS, then --import ... will work.
    • However, I'm using top-level await in the bootstrap code for the coming new import-in-the-middle feature. Does that mean that TypeScript compilation targeting CJS will blow up on that ./instrumentation.ts?
  • I haven't discussed bundling issues at all.
  • This bootstrap code could be written to work for users not using ESM: if (typeof register === 'function') { ... }. Then we could document this one-true-path for all users -- with a callout to a separate "If you are using ESM code ..." section that explains those limitations.

Originally posted by @trentm in #4845 (comment)

@trentm
Copy link
Contributor Author

trentm commented Aug 20, 2024

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:

  • The bootstrap file should be ESM: i.e. "telemetry.mjs" and probably not a ".ts" file which is dependent on tsconfig.json settings.
  • Using ESM means recommending --import and not --require.
  • Using --import means Node.js ^18.19.0 || ^20.6.0 || >=22 is required.

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.

Copy link

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.

@github-actions github-actions bot added the stale label Oct 28, 2024
@acidoxee
Copy link

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?

@trentm trentm removed the stale label Oct 28, 2024
@jpmunz
Copy link

jpmunz commented Nov 6, 2024

Just noting a couple things based on having looked at #5097 for loading typescript files specifically:

  • since tsconfig isn't read by the import-in-the-middle hook path aliases wouldn't work though there is a workaround I mention in that ticket that maybe could be included as part of this updated documentation
  • I did otherwise hit an error trying to use a .ts extension with this hook, e.g. npx tsx --experimental-loader=@opentelemetry/instrumentation/hook.mjs --import ./telemetry.ts something.ts raised TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts"

@trentm
Copy link
Contributor Author

trentm commented Nov 6, 2024

@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
Copy link
Contributor Author

trentm commented Nov 6, 2024

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: open-telemetry/opentelemetry-operator#3416

@jpmunz
Copy link

jpmunz commented Nov 7, 2024

@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
Copy link

douglasg14b commented Nov 7, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

trentm commented Nov 7, 2024

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

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 #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
Copy link
Contributor Author

trentm commented Nov 8, 2024

@zacharyblasczyk Let's please keep 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants