Backport PR #2669 to release/v1.7 for Fix gRPC error handling for gateway/filter handler #2689
Conversation
* use FromError Signed-off-by: Kosuke Morimoto <[email protected]> * fix Signed-off-by: Kosuke Morimoto <[email protected]> * fix Signed-off-by: Kosuke Morimoto <[email protected]> --------- Signed-off-by: Kosuke Morimoto <[email protected]>
Deploying vald with
|
| Latest commit: |
251d6d3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://00beacd1.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-refact-jtff.vald.pages.dev |
📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing error handling and logging within 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/gateway/filter/handler/grpc/handler.go (32 hunks)
🧰 Additional context used
🔇 Additional comments (3)
pkg/gateway/filter/handler/grpc/handler.go (3)
2016-2019: Ensure Consistent Error Logging in Stream MethodsIn the
StreamLinearSearchByIDandStreamUpsertmethods, the error handling for the bidirectional stream correctly records errors and sets span attributes. However, ensure that the error handling is consistent across all stream methods, including checking forspan != nilbefore recording errors to avoid potential nil pointer dereferences.Also applies to: 2787-2790, 2936-2939
1354-1357: Validate Updated Error Handling in Search MethodsThe updated error handling in search-related methods enhances robustness by properly extracting the status from errors and recording them. Ensure that all error cases are appropriately handled and that the status codes accurately reflect the errors encountered.
Also applies to: 1437-1440, 1526-1529, 1573-1576, 1630-1633, 1685-1688, 1945-1948, 1997-2000, 2055-2058, 2110-2113
758-761: Check for Potential Race Conditions in Concurrent Error HandlingIn methods using goroutines and errgroup, such as
MultiInsertObject,MultiUpdateObject, andMultiUpsertObject, error handling occurs within concurrent executions. Ensure that access to shared variables likeerrsis properly synchronized to prevent race conditions.Run the following script to check for race conditions:
Also applies to: 997-1000, 1244-1247, 2354-2357, 2586-2589, 2827-2830, 2975-2978
| st, _ := status.FromError(err) | ||
| if st != nil && sspan != nil { | ||
| sspan.RecordError(err) | ||
| sspan.SetAttributes(trace.FromGRPCStatus(st.Code(), msg)...) | ||
| sspan.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Refactoring Repeated Error Handling into a Helper Function
The error handling logic involving status.FromError(err), checking st != nil && span != nil, recording the error, and setting attributes is repeated across multiple methods such as MultiSearchObject, StreamSearchObject, MultiLinearSearchObject, StreamLinearSearchObject, InsertObject, UpdateObject, UpsertObject, and others. This repetition can be refactored into a helper function to reduce code duplication and improve maintainability.
Example of a helper function:
func recordSpanError(span trace.Span, err error) {
if span != nil {
st, _ := status.FromError(err)
if st != nil {
span.RecordError(err)
span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...)
span.SetStatus(trace.StatusError, err.Error())
}
}
}Usage in your methods:
recordSpanError(span, err)Also applies to: 285-288, 467-470, 517-520, 699-702, 758-761, 939-942, 997-1000, 1186-1189, 1244-1247, 1354-1357, 1437-1440, 1526-1529, 1573-1576, 1630-1633, 1685-1688, 1945-1948, 1997-2000, 2016-2019, 2055-2058, 2110-2113, 2297-2300, 2354-2357, 2535-2538, 2586-2589, 2768-2771, 2787-2790, 2827-2830, 2917-2920, 2936-2939, 2975-2978, 3163-3166
| st, _ := status.FromError(err) | ||
| if st != nil && sspan != nil { | ||
| sspan.RecordError(err) | ||
| sspan.SetAttributes(trace.FromGRPCStatus(st.Code(), msg)...) | ||
| sspan.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) |
There was a problem hiding this comment.
Handle Possible Nil Pointer Dereference
In StreamInsertObject, StreamUpdateObject, and StreamUpsertObject, when calling sspan.RecordError(err), ensure that sspan is not nil to prevent possible nil pointer dereferences.
Apply this diff to add a nil check:
if st != nil && sspan != nil {
sspan.RecordError(err)
+} else if sspan != nil {
+ sspan.RecordError(err)
}Also applies to: 939-942, 1186-1189
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v1.7 #2689 +/- ##
===============================================
Coverage ? 24.09%
===============================================
Files ? 539
Lines ? 46638
Branches ? 0
===============================================
Hits ? 11239
Misses ? 34622
Partials ? 777 ☔ View full report in Codecov by Sentry. |
kpango
deleted the
backport/release/v1.7/refactor/filter-grpc-status-handling
branch
Description
use FromError instead of ParseError
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Bug Fixes
Improvements