Do not propagate gRPC deadline when propagating OTel context via javaagent. #5543
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
|
Came up with a possible conclusion, adding to issue |
anuraaga
changed the title
|
Thanks for the detailed analysis! I tried out a local build with this change on my reproducer project ryandens/otel-grpc-context and confirmed the error no longer shows up. |
RashmiRam
pushed a commit
to RashmiRam/opentelemetry-auto-instr-java
that referenced
this pull request
May 23, 2022
…agent. (open-telemetry#5543) * Add test for early return in gRPC pattern. * Do not propagate gRPC deadline when propagating OTel context via javaagent.
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.
Fixes #4169
Some background below but have worked around with this PR
I have been able to add a test that reproduces the problem in the issue, but I'm still not being able to figure out a fix. One problem is that per my reasoning, a cancellation error seems to actually make sense. gRPC server cancels the context when the server request is
onCompleted()as expectedhttps://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/internal/ServerCallImpl.java#L365
For this pattern of early return, where the server request is ended but business logic continues, it seems correct for that business logic to have been cancelled by default (imagine if it wasn't an early return pattern but some async callback sequence continuing after a 10s request deadline, cancellation propagation exists mostly to handle such a case), and explicit opt in to this pattern by calling
Context.fork()in the business logic seems expected. So I'm not sure why without instrumentation there is no error.I'm not convinced that there is no bug in the instrumentation, but in my head the instrumented behavior makes sense and the non-instrumented behavior doesn't, so stuck on how to dig deeper. @amitgud-doordash @ryandens do you happen to have any clues on this?