Skip to content
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

Rework grpc cancelation propagation #8957

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Jul 16, 2023

Resolves #8923
When running with the agent we switch grpc context storage to use opentelemetry context. As described in #4169 (comment) this strategy works too well, context propagation sometimes happens when originally context was not propagated. This has the side effect of also propagating deadlines to calls that previously didn't have them. This is corrected in #5543 Unfortunately this has the side effect that now deadlines and cancellations aren't sometimes propagated when they are with the original context store implementation. #8924 introduces a flag that allows users to decide whether cancelation and deadlines should propagate. Such flags aren't ideal because users need to figure out what works for them. This pr explores an alternative strategy where, instead of completely replacing the original context storage, we'll track propagation through both otel context and the original context storage. When we discover that context was propagated through otel, but not with the original context storage, we'll fork the grpc context to break cancellation propagation. Hopefully this way deadlines and cancellation are propagated only when they are propagated without the agent. This pr can coexist with the flag introduced in #8924

@laurit laurit requested a review from a team as a code owner July 16, 2023 06:35
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkharlan Can you take a look at this PR as well?

@dkharlan
Copy link

dkharlan commented Jul 18, 2023

I like it! This is definitely better than mine (#8924), since this implies less knowledge of the problem. I haven't had time to review in detail yet, but I'll try to find some time today or tomorrow to test with our app and report whether it fixes the problem we've observed.

@dkharlan
Copy link

Sorry for the delay -- I built the agent from this branch and confirmed that it fixes #8923 for our app. Thanks for jumping on this so quickly!

@laurit laurit merged commit d749ac0 into open-telemetry:main Aug 8, 2023
45 checks passed
@laurit laurit deleted the grpc-context-propagation branch August 8, 2023 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-instrumented gRPC services ignore cancellation listeners
3 participants