Repository navigation
feat(storage): wire logical operation metrics in Retrying and StorageOptions - #14695
nidhiii-27 wants to merge 4 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.
608c46a to
1bf9a9d
Compare
b385679 to
0a3696b
Compare
…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]
df24cde to
2455496
Compare
98fb508 to
84a5ac6
Compare
2455496 to
02b170e
Compare
nidhiii-27
changed the title
f943017 to
edb3def
Compare
…Options [Generated-by: AI]
02b170e to
2b123b7
Compare
| } | ||
|
|
||
| @Nullable | ||
| private SdkMeterProvider createDefaultClientMeterProvider() { |
There was a problem hiding this comment.
Could we move createDefaultClientMeterProvider (and formatStorageEndpoint) into OpenTelemetryBootstrappingUtils?
StorageOptions is a public configuration/builder class (ServiceOptions), so it should ideally just hold user configuration and delegate initialization rather than containing ~80 lines of Cloud Monitoring exporter setup and endpoint string formatting.
| Attributes detectedAttributes = resourceProvider.getAttributes(); | ||
| String detectedProjectId = detectedAttributes.get(AttributeKey.stringKey("cloud.account.id")); | ||
| String configuredProjectId = getProjectId(); | ||
| if (configuredProjectId != null && configuredProjectId.trim().isEmpty()) { |
There was a problem hiding this comment.
In OpenTelemetryBootstrappingUtils.enableGrpcMetrics, detectedProjectId takes precedence over configuredProjectId (detectedProjectId == null ? projectId : detectedProjectId), whereas here configuredProjectId takes precedence over detectedProjectId. Is this difference intentional? If both are enabled on a gRPC client where getProjectId() differs from the compute environment's project, transport metrics and client metrics will be exported to different projects.
| } | ||
|
|
||
| @Nullable | ||
| static String extractServerAddress(@Nullable String host) { |
There was a problem hiding this comment.
Can we use Guava's HostAndPort.fromString(...) here instead of manually parsing colons and IPv6 brackets? Since formatStorageEndpoint above already strips the scheme (google-c2p:///, https://, http://), both methods could share a small helper that strips the scheme/path and returns a HostAndPort—then this is just .getHost() and formatStorageEndpoint is .withDefaultPort(443).toString().
Wires client-level metrics lifecycle and retrier operation metrics into StorageOptions and Retrying.
StorageClientMetricsprovider and lifecycle management inStorageOptions.Retryingto record operation attempts, durations, and status attributes.StorageOptionsTest,StorageClientMetricsTest,RetryingMetricsTest).Stacked on #14728.