fix(gax): propagate structured LRO error details to ApiException - #14022
fix(gax): propagate structured LRO error details to ApiException#14022nnicolee wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for propagating error details in long-running operations (LRO) for both gRPC and HTTP/JSON transports. This is achieved by adding getErrorDetails() to the OperationSnapshot interface and implementing it in GrpcOperationSnapshot and HttpJsonOperationSnapshot. The ProtoOperationTransformers are updated to pass these error details when throwing exceptions, and corresponding integration and unit tests are added. Feedback on the changes includes renaming a misleadingly named test that asserts error propagation rather than dropping, and ensuring that ErrorDetails is only instantiated and returned in GrpcOperationSnapshot and HttpJsonOperationSnapshot if there are actual details present (i.e., checking that the details count is greater than zero).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for propagating error details from long-running operations (LROs) across both gRPC and HTTP/JSON transports by introducing a getErrorDetails() method to the OperationSnapshot interface and implementing it in GrpcOperationSnapshot and HttpJsonOperationSnapshot. It also updates ProtoOperationTransformers to pass these error details when throwing exceptions and adds corresponding integration and unit tests. Feedback on the changes highlights a potential issue in HttpJsonOperationSnapshot.Builder.setOperation where errorDetails is not reset to null if the operation has no error, which could lead to stale state if the builder is reused.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for propagating error details in long-running operations (LRO) for both gRPC and HTTP/JSON transports. It introduces the getErrorDetails() method to the OperationSnapshot interface and implements it in GrpcOperationSnapshot and HttpJsonOperationSnapshot. Additionally, it updates the ProtoOperationTransformers to include these error details when constructing exceptions, and adds corresponding unit and integration tests. The reviewer suggested adding a timeout to the operationFuture.get() call in the new integration test to prevent the test suite from hanging indefinitely.
| default @Nullable ErrorDetails getErrorDetails() { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
qq, what do you think about returning a non-null empty ErrorDetails instead of null here? I think this would be similar to the proto messages where there is a default value.
IIUC, this is primarily used within ProtoOperationTransformers's ResponseTransformer which operations on a response and it should return Status.getDefaultInstance() if there is no error
There was a problem hiding this comment.
great suggestion! this would aligns more with the proto messages! I have updated the interface to return an empty ErrorDetails rather than null!
| @@ -63,7 +63,8 @@ public ResponseT apply(OperationSnapshot operationSnapshot) { | |||
| + operationSnapshot.getErrorMessage(), | |||
There was a problem hiding this comment.
I think part of the issue was that the default error message doesn't include the any of the structured error details. Perhaps we can update the default message here to include the errordetails?
We may need to figure out what it looks like if there are a bunch of repeated error details here.
There was a problem hiding this comment.
For standard LROs (like grpc), the server already returns a detailed, descriptive text error message in operation.getError().getMessage(), which is mapped to the snapshot's error message and appears in the exception string.
The issue ticket is for compute because compute put its detailed errors in a custom error.errors[] list instead of the standard error message field. Under phase 2, we will parse that custom errors[] list, format it into a descriptive strong and pass it as the snapshot's error message!
We add operationSnapshot.getErrorDetails() to the exception creation because it propagates structed, machine-readble metadata payloads associated with LRO failures directly to the client application!
|
|


Description:
When a Long-Running Operation (LRO) completes with a failure, structured error details inside the operation's error payload were previously dropped. This made it impossible for client applications to access fine-grained provider error messages (such as quota or usage violations) via the public
ApiException.getErrorDetails()API.This PR implements Phase 1 of the LRO error propagation design by establishing the transport-agnostic mechanism to propagate structured LRO error details in GAX for both gRPC and HTTP/JSON (REST) transports.
Design doc: go/sdk:java-lro-error-details
Key Changes:
TypeRegistryin generated stubs to parse custom/standard payload types packed inAnydetails, the test client fails to deserialize these details. Once we roll out the generator changes in Phase 2 to automatically add error details types to generated stub registries, the Showcase REST integration test will pass out-of-the-box. We have added unit tests to cover the HTTP/JSON LRO response parsing pathway in the interim.Testing:
mvn test -pl sdk-platform-java/gax-java/gax-grpc,sdk-platform-java/gax-java/gax-httpjson -Dtest=ProtoOperationTransformersTestmvn test -pl java-showcase/gapic-showcase -Dtest=ITLongRunningOperation