Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the observability capabilities by introducing a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new observability attribute error.type to provide a more specific, low-cardinality classification of errors. The core logic is implemented in ObservabilityUtils.extractErrorType, which determines the error type based on a clear priority: ErrorInfo.reason, server error codes, specific client-side exceptions, and then fallbacks. The changes are well-supported by comprehensive unit tests in SpanTracerTest and integration tests in ITOtelTracing for both gRPC and HTTP/JSON transports, which is great to see.
My review includes one suggestion for a minor code simplification. Overall, this is a solid implementation.
gax-java/gax/src/main/java/com/google/api/gax/tracing/ObservabilityUtils.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new observability attribute, error.type, to provide a low-cardinality classification of errors. The core logic is implemented in the new ErrorTypeUtil class, which extracts the error type based on a priority list, including ErrorInfo.reason, server status codes, and various client-side error categories. The changes are well-tested with both unit and integration tests.
My main feedback is regarding the implementation of client-side error detection in ErrorTypeUtil. Several checks rely on string matching on exception class names or messages (e.g., using String.contains()), which is fragile and can lead to incorrect error classification. I've left specific comments with suggestions to improve the robustness of these checks.
| private static boolean isClientRedirectError(Throwable e) { | ||
| return e.getMessage() != null && e.getMessage().contains("redirect"); | ||
| } |
There was a problem hiding this comment.
Relying on the content of an exception message with e.getMessage().contains("redirect") is highly fragile. Exception messages are not a stable API; they can be changed, might not be present, or could be localized, which would break this logic.
It would be much more robust to check for a specific exception type that indicates a redirect error. If no such standard exception exists, this approach has a high risk of being unreliable and should be documented as such.
There was a problem hiding this comment.
There are no redirect-specific exceptions in gax. This is only a heuristic to comply with the higher level effort.
| private static boolean isClientUnknownError(Throwable e) { | ||
| return e.getClass().getName().toLowerCase().contains("unknown"); | ||
| } |
There was a problem hiding this comment.
Matching unknown in a class name is very broad and risky. It could lead to misclassifying exceptions. For example, an exception from another library with unknown in its name would be incorrectly categorized as CLIENT_UNKNOWN_ERROR.
Given that UnknownHostException is already handled by isClientConnectionError, this check seems too generic. Please consider making this check more specific to the intended exception types or removing it if it's a speculative catch-all.
There was a problem hiding this comment.
Agreed we don't have a reliable heuristic that also prevents external code from being caught here. In favor of the current approach, this is the last case to be handled.
One alternative: do not handle CLIENT_UNKNOWN_ERROR.
gax-java/gax/src/main/java/com/google/api/gax/tracing/ErrorTypeUtil.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/ErrorTypeUtil.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/ErrorTypeUtil.java
Outdated
Show resolved
Hide resolved
error.type'error.type
error.typeerror.type
|
|
| if (isRequestBodyError(error)) { | ||
| return ErrorType.CLIENT_REQUEST_BODY_ERROR.toString(); | ||
| } | ||
| if (isClientUnknownError(error)) { |
There was a problem hiding this comment.
I thought this was the default option, as in there shouldn't be a check here?
There was a problem hiding this comment.
So the fallback sequence is "reason", then "server error" then "client error" then "exception name" then "INTERNAL".
This class is not only for "client error" but for the whole the fallback sequence, so we need the flow to check for other cases such as exception name and INTERNAL.
ldetmer
left a comment
There was a problem hiding this comment.
looks good, just one question





This PR introduces
error.typeobservability attribute, which is obtained from error information (i.e.Throwables).
Key Changes:
ErrorTypeUtils, withpublicsurface, that provides a string representation of the possible error types given aThrowable. Tracing classes such asSpanTracercan rely on it to obtain theerror.typeattribute from such a throwable.addAttributetoSpan. This is becauseattemptFailed()and other sibling methods must modify existing Spans. This will be reverted in the future once we "flatten" theSpanTracervsSpanManagerlayout.Logic
The logic is extensively explained in the javadoc of
ErrorTypeUtils.extractErrorType(), but in a nutshell it consists of:ErrorInforeason as a primary sourceINTERNALTesting
ITOtelTracing