Fix gRPC error msg handling for lb-gateway handler #2663
Conversation
Deploying vald with
|
| Latest commit: |
0b221ed
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://68ecff9a.vald.pages.dev |
| Branch Preview URL: | https://refactor-pkg-fix-handler-grp.vald.pages.dev |
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve 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.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2663 +/- ##
=======================================
Coverage ? 23.93%
=======================================
Files ? 539
Lines ? 46937
Branches ? 0
=======================================
Hits ? 11234
Misses ? 34927
Partials ? 776 ☔ View full report in Codecov by Sentry. |
vankichi
force-pushed
the
refactor/pkg/fix-handler-gRPC-errro-handling
branch
from
3933a21 to
efbfe88
Compare
vankichi
force-pushed
the
refactor/pkg/fix-handler-gRPC-errro-handling
branch
from
efbfe88 to
0303819
Compare
Signed-off-by: vankichi <[email protected]>
vankichi
force-pushed
the
refactor/pkg/fix-handler-gRPC-errro-handling
branch
from
0303819 to
59a39bc
Compare
![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
🧹 Outside diff range and nitpick comments (11)
pkg/gateway/lb/handler/grpc/handler.go (10)
258-270: LGTM! Consider adding unit tests for this section.The error handling and status code assignment logic in this section are well-implemented. The use of
status.FromErroris a good practice for handling gRPC errors. However, this newly added code is not covered by tests.To improve the overall quality and maintainability of the codebase, consider adding unit tests to cover this section of the
Existsmethod, particularly focusing on different error scenarios and status code assignments.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 258-260: pkg/gateway/lb/handler/grpc/handler.go#L258-L260
Added lines #L258 - L260 were not covered by tests
[warning] 262-268: pkg/gateway/lb/handler/grpc/handler.go#L262-L268
Added lines #L262 - L268 were not covered by tests
[warning] 270-270: pkg/gateway/lb/handler/grpc/handler.go#L270
Added line #L270 was not covered by tests
313-313: LGTM! Consider adding unit tests for this section.The use of
doSearchwith a function argument is a good practice for code reuse and flexibility. However, this newly added code is not covered by tests.To ensure the reliability of the
Searchmethod, consider adding unit tests that cover this call todoSearch, including various scenarios for the function argument passed to it.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 313-313: pkg/gateway/lb/handler/grpc/handler.go#L313
Added line #L313 was not covered by tests
318-320: LGTM! Consider adding unit tests for error handling.The error handling and span recording in this section are appropriate for maintaining observability. However, this newly added code is not covered by tests.
To ensure robust error handling, consider adding unit tests that cover these error scenarios in the
Searchmethod. This will help verify that span attributes and statuses are correctly set when errors occur.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 318-318: pkg/gateway/lb/handler/grpc/handler.go#L318
Added line #L318 was not covered by tests
[warning] 320-320: pkg/gateway/lb/handler/grpc/handler.go#L320
Added line #L320 was not covered by tests
370-377: LGTM! Consider adding unit tests for fallback mechanism.The error handling and fallback mechanism to the agent's SearchByID method are well-implemented. The use of
status.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure the reliability of the
SearchByIDmethod, consider adding unit tests that cover this error handling and fallback mechanism. Specifically, test scenarios where the initial search fails and the fallback to the agent's SearchByID is triggered.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 370-370: pkg/gateway/lb/handler/grpc/handler.go#L370
Added line #L370 was not covered by tests
[warning] 377-377: pkg/gateway/lb/handler/grpc/handler.go#L377
Added line #L377 was not covered by tests
490-493: LGTM! Consider adding unit tests for error handling in StreamSearch.The error handling and span recording in the
StreamSearchmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure robust error handling in the streaming context, consider adding unit tests that cover these error scenarios in the
StreamSearchmethod. This will help verify that span attributes and statuses are correctly set when errors occur during streaming operations.
509-512: LGTM! Consider adding unit tests for final error handling in StreamSearch.The final error handling and span recording in the
StreamSearchmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure comprehensive error handling, consider adding unit tests that cover this final error check in the
StreamSearchmethod. This will help verify that span attributes and statuses are correctly set when errors occur at the end of the streaming operation.
537-540: LGTM! Consider adding unit tests for error handling in StreamSearchByID.The error handling and span recording in the
StreamSearchByIDmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure robust error handling in the streaming context, consider adding unit tests that cover these error scenarios in the
StreamSearchByIDmethod. This will help verify that span attributes and statuses are correctly set when errors occur during streaming operations.
556-559: LGTM! Consider adding unit tests for final error handling in StreamSearchByID.The final error handling and span recording in the
StreamSearchByIDmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure comprehensive error handling, consider adding unit tests that cover this final error check in the
StreamSearchByIDmethod. This will help verify that span attributes and statuses are correctly set when errors occur at the end of the streaming operation.
624-628: LGTM! Consider adding unit tests for error handling in MultiSearch.The error handling and span recording in the
MultiSearchmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure robust error handling for multiple search operations, consider adding unit tests that cover these error scenarios in the
MultiSearchmethod. This will help verify that span attributes and statuses are correctly set when errors occur during multiple search operations.
693-697: LGTM! Consider adding unit tests for error handling in MultiSearchByID.The error handling and span recording in the
MultiSearchByIDmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure robust error handling for multiple search-by-ID operations, consider adding unit tests that cover these error scenarios in the
MultiSearchByIDmethod. This will help verify that span attributes and statuses are correctly set when errors occur during multiple search-by-ID operations.pkg/gateway/lb/handler/grpc/aggregation.go (1)
34-34: Consider aliasing the importedsyncpackage to avoid confusionImporting the internal
syncpackage assyncmay cause confusion with Go's standard librarysyncpackage. To improve code clarity and prevent potential conflicts, consider aliasing the import. For example:import ( ... - "github.com/vdaas/vald/internal/sync" + internalSync "github.com/vdaas/vald/internal/sync" )This helps differentiate between the standard
syncpackage and the internal one.
📜 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 (57 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/gateway/lb/handler/grpc/aggregation.go
[warning] 60-60: pkg/gateway/lb/handler/grpc/aggregation.go#L60
Added line #L60 was not covered by tests
[warning] 239-239: pkg/gateway/lb/handler/grpc/aggregation.go#L239
Added line #L239 was not covered by tests
[warning] 242-242: pkg/gateway/lb/handler/grpc/aggregation.go#L242
Added line #L242 was not covered by tests
[warning] 245-245: pkg/gateway/lb/handler/grpc/aggregation.go#L245
Added line #L245 was not covered by tests
[warning] 266-266: pkg/gateway/lb/handler/grpc/aggregation.go#L266
Added line #L266 was not covered by tests
[warning] 269-269: pkg/gateway/lb/handler/grpc/aggregation.go#L269
Added line #L269 was not covered by tests
[warning] 272-272: pkg/gateway/lb/handler/grpc/aggregation.go#L272
Added line #L272 was not covered by tests
[warning] 287-287: pkg/gateway/lb/handler/grpc/aggregation.go#L287
Added line #L287 was not covered by tests
[warning] 290-290: pkg/gateway/lb/handler/grpc/aggregation.go#L290
Added line #L290 was not covered by tests
[warning] 293-293: pkg/gateway/lb/handler/grpc/aggregation.go#L293
Added line #L293 was not covered by tests
[warning] 308-308: pkg/gateway/lb/handler/grpc/aggregation.go#L308
Added line #L308 was not covered by tests
[warning] 311-311: pkg/gateway/lb/handler/grpc/aggregation.go#L311
Added line #L311 was not covered by tests
[warning] 316-316: pkg/gateway/lb/handler/grpc/aggregation.go#L316
Added line #L316 was not covered by tests
[warning] 332-332: pkg/gateway/lb/handler/grpc/aggregation.go#L332
Added line #L332 was not covered by tests
[warning] 335-335: pkg/gateway/lb/handler/grpc/aggregation.go#L335
Added line #L335 was not covered by tests
[warning] 338-338: pkg/gateway/lb/handler/grpc/aggregation.go#L338
Added line #L338 was not covered by tests
[warning] 345-345: pkg/gateway/lb/handler/grpc/aggregation.go#L345
Added line #L345 was not covered by tests
[warning] 348-348: pkg/gateway/lb/handler/grpc/aggregation.go#L348
Added line #L348 was not covered by tests
[warning] 363-363: pkg/gateway/lb/handler/grpc/aggregation.go#L363
Added line #L363 was not covered by tests
[warning] 366-366: pkg/gateway/lb/handler/grpc/aggregation.go#L366
Added line #L366 was not covered by tests
[warning] 369-369: pkg/gateway/lb/handler/grpc/aggregation.go#L369
Added line #L369 was not covered by tests
[warning] 372-372: pkg/gateway/lb/handler/grpc/aggregation.go#L372
Added line #L372 was not covered by testspkg/gateway/lb/handler/grpc/handler.go
[warning] 258-260: pkg/gateway/lb/handler/grpc/handler.go#L258-L260
Added lines #L258 - L260 were not covered by tests
[warning] 262-268: pkg/gateway/lb/handler/grpc/handler.go#L262-L268
Added lines #L262 - L268 were not covered by tests
[warning] 270-270: pkg/gateway/lb/handler/grpc/handler.go#L270
Added line #L270 was not covered by tests
[warning] 313-313: pkg/gateway/lb/handler/grpc/handler.go#L313
Added line #L313 was not covered by tests
[warning] 318-318: pkg/gateway/lb/handler/grpc/handler.go#L318
Added line #L318 was not covered by tests
[warning] 320-320: pkg/gateway/lb/handler/grpc/handler.go#L320
Added line #L320 was not covered by tests
[warning] 370-370: pkg/gateway/lb/handler/grpc/handler.go#L370
Added line #L370 was not covered by tests
[warning] 377-377: pkg/gateway/lb/handler/grpc/handler.go#L377
Added line #L377 was not covered by tests
|
[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/refactor%2Fpkg%2Ffix-handler-gRPC-errro-handling/389a22bad3bf38688eb0bf490534aa3d29466405 |
Profile Report
|
vankichi
requested review from
a team and
kmrmt
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
pkg/gateway/lb/handler/grpc/handler.go (4)
508-511: Ensure error is properly handled after streamingAfter the
grpc.BidirectionalStreamcall, check if the errorerris notio.EOFbefore handling it as a streaming error. This will prevent unnecessary error logging when the stream ends naturally.Apply this diff to adjust the error handling:
if err != nil { + if errors.Is(err, io.EOF) { + return nil + } st, _ := status.FromError(err) if st != nil && span != nil { span.RecordError(err)
727-734: Redundant error check forspanIn the
LinearSearchmethod, the conditionif span != nilis already being checked earlier. Ifspanis guaranteed to be non-nil, you can remove the redundant check to simplify the code.Apply this diff to remove the redundant check:
if err != nil { - if span != nil { span.RecordError(err) span.SetAttributes(attrs...) span.SetStatus(trace.StatusError, err.Error()) - } return nil, err }
2281-2282: Clarify comment regarding error parsingThe comment mentions uncertainty about using
status.FromError(err)orstatus.ParseError(err). Clarify which function is appropriate and remove the commented code to maintain code cleanliness.Apply this diff to update the comment:
-// Should we use `status.FromError(err)` instead of `status.PraseError(err)` ? -// It seems `operation` has an important role for tracing, so I don't refactor for now. +// Ensure using the correct function for error parsing; `status.ParseError(err)` is appropriate here.
3757-3760: Check forerrbefore logging inIndexStatisticsBefore logging the error, ensure that
erris notnil. This prevents logging nil errors and potential confusion when diagnosing issues.Apply this diff to add the nil check:
if err != nil { log.Debug(err) if span != nil { span.RecordError(err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/gateway/lb/handler/grpc/handler.go (63 hunks)
🧰 Additional context used
🔇 Additional comments (1)
pkg/gateway/lb/handler/grpc/handler.go (1)
318-320: Check forattrsbefore setting attributes onspanIn the condition
if attrs != nil && span != nil,attrsis already being checked fornil. Ensure thatattrscontains the necessary attributes before callingspan.SetAttributes(attrs...)to avoid potential issues.
Signed-off-by: vankichi <[email protected]>
vankichi
force-pushed
the
refactor/pkg/fix-handler-gRPC-errro-handling
branch
from
6dcae2b to
3c18c3e
Compare
Signed-off-by: vankichi <[email protected]>
* ♻️ 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]>
* ♻️ refactor: replace ParseError with FromError * ♻️ fix * ♻️ fix * ♻️ fix --------- Signed-off-by: vankichi <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]> Co-authored-by: Kosuke Morimoto <[email protected]>
* ♻️ 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]>
* ♻️ 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]>
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor