-
Notifications
You must be signed in to change notification settings - Fork 767
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
Comments
Some thoughts for discussion. (@JamieDanielson thanks for the start at #4876 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 contentThe 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 To use this, bootstrap code will move to something like:
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:
Finally: What should that "Init the SDK" code be?
|
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.
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 |
@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. |
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:
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.
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.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 ofimport-in-the-middle/hook.mjs
is not supported, we can change the instrumentation package to not work withimport-in-the-middle
should the need arise to do so.This issue is considered done when:
and
Additional Notes:
This issue is part of #4586
The text was updated successfully, but these errors were encountered: