Repository navigation
feat(storage): wire logical operation metrics in Retrying chokepoint - #14695
nidhiii-27 wants to merge 5 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 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]
b385679 to
0a3696b
Compare
| if (otel != null) { | ||
| mp = otel.getMeterProvider(); | ||
| } | ||
| } |
There was a problem hiding this comment.
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).
| StorageMetricsContext activeContext = | ||
| metricsContext != null | ||
| ? metricsContext | ||
| : StorageMetricsContext.create("storage.operation", clientAttributes); |
There was a problem hiding this comment.
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.
| .putAll(activeContext.getBaseAttributes()) | ||
| .put(StorageClientMetrics.KEY_GCP_STORAGE_OPERATION, activeContext.getOperationName()) | ||
| .put(StorageClientMetrics.KEY_RPC_METHOD, activeContext.getOperationName()) | ||
| .build(); |
There was a problem hiding this comment.
A few attribute mismatches here:
rpc.methodshould only be onrpc.client.call.durationandgcp.client.request.duration(with the full name likegoogle.storage.v2.Storage/ReadObject), not onoperations,attempts,retries, orerrors.operationsandgcp.client.request.durationare missinggcp.client.method.retriesis missingerror.type(from the failed attempt that triggered the retry) andnetwork.path, anderrorsis missingnetwork.path.attemptsis incremented before the call withouterror.type,rpc.response.status_code/http.response.status_code, ornetwork.path.
| .put(KEY_GCP_CLIENT_ARTIFACT, ARTIFACT_STORAGE) | ||
| .put(KEY_GCP_CLIENT_INSTANCE_ID, instanceId) | ||
| .build(); | ||
| .put(KEY_GCP_CLIENT_INSTANCE_ID, instanceId); |
There was a problem hiding this comment.
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.
| .setByteStringStrategy(ByteStringStrategy.noCopy()) | ||
| .resumable() | ||
| .withRetryConfig(retrier.withAlg(retryAlgorithmManager.idempotent())) | ||
| .withRetryConfig(retrier.withAlg(retryAlgorithmManager.idempotent(), metricsContext)) |
There was a problem hiding this comment.
Two issues with the operation wiring here (and similarly in StorageImpl.java):
gcp.storage.operationshould use the short canonical names (ReadObject,GetObject,WriteObject,CreateBucket,ListObjects, etc.) rather thanstorage.objects.get. Also, metadata calls (get) should beGetObjectwhile media reads (reader,readAllBytes,downloadTo) should beReadObject.- Because
DefaultRetrier.run()recordsoperationsandgcp.client.request.durationevery time it finishes, multi-step operations end up recording multiple operations—e.g.createFromandwriterrecord once forstartResumableWriteand once per chunk,copyrecords once per rewrite chunk,listrecords a new operation on everygetNextPage(), and ACL updates record both a get and an update. Meanwhilereader()never goes throughDefaultRetrier.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]
This branch has not been deployed
2 participants
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 witherror.type(OKor 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.Key Changes
Retrying.java:Retrier.run(ResultRetryAlgorithm<?> alg, Callable<Response> c, Decoder<Response, Model> d, @Nullable StorageMetricsContext metricsContext)acceptingmetricsContextas the optional last parameter.run(alg, c, d)delegating cleanly torun(alg, c, d, null).RetrierWithAlg.run(Callable, Decoder, @Nullable StorageMetricsContext).Retrying.newCallContext(@Nullable StorageMetricsContext metricsContext)to attach context to gRPC call context.runWithoutMetricswhen metrics are null/disabled.activeContext.getOtelContext().makeCurrent().StorageException.coalesce(e).StorageOptions.java:createMetricsContext(String operationName)package-private helper.getStorageClientMetrics()to options.HttpStorageOptions.java:getStorageClientMetrics()inRetryDependenciesAdapter.RetryingMetricsTest.java:Verification
fmt-maven-plugin:format(0 files reformatted).checkstyle:check@checkstyle(0 Checkstyle violations).RetryingMetricsTest,StorageMetricsContextTest,StorageOptionsTest,StorageClientMetricsTest,RetryingTest).[Generated-by: AI]