Fix gRPC error handling for mirror-gateway handler #2665
Conversation
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes enhance error handling and standardize the structure of gRPC method implementations in the Changes
Sequence Diagram(s)Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2665 +/- ##
==========================================
- Coverage 24.02% 23.85% -0.17%
==========================================
Files 539 539
Lines 47117 47071 -46
==========================================
- Hits 11320 11231 -89
- Misses 35027 35064 +37
- Partials 770 776 +6 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
pkg/gateway/mirror/handler/grpc/handler.go (2)
393-398: Typographical Error in Log MessageThere's a minor typo in the log message at line 396:
log.Errorf("gRPC call returned not a gRPC status error: %v", err)Consider rephrasing for clarity.
Update the log message:
-log.Errorf("gRPC call returned not a gRPC status error: %v", err) +log.Errorf("gRPC call did not return a gRPC status error: %v", err)
2730-2738: Typo in Error Message: "canceld"At line 2733, there's a typo in the error message:
err = status.WrapWithCanceled(vald.RemoveRPCName+" API canceld", err, reqInfo, resInfo)Correct the spelling of "canceled".
-err = status.WrapWithCanceled(vald.RemoveRPCName+" API canceld", err, reqInfo, resInfo) +err = status.WrapWithCanceled(vald.RemoveRPCName+" API canceled", err, reqInfo, resInfo)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/gateway/mirror/handler/grpc/handler.go (27 hunks)
🔇 Additional comments (6)
pkg/gateway/mirror/handler/grpc/handler.go (6)
120-131: Correct Error Wrapping withstatus.WrapWithInternalThe error wrapping at lines 120-131 correctly standardizes the use of
status.WrapWithInternal, enhancing consistency in error reporting. This aligns with the PR objective to improve error handling in gRPC implementations.
1997-1997: Consistent Use of Error CodesAt line 1997, ensure that the correct gRPC status codes are used in
sspan.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...). Consistency in status codes helps with accurate error tracking.Ensure that
st.Code()returns the appropriate status code for the error.
2801-2804: Ensure WaitGroup SynchronizationAt lines 2801-2804, confirm that the
WaitGroupis correctly synchronized to prevent panics due to concurrent access.Review the usage of
wg.Add(1)andwg.Done()to ensure they are correctly balanced.
2166-2175:⚠️ Potential issuePotential Race Condition in Concurrent Map Access
At lines 2166-2175, there is concurrent access to the
resultmap without synchronization, which could lead to race conditions.Protect access to
resultwith a mutex or use a concurrent map.+var resultMu sync.Mutex ... resultMu.Lock() result.Store(target, &errorState{err, code}) resultMu.Unlock()Likely invalid or redundant comment.
393-398:⚠️ Potential issueCheck for Possible Nil Pointer Dereference
At lines 393-398, there's a check for
st, ok := status.FromError(err). While the comment mentions it's unlikely, ensure thatst.Proto()won't cause a nil pointer dereference even in edge cases.Consider adding an explicit nil check before calling
st.Proto():if !ok || st == nil || st.Message() == "" {Likely invalid or redundant comment.
1560-1566:⚠️ Potential issueMissing Error Handling for
s.gateway.DoAt lines 1560-1566, after calling
s.gateway.Do, consider handling any potential errors returned by this function to prevent unexpected issues.Add error handling for
s.gateway.Doto capture and log any errors._, errGateway := s.gateway.Do(ctx, s.vAddr, func(ctx context.Context, _ string, vc service.MirrorClient, copts ...grpc.CallOption) (any, error) { loc, err = vc.Update(ctx, req, copts...) return loc, err }) +if errGateway != nil { + log.Warnf("Error from s.gateway.Do: %v", errGateway) + // Handle error as appropriate +}Likely invalid or redundant comment.
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
Deploying vald with
|
| Latest commit: |
794d70c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f191dd58.vald.pages.dev |
| Branch Preview URL: | https://fix-mirror-error-handling.vald.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
pkg/gateway/mirror/service/mirror.go (3)
218-223: Ensure Proper Handling of gRPC Status ErrorsIn the condition checking
!ok || st == nil || st.Message() == "", there's concern about potential nil pointer dereferences. The comment mentions the unlikelihood of this condition. Considering this, ensure that the error handling is robust and logs meaningful messages.Optionally, you could add more context to the log message for clarity:
log.Errorf("gRPC call returned not a gRPC status error: %v", err) +log.Debugf("Original error: %v", err)
Line range hint
228-240: Handle Errors Returned byDisconnectMethodWhen disconnecting due to an unimplemented error, the error returned by
m.Disconnectis logged but not handled. Consider handling this error to ensure that the disconnection process is reliable.Modify the code to handle the error:
if st.Code() == codes.Unimplemented { host, port, err := net.SplitHostPort(target) if err != nil { log.Warn(err) } else { if err := m.Disconnect(ctx, &payload.Mirror_Target{ Host: host, Port: uint32(port), }); err != nil { log.Errorf("failed to disconnect %s, err: %v", target, err) + return err } } }
Line range hint
273-282: Mismatch Between Error Handling and Logging SeverityAn internal error is wrapped and then logged with
log.Warn(err). It may be more appropriate to uselog.Error(err)to reflect the severity of the issue.Apply this diff to adjust the logging level:
err = status.WrapWithInternal( mirror.RegisterRPCName+" API connection not found", err, ) -log.Warn(err) +log.Error(err) if span != nil { span.RecordError(err) span.SetAttributes(trace.StatusCodeInternal(err.Error())...) span.SetStatus(trace.StatusError, err.Error()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/gateway/mirror/service/mirror.go (4 hunks)
🔇 Additional comments (3)
pkg/gateway/mirror/service/mirror.go (3)
Line range hint
242-249: Use Appropriate Logging Level for ErrorsThe error is being logged with
log.Errorf, which is suitable. Ensure that all errors of similar severity use consistent logging levels.
204-206:⚠️ Potential issueIncorrect Error Wrapping: Use
WrapWithDeadlineExceededThe
context.DeadlineExceedederror should be wrapped withstatus.WrapWithDeadlineExceededinstead ofstatus.WrapWithCanceledto accurately represent the error type.Apply this diff to correct the error wrapping:
-err = status.WrapWithCanceled( +err = status.WrapWithDeadlineExceeded( mirror.RegisterRPCName+" API deadline exceeded", err, )Likely invalid or redundant comment.
209-211: Consistent Error Wrapping: Verify the Use ofWrapWithInternalThe error is being wrapped with
status.WrapWithInternal, which is appropriate for internal errors. Ensure that this aligns with the intended error classification for a "connection not found" scenario.Run the following script to confirm consistent usage of error wrapping:
✅ Verification successful
Consistent Use of
WrapWithInternalConfirmedThe usage of
status.WrapWithInternalfor "API connection not found" errors is consistent across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of WrapWithInternal in similar contexts. rg --type go -A 2 $'status.WrapWithInternal(' | rg 'connection not found'Length of output: 173
Script:
#!/bin/bash # Description: Search for usages of WrapWithInternal in similar contexts with corrected regex. rg --type go 'status\.WrapWithInternal\(' -A 2 | rg 'connection not found'Length of output: 6291
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/fix%2Fmirror%2Ferror-handling/e64438b76000061e5add397fee3158bc77b3d0bb |
Profile Report
|
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/gateway/mirror/handler/grpc/handler.go (2)
Line range hint
145-153: Ensure Consistent Use of Error Codes and DetailsAt lines 145-153, when handling errors from
s.mirror.MirrorTargets(ctx), the error is wrapped withstatus.WrapWithInternaland includes anerrdetails.BadRequest. Similar to the previous comment,BadRequestdetails are usually paired withInvalidArgumentstatus codes.To maintain consistency and proper error handling, consider using
status.WrapWithInvalidArgumentor adjusting the error details.Apply this diff to correct the error code:
- err = status.WrapWithInternal(mirror.RegisterRPCName+" API failed to get connected mirror gateway targets", err, + err = status.WrapWithInvalidArgument(mirror.RegisterRPCName+" API failed to get connected mirror gateway targets", err, &errdetails.BadRequest{ FieldViolations: []*errdetails.BadRequestFieldViolation{ { Field: "mirror gateway targets", Description: err.Error(), }, }, }, &errdetails.ResourceInfo{ ResourceType: errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + mirror.RegisterRPCName, ResourceName: fmt.Sprintf("%s: %s(%s)", apiName, s.name, s.ip), }, )
1430-1438: Correct Spelling in Error MessageAt line 1434, the error message should use the status code
codes.Internalinstead ofcodes.Unknownfor internal errors:st = status.New(codes.Unknown, "failed to parse "+vald.InsertRPCName+" gRPC error response")Update the status code to accurately reflect the nature of the error.
Apply this diff to fix the status code:
- st = status.New(codes.Unknown, "failed to parse "+vald.InsertRPCName+" gRPC error response") + st = status.New(codes.Internal, "failed to parse "+vald.InsertRPCName+" gRPC error response")
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/gateway/mirror/handler/grpc/handler.go (2)
Line range hint
145-153: Use Appropriate Error Code with BadRequest DetailsAt lines 145-153, the error is wrapped with
status.WrapWithInternalwhile including anerrdetails.BadRequest. TheBadRequesterror detail is typically associated withInvalidArgumenterrors, notInternalerrors. This mismatch can lead to confusion in error handling and inconsistency in error reporting.Apply this diff to align the status code with the error details:
- err = status.WrapWithInternal( + err = status.WrapWithInvalidArgument( mirror.RegisterRPCName+" API failed to get connected mirror gateway targets", err, &errdetails.BadRequest{ FieldViolations: []*errdetails.BadRequestFieldViolation{ { Field: "mirror gateway targets", Description: err.Error(), }, }, }, &errdetails.ResourceInfo{ ResourceType: errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + mirror.RegisterRPCName, ResourceName: fmt.Sprintf("%s: %s(%s)", apiName, s.name, s.ip), }, )
120-121: Fix Spelling Mistake in Error MessagesThe error message contains a typo: "canceld" should be "canceled". This typo appears in multiple error messages throughout the code at lines 120-121, 1427-1430, 1557-1560, 1986-1989, 2358-2361, and 2857-2860.
Apply the following diff to correct the typo:
- err = status.WrapWithCanceled( - <APIName>+" API canceld", err, reqInfo, resInfo, + err = status.WrapWithCanceled( + <APIName>+" API canceled", err, reqInfo, resInfo,Please correct this typo in all occurrences within the codebase.
Also applies to: 1427-1430, 1557-1560, 1986-1989, 2358-2361, 2857-2860
Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/gateway/mirror/handler/grpc/handler.go (2)
Line range hint
145-154: Use Appropriate Error Code with BadRequest DetailsYou're wrapping the error with
status.WrapWithInternalwhile attaching anerrdetails.BadRequest. TheBadRequesterror detail is typically associated withInvalidArgumenterrors, notInternalerrors. This could lead to confusion in error handling and inconsistency in error reporting.Consider changing the status code to
InvalidArgumentto align with theBadRequesterror detail, or adjust the error details to match theInternalstatus code.Apply this diff to align the status code with the error details:
- err = status.WrapWithInternal( + err = status.WrapWithInvalidArgument( mirror.RegisterRPCName+" API failed to get connected mirror gateway targets", err, &errdetails.BadRequest{ FieldViolations: []*errdetails.BadRequestFieldViolation{ { Field: "mirror gateway targets", Description: err.Error(), }, }, }, &errdetails.ResourceInfo{ ResourceType: errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + mirror.RegisterRPCName, ResourceName: fmt.Sprintf("%s: %s(%s)", apiName, s.name, s.ip), }, )
3247-3247: Ensure Consistency in Error CodesIn this error handling block, you're using
codes.Internalwhen creating a new status, whereas similar blocks usecodes.Unknown. For consistency and clarity, consider using the same error code across similar error handling sections.Apply this diff to standardize the error code:
- st = status.New(codes.Internal, "failed to parse "+vald.GetObjectRPCName+" gRPC error response") + st = status.New(codes.Unknown, "failed to parse "+vald.GetObjectRPCName+" gRPC error response")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/gateway/mirror/handler/grpc/handler.go (37 hunks)
🧰 Additional context used
🔇 Additional comments (1)
pkg/gateway/mirror/handler/grpc/handler.go (1)
120-121:⚠️ Potential issueFix typo in error message: "canceld" should be "canceled"
The error message misspells "canceled" as "canceld". Please correct the spelling to improve clarity and professionalism. This typo occurs in multiple places throughout the code.
Apply this diff to correct the typo:
- err = status.WrapWithCanceled( - mirror.RegisterRPCName+" API canceld", err, reqInfo, resInfo, + err = status.WrapWithCanceled( + mirror.RegisterRPCName+" API canceled", err, reqInfo, resInfo,Likely invalid or redundant comment.
* fix: use FromError function when stream rpc error occurs Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: delete ParseError of Register RPC error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: crud rpc error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: add code comment and refactor multi crud error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: deleted unnecessary error resource info Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: typo Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: status code when the parse is failed Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: ignore parse success variable Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: use FromError function for stream error Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: error status handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: typo Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> --------- Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> Co-authored-by: Kosuke Morimoto <ksk@vdaas.org>
* fix: use FromError function when stream rpc error occurs * fix: delete ParseError of Register RPC error handling * fix: crud rpc error handling * fix: add code comment and refactor multi crud error handling * fix: deleted unnecessary error resource info * fix: typo * fix: status code when the parse is failed * fix: ignore parse success variable * fix: use FromError function for stream error * fix: error status handling * fix: typo --------- Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> Co-authored-by: Kosuke Morimoto <ksk@vdaas.org> Co-authored-by: Yusuke Kato <kpango@vdaas.org>
* fix: use FromError function when stream rpc error occurs Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: delete ParseError of Register RPC error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: crud rpc error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: add code comment and refactor multi crud error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: deleted unnecessary error resource info Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: typo Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: status code when the parse is failed Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: ignore parse success variable Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: use FromError function for stream error Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: error status handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: typo Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> --------- Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> Co-authored-by: Kosuke Morimoto <ksk@vdaas.org>
* fix: use FromError function when stream rpc error occurs Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: delete ParseError of Register RPC error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: crud rpc error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: add code comment and refactor multi crud error handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: deleted unnecessary error resource info Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: typo Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: status code when the parse is failed Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: ignore parse success variable Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: use FromError function for stream error Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: error status handling Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> * fix: typo Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> --------- Signed-off-by: hlts2 <hiroto.funakoshi.hiroto@gmail.com> Co-authored-by: Kosuke Morimoto <ksk@vdaas.org>
Description
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
registers,Connect, andDisconnectmethods for improved robustness.