Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors observability tracing by centralizing response attribute population in ObservabilityUtils and removing artifact name and version metadata from tracing attributes. The updates ensure consistent logging of exception types and messages across GoldenSignalsMetricsTracer, LoggingTracer, and SpanTracer. Feedback suggests using Strings.isNullOrEmpty() for message checks, defining a constant for the exception message attribute key, and specifying an initial capacity for the HashMap in ObservabilityUtils for better efficiency.
I am having trouble creating individual review comments. Click here to see my feedback.
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/LoggingTracer.java (82-84)
It is recommended to use Strings.isNullOrEmpty() to check the exception message. This ensures that empty strings are not added to the log context, maintaining consistency with SpanTracer.java. Additionally, using a constant for the attribute key is preferred over a hardcoded string.
if (error != null && !Strings.isNullOrEmpty(error.getMessage())) {
logContext.put(ObservabilityAttributes.EXCEPTION_MESSAGE_ATTRIBUTE, error.getMessage());
}
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/ObservabilityAttributes.java (51)
Define a constant for the exception message attribute key to be used in logging, following the naming convention of other exception-related attributes.
public static final String REPO_ATTRIBUTE = "gcp.client.repo";
/** The exception message. */
public static final String EXCEPTION_MESSAGE_ATTRIBUTE = "exception.message";
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/ObservabilityUtils.java (176)
Specifying an initial capacity for the HashMap is a good practice when the number of entries is small and known, as it avoids potential resizing and is slightly more efficient for high-frequency utility methods.
Map<String, Object> attributes = new HashMap<>(8);
sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/tracing/LoggingTracerTest.java (199)
Use the constant ObservabilityAttributes.EXCEPTION_MESSAGE_ATTRIBUTE instead of a hardcoded string for consistency.
assertEquals("test error message", attributesMap.get(ObservabilityAttributes.EXCEPTION_MESSAGE_ATTRIBUTE));
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/LoggingTracer.java
Outdated
Show resolved
Hide resolved
|
|
||
| Map<String, ?> attributesMap = getAttributesMap(); | ||
|
|
||
| assertTrue(attributesMap != null && !attributesMap.isEmpty()); |
There was a problem hiding this comment.
nit: Can this be broken into two assert: assertNotNull(attributeMap) and assertFalse(attributeMaps.isEmpty()).
IIRC, the error message for assertTrue isn't as clear when indicating which condition failed
There was a problem hiding this comment.
Agreed. I was following the existing pattern but I don't think these assertions are needed anymore. Because attributeMap can never be null and we are already asserting a key later so not empty is also not necessary.
Removed this assertion altogether.
2 participants
This PR
exception.messageattribute to loggingexception.typeattribute to loggingerror.typeto use ObservabilityUtils#extractErrorType