Repository navigation
feat(storage): add StorageMetricsContext and StorageErrorTypeMapper for standard metrics - #14694
nidhiii-27 wants to merge 5 commits into
Conversation
a1393ca to
41c47ff
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for OpenTelemetry client metrics and debug metrics in Google Cloud Storage, including configuration options in StorageOptions, GrpcStorageOptions, and HttpStorageOptions. It adds utility classes such as StorageClientMetrics for instrument registration, StorageErrorTypeMapper for mapping exceptions to canonical error strings, StorageMetricsConfig for environment-based configuration, and StorageMetricsContext for tracking metric state across boundaries, along with corresponding unit tests. Feedback on the changes suggests simplifying a redundant null check on a transient field in StorageOptions, defensively preventing infinite loops from self-causation in StorageErrorTypeMapper's exception unwrapping, and robustly checking for authentication exceptions by using full class names instead of simple names to correctly handle anonymous classes.
I am having trouble creating individual review comments. Click here to see my feedback.
java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageOptions.java (262)
Since meterProvider is marked as transient, it will not be serialized and will always be null after deserialization. Therefore, the check this.meterProvider != null in readObject is redundant and will always evaluate to false. We can simplify this to only check this.customMeterProviderConfigured.
if (this.customMeterProviderConfigured) {
java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageErrorTypeMapper.java (159-162)
To prevent potential infinite loops in case of custom exceptions with self-causation (where t.getCause() == t), we should defensively add a check for t.getCause() != t in the while loop condition, similar to how it is handled at the end of the mapThrowable method.
while ((t instanceof ExecutionException || t instanceof CompletionException)
&& t.getCause() != null
&& t.getCause() != t) {
t = t.getCause();
}
java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageErrorTypeMapper.java (258)
Using t.getClass().getSimpleName() can return an empty string "" for anonymous classes, which would bypass the contains("Auth") check even if the base class name contains "Auth". Using the full class name className.contains("Auth") is more robust and avoids extra string parsing overhead.
if (className.contains("Auth")) {
…or standard metrics Introduce package-private StorageMetricsContext to track operation metric state across asynchronous and thread-hopping boundaries (such as ApiFuture callbacks and thread pools). Introduce package-private StorageErrorTypeMapper to provide canonical uppercase error mapping for gRPC status codes, HTTP status codes, network/socket/TLS failures, auth errors, and checksum mismatches per O11y 2026 specification. Add comprehensive unit test suites covering both classes. [Generated-by: AI]
…standard metrics [Generated-by: AI]
608c46a to
1bf9a9d
Compare
| case 501: | ||
| return STATUS_UNIMPLEMENTED; | ||
| case 503: | ||
| return STATUS_UNAVAILABLE; |
There was a problem hiding this comment.
Two small mapping fixes:
- HTTP
401should map to"UNAUTHENTICATED"(matching gRPCStatus.UNAUTHENTICATED), while"AUTHENTICATION_ERROR"is for client-side token refresh/auth exceptions. - HTTP
502should also map to"UNAVAILABLE"alongside503.
There was a problem hiding this comment.
Done
Co-authored by AI Agent
…orage-metrics-context-error-mapping [Generated-by: AI]
…apper - Align Status.UNAUTHENTICATED and auth status mappings to AUTHENTICATION_ERROR. - Unwrap specific client causes (ApiException, StatusRuntimeException, StatusException, SocketException) from StorageException and IOException. - Check network/timeout root causes in auth exceptions before defaulting to AUTHENTICATION_ERROR. - Add unit tests for gRPC status unwrapping and network-caused auth errors. [Generated-by: AI]
- Map HTTP 401 to UNAUTHENTICATED and HTTP 502 to UNAVAILABLE. - Guard against self-causation cycles in ExecutionException/CompletionException loops. - Check hasSpecificClientCause on StatusRuntimeException, StatusException, and ApiException before status code mapping. - Support cause unwrapping for gRPC exceptions on StorageException. - Add unit tests for gRPC network causes and self-referencing exceptions. [Generated-by: AI]
Description
This PR implements PR 1 of the Standard Metrics phase for the Google Cloud Storage Java SDK (
google-cloud-java), based on the O11y 2026 High Level Design (HLD) and Java Low Level Design (LLD).It introduces the foundation for context propagation and canonical error mapping:
StorageMetricsContext:ApiFuturecallbacks and thread pools).operationName,startTimeNanos,attemptCount, user-facingrequestBytesandresponseBytes, and base OTelAttributes.CallOptions/GrpcCallContext/ApiCallContextand HTTP property maps.StorageErrorTypeMapper:NOT_FOUND,UNAVAILABLE,DEADLINE_EXCEEDED->TIMEOUT, etc.).OK, 400 ->INVALID_ARGUMENT, 401 ->AUTHENTICATION_ERROR, 403 ->PERMISSION_DENIED, 404 ->NOT_FOUND, 409 ->ALREADY_EXISTS, 412 ->FAILED_PRECONDITION, 429 ->RESOURCE_EXHAUSTED, 500 ->INTERNAL, 503 ->UNAVAILABLE, 504 ->TIMEOUT, etc.).SocketTimeoutException->TIMEOUT,UnknownHostException->DNS_FAILURE,ConnectException->CONNECTION_REFUSED,SocketException->CONNECTION_RESET,SSLHandshakeException->TLS_FAILURE, genericIOException->CONNECTIVITY,CancellationException->CANCELLED).AUTHENTICATION_ERROR) and checksum mismatches (CHECKSUM_MISMATCH).StorageErrorTypeMapperTestandStorageMetricsContextTest.Verification
mvn com.spotify.fmt:fmt-maven-plugin:check -pl java-storage/google-cloud-storage(0 non-complying files)mvn checkstyle:check@checkstyle -pl java-storage/google-cloud-storage(0 violations)mvn test -pl java-storage/google-cloud-storage -Dtest=StorageErrorTypeMapperTest,StorageMetricsContextTest,StorageClientMetricsTest,StorageOptionsTest(39 tests run, 0 failures, 0 errors)