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

[instrumentation] update "Instrumentation for ES Modules" documentation #4845

Open
4 tasks
Tracked by #4586
pichlermarc opened this issue Jul 3, 2024 · 3 comments
Open
4 tasks
Tracked by #4586
Assignees
Labels
needs:refinement This issue needs to be refined/broken apart into sub-issues before implementation pkg:instrumentation type:feature A feature with no sub-issues to address

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Jul 3, 2024

Description

This issue should be handled by a maintainer or approver.

The section "Instrumentation for ES Modules In Node.js (experimental)" in README.md of the instrumentation package is outdated. In order to release @opentelemtry/instrumentation as stable, we need to ensure that we properly mark the state of ES Module instrumentation either as stable or experimental, and we should clearly state which Node.js versions we support and which ones we do not.

The goal of this issue is to properly document the following topics:

  1. We currently offer an instrumentation customization hook for ESM in Node.js which uses import-in-the-middle. As of writing, the customization hooks concept is not yet stable in Node.js, but it is already available as a RC. We should not mark any instrumenting ES Modules as stable unless the underlying customization hook mechanism is stable. We may mark Node.js versions as experimentally supported.

  2. We should also update documentation on how to set this customization hook when using Node.js, be it via --import or otherwise, and explicitly declare all other ways that may be introduced in future Node.js versions as not directly supported (we may declare them as supported in a new minor version). Ideally we only state one supported way of adding the hook.

  3. We should also document that direct usage of import-in-the-middle/hook.mjs may cease to work in the future. We re-export a hook @opentelemetry/instrumentation/hook.mjs and this one should be used. By explicitly stating that direct usage of import-in-the-middle/hook.mjs is not supported, we can change the instrumentation package to not work with import-in-the-middle should the need arise to do so.

This issue is considered done when:

  • Documentation is up-to-date

and

  • A statement is added that describes which Node.js versions are supported for ES Modules instrumentation, and what the level of stability for these versions is.
  • A statement is added that future ways to add customization hooks may not stay supported
  • A statement is added that only usage of ``@opentelemetry/instrumentation/hook.mjs` is supported

Additional Notes:

This issue is part of #4586

@pichlermarc pichlermarc changed the title document Node.js versions supported by the customization hook, or none if the concept is not stable yet [instrumentation] document Node.js versions supported by ESM instrumentation Jul 3, 2024
@pichlermarc pichlermarc added pkg:instrumentation type:feature A feature with no sub-issues to address needs:refinement This issue needs to be refined/broken apart into sub-issues before implementation labels Jul 3, 2024
@pichlermarc pichlermarc changed the title [instrumentation] document Node.js versions supported by ESM instrumentation [instrumentation] update Instrumentation for ES Modules documentation Jul 3, 2024
@pichlermarc pichlermarc changed the title [instrumentation] update Instrumentation for ES Modules documentation [instrumentation] update "Instrumentation for ES Modules" documentation Jul 3, 2024
@JamieDanielson JamieDanielson self-assigned this Jul 23, 2024
@trentm
Copy link
Contributor

trentm commented Jul 26, 2024

Some thoughts for discussion.

(@JamieDanielson thanks for the start at #4876
I think we could still get your changes in, and then perhaps later something like what I describe below.)

User Guide content (opentelemetry.io)

Current advice on opentelemetry.io ignores ESM:

Most users that happen to be using ESM and/or bundling will have a bad experience. Even though parts of ESM and bundler support is not supported or experimental, I think we should get some mention of these into those docs... perhaps with link out to the reference/README where we go into detail.

Reference content

The current ESM-related reference content is in opentelemetry-instrumentation/README.md. See below for a discussion of whether it should stay there or move.

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:

instrumentation.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=./instrumentation.mjs app.js

# 2.
export NODE_OPTIONS='--import=./instrumentation.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=./instrumentation.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.

Finally: What should that "Init the SDK" code be?

  • Reference docs in opentelemetry-instrumentation/README.md surely would want to show using the primitives (NodeTracerProvider, registerInstrumentations, etc.) as it does now.
  • Though we've debated this, I think users would be better served showing NodeSDK usage (with callouts to both auto-instrumentations-node/register and to the lower-level primitives for full control). If so, then, I think we should move this whole "reference docs on ESM" over to sdk-node/README.md or somewhere other than opentelemetry-instrumentation/README.md.

@JamieDanielson
Copy link
Member

Starting on this sort of response checklist of things to update. I'll have more detail to add but wanted to save this for now.

  • Add note in public docs explaining current directions are for CommonJS (in zero-code and getting started)
  • Add note in sdk-node README explaining current directions are for CommonJS
  • Add note in auto-instr-node README explaining current directions are for CommonJS
  • Add note to sdk-node README describing ESM/CJS and how to know which you are using when writing TS
  • Add note to sdk-node README describing the details of the experimental loader and supported versions
  • Add note to instrumentation README describing details of the loader... with a link to sdk-node?
  • Add note to sdk-node, auto-instr-node, public docs explaining that bundling is not supported
  • Rename instrumentation.mjs to telemetry.mjs.

I agree that most of the content should not live exclusively in the instrumentation package README, because people don't generally look there. The main READMEs of interest are probably sdk-node, auto-instr-node, and the main README. Then the public docs on opentelemetry.io should have this.

@JamieDanielson
Copy link
Member

@trentm I've updated the PR in this repo to add a dedicated doc that includes a bunch of details around this, with the goal of being able to link to it from various READMEs and places to avoid duplicating content, and to make it easier to see the detail in one place. If we decide on that content being reasonable at least for now, I can also use it to guide the changes to be made on the website.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:refinement This issue needs to be refined/broken apart into sub-issues before implementation pkg:instrumentation type:feature A feature with no sub-issues to address
Projects
None yet
Development

No branches or pull requests

3 participants