Repository navigation
feat(storage): add StorageMetricsContext for operation metrics state - #14728
nidhiii-27 wants to merge 13 commits into
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 introduces StorageMetricsContext and its corresponding unit tests to manage operation metric state across asynchronous and thread-hopping boundaries. The review feedback suggests simplifying the API surface of StorageMetricsContext by making the primary constructor private, relying on static factory methods, and refactoring from(ApiCallContext) to delegate directly to from(GrpcCallContext) to eliminate duplicate logic. Additionally, the feedback recommends updating the unit tests to utilize these static factory methods instead of constructors.
84a5ac6 to
ed62afa
Compare
…[Generated-by: AI]
f943017 to
edb3def
Compare
…sContext [Generated-by: AI]
…torage-metrics-context
… [Generated-by: AI]
… recording [Generated-by: AI]
…d status overrides
…ring for unused transport [Generated-by: AI]
…torage-metrics-context [Generated-by: AI]
Explicitly declare com.google.api:gax-httpjson in java-storage/google-cloud-storage/pom.xml to satisfy Maven dependency:analyze. [Generated-by: AI]
cpriti-os
left a comment
cpriti-os
left a comment
There was a problem hiding this comment.
Left a few small comments on attribute filtering and status code fallbacks.
[Generated-by: AI]
- Move self-causation check to the bottom of the while loop in extractRpcStatusCode and extractHttpStatusCode so top-level self-causing exceptions are inspected first - Add defensive null checks on getStatus() and getCode() for StatusRuntimeException and StatusException in extractRpcStatusCode - Add unit tests for top-level self-causing exceptions and null status on gRPC status exceptions [Generated-by: AI]
cpriti-os
left a comment
cpriti-os
left a comment
There was a problem hiding this comment.
LGTM! Left a few optional cleanup/idiomatic suggestions that can be addressed here or in a follow-up.
| this.attemptCount = requireNonNull(attemptCount, "attemptCount cannot be null"); | ||
| this.requestBytes = requireNonNull(requestBytes, "requestBytes cannot be null"); | ||
| this.responseBytes = requireNonNull(responseBytes, "responseBytes cannot be null"); | ||
| } |
There was a problem hiding this comment.
Can we use ApiClock here instead of System.nanoTime() so it stays consistent with the rest of the repo and the interceptors? Also, we can initialize the Atomic* fields inline rather than passing them into the constructor.
| StorageClientMetrics.KEY_HTTP_RESPONSE_STATUS_CODE, httpCode != null ? httpCode : ""); | ||
| } | ||
| metrics.getAttempts().add(1, builder.build()); | ||
| } |
There was a problem hiding this comment.
If the context holds a reference to StorageClientMetrics, callers won't have to pass both objects around everywhere. We can also pull the duplicated gRPC/HTTP status-code branching in recordAttemptSuccess and recordAttemptFailure into a small helper.
| } | ||
|
|
||
| private @Nullable String extractRpcStatusCode(@Nullable Throwable e) { | ||
| String fromContext = baseAttributes.get(StorageClientMetrics.KEY_RPC_RESPONSE_STATUS_CODE); |
There was a problem hiding this comment.
Could we move the exception status-code extraction into StorageErrorTypeMapper so we only have one place walking the cause chain? Also, baseAttributes already includes the client attributes, so we don't need to copy them twice in buildAttemptBaseAttributes.
| } | ||
|
|
||
| AttributesBuilder attributesBuilder() { | ||
| return baseAttributes.toBuilder(); |
There was a problem hiding this comment.
Several attachment helpers (Map, ApiCallContext), attemptCount, and attributesBuilder() aren't used anywhere in the stack—let's drop the unused ones to keep this class lean.
Introduces
StorageMetricsContextfor tracking per-operation and per-attempt OpenTelemetry metrics state.StorageMetricsContextto manage operation attributes, attempt counters, and durations.StorageMetricsContextTestunit tests.Stacked on #14694.