-
Notifications
You must be signed in to change notification settings - Fork 395
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
fix!: Replaced esm loader with import-in-the-middle to fix instrumentation firing for both commonjs and esm #1760
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1760 +/- ##
==========================================
- Coverage 96.88% 96.88% -0.01%
==========================================
Files 200 199 -1
Lines 39210 38838 -372
Branches 22 0 -22
==========================================
- Hits 37989 37627 -362
+ Misses 1221 1211 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -272,8 +272,7 @@ const INFINITE_TRACING = { | |||
const FEATURES = { | |||
ESM: { | |||
LOADER: `${SUPPORTABILITY.FEATURES}/ESM/Loader`, | |||
UNSUPPORTED_LOADER: `${SUPPORTABILITY.FEATURES}/ESM/UnsupportedLoader`, | |||
CUSTOM_INSTRUMENTATION: `${SUPPORTABILITY.FEATURES}/ESM/CustomInstrumentation` |
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.
Same thing about Angler here, should update to remove this metric
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.
yep, ill get all these queued up in one PR shortly
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.
Here's the PR https://source.datanerd.us/agents/angler/pull/476
@@ -13,16 +13,19 @@ module.exports = [ | |||
{ | |||
type: 'generic', | |||
moduleName: '@grpc/grpc-js/build/src/resolving-call', | |||
isEsm: true, |
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.
Wait, why did this have to get added? It's not an ESM module, and I thought that's what the flag is for?
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 code is getting transpiled and it has named exports instead of default, that's why
…rics for ESM, and removed esm unit tests as they are no longer relevant
…vs the default key
Details
This PR restores support for ESM in Node 20. It also supersedes #1759. It's also worth noting at the time of opening this it layers on top of #1758 . By relying on import-in-the-middle it de-couples are agent from proxying ESM packages to instrument. However this now requires us to do
--loader newrelic/esm-loader.mjs -r newrelic
. You can see by the PR that it also removes the complexities we created by having a test loader that injects a fake agent before calling the new loader, this is no longer needed. Also since our esm loader now just calls import in the middle, there is no need to test esm files and mock esm with testdouble. I also shifted some supportability metrics around ESM to the agent.Changelog details
node --experimental-loader newrelic/esm-loader.mjs -r newrelic path/to/app.js
config.esm.custom_instrumentation_entrypoint
to register ESM instrumentation.newrelic.instrument*
APIs but you must pass in an object and specifyisEsm: true
.Related Issues
Closes #1646
Closes #1720