Backport PR #2667 to release/v1.7 for modify ParseError to FromError for agent handler #2679
Conversation
* modify ParseError to FromError * fix typo * fix typo * fix typo * fix typo * fix imported and not used error --------- Co-authored-by: Kosuke Morimoto <ksk@vdaas.org>
Deploying vald with
|
| Latest commit: |
ae506e1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3bbccfb6.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-refact-vvwr.vald.pages.dev |
📝 WalkthroughWalkthroughThe changes in this pull request involve significant modifications to the error handling and logging mechanisms in 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
pkg/agent/core/ngt/handler/grpc/object.go (2)
173-176: Consistent error handling improvement in StreamListObjectThe change from
status.ParseErrortostatus.FromErrorin this method is consistent with the earlier modification inStreamGetObject. This improves the overall consistency of error handling in the file.For even better consistency, consider updating the error handling in the
StreamListObjectmethod's loop (around line 250) to usestatus.FromErroras well.
Line range hint
1-324: Overall improvement in error handlingThe changes in this file successfully implement the PR objective of replacing
ParseErrorwithFromErrorfor error handling in the agent handler. These modifications improve code consistency and simplify error processing across theStreamGetObjectandStreamListObjectmethods.To further enhance consistency, consider updating the error handling in the
StreamListObjectmethod's loop (around line 250) to usestatus.FromErroras well. This would ensure uniform error handling throughout the file.pkg/agent/core/ngt/handler/grpc/insert.go (1)
Line range hint
256-265: Improved error handling for already existing UUIDs in MultiInsert functionThe changes to the error handling logic in the
MultiInsertfunction, particularly for handling already existing UUIDs, are well-implemented. Encapsulating this logic into a separate anonymous function improves readability and maintainability.For consistency with the other error handling cases in this function, consider extracting the anonymous function to a named function at the package level. This would further improve readability and make the error handling pattern more uniform across the function.
Consider refactoring the anonymous function to a named function at the package level:
func collectExistingIDs(uuids []string, err error) []string { aids := make([]string, 0, len(uuids)) for _, id := range uuids { if errors.Is(err, errors.ErrUUIDAlreadyExists(id)) { aids = append(aids, id) } } return aids }Then use it in the
MultiInsertfunction:} else if alreadyExistsIDs := collectExistingIDs(uuids, err); len(alreadyExistsIDs) != 0 { // ... rest of the error handling ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- pkg/agent/core/ngt/handler/grpc/insert.go (2 hunks)
- pkg/agent/core/ngt/handler/grpc/linear_search.go (8 hunks)
- pkg/agent/core/ngt/handler/grpc/object.go (2 hunks)
- pkg/agent/core/ngt/handler/grpc/remove.go (3 hunks)
- pkg/agent/core/ngt/handler/grpc/search.go (8 hunks)
- pkg/agent/core/ngt/handler/grpc/update.go (2 hunks)
- pkg/agent/core/ngt/handler/grpc/upsert.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (18)
pkg/agent/core/ngt/handler/grpc/object.go (1)
154-157: Improved error handling in StreamGetObjectThe change from
status.ParseErrortostatus.FromErroris a good improvement. It simplifies the error handling logic while maintaining the existing functionality. This modification aligns well with the PR objective and should lead to more efficient error processing.pkg/agent/core/ngt/handler/grpc/insert.go (2)
165-168: Improved error handling in StreamInsert functionThe changes to the error handling logic in the
StreamInsertfunction are well-implemented. The use ofstatus.FromErrorinstead ofstatus.ParseErrorsimplifies the code and makes it more consistent with other parts of the codebase. This approach directly uses the returned status to set span attributes and status, which is more efficient and less error-prone.Also applies to: 184-187
Line range hint
1-324: Overall improvements in error handling and loggingThe changes made to this file are well-implemented and align with the PR objectives. The error handling has been streamlined and made more consistent across the
StreamInsertandMultiInsertfunctions. These improvements enhance the code's readability, maintainability, and error reporting capabilities.pkg/agent/core/ngt/handler/grpc/search.go (7)
350-353: Improved error handling and span recordingThe changes in this segment enhance the error handling process by replacing
status.ParseErrorwithstatus.FromError. This modification simplifies the code and potentially improves performance by avoiding unnecessary parsing. Additionally, the span recording logic is now more robust with proper nil checks.
369-372: Consistent error handling improvementThese changes align with the previous modifications, ensuring consistent error handling throughout the
StreamSearchfunction. The use ofstatus.FromErrorand the conditional span recording maintain the improved approach to error management.
397-400: Uniform error handling across search functionsThe changes in the
StreamSearchByIDfunction mirror those inStreamSearch, demonstrating a consistent approach to error handling and span recording across different search operations. This uniformity enhances code maintainability and readability.
416-419: Consistent error handling at function levelThese changes in the
StreamSearchByIDfunction's error handling section maintain the improved approach seen in previous modifications. The consistent application ofstatus.FromErrorand conditional span recording across different parts of the function enhances overall code quality and error management.
457-460: Improved error handling in concurrent operationsThe changes in the
MultiSearchfunction apply the same improved error handling approach within a concurrent context. This ensures that the benefits of usingstatus.FromErrorand conditional span recording are maintained even in parallel operations, contributing to more robust and consistent error management across different execution scenarios.
478-484: Consistent error handling with return statement modificationThe changes in this segment of the
MultiSearchfunction maintain the improved error handling approach seen throughout the file. Additionally, the return statement has been modified to returnnilinstead ofresin case of errors. This change ensures that no partial results are returned when errors occur, which is a more appropriate behavior for error cases.
Line range hint
519-546: Uniform improvements across search functionsThe changes in the
MultiSearchByIDfunction mirror those inMultiSearch, demonstrating a consistent approach to error handling, span recording, and result handling across different search operations. This uniformity extends to the modification of the return statement in error cases, ensuring that all search functions behave consistently when errors occur. Such consistency significantly enhances the maintainability and reliability of the codebase.pkg/agent/core/ngt/handler/grpc/upsert.go (3)
153-156: Ensure complete error information is preserved when usingstatus.FromErrorThe change from
status.ParseErrortostatus.FromErroralters how error details are extracted. Verify that all necessary error information, such as custom messages and error codes, is still being captured and logged appropriately. This ensures that error handling remains robust and informative.
172-175: Confirm accurate span attributes after replacingstatus.ParseErrorwithstatus.FromErrorWith the switch to
status.FromError, please ensure that the span attributes set bytrace.FromGRPCStatus(st.Code(), st.Message())...accurately reflect the error status. This is crucial for effective tracing and debugging.
319-322: Validate error handling logic inMultiUpsertafter modifying error parsingIn the
MultiUpsertfunction, the error parsing has been updated. Ensure thatstatus.FromError(err)provides all the necessary error details for proper error handling and that the span records accurately represent any errors encountered.pkg/agent/core/ngt/handler/grpc/linear_search.go (5)
346-349: Consistent error handling withstatus.FromErrorThe use of
status.FromError(err)in theStreamLinearSearchfunction ensures consistent extraction of gRPC status from errors. The nil checks forstandsspanbefore accessingst.Code()andst.Message()prevent potential nil pointer dereferences.
365-368: Appropriate error handling in the outer scopeIn the outer scope of
StreamLinearSearch, the error handling correctly extracts the status fromerrand updates the span attributes accordingly. This maintains consistency with the inner function.
395-398: Error handling inStreamLinearSearchByIDis well-implementedThe error extraction and span updates in
StreamLinearSearchByIDare correctly implemented. The checks for nil values ensure safe access to status codes and messages.
414-417: Consistent outer error handling inStreamLinearSearchByIDThe outer error handling in
StreamLinearSearchByIDmirrors the pattern used elsewhere, ensuring consistent and reliable error reporting.
517-520: Proper error handling inMultiLinearSearchByIDThe error extraction and tracing in
MultiLinearSearchByIDare correctly handled, ensuring that errors are appropriately recorded and statuses are set.
| st, _ := status.FromError(err) | ||
| if st != nil && span != nil { | ||
| span.RecordError(err) | ||
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), msg)...) | ||
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) |
There was a problem hiding this comment.
Check the ok value returned by status.FromError
In this error handling block, similar to the previous instance, the ok value returned from status.FromError(err) is ignored. It is important to verify that the error was successfully converted before using the status object.
Update the code as follows:
-st, _ := status.FromError(err)
+st, ok := status.FromError(err)
-if st != nil && span != nil {
+if ok && span != nil {By checking ok, you ensure that st is a valid gRPC status, preventing potential issues arising from using an invalid status object.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| st, _ := status.FromError(err) | |
| if st != nil && span != nil { | |
| span.RecordError(err) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), msg)...) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | |
| st, ok := status.FromError(err) | |
| if ok && span != nil { | |
| span.RecordError(err) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) |
| st, _ := status.FromError(errs) | ||
| log.Error(errs) | ||
| if st != nil && span != nil { | ||
| span.RecordError(errs) | ||
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | ||
| span.SetStatus(trace.StatusError, errs.Error()) | ||
| } | ||
| return nil, err | ||
| return nil, errs |
There was a problem hiding this comment.
Verify successful conversion when using status.FromError
In the RemoveByTimestamp function, the code calls status.FromError(errs) but does not check the ok value. This could result in using an invalid status object if the conversion fails.
Please modify the code to include the ok check:
-st, _ := status.FromError(errs)
+st, ok := status.FromError(errs)
-if st != nil && span != nil {
+if ok && span != nil {Ensuring that ok is true before using st helps maintain robust error handling and prevents potential nil pointer dereferences.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| st, _ := status.FromError(errs) | |
| log.Error(errs) | |
| if st != nil && span != nil { | |
| span.RecordError(errs) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | |
| span.SetStatus(trace.StatusError, errs.Error()) | |
| } | |
| return nil, err | |
| return nil, errs | |
| st, ok := status.FromError(errs) | |
| log.Error(errs) | |
| if ok && span != nil { | |
| span.RecordError(errs) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | |
| span.SetStatus(trace.StatusError, errs.Error()) | |
| } | |
| return nil, errs |
| 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.
Ensure proper handling of status.FromError conversion
In the StreamRemove function, you are calling status.FromError(err) and assigning the result to st, _. The second return value indicates whether the error was successfully converted to a gRPC status. Currently, the code does not check this value, which could lead to incorrect behavior if the conversion fails.
Consider modifying the code to check the ok value before using st:
-st, _ := status.FromError(err)
+st, ok := status.FromError(err)
-if st != nil && sspan != nil {
+if ok && sspan != nil {This change ensures that st is valid and prevents potential nil pointer dereferences.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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())...) | |
| st, ok := status.FromError(err) | |
| if ok && sspan != nil { | |
| sspan.RecordError(err) | |
| sspan.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) |
| st, _ := status.FromError(err) | ||
| if st != nil && span != nil { | ||
| span.RecordError(err) | ||
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), msg)...) | ||
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Check the ok value from status.FromError(err)
Consistent with the previous change, use the ok boolean to verify if the error contains a gRPC status.
Apply this change:
-st, _ := status.FromError(err)
-if st != nil && span != nil {
+st, ok := status.FromError(err)
+if ok && span != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| st, _ := status.FromError(err) | |
| if st != nil && span != nil { | |
| span.RecordError(err) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), msg)...) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | |
| st, ok := status.FromError(err) | |
| if ok && span != nil { | |
| span.RecordError(err) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) |
| 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
Suggestion: Use the ok value from status.FromError(err)
When calling status.FromError(err), it returns (*status.Status, bool). The boolean ok indicates whether the error can be represented as a gRPC status. Checking ok is more reliable than checking if st is not nil.
Apply this change:
-st, _ := status.FromError(err)
-if st != nil && sspan != nil {
+st, ok := status.FromError(err)
+if ok && sspan != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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())...) | |
| st, ok := status.FromError(err) | |
| if ok && sspan != nil { | |
| sspan.RecordError(err) | |
| sspan.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) |
| st, _ := status.FromError(errs) | ||
| if st != nil && span != nil { | ||
| span.RecordError(errs) | ||
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | ||
| span.SetStatus(trace.StatusError, errs.Error()) | ||
| } | ||
| return nil, err | ||
| return nil, errs |
There was a problem hiding this comment.
Handling combined errors safely
Similar to MultiLinearSearch, when dealing with errs that may contain multiple errors, it's important to ensure that status.FromError(errs) correctly handles combined errors to prevent nil pointer dereferences.
Consider updating the error handling as suggested earlier to process individual errors within errs.
| 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.
Potential issue with error status extraction
When using errors.Join to accumulate errors into errs, calling status.FromError(errs) may not correctly extract the gRPC status, as errs could be a composite error. This might result in st being nil, leading to a potential nil pointer dereference when accessing st.Code() and st.Message().
Consider iterating over the individual errors within errs to extract and handle their statuses separately. Here's a possible fix:
if err != nil {
mu.Lock()
if errs == nil {
errs = err
} else {
errs = errors.Join(errs, err)
}
mu.Unlock()
}At the point of handling errs:
if errs != nil {
- st, _ := status.FromError(errs)
- if st != nil && span != nil {
- span.RecordError(errs)
- span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...)
- span.SetStatus(trace.StatusError, errs.Error())
- }
+ if span != nil {
+ span.RecordError(errs)
+ var merr errors.ErrorList
+ if errors.As(errs, &merr) {
+ for _, e := range merr {
+ if st, _ := status.FromError(e); st != nil {
+ span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...)
+ }
+ }
+ } else {
+ if st, _ := status.FromError(errs); st != nil {
+ span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...)
+ }
+ }
+ span.SetStatus(trace.StatusError, errs.Error())
+ }
return nil, errs
}Committable suggestion was skipped due to low confidence.
| st, _ := status.FromError(errs) | ||
| if st != nil && span != nil { | ||
| span.RecordError(errs) | ||
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | ||
| span.SetStatus(trace.StatusError, errs.Error()) | ||
| } | ||
| return nil, err | ||
| return nil, errs |
There was a problem hiding this comment.
Potential nil pointer dereference with combined errors
In the MultiLinearSearch function, using status.FromError(errs) on a combined error may not extract the proper status. This could result in st being nil, leading to a nil pointer dereference.
Modify the error handling to safely extract statuses from individual errors:
if errs != nil {
- st, _ := status.FromError(errs)
- if st != nil && span != nil {
- span.RecordError(errs)
- span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...)
- span.SetStatus(trace.StatusError, errs.Error())
- }
+ if span != nil {
+ span.RecordError(errs)
+ var merr errors.ErrorList
+ if errors.As(errs, &merr) {
+ for _, e := range merr {
+ if st, _ := status.FromError(e); st != nil {
+ span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...)
+ }
+ }
+ } else {
+ if st, _ := status.FromError(errs); st != nil {
+ span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...)
+ }
+ }
+ span.SetStatus(trace.StatusError, errs.Error())
+ }
return nil, errs
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| st, _ := status.FromError(errs) | |
| if st != nil && span != nil { | |
| span.RecordError(errs) | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | |
| span.SetStatus(trace.StatusError, errs.Error()) | |
| } | |
| return nil, err | |
| return nil, errs | |
| if errs != nil { | |
| if span != nil { | |
| span.RecordError(errs) | |
| var merr errors.ErrorList | |
| if errors.As(errs, &merr) { | |
| for _, e := range merr { | |
| if st, _ := status.FromError(e); st != nil { | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | |
| } | |
| } | |
| } else { | |
| if st, _ := status.FromError(errs); st != nil { | |
| span.SetAttributes(trace.FromGRPCStatus(st.Code(), st.Message())...) | |
| } | |
| } | |
| span.SetStatus(trace.StatusError, errs.Error()) | |
| } | |
| return nil, errs | |
| } |
|
[CHATOPS:HELP] ChatOps commands.
|
vankichi
deleted the
backport/release/v1.7/refactor/pkg/fix-agent-handler
branch
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v1.7 #2679 +/- ##
===============================================
Coverage ? 24.02%
===============================================
Files ? 539
Lines ? 47117
Branches ? 0
===============================================
Hits ? 11322
Misses ? 35026
Partials ? 769 ☔ View full report in Codecov by Sentry. |
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
These changes collectively improve the reliability and maintainability of error handling within the application.