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

refactor!: update instrumentation registration to work for CJS deps in ESM without use of a loader #1759

Closed

Conversation

jmartin4563
Copy link
Contributor

@jmartin4563 jmartin4563 commented Aug 14, 2023

Description

#1646 revealed that with the introduction of version 10 and the ability to add multiple instrumentation for a given package, that for ESM applications there were edge cases where we could double instrument the package, specifically if the ESM application and a dependency both used the same package. Additionally, @bizob2828's extensive research into the changes Node 20 made with regards to loader hooks and worker threads culminated in the decision that we won't be continuing down the loader path for Node 20, and will be dropping support for registering custom ESM instrumentation.

This change replaces the major use case of the loader, which is registering CJS package instrumentation in an ESM application. All of the packages we currently implement are CJS packages (express/pg/koa/etc.). As such, and due to our wrapping Module internals, we're actually able to go through common paths regardless of whether or not an app is CJS or ESM.

With this change, we detect if we're in an ESM import (which we're able to do because the module name is the same fully resolved filepath in ESM, which was why we originally introduced the loader in the first place), and instead of using the fully resolve filepath as our instrumentation registration key, we use module-details-from-path to reverse lookup the package's "short name" from the file path (IE: given /this/is/a/fully/resolved/file/path/to/node_modules/express/index.js, we'll get express as the "short name"). This allows us to make sure we don't double instrument packages.

Additionally, I've removed the ESM loader from the code base, as in Node 20 it will not work at all. A sibling PR exists for test-utils that gates using the loader in versioned tests through an env var. This PR's CI won't pass until the test utils pr is merged and published, but can be vetted locally through npm link.

How to Test

Related Issues

Closes #1646

@jmartin4563 jmartin4563 added the semver: major Backwards incompatible changes. label Aug 14, 2023
@@ -10,12 +9,15 @@
"node": ">=16.12.0 <20"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't running on node 20

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad copy/paste, thank you for the catch!

@bizob2828
Copy link
Member

Closing this as #1760 supersedes this but great work @jmartin4563

@bizob2828 bizob2828 closed this Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major Backwards incompatible changes.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Transaction name multiplies when upgrading to v10 with ESM
2 participants