-
Notifications
You must be signed in to change notification settings - Fork 534
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)!: Remove explicit x-ray context in favor of global propagator #2369
feat(aws-lambda)!: Remove explicit x-ray context in favor of global propagator #2369
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2369 +/- ##
==========================================
- Coverage 90.85% 90.83% -0.03%
==========================================
Files 159 159
Lines 7853 7831 -22
Branches 1622 1610 -12
==========================================
- Hits 7135 7113 -22
Misses 718 718
|
plugins/node/opentelemetry-instrumentation-aws-lambda/README.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Tyler Benson <[email protected]>
plugins/node/opentelemetry-instrumentation-aws-lambda/README.md
Outdated
Show resolved
Hide resolved
ab400e8
to
3633a92
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 @martinkuba .
Great to see this cleaned up, aligned to spec and simplified.
The changes LGTM, I am a bit worried that existing users might not be aware if they should actively take action or will potentially break when upgrading to this new version.
Please mark it as breaking and consider adding some more guidance and examples in the README on how to use it correctly.
plugins/node/opentelemetry-instrumentation-aws-lambda/README.md
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/README.md
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/README.md
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/README.md
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.
Thanks @martinkuba this looks great!
Added 2 minor comments about some cleanups that I think we need to apply before merging
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
Show resolved
Hide resolved
2bab72d
to
a954164
Compare
This is part of open-telemetry/opentelemetry-js#4494
Currently, the instrumentation is explicitly retrieving context from the X-Ray
_X_AMZN_TRACE_ID
environment variable. It is possible now to handle this using the xray-lambda propagator, which is the approach described in the specification.This change removes the explicit handling including the
disableAwsContextPropagation
configuration option.