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

feat(instrumentation-aws-lambda): Adds lambdaHandler config option #1627

Merged
merged 5 commits into from
Aug 14, 2023

Conversation

schmalzs
Copy link
Contributor

@schmalzs schmalzs commented Aug 8, 2023

Which problem is this PR solving?

This PR is scoped to the @opentelemetry/instrumentation-aws-lambda package.

Currently, the AWS Lambda instrumentation uses the _HANDLER environment variable to determine which module to instrument. The _HANDLER environment variable is set by AWS itself and, for most use cases, will accurately represent the handler that should be targeted by this instrumentation.

There exist use cases where the _HANDLER environment variable does not accurately represent the module that should be targeted by this instrumentation.

Specifically, I have Lambdas that are integrated with Datadog. The Datadog integration injects itself into the Lambda runtime by overriding the _HANDLER environment variable. It points the _HANDLER to Datadog code (loaded via a Lambda Layer). That Datadog code acts as a proxy that wraps my actual Lambda handler code.

When attempting to use this instrumentation alongside the Datadog integration, the instrumentation does not work. It ends up targeting the Datadog code (due to the value of the _HANDLER environment variable) rather than my actual Lambda code.

My particular use case is specific to utilizing this instrumentation alongside Datadog, however, I imagine there are other use cases that would run into similar problems.

Short description of the changes

This PR adds an optional handler config option. This option accepts a string that can be used to manually specify the handler that the instrumentation should target. If the handler option is omitted, the default behavior will be to use the _HANDLER environment variable, as it does today.

I have tested this alongside the Datadog integration and have confirmed it works as expected.

@schmalzs schmalzs requested a review from a team August 8, 2023 17:01
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 8, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@pichlermarc pichlermarc changed the title feat(lambda): Adds handler config option feat(instrumentation-aws-lambda): Adds handler config option Aug 9, 2023
@pichlermarc
Copy link
Member

cc @carolabadeer (Component Owner)

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #1627 (bec3509) into main (825b5a8) will decrease coverage by 0.02%.
The diff coverage is 66.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1627      +/-   ##
==========================================
- Coverage   91.78%   91.77%   -0.02%     
==========================================
  Files         139      139              
  Lines        7110     7112       +2     
  Branches     1426     1427       +1     
==========================================
+ Hits         6526     6527       +1     
- Misses        584      585       +1     
Files Changed Coverage Δ
...-instrumentation-aws-lambda/src/instrumentation.ts 93.67% <66.66%> (-0.51%) ⬇️

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Thanks for the great research and for adding this feature

@schmalzs schmalzs force-pushed the main branch 3 times, most recently from 6976ae5 to 06788e2 Compare August 10, 2023 02:59
@schmalzs schmalzs changed the title feat(instrumentation-aws-lambda): Adds handler config option feat(instrumentation-aws-lambda): Adds lambdaHandler config option Aug 10, 2023
Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments 💯
Left a few nit suggestions

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM

@blumamir
Copy link
Member

Will merge after @carolabadeer review as component owner

@blumamir
Copy link
Member

@schmalzs seems that lint is failing in CI.
Can you please run npm run lint:fix for this package and push the changes? thanks

@schmalzs
Copy link
Contributor Author

@schmalzs seems that lint is failing in CI. Can you please run npm run lint:fix for this package and push the changes? thanks

Just updated the linting fixes ✅

Thanks for your quick review of this PR, @blumamir, and all your thoughtful feedback. Really appreciate it!

Copy link
Contributor

@carolabadeer carolabadeer left a comment

Choose a reason for hiding this comment

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

Left a few small nit comments, otherwise changes look good to me! Thanks @schmalzs for this change and your attention to detail, and thanks @blumamir for your review :)

@schmalzs
Copy link
Contributor Author

Thank you for your thoughtful review, @carolabadeer! I just made the updates you suggested. Please let me know what you think. Thanks!

Copy link
Contributor

@carolabadeer carolabadeer left a comment

Choose a reason for hiding this comment

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

LGTM

@blumamir blumamir merged commit c4a8e82 into open-telemetry:main Aug 14, 2023
@dyladan dyladan mentioned this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants