-
Notifications
You must be signed in to change notification settings - Fork 122
Fix/check context cancel logging #773
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
Fix/check context cancel logging #773
Conversation
62b551d to
10f7ff8
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.
Hi @abdullahalrifat - thanks for the contribution. I left a review with some suggested changes, but I'd be happy to help you get this in. That said, I'm curious how you found this behavior? Did it arise in a production deployment? Some background about how this context might be cancelled would be helpful - are you seeing context cancellations from Envoy?
Once we better understand that and you are able to make the suggested changes, I will need you to squash your commits and rebase on top of the latest main as well.
Thanks for contributing!
When I deployed OPA with Envoy in a Kubernetes cluster, I noticed that some incoming requests occasionally failed. These failures could happen for various reasons, such as networking issues. On the client side, this wasn’t a problem since they had a retry mechanism in place, but OPA was still logging these as failures. When analyzing OPA failure logs, we generally classify them into categories like:
However, the context_cancelled errors are different — they aren’t actual OPA failures, since OPA itself isn’t at fault. I wanted to separate these out to ensure that OPA failure logging accurately reflects real issues. |
117e916 to
f599383
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.
This LGTM, @abdullahalrifat - can you please rebase on the latest main and squash your commits?
aee76ee to
1558934
Compare
fixed context cancelled with proper logging Signed-off-by: abdullahalrifat <[email protected]> Signed-off-by: tjons <[email protected]>
1558934 to
ff43280
Compare
This PR adds a unit test to verify that Check correctly handles explicit context cancellation before query execution.
Introduced a testServer wrapper with a beforeCheck hook to simulate cancellation during request handling.
Ensured that the expected error (context canceled) is returned and properly logged.
Validated that the error metric CheckRequestCancelledErr is incremented.
This strengthens test coverage around graceful handling of cancelled requests and ensures consistent logging/metrics behavior.