Skip to content

feat(storage): add gcp.storage.client.request.body.size metric for write operations - #14746

Draft
nidhiii-27 wants to merge 2 commits into
feat/storage-ttfb-metricfrom
feat/storage-request-body-size
Draft

nidhiii-27 wants to merge 2 commits into
feat/storage-ttfb-metricfrom
feat/storage-request-body-size

Conversation

@nidhiii-27

@nidhiii-27 nidhiii-27 commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Implements the gcp.storage.client.request.body.size metric for WriteObject operations across HTTP and gRPC transports.

Changes

  • StorageMetricsContext:
    • Adds setRequestBytes(long) and addRequestBytes(long) for tracking request payload size.
    • Records gcp.storage.client.request.body.size in recordOperation() when operationName is "WriteObject" and requestBytes > 0, filtering out attempt-only and HTTP-only attributes via buildBodySizeAttributes(...).
  • BaseStorageWriteChannel:
    • Accumulates written bytes on metricsContext via addRequestBytes(write) during write(ByteBuffer) and records once on close().
  • StorageImpl & GrpcStorageImpl:
    • Instruments create(BlobInfo, ...) overloads (byte[], InputStream), createFrom(BlobInfo, Path, ...), createFrom(BlobInfo, InputStream, ...), and internalCreateFrom(Path, BlobInfo, Opts<ObjectTargetOpt>) to record request payload size on WriteObject metrics contexts.
    • Separates the 3-argument StorageInternal.internalDirectUpload(BlobInfo, Opts<ObjectTargetOpt>, ByteBuffer) hook (used by ParallelCompositeUploadWritableByteChannel part uploads) from the private 4-argument helper so PCU part uploads do not double-count WriteObject operations or request body bytes.
  • StorageMetricsContextTest:
    • Adds unit tests verifying gcp.storage.client.request.body.size recording, attribute filtering (BODY_SIZE_EXCLUDED_KEYS), 0-byte and non-WriteObject suppression, BaseStorageWriteChannel write accumulation, StorageImpl and GrpcStorageImpl write entrypoints, and PCU internalDirectUpload suppression.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant