Properly handle grpc dial errors in the throttler metric aggregation #18073
Conversation
This file contains hidden or 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
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
arthurschreiber
requested review from
harshit-gangal,
systay,
shlomi-noach,
rohit-nayak-ps and
timvaillancourt
as code owners
arthurschreiber
added
Type: Bug
Component: Throttler
Backport to: release-20.0
Signed-off-by: Arthur Schreiber <[email protected]>
arthurschreiber
force-pushed
the
arthur/fix-throttler-dial-errors
branch
from
c173bc5 to
0534dc5
Compare
arthurschreiber
commented
Mar 31, 2025
| if s, ok := status.FromError(err); ok { | ||
| return s.Code() == codes.Unavailable || s.Code() == codes.DeadlineExceeded | ||
| } | ||
|
|
||
| switch err := err.(type) { |
There was a problem hiding this comment.
I don't think the logic down here makes any sense, because we never have or had access to a net.OpError here. I left this in place, but I think this should be removed to be less confusing.
frouioui
approved these changes
Mar 31, 2025
Comment on lines
846
to
856
| func TestIsDialTCPError(t *testing.T) { | ||
| // Verify that IsDialTCPError actually recognizes grpc dial errors | ||
| cc, err := grpcclient.DialContext(t.Context(), ":0", true, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| require.NoError(t, err) | ||
| defer cc.Close() | ||
|
|
||
| err = cc.Invoke(context.Background(), "/Fail", nil, nil) | ||
|
|
||
| require.True(t, base.IsDialTCPError(err)) | ||
| require.True(t, base.IsDialTCPError(fmt.Errorf("wrapped: %w", err))) | ||
| } |
There was a problem hiding this comment.
We should add a require.False case here
There was a problem hiding this comment.
hi! can you sign your commit please, the DCO check is failing
This was referenced Apr 1, 2025
Closed
This was referenced Apr 7, 2025
Closed
Signed-off-by: Mohamed Hamza <[email protected]>
mhamza15
force-pushed
the
arthur/fix-throttler-dial-errors
branch
from
a259112 to
eeaad49
Compare
…r-dial-errors Signed-off-by: Mohamed Hamza <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18073 +/- ##
=======================================
Coverage ? 67.56%
=======================================
Files ? 1601
Lines ? 261487
Branches ? 0
=======================================
Hits ? 176673
Misses ? 84814
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Description
This fixes an issue in the throttler code for aggregating metric results.
Most of this code was lifted from freno, where metrics were collected using a MySQL connection, and the errors returned on connection failure were low level tcp dial errors.
When the code was moved from freno to Vitess, the connections were changed to use gRPC, and the errors returned on connection failure are now grpc dial errors instead. On top of that, the errors were actually not being fully passed through, as they are being wrapped in a
fmt.Errorfcall (but without using%wto actually allow access to the original error).This caused metric aggregation to incorrectly treat connection errors as an "unhealthy" state, causing all users of the throttler (like
VReplication) to throttle.This fixes this issue by correctly wrapping the error using
%wand then checking the grpc status code inIsDialTCPError.Related Issue(s)
Fixes: #18022
Checklist
Deployment Notes