Backport PR #2665 to to release/v1.7 for Fix gRPC error handling for mirror-gateway handler #2681
Conversation
* fix: use FromError function when stream rpc error occurs Signed-off-by: hlts2 <[email protected]> * fix: delete ParseError of Register RPC error handling Signed-off-by: hlts2 <[email protected]> * fix: crud rpc error handling Signed-off-by: hlts2 <[email protected]> * fix: add code comment and refactor multi crud error handling Signed-off-by: hlts2 <[email protected]> * fix: deleted unnecessary error resource info Signed-off-by: hlts2 <[email protected]> * fix: typo Signed-off-by: hlts2 <[email protected]> * fix: status code when the parse is failed Signed-off-by: hlts2 <[email protected]> * fix: ignore parse success variable Signed-off-by: hlts2 <[email protected]> * fix: use FromError function for stream error Signed-off-by: hlts2 <[email protected]> * fix: error status handling Signed-off-by: hlts2 <[email protected]> * fix: typo Signed-off-by: hlts2 <[email protected]> --------- Signed-off-by: hlts2 <[email protected]> Co-authored-by: Kosuke Morimoto <[email protected]>
📝 WalkthroughWalkthroughThe changes in this pull request primarily enhance error handling and logging mechanisms within the gRPC server methods of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
pkg/gateway/mirror/service/mirror.go (3)
Line range hint
228-239: Handle Errors When Disconnecting on Unimplemented CodeWhen an
Unimplementederror is received (line 228), the code attempts to disconnect from the target. However, ifnet.SplitHostPort(target)fails, the error handling could be improved by providing more context.Enhance error handling and logging:
if st.Code() == codes.Unimplemented { host, port, err := net.SplitHostPort(target) if err != nil { - log.Warn(err) + log.Warnf("Failed to split host and port for target '%s': %v", target, 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) + log.Errorf("Failed to disconnect from target '%s': %v", target, err) } } }
Line range hint
242-246: Standardize Error Logging MessagesThe log message on line 242 uses inconsistent formatting. Standardizing log messages improves readability and maintainability.
Update the log message format:
-log.Errorf("failed to send Register API to %s\t: %v", target, err) +log.Errorf("Failed to send Register API to target '%s': %v", target, err)
Line range hint
273-283: Ensure All Targets are Returned Despite ErrorsIn lines 273-283, when handling
errors.ErrGRPCClientConnNotFound, the function returnsnilforresTgts, even though some targets might have been successfully processed. This could lead to loss of valid data.Modify the return statement to ensure all gathered targets are returned:
if errors.Is(err, errors.ErrGRPCClientConnNotFound("*")) { err = status.WrapWithInternal( mirror.RegisterRPCName+" API connection not found", err, ) log.Warn(err) if span != nil { span.RecordError(err) span.SetAttributes(trace.StatusCodeInternal(err.Error())...) span.SetStatus(trace.StatusError, err.Error()) } - return nil, err + return resTgts, err }By returning
resTgtsinstead ofnil, the function preserves any successfully retrieved targets despite the connection error.pkg/gateway/mirror/handler/grpc/handler.go (2)
394-398: Simplify unlikely nil check conditionThe condition
st == nil || st.Message() == ""is unlikely to occur, as noted in the comment. Simplifying or removing this check can streamline the code.
1591-1602: Ensure consistent error loggingIn lines 1591-1602, after wrapping the error, the log message lacks additional context compared to similar logging elsewhere. Including more context can aid in debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- pkg/gateway/mirror/handler/grpc/handler.go (37 hunks)
- pkg/gateway/mirror/service/mirror.go (4 hunks)
🧰 Additional context used
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v1.7 #2681 +/- ##
===============================================
Coverage ? 23.92%
===============================================
Files ? 539
Lines ? 46937
Branches ? 0
===============================================
Hits ? 11231
Misses ? 34928
Partials ? 778 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
pkg/gateway/mirror/service/mirror.go (5)
199-214: Improve error context instatus.WrapWith*functionsIn the error handling blocks at lines 199, 204, 209, and 214, you are wrapping errors using
status.WrapWith*functions, which is appropriate. However, consider adding more contextual information to the error messages to aid in debugging. Including details about the request or target could provide valuable insights when investigating issues.
218-224: Ensure comprehensive logging of non-gRPC errorsAt lines 218-224, when
status.FromError(err)returns anilstatus or empty message, you log an error indicating that the call did not return a gRPC status error. While you handle this scenario to prevent nil pointer dereferences, consider logging the original error with more context to aid in troubleshooting unexpected error types.
273-274: Consistent error wrapping for connection not foundAt lines 273-274, you wrap the error when a connection is not found:
err = status.WrapWithInternal( mirror.RegisterRPCName+" API connection not found", err, )Ensure that this error wrapping is consistent with how similar errors are handled elsewhere in the codebase. Consistent error messages improve maintainability and debugging efficiency.
283-283: Provide additional context in error logsAt line 283, you log the error without additional context:
log.Error(err)Consider providing more information in the log message to clarify where and why the error occurred. For example:
log.Errorf("failed during %s: %v", mirror.RegisterRPCName, err)This adds valuable context that can assist in debugging.
286-289: Enhance error message clarity instatus.ParseErrorIn the error wrapping at lines 286-289:
st, msg, err := status.ParseError(err, codes.Internal, "failed to parse "+mirror.RegisterRPCName+" gRPC error response", )Ensure that the error message clearly conveys the nature of the failure. Including specific details from the original error can make troubleshooting more straightforward.
pkg/gateway/mirror/handler/grpc/handler.go (2)
120-131: Increase test coverage for new error handling blocksThe newly added error handling blocks in the
Registermethod are not covered by unit tests. Improving test coverage ensures that these error scenarios are properly handled and prevents regressions in the future.Consider adding unit tests that:
- Trigger the default case in the error handling switch statement by simulating unexpected errors.
- Verify that the
status.WrapWithInternalfunction wraps errors correctly.- Check that the error details (
BadRequest,FieldViolations) are populated as expected.Would you like assistance in creating these tests or opening a GitHub issue to track this task?
Also applies to: 145-145
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-123: pkg/gateway/mirror/handler/grpc/handler.go#L120-L123
Added lines #L120 - L123 were not covered by tests
[warning] 125-129: pkg/gateway/mirror/handler/grpc/handler.go#L125-L129
Added lines #L125 - L129 were not covered by tests
[warning] 131-131: pkg/gateway/mirror/handler/grpc/handler.go#L131
Added line #L131 was not covered by tests
393-401: Increase test coverage for stream error handlingThe added error handling code within streaming methods like
StreamSearch,StreamSearchByID, and others is not covered by tests. Ensuring these paths are tested is crucial for robust error handling in streaming scenarios.Consider:
- Adding integration tests that simulate errors during streaming operations to trigger the new error handling code.
- Verifying that the server responds with appropriate gRPC status codes and error messages when errors occur.
Would you like assistance in writing these tests or creating a GitHub issue to address this?
Also applies to: 418-426, 452-460, 477-485, 781-789, 806-814, 845-853, 870-878
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 393-394: pkg/gateway/mirror/handler/grpc/handler.go#L393-L394
Added lines #L393 - L394 were not covered by tests
[warning] 396-397: pkg/gateway/mirror/handler/grpc/handler.go#L396-L397
Added lines #L396 - L397 were not covered by tests
[warning] 401-401: pkg/gateway/mirror/handler/grpc/handler.go#L401
Added line #L401 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- pkg/gateway/mirror/handler/grpc/handler.go (37 hunks)
- pkg/gateway/mirror/service/mirror.go (4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/gateway/mirror/handler/grpc/handler.go
[warning] 120-123: pkg/gateway/mirror/handler/grpc/handler.go#L120-L123
Added lines #L120 - L123 were not covered by tests
[warning] 125-129: pkg/gateway/mirror/handler/grpc/handler.go#L125-L129
Added lines #L125 - L129 were not covered by tests
[warning] 131-131: pkg/gateway/mirror/handler/grpc/handler.go#L131
Added line #L131 was not covered by tests
[warning] 145-145: pkg/gateway/mirror/handler/grpc/handler.go#L145
Added line #L145 was not covered by tests
[warning] 393-394: pkg/gateway/mirror/handler/grpc/handler.go#L393-L394
Added lines #L393 - L394 were not covered by tests
[warning] 396-397: pkg/gateway/mirror/handler/grpc/handler.go#L396-L397
Added lines #L396 - L397 were not covered by tests
[warning] 401-401: pkg/gateway/mirror/handler/grpc/handler.go#L401
Added line #L401 was not covered by tests
[warning] 418-419: pkg/gateway/mirror/handler/grpc/handler.go#L418-L419
Added lines #L418 - L419 were not covered by tests
[warning] 421-422: pkg/gateway/mirror/handler/grpc/handler.go#L421-L422
Added lines #L421 - L422 were not covered by tests
[warning] 426-426: pkg/gateway/mirror/handler/grpc/handler.go#L426
Added line #L426 was not covered by tests
[warning] 452-453: pkg/gateway/mirror/handler/grpc/handler.go#L452-L453
Added lines #L452 - L453 were not covered by tests
[warning] 455-456: pkg/gateway/mirror/handler/grpc/handler.go#L455-L456
Added lines #L455 - L456 were not covered by tests
[warning] 460-460: pkg/gateway/mirror/handler/grpc/handler.go#L460
Added line #L460 was not covered by tests
[warning] 477-478: pkg/gateway/mirror/handler/grpc/handler.go#L477-L478
Added lines #L477 - L478 were not covered by tests
[warning] 480-481: pkg/gateway/mirror/handler/grpc/handler.go#L480-L481
Added lines #L480 - L481 were not covered by tests
[warning] 485-485: pkg/gateway/mirror/handler/grpc/handler.go#L485
Added line #L485 was not covered by tests
[warning] 781-782: pkg/gateway/mirror/handler/grpc/handler.go#L781-L782
Added lines #L781 - L782 were not covered by tests
[warning] 784-785: pkg/gateway/mirror/handler/grpc/handler.go#L784-L785
Added lines #L784 - L785 were not covered by tests
[warning] 789-789: pkg/gateway/mirror/handler/grpc/handler.go#L789
Added line #L789 was not covered by tests
[warning] 806-807: pkg/gateway/mirror/handler/grpc/handler.go#L806-L807
Added lines #L806 - L807 were not covered by tests
[warning] 809-810: pkg/gateway/mirror/handler/grpc/handler.go#L809-L810
Added lines #L809 - L810 were not covered by tests
[warning] 814-814: pkg/gateway/mirror/handler/grpc/handler.go#L814
Added line #L814 was not covered by tests
[warning] 845-846: pkg/gateway/mirror/handler/grpc/handler.go#L845-L846
Added lines #L845 - L846 were not covered by tests
[warning] 848-849: pkg/gateway/mirror/handler/grpc/handler.go#L848-L849
Added lines #L848 - L849 were not covered by tests
[warning] 853-853: pkg/gateway/mirror/handler/grpc/handler.go#L853
Added line #L853 was not covered by tests
[warning] 870-871: pkg/gateway/mirror/handler/grpc/handler.go#L870-L871
Added lines #L870 - L871 were not covered by tests
[warning] 873-874: pkg/gateway/mirror/handler/grpc/handler.go#L873-L874
Added lines #L873 - L874 were not covered by tests
[warning] 878-878: pkg/gateway/mirror/handler/grpc/handler.go#L878
Added line #L878 was not covered by tests
[warning] 1045-1046: pkg/gateway/mirror/handler/grpc/handler.go#L1045-L1046
Added lines #L1045 - L1046 were not covered by tests
[warning] 1048-1049: pkg/gateway/mirror/handler/grpc/handler.go#L1048-L1049
Added lines #L1048 - L1049 were not covered by tests
🔇 Additional comments (1)
pkg/gateway/mirror/service/mirror.go (1)
Line range hint
228-236: Verify handling ofcodes.Unimplementedto prevent unintended disconnectionsAt lines 228-236, when the status code is
codes.Unimplemented, the code attempts to disconnect from the target:if st.Code() == codes.Unimplemented { // ... 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) } }Ensure that this behavior is appropriate and that
codes.Unimplementederrors only occur in scenarios where disconnection is intended. It's important to confirm that valid connections are not being disconnected due to unexpectedUnimplementederrors.Run the following script to identify where
codes.Unimplementedis returned in the codebase:This will help verify if there are any cases where
codes.Unimplementedcould be returned unintentionally, leading to undesired disconnections.✅ Verification successful
Handling of
codes.UnimplementedVerifiedThe usage of
codes.Unimplementedis confined to auto-generated gRPC server interfaces for unimplemented methods. Therefore, the disconnection logic inmirror.gotriggered bycodes.Unimplementedis appropriate and does not lead to unintended disconnections.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all occurrences of `codes.Unimplemented` being returned. # Test: Find all instances where `codes.Unimplemented` is used in returning errors. rg --type go 'status\.New\(codes\.Unimplemented' -A 5 -B 5 # Test: Find all instances where `status.Error` or `status.Errorf` is called with `codes.Unimplemented`. rg --type go 'status\.Errorf?\(codes\.Unimplemented' -A 5 -B 5Length of output: 35012
Deploying vald with
|
| Latest commit: |
011b2ad
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://358c835f.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-fix-mi-xwc2.vald.pages.dev |
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
registers,Connect, andDisconnectmethods for improved robustness.