feat!: better ESM instrumentation; bump min-supported node to ^18.19.0 || >=20.6.0#584
Merged
Conversation
…0 || >=20.6.0
This uses the IITM feature to only hook ES modules that will be patched by
instrumentations. This means instr-openai can possibly work with ESM usage of
openai -- before this it would crash from IITM hook changes.
This drops Node.js 14 and 16 support, in favour of the new base versions
that support module.register(). This is the same base version for coming
upstream OTel JS SDK 2.0. This allows us to simplify the ESM story: both
for docs and implementation.
Using --experimental-loader to somewhat support ESM for older node.js
versions is no longer supported. The main recommendation is to use
node --import @elastic/opentelemetry-node ...
I.e. to use --import rather than --require. Only with --import will
EDOT Node.js register the loader hook for ESM instrumentation.
Using --require is still fine for CommonJS-only instr.
Member
Author
|
If it helps, this is what attempting to instrument ESM code using openai looked like before this PR: |
…0 that are supported
trentm
commented
Feb 4, 2025
| node: '>=16.20.1', | ||
| }, | ||
| // verbose: true, | ||
| verbose: true, |
Member
Author
There was a problem hiding this comment.
Reviewer note: This could be excluded. I added this as a first step to trying to observe flaky test failures from this test case. I've seen it fail in local runs twice now.
trentm
commented
Feb 4, 2025
david-luna
approved these changes
Feb 4, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This uses the IITM feature to only hook ES modules that will be patched by
instrumentations. This means instr-openai can possibly work with ESM usage of
openai -- before this it would crash from IITM hook changes.
This drops Node.js 14 and 16 support, in favour of the new base versions
that support module.register(). This is the same base version for coming
upstream OTel JS SDK 2.0. This allows us to simplify the ESM story: both
for docs and implementation.
Using --experimental-loader to somewhat support ESM for older node.js
versions is no longer supported. The main recommendation is to use
node --import @elastic/opentelemetry-node ...
I.e. to use --import rather than --require. Only with --import will
EDOT Node.js register the loader hook for ESM instrumentation.
Using --require is still fine for CommonJS-only instr.
Closes: #288
@david-luna This is a re-run at #499.
I've not added anything for handling multiple installations of IITM. I will follow-up with an issue to discuss options for that... and possibly a PR for that, though I think it isn't necessary for GA.