feat(storage): wire logical operation metrics in Retrying chokepoint by nidhiii-27 · Pull Request #14695 · googleapis/google-cloud-java

nidhiii-27 · 2026-10-20T06:23:38Z

Description

This PR is part of the Standard Metrics implementation phase for Google Cloud Storage Java SDK (PR 2 of 3), stacked on #14694.

It wires the logical client metrics into the retry execution chokepoint (Retrying$DefaultRetrier.run(...)):

gcp.client.request.duration: Histogram recording total logical operation latency across all attempts.
gcp.storage.client.operations: Counter recording completed logical operations tagged with error.type (OK or mapped error status).
gcp.storage.client.attempts: Counter tracking every attempt executed.
gcp.storage.client.retries: Counter tracking retry attempts initiated after failure.
gcp.storage.client.errors: Counter tracking attempt-level errors categorized by canonical error type.

Note: Veneer operation-level name wiring across GrpcStorageImpl, GapicCopyWriter, and StorageImpl has been split into a dedicated follow-up stacked PR (feat/storage-veneer-operation-metrics).

Key Changes

Retrying.java:
- Overloaded Retrier.run(ResultRetryAlgorithm<?> alg, Callable<Response> c, Decoder<Response, Model> d, @Nullable StorageMetricsContext metricsContext) accepting metricsContext as the optional last parameter.
- Preserved default run(alg, c, d) delegating cleanly to run(alg, c, d, null).
- Added RetrierWithAlg.run(Callable, Decoder, @Nullable StorageMetricsContext).
- Added Retrying.newCallContext(@Nullable StorageMetricsContext metricsContext) to attach context to gRPC call context.
- Zero-overhead fast path in runWithoutMetrics when metrics are null/disabled.
- Trace exemplar support by scoping activeContext.getOtelContext().makeCurrent().
- Raw exception recording before StorageException.coalesce(e).
StorageOptions.java:
- Added createMetricsContext(String operationName) package-private helper.
- Connected getStorageClientMetrics() to options.
HttpStorageOptions.java:
- Exposed getStorageClientMetrics() in RetryDependenciesAdapter.
RetryingMetricsTest.java:
- Comprehensive unit tests covering first-attempt success, retry recoveries, terminal errors, disabled metrics fast-path, custom metrics context propagation, exemplar scope attachment, and gRPC call context attachment.

Verification

Formatted with fmt-maven-plugin:format (0 files reformatted).
Verified with checkstyle:check@checkstyle (0 Checkstyle violations).
Verified unit tests (71 tests passing in RetryingMetricsTest, StorageMetricsContextTest, StorageOptionsTest, StorageClientMetricsTest, RetryingTest).

[Generated-by: AI]

gemini-code-assist

Code Review

This pull request integrates OpenTelemetry metrics into the client-side retry mechanism of Google Cloud Storage. It updates StorageOptions, GrpcStorageOptions, and HttpStorageOptions to expose StorageClientMetrics, and modifies Retrying.DefaultRetrier to record attempts, errors, retries, operations, and duration metrics when metrics are enabled. Additionally, a comprehensive test suite RetryingMetricsTest has been added to verify metric collection across various retry scenarios. There are no review comments, and I have no feedback to provide.

Wire standard client metrics into Retrying$DefaultRetrier to record logical operation metrics across all retry attempts: - gcp.client.request.duration: Measures total logical operation latency across all attempts. - gcp.storage.client.operations: Counter recording completed logical operations by status (OK, error type). - gcp.storage.client.attempts: Counter tracking every attempt executed. - gcp.storage.client.retries: Counter tracking retry attempts initiated after failure. - gcp.storage.client.errors: Counter tracking attempt-level errors categorized by canonical error type. Expose StorageClientMetrics through StorageOptions, RetryingDependencies, HttpStorageOptions, and GrpcStorageOptions, ensuring clean propagation into Retrying.run(...) while preserving zero overhead when metrics are disabled or null. Add comprehensive unit test suite in RetryingMetricsTest covering: - Successful call on first attempt - Retried call succeeding on 3rd attempt - Terminal failure when all attempts fail - Disabled metrics producing zero metric recordings - Fast-path behavior when StorageClientMetrics is null - Retrier creation and wiring from StorageOptions [Generated-by: AI]

… operations across transports - Overload Retrier.run() and RetrierWithAlg.run() to accept @nullable StorageMetricsContext as the last parameter - Constructor-inject @nullable StorageMetricsContext in DefaultRetrierWithAlg - Attach StorageMetricsContext to GrpcCallContext in Retrying.newCallContext(metricsContext) - Fast-path zero overhead in DefaultRetrier.run() when metrics are null/disabled - Wrap execution in otelContext.makeCurrent() scope for trace exemplar collection - Record raw exceptions before StorageException.coalesce() - Wire operation-scoped metrics context across StorageImpl and GrpcStorageImpl - Propagate StorageMetricsContext in GrpcStorageImpl pagination decorators - Add rpc.system.name and server.address attributes to StorageClientMetrics - Add comprehensive unit tests in RetryingMetricsTest, StorageClientMetricsTest, and StorageOptionsTest [Generated-by: AI]

cpriti-os · 2026-10-20T13:08:05Z

+        if (otel != null) {
+          mp = otel.getMeterProvider();
+        }
+      }


getOpenTelemetry() defaults to OpenTelemetry.noop() (non-null), so when enableOtelMetrics=true and no custom MeterProvider is set, mp becomes a no-op provider and OpenTelemetryBootstrappingUtils.createClientMeterProvider(...) never gets called. We should only use getOpenTelemetry().getMeterProvider() if isCustomOpenTelemetryConfigured() is true, and otherwise create the default Cloud Monitoring provider (and close it when the client closes).

cpriti-os · 2026-10-20T13:08:05Z

+      StorageMetricsContext activeContext =
+          metricsContext != null
+              ? metricsContext
+              : StorageMetricsContext.create("storage.operation", clientAttributes);


When metrics are enabled and an internal caller calls the 3-arg retrier.run(alg, c, d) without a context (e.g. HttpCopyWriter.copyChunk() or page fetchers), this falls back to "storage.operation", which isn't a valid operation name and emits extra operation points for internal sub-calls.

cpriti-os · 2026-10-20T13:08:05Z

+              .putAll(activeContext.getBaseAttributes())
+              .put(StorageClientMetrics.KEY_GCP_STORAGE_OPERATION, activeContext.getOperationName())
+              .put(StorageClientMetrics.KEY_RPC_METHOD, activeContext.getOperationName())
+              .build();


A few attribute mismatches here:

rpc.method should only be on rpc.client.call.duration and gcp.client.request.duration (with the full name like google.storage.v2.Storage/ReadObject), not on operations, attempts, retries, or errors.

operations and gcp.client.request.duration are missing gcp.client.method.

retries is missing error.type (from the failed attempt that triggered the retry) and network.path, and errors is missing network.path.

attempts is incremented before the call without error.type, rpc.response.status_code / http.response.status_code, or network.path.

cpriti-os · 2026-10-20T13:08:05Z

            .put(KEY_GCP_CLIENT_ARTIFACT, ARTIFACT_STORAGE)
-            .put(KEY_GCP_CLIENT_INSTANCE_ID, instanceId)
-            .build();
+            .put(KEY_GCP_CLIENT_INSTANCE_ID, instanceId);


instance_id is only a Monitored Resource label (already set on Resource in OpenTelemetryBootstrappingUtils), not a metric point attribute. Let's drop KEY_GCP_CLIENT_INSTANCE_ID from clientAttributes.

cpriti-os · 2026-10-20T13:08:05Z

            .setByteStringStrategy(ByteStringStrategy.noCopy())
            .resumable()
-            .withRetryConfig(retrier.withAlg(retryAlgorithmManager.idempotent()))
+            .withRetryConfig(retrier.withAlg(retryAlgorithmManager.idempotent(), metricsContext))


Two issues with the operation wiring here (and similarly in StorageImpl.java):

gcp.storage.operation should use the short canonical names (ReadObject, GetObject, WriteObject, CreateBucket, ListObjects, etc.) rather than storage.objects.get. Also, metadata calls (get) should be GetObject while media reads (reader, readAllBytes, downloadTo) should be ReadObject.

Because DefaultRetrier.run() records operations and gcp.client.request.duration every time it finishes, multi-step operations end up recording multiple operations—e.g. createFrom and writer record once for startResumableWrite and once per chunk, copy records once per rewrite chunk, list records a new operation on every getNextPage(), and ACL updates record both a get and an update. Meanwhile reader() never goes through DefaultRetrier.run(), so its operation metrics aren't recorded on close.

…options - Narrow PR #14695 to core Retrying chokepoint and options wiring - Remove per-run StorageMetricsContext binding from DefaultRetrierWithAlg to avoid elapsed time drift across runs - Add createMetricsContext helper on StorageOptions - Revert veneer operation wiring and server address attributes to separate stacked PR - Update unit tests in RetryingMetricsTest [Generated-by: AI]

… feat/storage-logical-metrics [Generated-by: AI]

…sContext - Include rpc.system.name and server.address in createMetricsContext Attributes - Align RetryingMetricsTest operation names to canonical PascalCase GetObject - Add tests in StorageOptionsTest for extractServerAddress and createMetricsContext [Generated-by: AI]

nidhiii-27 added the ai-generated label Oct 20, 2026

gemini-code-assist Bot reviewed Oct 20, 2026

View reviewed changes

nidhiii-27 added this pull request to stack #14485 October 20, 2026 10:06

nidhiii-27 added 2 commits October 20, 2026 11:51

nidhiii-27 force-pushed the feat/storage-logical-metrics branch from b385679 to 0a3696b Compare October 20, 2026 12:11

cpriti-os requested changes Oct 20, 2026

View reviewed changes

nidhiii-27 added 2 commits October 20, 2026 13:41

chore(storage): merge feat/storage-metrics-context-error-mapping into…

e0256ac

… feat/storage-logical-metrics [Generated-by: AI]

nidhiii-27 mentioned this pull request Oct 20, 2026

feat(storage): wire veneer operation metrics in StorageImpl and GrpcStorageImpl #14698

Open

Navigation Menu

Repository navigation

feat(storage): wire logical operation metrics in Retrying chokepoint - #14695

feat(storage): wire logical operation metrics in Retrying chokepoint#14695
nidhiii-27 wants to merge 5 commits into
feat/storage-metrics-context-error-mappingfrom
feat/storage-logical-metrics

nidhiii-27 commented Oct 20, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

cpriti-os Oct 20, 2026

Uh oh!

cpriti-os Oct 20, 2026

Uh oh!

cpriti-os Oct 20, 2026

Uh oh!

cpriti-os Oct 20, 2026

Uh oh!

cpriti-os Oct 20, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Description

Key Changes

Verification

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

cpriti-os Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

cpriti-os Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

cpriti-os Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

cpriti-os Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

cpriti-os Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nidhiii-27 commented Oct 20, 2026 •

edited

Loading