Repository navigation
feat(storage): add gcp.storage.client.request.body.size metric for write operations - #14746
nidhiii-27 wants to merge 9 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 request body size tracking for 'WriteObject' operations in Google Cloud Storage by capturing and recording request bytes in the StorageMetricsContext. However, a critical compilation error was identified in GrpcStorageImpl.java where Files.size(path) is called outside of a try-catch block, throwing an unhandled checked IOException. Moving this call inside the try block will resolve the compilation issue and ensure proper exception handling.
There was a problem hiding this comment.
Code Review
This pull request implements tracking and recording of request body sizes for 'WriteObject' operations across various storage implementation classes, including BaseStorageWriteChannel, GrpcStorageImpl, and StorageImpl, using StorageMetricsContext. It also adds unit tests to verify this behavior. A review comment suggests moving the Files.size(path) call inside the try block in GrpcStorageImpl.java to ensure that any potential IOException is properly caught and handled by the existing error-handling logic.
…coverage - Move multiContext.setRequestBytes(Files.size(path)) inside the try block in GrpcStorageImpl.internalCreateFrom so IOExceptions from Files.size(path) are recorded on multiContext - Separate 3-arg StorageInternal.internalDirectUpload (PCU hook) from 4-arg internalDirectUpload in StorageImpl and GrpcStorageImpl to prevent PCU part upload double-counting - Instrument StorageImpl.create(BlobInfo, InputStream) and StorageImpl.internalCreateFrom(Path, BlobInfo, Opts) for dual-transport parity - Exclude HTTP-only and attempt/operation-only attributes in StorageMetricsContext.buildBodySizeAttributes - Expand StorageMetricsContextTest to cover BaseStorageWriteChannel, StorageImpl, and GrpcStorageImpl write paths [Generated-by: AI]
…ord uploaded bytes after transfer [Generated-by: AI]
| return; | ||
| } | ||
| long reqBytes = requestBytes.get(); | ||
| if ("WriteObject".equals(operationName) && reqBytes > 0) { |
There was a problem hiding this comment.
Should we record 0 here as well so empty uploads show up in the size distribution? The spec includes 0 B objects, so the two zero-byte tests would flip to expect a single 0 B point.
There was a problem hiding this comment.
Done! Updated recordRequestBodySize to record reqBytes >= 0 so empty (0 B) uploads are included in the distribution, and updated the two zero-byte tests to assert a single 0.0 histogram point.
Co-authored by AI Agent
| } | ||
| } | ||
|
|
||
| private Attributes buildBodySizeAttributes(StorageClientMetrics metrics) { |
There was a problem hiding this comment.
Should request.body.size carry gcp.client.method like operations does? Looks like we could reuse operationAttributes minus error.type here and drop BODY_SIZE_EXCLUDED_KEYS.
There was a problem hiding this comment.
Done! Reused operationAttributes (minus error.type) for request.body.size so it carries gcp.client.method and dropped BODY_SIZE_EXCLUDED_KEYS and buildBodySizeAttributes.
Co-authored by AI Agent
| HttpContentRange contentRange = HttpContentRange.of(ByteRangeSpec.explicit(0L, size), size); | ||
| ResumableOperationResult<StorageObject> put = | ||
| session.put(RewindableContent.of(path), contentRange); | ||
| if (multiContext != null) { |
There was a problem hiding this comment.
This createFrom overload is already a one-line delegate to internalCreateFrom on #14698, can you merge the latest base? This setRequestBytes then goes away since internalCreateFrom already has it.
There was a problem hiding this comment.
Done! Merged the latest base from #14698 so createFrom(BlobInfo, Path, int, BlobWriteOption...) delegates to internalCreateFrom and the duplicate setRequestBytes is gone.
Co-authored by AI Agent
| ResumableOperationResult<StorageObject> put = | ||
| session.put(RewindableContent.of(path), contentRange); | ||
| if (multiContext != null) { | ||
| multiContext.setRequestBytes(size); |
There was a problem hiding this comment.
Nit: Should this move before put so a failed upload still records its size, like the InputStream paths do in their finally blocks? Same for the gRPC side.
There was a problem hiding this comment.
Done! Moved setRequestBytes before put / retrier.run in internalCreateFrom and internalDirectUpload on both StorageImpl and GrpcStorageImpl.
Co-authored by AI Agent
…est-body-size [Generated-by: AI]
[Generated-by: AI]
…y-size [Generated-by: AI]
Summary
Implements the
gcp.storage.client.request.body.sizemetric forWriteObjectoperations across HTTP and gRPC transports.Changes
StorageMetricsContext:setRequestBytes(long)andaddRequestBytes(long)for tracking request payload size.gcp.storage.client.request.body.sizeinrecordOperation()whenoperationNameis"WriteObject"andrequestBytes > 0, filtering out attempt-only and HTTP-only attributes viabuildBodySizeAttributes(...).BaseStorageWriteChannel:metricsContextviaaddRequestBytes(write)duringwrite(ByteBuffer)and records once onclose().StorageImpl&GrpcStorageImpl:create(BlobInfo, ...)overloads (byte[],InputStream),createFrom(BlobInfo, Path, ...),createFrom(BlobInfo, InputStream, ...), andinternalCreateFrom(Path, BlobInfo, Opts<ObjectTargetOpt>)to record request payload size onWriteObjectmetrics contexts.StorageInternal.internalDirectUpload(BlobInfo, Opts<ObjectTargetOpt>, ByteBuffer)hook (used byParallelCompositeUploadWritableByteChannelpart uploads) from the private 4-argument helper so PCU part uploads do not double-countWriteObjectoperations or request body bytes.StorageMetricsContextTest:gcp.storage.client.request.body.sizerecording, attribute filtering (BODY_SIZE_EXCLUDED_KEYS), 0-byte and non-WriteObjectsuppression,BaseStorageWriteChannelwrite accumulation,StorageImplandGrpcStorageImplwrite entrypoints, and PCUinternalDirectUploadsuppression.