Make sure AWS SDK spans suppress client spans all the time. #1837
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
anuraaga
requested review from
iNikem,
jkwatson,
mateuszrzeszutek,
trask and
tylerbenson
as code owners
anuraaga
commented
Dec 6, 2020
| public static class ScopeHolder { | ||
| public static final ThreadLocal<Scope> CURRENT = new ThreadLocal<>(); | ||
| } | ||
|
|
||
| private final ExecutionInterceptor delegate; | ||
|
|
||
| public TracingExecutionInterceptor() { |
There was a problem hiding this comment.
I think after #1643 I may try to remove this class, instead just service loading the shaded library instrumentation.
anuraaga
force-pushed
the
aws-sdk-fix-signature
branch
from
1c618e4
to
61a5f3a
Compare
trask
approved these changes
Dec 7, 2020
...java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/NettyHttpClientTracer.java
Outdated
Show resolved
Hide resolved
…ntelemetry/javaagent/instrumentation/netty/v4_1/client/NettyHttpClientTracer.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
trask
reviewed
Dec 7, 2020
Comment on lines
73
to
80
| // The AWS SDK uses Netty for asynchronous clients but constructs a request signature before | ||
| // beginning transport. This means we MUST suppress Netty spans we would normally create or | ||
| // they will inject their own trace header, which does not match what was present when the | ||
| // signature was computed, breaking the SDK request completely We have not found how to | ||
| // cleanly propagate context from the SDK instrumentation, which executes on an application | ||
| // thread, to Netty instrumentation, which executes on event loops. If it's possible, it may | ||
| // require instrumenting internal classes. Using a header which is more or less guaranteed to | ||
| // always exist is arguably more stable. |
iNikem
approved these changes
Dec 7, 2020
| OpenTelemetry.getGlobalPropagators() | ||
| .getTextMapPropagator() | ||
| .inject(otelContext, builder, AwsSdkInjectAdapter.INSTANCE); | ||
| AwsXRayPropagator.getInstance().inject(otelContext, builder, AwsSdkInjectAdapter.INSTANCE); |
There was a problem hiding this comment.
Even if you are sure that AWS SDK calls are always terminal and that nobody uses some alternative implementations of AWS services (like local S3 server?), we at least have to document this behaviour very clearly.
There was a problem hiding this comment.
Got a bit of extra README out of it too :)
bhautikpip
pushed a commit
to bhautikpip/opentelemetry-java-instrumentation
that referenced
this pull request
Dec 18, 2020
…emetry#1837) * Make sure AWS SDK spans suppress client spans all the time. * Make more consistent with other instrumentation * Update instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/NettyHttpClientTracer.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com> * more dragons * Grammar * README Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
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.
Also explicitly populate aws header instead of global propagators.
A lot to process here.
Currently, AWS SDK instrumentation always breaks the signature because transport spans aren't suppressed.
When separating out library / javaagent instrumentation ages ago, I was still very green in this repo and don't think I understood the point of the
testImplementationdependencies on downstream instrumentation and didn't port them over. So for a while we haven't verified suppression of transport spans.Sync: This was working in 0.10.0 and stopped working in 0.11.0. I haven't dug into why but presume some refactoring around client tracers is related. We would have caught the issue if I hadn't broken the tests in the first place.
Async: I suspect this has been broken for a long time, but none of our (e.g., aws-otel-java) integration tests use async clients yet. The docs from age-old code in the instrumentation seemed to indicate context is propagated to Netty properly, though extremely hackily, so I removed the even more hacky
amz-sdk-invocation-idheader check that we used to have to prevent signature breakage. But after adding the downstream instrumentation to the tests, I found that this wasn't the case. Using an IntelliJ debugger, I could confirmbeforeTransmissionis run on the application thread, not Netty thread, so the complicated instrumentation hack was having no effect (our executor instrumentation does not apply to netty event loops so I wouldn't expect those to ever have been helping here. Anyways, it's too deep to rely on IMO.). I went ahead and removed the hacky instrumentation and moved the scope management to the interceptor for use in synchronous clients only. For the netty spans, I restored the funamz-sdk-invocation-idas no alternative seems clearly better.I also went ahead and had the SDK explicitly use aws propagation instead of the global ones. Unlike lambda, there is no use case for any other header, probably not for the next several years really, so this will allow a user's app to be more efficient, they don't have to enable aws globally (meaning on their own ingestion and extraction), and still get the benefit it provides on AWS SDK calls. This makes the integration test confirming that headers haven't been added (breaking signature, the worst thing our instrumentation could ever do) more robust in the process.