-
Notifications
You must be signed in to change notification settings - Fork 810
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 wrapping ESM files with absolute path #5094
Fix wrapping ESM files with absolute path #5094
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5094 +/- ##
=======================================
Coverage 93.17% 93.17%
=======================================
Files 315 315
Lines 8086 8086
Branches 1617 1617
=======================================
Hits 7534 7534
Misses 552 552 |
f356048
to
123fac0
Compare
123fac0
to
7c2f321
Compare
7c2f321
to
82379b8
Compare
🔥 |
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 @serkan-ozal can you also add a changelog entry?
@JamieDanielson @dyladan Done. Added README. All is green now. |
Which problem is this PR solving?
Currently ESM files are not able to instrumented with absolute path, but as modules. Instrumenting ESM files with absolute path is needed for AWS Lambda functions, as ESM based handlers are supported since long time (https://aws.amazon.com/tr/about-aws/whats-new/2022/01/aws-lambda-es-modules-top-level-await-node-js-14/)
For example, here is the issue we have faced in AWS Lambda Node.js layer: open-telemetry/opentelemetry-lambda#1557
Note: To have ESM support for AWS Lambda functions, there will be subsequent PRs to the
opentelemetry-js-contrib
andopentelemetry-lambda
repositories based on this.Short description of the changes
When ESM files to be instrumented are provided with their absolute paths,
import-in-the-middle
library passes modulename
parameter as full path without anybaseDir
parameter (butrequire-in-the-middle
library doesn't work like this for the instrumentation of the CommonJS modules).However,
_onRequire
method requires those parameters with proper values (name
as filename andbaseDir
as the directory of the file). So, this PR makes the required conversion for thename
andbaseDir
parameters to be able to used properly by the_onRequire
method.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added tests to
experimental/packages/opentelemetry-instrumentation/test/node/EsmInstrumentation.test.mjs
EsmInstrumentation.test
Checklist: