-
Notifications
You must be signed in to change notification settings - Fork 811
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
docs: Describe support for ESM #4876
docs: Describe support for ESM #4876
Conversation
Co-authored-by: Trent Mick <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Thank you.
Some nits and suggestions below.
doc/esm-vs-cjs.md
Outdated
### Additional Notes on Experimental Loaders | ||
|
||
Though the OpenTelemetry loader currently relies on `import-in-the-middle`, direct usage of `import-in-the-middle/hook.mjs` may cease to work in the future. | ||
The only currently supported loader hook is `@opentelemetry/instrumentation/hook.mjs`. | ||
|
||
Experimental loader is intended to be deprecated, and will be replaced with something like `--import=@opentelemetry/instrumentation/hook.mjs` | ||
|
||
Regarding `--experimental-loader` per [Node.js docs](https://nodejs.org/api/cli.html#--experimental-loadermodule): | ||
|
||
> This flag is discouraged and may be removed in a future version of Node.js. Please use `--import` with `register()` instead. | ||
|
||
<!-- | ||
TODO | ||
import * as module from 'module' | ||
|
||
module.register('@opentelemetry/instrumentation/hook.mjs', import.meta.url) | ||
--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this section could be replaced with a note something like:
**Note:** Eventually the recommendation for how to setup OpenTelemetry for usage with ESM will change to no longer require `--experimental-loader=@opentelemetry/instrumentation/hook.mjs`. Instead the bootstrap code (in "./telemetry.js") will use Node.js's newer `module.register(...)`. See https://github.com/open-telemetry/opentelemetry-js/issues/4933 for details.
If not, then at least we should correct this:
and will be replaced with something like
--import=@opentelemetry/instrumentation/hook.mjs
We won't have users do that. Instead we'll have them use module.register(...)
in their ./telemetry.mjs
bootstrap file and then no longer need to have a separate --experimental-loader
argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used your verbiage. When we're ready to make the new recommendation we can update separately.
doc/esm-vs-cjs.md
Outdated
| >=16.0.0 | `--require ./telemetry.cjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | ||
| >=18.1.0 <18.19.0 | `--require ./telemetry.cjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | ||
| >=18.19.0 | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | ||
| 20.x | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | ||
| 22.x | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| >=16.0.0 | `--require ./telemetry.cjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| >=18.1.0 <18.19.0 | `--require ./telemetry.cjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| >=18.19.0 | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| 20.x | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| 22.x | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| 16.x | `--require ./telemetry.cjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| >=18.1.0 <18.19.0 | `--require ./telemetry.cjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| ^18.19.0 | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| ^20.6.0 | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | | |
| 22.x | `--import ./telemetry.mjs --experimental-loader=@opentelemetry/instrumentation/hook.mjs` | |
I suppose I left out the >=20.0.0 <20.6.0
case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the ^20.6.0
case a known cutoff or just not something you have tried? I just tested v20.0.0 and 20.3.0 and 20.9.0 and they all seem to work with this start command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad. ^20.6.0
is needed for module.register(...)
. https://nodejs.org/en/blog/release/v20.6.0#new-nodemodule-api-register-for-module-customization-hooks-new-initialize-hook
But we're not talking about module.register() here yet.
I think we do want to limit our supported Node.js versions for ESM support to ^20.6.0
if we do add a statement about supported node.js versions -- on the expectation that we'll be moving official recommendation to module.register(...)
usage.
* Cleanup note on experimental loaders and future support and usage * Clarify issues with ts-node and tsx (see multiple open issues for ts-node and ESM) * Move auto-instr node above examples and fix startup commands to include ESM usage * Fix startup command for compiled ESM * Adjust node.js versions in table
Startup command for ESM: | ||
|
||
```sh | ||
node --experimental-loader=@opentelemetry/instrumentation/hook.mjs --import @opentelemetry/auto-instrumentations-node/register app.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: It is also fine to use --require
here, even for ESM support.
I don't think we need to mention that here, however.
| 1.20.x | 0.47.x | | ||
| 1.19.x | 0.46.x | | ||
| 1.18.x | 0.45.x | | ||
| 1.17.x | 0.44.x | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table cleanup is cool. This makes me realize this section is way out of date and a maintenance pain.
The "semconv" package is now a new category for the "OpenTelemetry is released as a set of distinct packages in 3 categories:" sentence at the start of this section.
Anyway... not for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually I didn't mean to reformat anything in this doc. It must have auto-formatted when I added the first section and saved.
Co-authored-by: Trent Mick <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this 👍
Co-authored-by: Trent Mick <[email protected]>
Which problem is this PR solving?
Updates #4845
Short description of the changes
NODE_OPTIONS
I am thinking it makes sense to tidy up the content here, and once approved I can move some of the notes into the public docs for the website.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
relevant tsconfig compiler options
package.json scripts
Checklist: