Include causal status details in higher-level statuses #12409
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When an operation fails and we want to produce a new status at a higher level, we commonly are turning the first status into an exception to attach to the new exception. We should instead prefer to keep as much information in the status description itself, as cause is not as reliable to be logged/propagated. I do expect long-term we'll want to expose an API in grpc-api for this, but for the moment let's keep it internal. In particular, we'd have to figure out its name. I could also believe we might want different formatting, which becomes a clearer discussion when we can see the usages. I'm pretty certain there are some other places that could benefit from this utility, as I remember really wishing I had these functions a month or two ago. But these are the places I could immediately find. OutlierDetectionLoadBalancerConfig had its status code changed from INTERNAL to UNAVAILABLE because the value comes externally, and so isn't a gRPC bug or such. I didn't change the xds policies in the same way because it's murkier as the configuration for those is largely generated within xds itself.
shivaspeaks
reviewed
Oct 14, 2025
| .withCause(childConfig.getError().asRuntimeException())); | ||
| return ConfigOrError.fromError(GrpcUtil.statusWithDetails( | ||
| Status.Code.UNAVAILABLE, | ||
| "Failed to parse child in outlier_detection_experimental", |
There was a problem hiding this comment.
I notice that we're no longer including the rawConfig in the error description. I agree that this makes the error message much cleaner, but do you think we might be losing valuable context for debugging? Perhaps we could log the rawConfig at a FINE level, or do you think the information while in this error is always sufficient?
There was a problem hiding this comment.
Per go/java-logging-best-practices#logging-levels-and-avoiding-spam
| .withCause(childConfig.getError().asRuntimeException())); | ||
| return ConfigOrError.fromError(GrpcUtil.statusWithDetails( | ||
| Status.Code.INTERNAL, | ||
| "Failed to parse child policy in wrr_locality LB policy", |
There was a problem hiding this comment.
Same as above comment for rawConfig.
shivaspeaks
approved these changes
Oct 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an operation fails and we want to produce a new status at a higher level, we commonly are turning the first status into an exception to attach to the new exception. We should instead prefer to keep as much information in the status description itself, as cause is not as reliable to be logged/propagated.
I do expect long-term we'll want to expose an API in grpc-api for this, but for the moment let's keep it internal. In particular, we'd have to figure out its name. I could also believe we might want different formatting, which becomes a clearer discussion when we can see the usages.
I'm pretty certain there are some other places that could benefit from this utility, as I remember really wishing I had these functions a month or two ago. But these are the places I could immediately find.
OutlierDetectionLoadBalancerConfig had its status code changed from INTERNAL to UNAVAILABLE because the value comes externally, and so isn't a gRPC bug or such. I didn't change the xds policies in the same way because it's murkier as the configuration for those is largely generated within xds itself.