Repository navigation
feat(storage): wire operation metrics in HTTP veneer and storage channels - #14698
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 integrates StorageMetricsContext across various storage operations in GrpcStorageImpl, StorageImpl, and GapicCopyWriter to enable operation-specific metrics tracking. However, in GrpcStorageImpl.java's moveBlob method, the metricsContext is passed to the retrier but is not merged into the GrpcCallContext or passed to the moveObjectCallable call, which prevents proper propagation of the metrics context to the gRPC request.
29c672c to
6c828d8
Compare
2455496 to
02b170e
Compare
6c828d8 to
3011be4
Compare
nidhiii-27
changed the title
02b170e to
2b123b7
Compare
…nels [Generated-by: AI]
3011be4 to
261bc60
Compare
…operation-metrics
…operation-metrics
…operation-metrics [Generated-by: AI]
…TTP veneer [Generated-by: AI]
…er-counting, and add metrics unit tests - Preserve read/write exceptions in BaseStorageReadChannel and BaseStorageWriteChannel so close() records the failure error.type instead of OK (while ignoring HTTP 416 normal EOF). - Use multi-step StorageMetricsContext on subsequent page fetches in BucketPageFetcher, BlobPageFetcher, and HmacKeyMetadataPageFetcher so pagination increments attempts without over-counting logical operations. - Suppress top-level operation metrics on StorageInternal PCU sub-operation hooks (internalDirectUpload, internalObjectDelete, internalObjectGet) while instrumenting public create(BlobInfo, byte[], ...) and internalCreateFrom. - Wire multi-step StorageMetricsContext and StorageClientMetrics into RestorableState.restore() for HttpCopyWriter, BlobReadChannelV2, and BlobWriteChannelV2, and validate signed URLs before recording operation metrics in writer(URL). - Replace inline FQCNs, use Truth assertions, and add unit tests in CopyWriterTest covering channel errors, pagination, PCU suppression, and RestorableState. [Generated-by: AI]
…act StorageImplMetricsTest [Generated-by: AI]
cpriti-os
left a comment
cpriti-os
left a comment
There was a problem hiding this comment.
Thanks Nidhi! Left a couple of comments on 404 attempt/error recording and writer(URL), plus two small cleanup nits.
…s, writer(URL), createFrom, and copy [Generated-by: AI]
…ied update calls [Generated-by: AI]
cpriti-os
left a comment
cpriti-os
left a comment
There was a problem hiding this comment.
LGTM! Left a couple of minor follow-up comments.
| lastOperationError = e; | ||
| throw new IOException(e); | ||
| } catch (IOException e) { | ||
| lastOperationError = e; |
There was a problem hiding this comment.
When this.write(ByteBuffer.allocate(0)) in close() (or an underlying channel read/write) wraps a StorageException in new IOException(e), passing that outer IOException to recordOperation causes StorageErrorTypeMapper to map it to CONNECTIVITY instead of the StorageException status. Can we unwrap IOException causes that are StorageException when capturing error and lastOperationError (here and in BaseStorageReadChannel)?
There was a problem hiding this comment.
Good catch! Unwrapped IOException instances whose cause is a StorageException when capturing error and lastOperationError in both BaseStorageWriteChannel and BaseStorageReadChannel, and added test coverage in StorageImplMetricsTest.
Co-authored by AI Agent
| } | ||
| } | ||
|
|
||
| private boolean runBoolean( |
There was a problem hiding this comment.
Nit: Can runBoolean delegate to runNullable (() -> Boolean.TRUE.equals(c.call()) ? Boolean.TRUE : null) instead of duplicating this block?
There was a problem hiding this comment.
Done! Simplified runBoolean to delegate directly to runNullable.
Co-authored by AI Agent
…veneer-operation-metrics [Generated-by: AI]
…to feat/storage-veneer-operation-metrics
… and simplify runBoolean [Generated-by: AI]
|
|
||
| private <T, U> U run(ResultRetryAlgorithm<?> algorithm, Callable<T> c, Decoder<T, U> f) { | ||
| return retrier.run(algorithm, c, f); | ||
| private <T, U> U runNullable( |
There was a problem hiding this comment.
IIUC, get/delete now throw and catch an internal 404 exception even when metrics are off. This creates an exception on every call, and for tracing users a normal "not found" now appears as an exception on their span. Could we only do this when metrics are enabled, and create the exception only when the result is null/false?
Wires operation metrics into shared storage channels and the HTTP/JSON veneer implementation.
StorageMetricsContextintoBaseStorageReadChannelandBaseStorageWriteChannelto record operations on close.StorageImpl,HttpCopyWriter, and HTTP channel wrappers (BlobReadChannelV2,BlobWriteChannelV2).CopyWriterTestunit tests.Stacked on #14695.