Backport PR #2663 to to release/v1.7 for Fix gRPC error msg handling for lb-gateway handler #2682
Conversation
* ♻️ refactor: replace ParseError with FromError Signed-off-by: vankichi <[email protected]> * ♻️ fix Signed-off-by: vankichi <[email protected]> * ♻️ fix Signed-off-by: vankichi <[email protected]> * ♻️ fix Signed-off-by: vankichi <[email protected]> --------- Signed-off-by: vankichi <[email protected]> Co-authored-by: Kosuke Morimoto <[email protected]>
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- pkg/gateway/lb/handler/grpc/aggregation.go (8 hunks)
- pkg/gateway/lb/handler/grpc/handler.go (64 hunks)
🧰 Additional context used
🔇 Additional comments (21)
pkg/gateway/lb/handler/grpc/aggregation.go (8)
34-35: Approved: Importing Observability PackagesThe addition of the
attributeandtracepackages is appropriate for enhancing observability.
266-272: Duplicate Error Handling LogicSimilar to the previous comment, this error handling code is duplicated. Refactoring as suggested earlier will improve maintainability.
287-293: Duplicate Error Handling LogicThis block also contains duplicated error handling code. Consider refactoring to reduce redundancy.
308-316: Duplicate Error Handling LogicThe error handling code here is similar to earlier instances. Refactoring can help reduce code duplication.
332-338: Duplicate Error Handling LogicThis is another instance of duplicated error handling code. Refactoring as suggested will enhance code clarity and maintainability.
Line range hint
345-369: Duplicate Error Handling LogicThe error handling logic in these lines is repetitive. Refactoring to use a helper function can improve maintainability and reduce code duplication.
372-372: Ensureattrsis Appropriately InitializedIn the success path at the end of the function,
attrsmay benil. Ensure that downstream code can handle anilslice without issues.If necessary, initialize
attrsto an empty slice before returning:if attrs == nil { attrs = []attribute.KeyValue{} } return res, attrs, nil
60-60: Verify Callers Handle Updated Function SignatureThe
aggregationSearchmethod now returns an additionalattrs []attribute.KeyValue. Please ensure that all callers of this method are updated to handle the new return value appropriately.Run the following script to identify and verify callers of
aggregationSearch:pkg/gateway/lb/handler/grpc/handler.go (13)
42-43: Add necessary imports for tracing functionalityThe addition of
observability/attributeandobservability/traceimports is appropriate for the enhanced tracing and error handling implemented in this PR.
313-320: Correctly propagate attributes fromdoSearchand set tracing informationThe changes in the
Searchmethod properly captureattrsreturned fromdoSearchand use them to set span attributes, enhancing tracing and observability.
370-373:⚠️ Potential issueEnsure proper handling of
status.FromErrorinSearchByIDSimilar to an earlier issue, the boolean return value from
status.FromError(err)is ignored. This may lead to a nil pointer dereference ifstisnil.Apply this diff to handle the boolean return value correctly:
- st, _ := status.FromError(err) - if span != nil && st != nil && st.Code() != codes.NotFound { + st, ok := status.FromError(err) + if span != nil && ok && st.Code() != codes.NotFound { span.RecordError(err) span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) span.SetStatus(trace.StatusError, err.Error()) }
784-788:⚠️ Potential issueHandle
status.FromErrorboolean return value inLinearSearchByIDEnsure that the boolean return value from
status.FromError(err)is handled to prevent potential nil pointer dereference when accessingst.Code().
1635-1638:⚠️ Potential issueProper error handling in
UpdatemethodIn the
Updatemethod, handle the boolean return value fromstatus.FromError(err)to ensurestis notnilbefore accessing its methods.
2110-2113:⚠️ Potential issueEnsure safe usage of
status.FromErrorinUpdateTimestampIn the
UpdateTimestampmethod, handle the boolean return value fromstatus.FromError(err)to avoid nil pointer dereference.
2511-2514:⚠️ Potential issueHandle
status.FromErrorreturn value inRemovemethodIn the
Removemethod, correctly handle the boolean return value fromstatus.FromError(err)to prevent accessing anilst.
3148-3151:⚠️ Potential issueProper handling of
status.FromErrorinGetObjectEnsure that the boolean return value from
status.FromError(err)is handled in theGetObjectmethod to avoid potential nil pointer dereference.
3410-3413:⚠️ Potential issueHandle
status.FromErrorreturn value inIndexInfoIn the
IndexInfomethod, handle the boolean return value fromstatus.FromError(err)to ensure safe error handling.
3541-3544:⚠️ Potential issueEnsure correct usage of
status.FromErrorinIndexDetailHandle the boolean return value from
status.FromError(err)in theIndexDetailmethod to prevent nil pointer dereference.
3707-3710:⚠️ Potential issueAvoid nil pointer dereference in
GetTimestampby handlingstatus.FromErrorreturn valueIn the
GetTimestampmethod, ensure proper handling of the boolean return value fromstatus.FromError(err).
3874-3877:⚠️ Potential issueHandle boolean return value from
status.FromErrorinIndexStatisticsDetailProperly handle the boolean return value from
status.FromError(err)in theIndexStatisticsDetailmethod to avoid potential nil pointer dereference.
4112-4115:⚠️ Potential issueEnsure safe usage of
status.FromErrorinIndexPropertyIn the
IndexPropertymethod, handle the boolean return value fromstatus.FromError(err)to prevent accessing anilst.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v1.7 #2682 +/- ##
===============================================
Coverage ? 24.09%
===============================================
Files ? 539
Lines ? 46983
Branches ? 0
===============================================
Hits ? 11322
Misses ? 34892
Partials ? 769 ☔ View full report in Codecov by Sentry. |
kpango
deleted the
backport/release/v1.7/refactor/pkg/fix-handler-gRPC-errro-handling
branch
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor