-
Notifications
You must be signed in to change notification settings - Fork 873
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
aws-sdk-2.2: Support non-X-Ray propagation for SQS #8405
Merged
trask
merged 9 commits into
open-telemetry:main
from
dynatrace-oss-contrib:feature/add-aws-sdk-2-sqs-inject
May 16, 2023
Merged
aws-sdk-2.2: Support non-X-Ray propagation for SQS #8405
trask
merged 9 commits into
open-telemetry:main
from
dynatrace-oss-contrib:feature/add-aws-sdk-2-sqs-inject
May 16, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oberon00
commented
May 3, 2023
// Specific AWS SDK conventions also don't mention this peculiar hybrid span convention, see | ||
// https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/instrumentation/aws-sdk.md | ||
// | ||
// TODO: Consider removing net+http conventions & relying on lower-level client instrumentation |
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.
Calling out this TODO, and here the clickable links:
- HTTP conventions: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md#http-request-retries-and-redirects
- AWS SDK conventions: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/instrumentation/aws-sdk.md
Oberon00
changed the title
aws-sdk-2.2: Support non-XRay propagation for SQS.
aws-sdk-2.2: Support non-X-Ray propagation for SQS
May 3, 2023
mateuszrzeszutek
approved these changes
May 5, 2023
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.
LEGTM, but left a couple of minor messages
.../io/opentelemetry/instrumentation/awssdk/v2_2/autoconfigure/TracingExecutionInterceptor.java
Show resolved
Hide resolved
...-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/AwsSdkTelemetry.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java
Outdated
Show resolved
Hide resolved
Apply suggestions from code review Co-authored-by: Mateusz Rzeszutek <[email protected]>
…pentelemetry/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java Co-authored-by: Mateusz Rzeszutek <[email protected]>
mateuszrzeszutek
approved these changes
May 9, 2023
Is there anything more I should do to get this merged? |
This was referenced Jun 1, 2023
Draft
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This PR, despite being large, tries to do just what the title says: Adding support only for the v2 SDK (not v1 yet) only for SQS (not SNS yet) to inject into sent message attributes (no support for batch send yet) and the other side, adding support for extracting with the configured text map propagator (usually W3C) instead of X-Ray.
It also adds an option to the autoconfigure library to expose the (off-by-default) feature:
otel.instrumentation.aws-sdk.experimental-use-propagator-for-messaging
As mentioned in the README of the aws-sdk-2.2 library at https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/aws-sdk/aws-sdk-2.2/library#trace-propagation:
Motivation & Prior art
The concrete use-case that triggered this PR was that for Dynatrace we need certain tracestate entries for more efficient span routing & linking so we'd like tracestate support, but I think it is also generally useful (e.g. (limited) baggage could also be propagated this way, or you could bypass X-Ray created intermediate spans if there are any.
If you are interested, this is the Dynatrace documentation for the scenario for Python that we'd like to enable for Java: https://www.dynatrace.com/support/help/shortlink/opentel-lambda#python-sqs-sns
Note that the AWS SDK instrumentations for Python & Node.js also support this feature (or even have it always-on) and should be wire-compatible with this (as long as the configured propagator matches of course):
Challenges
Obviously, this PR is rather large for implementing such a minimal feature set. There are two things that I think could be improved, but didn't dare to without discussion (I'm personally also OK with not improving them yet, as for me it's most important to get the functionality added in this PR supported):
@NoMuzzle
but for this PR I was following the previous approach with reflection. Please see this separate GH discussion: Addressing aws-sdk + plugin library muzzle issues / reflection creep #8394Testing done
I extended the library unit tests, also ensuring that it works w/o additional X-Ray. I also manually tested sending to an actual SQS queue and verified the attributes are set as expected.
Further follow-ups
As mentioned initially, I tried to keep this PR minimal. If I can get this accepted in some form, I'd like to extend the non-X-Ray propagation support to