-
Notifications
You must be signed in to change notification settings - Fork 544
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(aws-lambda): added eventContextExtractor config option #566
feat(aws-lambda): added eventContextExtractor config option #566
Conversation
Codecov Report
@@ Coverage Diff @@
## main #566 +/- ##
==========================================
- Coverage 94.77% 94.72% -0.05%
==========================================
Files 179 174 -5
Lines 10921 10822 -99
Branches 1083 1057 -26
==========================================
- Hits 10350 10251 -99
Misses 571 571 |
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.
lgtm, i tagged AWS people so they can check too
9d9f22f
to
f596404
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 for the addition! Generally LGTM as well, but can you explain a bit more how context would be extracted from those event types other than an HTTP header? I assume it's for context propagation for vendors other than X-Ray since we don't have any special context stored in e.g. Kinesis/CW Events. But perhaps a small example code in the readme or unit test for what the custom extractors would look like would help me greatly.
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Show resolved
Hide resolved
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.
2 things, with the rest will wait for creator of this instrumentation
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/README.md
Outdated
Show resolved
Hide resolved
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.
LGTM with others' comments, thanks!
Sure!
The custom context extractor would be able to catch such cases and extract the context from the event. |
f596404
to
297e26d
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.
lgtm as well, thanks for the walkthrough @prsnca!
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Show resolved
Hide resolved
Please don't force push after any first review / comment |
Sure! Thanks for the comment. |
Have a question regarding this.How will the context be propagated in terms of SQS batching or other services which support batching. As the context can be fetched for a single message at a single point of time. |
@NikunjShah That's a great question. You could handle that in several ways:
|
Which problem is this PR solving?
Short description of the changes
eventContextExtractor
config option to allow custom handling of events which carry the context in different ways. If not provided, a default context extractor is used and tries to pull the context from the HTTP headers - same as the behavior today.