-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update StorageException translation of an ApiException to include error details #2872
Conversation
dbfe59e
to
8a679df
Compare
…e error details Update StorageException logic for coalescing ApiExceptions to add a formatted string including fields from error details as a suppressed exception on the api exception. This keeps the diagnostic information at the "gapic layer" in the printed stacktrace similar to the json document being on the "apiary layer" of the cause.
8a679df
to
7bd8545
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were we always missing ErrorInfo + details? What happened?
private static void attachErrorDetails(ApiException ae) { | ||
if (ae != null && ae.getErrorDetails() != null) { | ||
final StringBuilder sb = new StringBuilder(); | ||
ErrorDetails ed = ae.getErrorDetails(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ErrorDetails have a toString function? Shouldn't this be handled there? My only concern is if any new fields are added, we'd have remember to pull it into this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does have a toString, however that will print out the packed protobuf bytes rather than the actual fields and info contained within.
The default toString
:
ErrorDetails{rawErrorMessages=[type_url: "type.googleapis.com/google.rpc.ErrorInfo"
value: "\n\bSTACKOUT\022\026spanner.googlepais.com\032(\n\020availableRegions\022\024us-central1,us-east2"
, type_url: "type.googleapis.com/google.rpc.DebugInfo"
value: "\n\004HEAD\n\006HEAD~1\n\006HEAD~2\n\006HEAD~3\022\vsome detail"
, type_url: "type.googleapis.com/google.rpc.QuotaFailure"
value: "\n!\n\022clientip:127.0.3.3\022\vDaily limit"
, type_url: "type.googleapis.com/google.rpc.PreconditionFailure"
value: "\n6\n\003TOS\022\020google.com/cloud\032\035Terms of service not accepted"
, type_url: "type.googleapis.com/google.rpc.BadRequest"
value: "\n~\n\032email_addresses[3].type[2]\022\026duplicate value \'WORK\'\032\032INVALID_EMAIL_ADDRESS_TYPE\",\n\005en-US\022#Invalid email type: duplicate value"
, type_url: "type.googleapis.com/google.rpc.Help"
value: "\n\033\n\005link1\022\022https://google.com"
]}
Compared with our formatting:
ErrorDetails {
ErrorInfo: { reason: "STACKOUT" domain: "spanner.googlepais.com" metadata { key: "availableRegions" value: "us-central1,us-east2" } }
DebugInfo: { stack_entries: "HEAD" stack_entries: "HEAD~1" stack_entries: "HEAD~2" stack_entries: "HEAD~3" detail: "some detail" }
QuotaFailure: { violations { subject: "clientip:127.0.3.3" description: "Daily limit" } }
PreconditionFailure: { violations { type: "TOS" subject: "google.com/cloud" description: "Terms of service not accepted" } }
BadRequest: { field_violations { field: "email_addresses[3].type[2]" description: "duplicate value \'WORK\'" reason: "INVALID_EMAIL_ADDRESS_TYPE" localized_message { locale: "en-US" message: "Invalid email type: duplicate value" } } }
Help: { links { description: "link1" url: "https://google.com" } }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is much better. If changing the toString isnt a breaking change, we should add a follow up to fix this there
The information itself has always been present in the cause of the StorageException, but it required calling into it to access the info. This change mainly includes it in the stacktrace so that it is printed implicitly. |
Update StorageException logic for coalescing ApiExceptions to add a formatted string including fields from error details as a suppressed exception on the api exception.
This keeps the diagnostic information at the "gapic layer" in the printed stacktrace similar to the json document being on the "apiary layer" of the cause.
The change here will produce something like the following: