feat(storage): add gcp.storage.client.response.body.size metric for read operations by nidhiii-27 · Pull Request #14747 · googleapis/google-cloud-java

nidhiii-27 · 2026-10-23T09:39:06Z

Summary

This PR adds the gcp.storage.client.response.body.size metric for read operations.

Changes

StorageMetricsContext:
- Records gcp.storage.client.response.body.size inside recordOperation(...) when operationName is "ReadObject" and responseBytes > 0.
- Extracts BODY_SIZE_EXCLUDED_KEYS and buildBodySizeAttributes(StorageClientMetrics) to share attribute filtering between request.body.size and response.body.size.
BaseStorageReadChannel / BlobReadChannelV2 / GrpcBlobReadChannel:
- Accumulates response bytes read in BaseStorageReadChannel.read(ByteBuffer) via metricsContext.addResponseBytes(read).
- Tracks lastReadError across read(ByteBuffer) so mid-stream errors are passed to recordOperation(...) on close() while preserving partial byte counts.
StorageImpl:
- Sets response bytes on readAllBytes and downloadTo (using CountingOutputStream.getCount() across retries without double-counting).
GrpcStorageImpl & BlobReadSessionAdapter:
- Sets response bytes on GrpcStorageImpl.readAllBytes (when error == null) and accumulates written bytes in GrpcStorageImpl.downloadTo via countBytes.
- Wires GrpcStorageImpl.blobReadSession and BlobReadSessionAdapter to accumulate delivered bytes across all ReadProjectionConfig types (asFutureBytes(), asFutureByteString(), asChannel(), asSeekableChannel()) and record the operation on close() (or on session open failure).
StorageMetricsContextTest:
- Adds unit tests covering recordOperation attribute filtering, BaseStorageReadChannel (full read, partial read + early close, 0-byte close, mid-stream error), StorageImpl (readAllBytes, downloadTo with retries), BlobReadSessionAdapter (all projections and open failure), and GrpcStorageImpl (readAllBytes, downloadTo, blobReadSession).

…tions

gemini-code-assist

Code Review

This pull request implements response body size tracking for the 'ReadObject' operation across both gRPC and JSON implementations of Google Cloud Storage, updating BaseStorageReadChannel, GrpcStorageImpl, StorageImpl, and StorageMetricsContext, along with adding corresponding unit tests. The review feedback suggests refactoring a long chain of excluded key checks in StorageMetricsContext into a static Set constant to improve maintainability, and replacing fully qualified class names with imports in the new test cases to enhance readability.

gemini-code-assist

Code Review

This pull request introduces tracking and recording of response body size metrics for "ReadObject" operations across both GRPC and JSON/HTTP storage implementations. It updates BaseStorageReadChannel, GrpcStorageImpl, and StorageImpl to capture the read or downloaded bytes and set them in the StorageMetricsContext, which now records the response body size metric. Corresponding unit tests are also added to StorageMetricsContextTest. The reviewer suggested avoiding the use of fully qualified class names in the new test cases to improve code readability, recommending importing them instead.

…se body size - Extract BODY_SIZE_EXCLUDED_KEYS and buildBodySizeAttributes(StorageClientMetrics) in StorageMetricsContext to deduplicate request/response body size attribute filtering. - Guard GrpcStorageImpl.readAllBytes response byte recording with error == null and move countBytes helper to the private helper section. - Wire GrpcStorageImpl.blobReadSession and BlobReadSessionAdapter to track delivered response bytes across all ReadProjectionConfig types and record gcp.storage.client.response.body.size on session close. - Track lastReadError in BaseStorageReadChannel so mid-stream read errors propagate to recordOperation on close while preserving partial read byte counts. - Replace inline FQCNs with imports in StorageMetricsContextTest and add unit tests for channel reads, StorageImpl, GrpcStorageImpl, and BlobReadSessionAdapter. [Generated-by: AI]

…rdResponseBodySize

…ge.operation on response.body.size [Generated-by: AI]

cpriti-os

Mostly looks good, left a few comments inline.

cpriti-os · 2026-11-11T05:32:01Z

+      return;
+    }
+    long respBytes = responseBytes.get();
+    if ("ReadObject".equals(operationName) && respBytes > 0) {


Same as recordRequestBodySize in #14746: should we record 0 here too so empty reads show up in the histogram? The two tests asserting the metric is absent for 0 bytes would flip.

Done! Updated recordResponseBodySize to record respBytes >= 0 so 0 B reads are included in the histogram, and updated the tests in StorageMetricsContextTest.

Co-authored by AI Agent

cpriti-os · 2026-11-11T05:32:01Z

  protected final ReentrantLock lock;
  @Nullable private final StorageMetricsContext metricsContext;
  @Nullable private final StorageClientMetrics storageClientMetrics;
+  @Nullable private Throwable lastReadError;


#14698 already adds this field as lastOperationError at these same catch sites, can we rebase and reuse it instead of adding lastReadError? Also, is the reset to null on a successful read intentional? The session adapter keeps its last error sticky.

Rebased onto #14698 to reuse lastOperationError, and removed the lastOperationError = null reset in BaseStorageReadChannel.read so the error remains sticky matching BlobReadSessionAdapter.

Co-authored by AI Agent

cpriti-os · 2026-11-11T05:32:01Z

+      public int read(ByteBuffer dst) throws IOException {
+        try {
+          int read = delegate.read(dst);
+          if (read > 0 && metricsContext != null) {


Is the metricsContext null check needed here? readAs only builds these wrappers when it is non-null.

Done! Removed the redundant metricsContext != null checks inside wrapSeekableByteChannel and wrapScatteringByteChannel.

Co-authored by AI Agent

cpriti-os · 2026-11-11T05:32:01Z

+        Attributes.builder()
+            .put(StorageClientMetrics.KEY_RPC_SYSTEM_NAME, "grpc")
+            .put(StorageClientMetrics.KEY_SERVER_ADDRESS, "storage.googleapis.com")
+            .put(StorageClientMetrics.KEY_SERVER_PORT, 443L)


Nit: server.port isn't part of the metric schema (only server.address is), so can we drop it from this fixture rather than asserting it passes through? If it ever shows up in real base attributes the exporter would reject the series.

Done! Dropped server.port from the test fixtures in StorageMetricsContextTest (and excluded KEY_SERVER_PORT in buildAttemptBaseAttributes).

Co-authored by AI Agent

…e-response-body-size [Generated-by: AI]

[Generated-by: AI]

feat: add gcp.storage.client.response.body.size metric for read opera…

e87b7d9

…tions

gemini-code-assist Bot reviewed Oct 23, 2026

View reviewed changes

Comment thread ...orage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageMetricsContext.java Resolved

Comment thread ...e/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageMetricsContextTest.java Resolved

gemini-code-assist Bot reviewed Oct 23, 2026

View reviewed changes

Comment thread ...e/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageMetricsContextTest.java Resolved

nidhiii-27 added 3 commits November 3, 2026 10:37

chore(storage): merge feat/storage-request-body-size and extract reco…

98d0b4e

…rdResponseBodySize

fix(storage): merge feat/storage-request-body-size and emit gcp.stora…

99caf7c

…ge.operation on response.body.size [Generated-by: AI]

nidhiii-27 marked this pull request as ready for review November 9, 2026 12:59

nidhiii-27 requested review from a team as code owners November 9, 2026 12:59

nidhiii-27 requested review from Dhriti07 and cpriti-os November 9, 2026 13:00

cpriti-os requested changes Nov 11, 2026

View reviewed changes

nidhiii-27 added 2 commits November 12, 2026 06:14

chore(storage): merge feat/storage-request-body-size into feat/storag…

1c66be1

…e-response-body-size [Generated-by: AI]

fix(storage): address PR review comments on response body size metric

b7c3a0d

[Generated-by: AI]

Conversation

nidhiii-27 commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

cpriti-os left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nidhiii-27 commented Oct 23, 2026 •

edited

Loading