-
Notifications
You must be signed in to change notification settings - Fork 539
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
feat(instrumentation-aws-lambda): take care of ESM based (.mjs
) handlers
#2508
feat(instrumentation-aws-lambda): take care of ESM based (.mjs
) handlers
#2508
Conversation
fea7f36
to
118eb8e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2508 +/- ##
==========================================
- Coverage 90.79% 90.77% -0.02%
==========================================
Files 169 169
Lines 8009 8015 +6
Branches 1632 1632
==========================================
+ Hits 7272 7276 +4
- Misses 737 739 +2
|
.mjs
) handlers for the AWS Lambda instrumentation.mjs
) handlers
Looks like a subset of #2000, thanks for addressing the issue in the instrumentation code wrt. absolute paths ! |
118eb8e
to
1025833
Compare
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
@pichlermarc I haven't merged this yet, in case OP would like to respond to my comments above. I think it is good to merge if you are doing a release that could include this tomorrow. |
@raphael-theriault-swi Ah, sorry I hadn't seen your earlier PR ... and I hadn't really been digging into Lambda-related PRs. I'll try to take a look in the next few days. |
@trentm I'd just go with whatever is easier to review and get out of the doors, ultimately the important part is ESM support |
@trentm Thanks for the review. I have updated PR according to your comments |
Which problem is this PR solving?
Currently, ESM based handlers with
.mjs
file extension are not taken care of by AWS Lambda instrumentation, but 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/).Short description of the changes
For the fix, while handler file is being resolved, in addition to
.js
and.cjs
file extensions,.mjs
file extension is also checked.This PR is the contrib counter part of the open-telemetry/opentelemetry-js#5094