[nidhiii-27]

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).

[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]

To improve maintainability and readability, avoid duplicating the long chain of !key.equals(...) checks. Instead, define a private static final Set<AttributeKey<?>> EXCLUDED_KEYS constant containing the excluded keys, and use !EXCLUDED_KEYS.contains(key) here.

        baseAttributes.forEach(
            (key, val) -> {
              if (!EXCLUDED_KEYS.contains(key)) {
                @SuppressWarnings("unchecked")
                AttributeKey<Object> objKey = (AttributeKey<Object>) key;
                bodyAttrBuilder.put(objKey, val);
              }
            });

[nidhiii-27]

Addressed in commit 404687b: extracted BODY_SIZE_EXCLUDED_KEYS and a buildBodySizeAttributes(StorageClientMetrics) helper method in StorageMetricsContext and used it for both request.body.size and response.body.size.

Co-authored by AI Agent

[gemini-code-assist]

Avoid using fully qualified class names (such as io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader) when they can be imported. This unnecessarily reduces code readability and violates the project's general rules.

    InMemoryMetricReader reader = InMemoryMetricReader.create();
    SdkMeterProvider provider =
        SdkMeterProvider.builder()
            .registerMetricReader(reader)
            .build();
    StorageClientMetrics metrics = StorageClientMetrics.create(provider, true, false);

    StorageMetricsContext context = StorageMetricsContext.create("ReadObject");
    context.setResponseBytes(4096L);
    context.recordOperation(metrics, null);

    Collection<MetricData> collected = reader.collectAllMetrics();
    MetricData bodySizeData =
        collected.stream()
            .filter(
                m ->
                    m.getName()
                        .equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_RESPONSE_BODY_SIZE))
            .findFirst()
            .orElse(null);

    assertThat(bodySizeData).isNotNull();
    assertThat(bodySizeData.getHistogramData().getPoints()).hasSize(1);
    HistogramPointData point =
        bodySizeData.getHistogramData().getPoints().iterator().next();

References

1. Do not use fully qualified class names if there is no class name conflict in the file and the class is already imported, as it unnecessarily reduces code readability.

[nidhiii-27]

Addressed in commit 404687b: imported HistogramPointData and replaced all inline fully qualified class names with simple class names and shared helper methods (findMetric, getSingleHistogramPoint) in StorageMetricsContextTest.

Co-authored by AI Agent

[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.

[gemini-code-assist]

Avoid using fully qualified class names (such as io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader, io.opentelemetry.sdk.metrics.SdkMeterProvider, java.util.Collection, io.opentelemetry.sdk.metrics.data.MetricData, and io.opentelemetry.sdk.metrics.data.HistogramPointData) when there is no class name conflict in the file and the classes are already imported or can be imported. This unnecessarily reduces code readability. Please import these classes and use their simple names instead. This also applies to the other new tests in this file.

    InMemoryMetricReader reader =
        InMemoryMetricReader.create();
    SdkMeterProvider provider =
        SdkMeterProvider.builder()
            .registerMetricReader(reader)
            .build();
    StorageClientMetrics metrics = StorageClientMetrics.create(provider, true, false);

    StorageMetricsContext context = StorageMetricsContext.create("ReadObject");
    context.setResponseBytes(4096L);
    context.recordOperation(metrics, null);

    Collection<MetricData> collected =
        reader.collectAllMetrics();
    MetricData bodySizeData =
        collected.stream()
            .filter(
                m ->
                    m.getName()
                        .equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_RESPONSE_BODY_SIZE))
            .findFirst()
            .orElse(null);

    assertThat(bodySizeData).isNotNull();
    assertThat(bodySizeData.getHistogramData().getPoints()).hasSize(1);
    HistogramPointData point =
        bodySizeData.getHistogramData().getPoints().iterator().next();
    assertThat(point.getCount()).isEqualTo(1L);
    assertThat(point.getSum()).isEqualTo(4096.0);
    assertThat(point.getAttributes().get(StorageClientMetrics.KEY_RPC_METHOD))
        .isEqualTo("ReadObject");

References

1. Do not use fully qualified class names (such as java.time.Duration) if there is no class name conflict in the file and the class is already imported, as it unnecessarily reduces code readability.

[nidhiii-27]

Addressed in commit 404687b: replaced all inline fully qualified class names with imported simple class names and shared helper methods in StorageMetricsContextTest.

Co-authored by AI Agent