Repository navigation
feat(storage): wire operation metrics in gRPC veneer - #14729
nidhiii-27 wants to merge 12 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 integrates StorageMetricsContext and StorageClientMetrics across various Google Cloud Storage operations in the Java SDK to enable comprehensive metric tracking for single and multi-step operations. The review feedback highlights several critical edge cases where metrics could be double-recorded or where operation failures (such as during chunk copying, initial rewrite calls, or starting resumable writes) would go unrecorded. The reviewer suggests introducing state tracking in GapicCopyWriter and wrapping initial asynchronous or multi-step operations in try-catch blocks to ensure metrics are accurately recorded exactly once for both successes and failures.
3011be4 to
261bc60
Compare
6b44605 to
354f72b
Compare
…e-grpc-veneer-operation-metrics
…s [Generated-by: AI]
…e-grpc-veneer-operation-metrics
…e-grpc-veneer-operation-metrics [Generated-by: AI]
…RPC veneer [Generated-by: AI]
…s [Generated-by: AI]
…r Java 8 compatibility [Generated-by: AI]
…tibility Configure Mockito to mock UnaryCallable without annotations on Java 8 to prevent ArrayStoreException from EnumConstantNotPresentExceptionProxy. [Generated-by: AI]
…nd bind list pagination retrier - Let NotFoundException propagate out of retrier.run and catch 404 outside in delete, internalBlobGet, and internalBucketGet, and set error on 404 in deleteAcl/deleteDefaultAcl so NOT_FOUND is recorded instead of OK. - Run StorageInternal hooks (internalDirectUpload, internalObjectDelete, internalObjectGet) with null metricsContext so PCU sub-operations do not emit top-level operations, and keep internalObjectGet @nonnull. - Bind TransformingPageDecorator retrier with multi-step ListBuckets context and use createMultiStepContext for page 2+ in ListBucketsWithPartialSuccessPage and ListObjectsWithSyntheticDirectoriesPage. [Generated-by: AI]
…RPC updates - Pass UpdateBucket and UpdateObject operation names into internalBucketGet and internalBlobGet when BucketInfo or BlobInfo has no modified fields in GrpcStorageImpl.update(...), while keeping GetBucket and GetObject for get(...) callers. - Add unit test in GrpcStorageImplTest verifying no-op update(BucketInfo) and update(BlobInfo) record UpdateBucket and UpdateObject operation metrics. [Generated-by: AI]
cpriti-os
left a comment
cpriti-os
left a comment
There was a problem hiding this comment.
LGTM! Left two minor comments inline.
| ResumableSession.grpc( | ||
| retrier.withAlg(retryAlgorithmManager.idempotent(), multiContext), | ||
| write, | ||
| storageClient.queryWriteStatusCallable(), |
There was a problem hiding this comment.
For resumable uploads, let's also attach multiContext to the call contexts for writeObjectCallable and queryWriteStatusCallable so the gRPC interceptor can see it on those calls too.
| multiContext != null | ||
| ? multiContext | ||
| : StorageClientMetrics.createContext( | ||
| getOptions().getStorageClientMetrics(), "RewriteObject"); |
There was a problem hiding this comment.
Nit: Since multiContext (and metricsContext in the helpers below) is only null when metrics are disabled, the fallback createContext call will also return null, so we can pass it directly.
Wires operation metrics into the gRPC veneer implementation.
GrpcStorageImplacross unary calls, multi-step ACL operations, and streaming operations.GapicCopyWriter,GrpcBlobReadChannel, andGrpcBlobWriteChannel.Stacked on #14698.