blob: connection-pool saturation metrics on every cloud - #532
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #532 +/- ##
============================================
+ Coverage 82.39% 82.41% +0.01%
Complexity 662 662
============================================
Files 210 216 +6
Lines 14334 14451 +117
Branches 1932 1948 +16
============================================
+ Hits 11811 11910 +99
- Misses 1696 1706 +10
- Partials 827 835 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d9a8595 to
1f46e3e
Compare
hkhiri
changed the title
…blisher # Conflicts: # blob/blob-ali/src/main/java/com/salesforce/multicloudj/blob/ali/AliBlobStore.java # blob/blob-ali/src/main/java/com/salesforce/multicloudj/blob/ali/async/AliAsyncBlobStore.java
Adds an opt-in withMetricsPublisher(...) hook that samples HTTP connection-pool utilization (max / leased / available / pending) and publishes cloud-agnostic Metric objects, so operators can observe pool saturation uniformly across providers. - multicloudj-common: MetricsPublisher, Metric, ConnectionPoolMetrics - blob-client: withMetricsPublisher on BlobStoreBuilder - AWS: AwsMetricsPublisherAdapter bridges the native MetricPublisher SPI - GCP: GcpConnectionPoolMetricsInterceptor samples the Apache pool per response (installed only when the builder builds its own transport) Validation evidence (see VALIDATION_532.md): - Accuracy (live GCS): under a saturated 4-connection pool with 24 concurrent uploads, published metrics track reality exactly (observedMax=4, peakLeased=4, peakPending=20, minAvailable=0); reproduced identically across two runs. - Overhead (JMH microbench): the full per-response sampling path costs 10.6 +/- 0.1 ns/op -- ~0.000007% of a ~150ms GCS request, confirming sampling sits effectively off the request path. Provider-agnostic (the sampled path lives in multicloudj-common). Builds on the approach proposed in the community PR salesforce#532. Alibaba support is deferred to a follow-up (accuracy not yet validated on OSS). Co-authored-by: Haythem Khiri <hxemos@gmail.com>
sandeepvinayak
left a comment
sandeepvinayak
left a comment
There was a problem hiding this comment.
these are the major changes for adding metrics in the SDK, please submit a design doc with business use-case. These changes cannot be accepted without design approval.
|
Thanks @sandeepvinayak for the feedback. Makes total sense to have an approved design doc for adding metrics capabilities across the SDK. I will draft the design document outlining the architecture, cloud-agnostic abstractions, and business use cases, and share it with you for review before we proceed further with this PR. Thanks! |
|
Hi @sandeepvinayak, |
| overrideConfig.apiCallTimeout( | ||
| Duration.ofMillis(config.getRetryConfig().getTotalTimeout())); | ||
| if (config.getMetricsPublisher() != null) { | ||
| overrideConfig.addMetricPublisher( |
There was a problem hiding this comment.
The CRT async client silently drops the metrics publisher, so withParallelDownloadsEnabled(true) produces zero metrics on AWS.
buildS3Client routes to buildCrtS3Client whenever parallelDownloadsEnabled is TRUE (line 597), and that path calls the S3CrtAsyncClientBuilder overload of applyCommonConfig (line 756), which this PR does not touch — no addMetricPublisher is wired there. It cannot be: in aws-sdk 2.42.40 S3CrtAsyncClientBuilder extends SdkBuilder only and exposes no overrideConfiguration/addMetricPublisher, so the CRT client can never emit these metrics. Meanwhile BlobStoreBuilder#withMetricsPublisher documents "AWS: fully supported ... cover both the synchronous and asynchronous HTTP connection pools." The net effect is that the highest-throughput AWS configuration — precisely the one where pool saturation matters most — reports nothing, with no exception, warning, or log to tell the operator their dashboard is empty for a reason.
Suggested fix: log a warning from buildCrtS3Client when getMetricsPublisher() != null, and state the CRT exclusion explicitly in the withMetricsPublisher javadoc.
| HttpClient client = AliInstrumentedHttpClientFactory.instrument(new NoopPublisher(), sync); | ||
|
|
||
| assertNotNull(client); | ||
| assertTrue(client instanceof AliConnectionPoolMetricsHttpClient); |
There was a problem hiding this comment.
Both tests in this class pass by construction and never check the property the class javadoc says they guard.
AliInstrumentedHttpClientFactory.instrument(...) unconditionally returns new AliConnectionPoolMetricsHttpClient(...), so assertNotNull and the instanceof assertion cannot fail no matter what the OSS SDK's transport looks like. What actually decides whether Alibaba emits anything is poolStatsSupplier(client), which returns null when client.getConnectionManager() is not a PoolingHttpClientConnectionManager; in that case the wrapper is still returned, still satisfies both assertions, and publishes nothing for the lifetime of the client. That silent-no-op is exactly the risk the class comment claims coverage for — "guards the assumption that the SDK-built clients expose Apache pooling managers whose getTotalStats() is readable" — and it is the one thing left unasserted. This is also the only test in the PR that exercises a real SDK-built Alibaba transport.
Suggested fix: assert the assumption directly, e.g. assertInstanceOf(PoolingHttpClientConnectionManager.class, sync.getConnectionManager()) (and the async equivalent), so a transport-shape change in the OSS SDK breaks the build instead of the metrics.
| } | ||
| // Attach response interceptor to sample Apache HTTP connection pool state per request | ||
| if (builder.getMetricsPublisher() != null) { | ||
| PoolingHttpClientConnectionManager connectionManager = buildConnectionManager(builder); |
There was a problem hiding this comment.
On GCP a metrics publisher alone swaps the transport and creates two independent pools that publish under identical, unlabelled metric names.
shouldConfigureHttpClient now returns true when only a publisher is set, and both buildStorage (line 1650) and buildMultipartUploadClient (line 1677) call buildTransportOptions(builder) independently. Each call runs buildHttpClient, so each gets its own PoolingHttpClientConnectionManager and its own interceptor. Metric carries only name/value/category, so a publisher has no way to tell the two apart: LeasedConcurrency alternates between the Storage pool and the MPU pool depending on which fired last, and MaxConcurrency reports 200 while the process may actually hold 400 connections to GCS. It also contradicts withMetricsPublisher's "It never alters client behavior" — a caller who set no other HTTP option is moved off the GCS SDK's default transport purely by observing it. Separately, PR #533 adds a buildConnectionManager to this same class with different per-route defaults; merging both will need care.
Suggested fix: build one connection manager per store and share it across both transports, or give Metric a pool identifier.
| * | ||
| * <p>Called when the owning client is closed. | ||
| */ | ||
| default void close() {} |
There was a problem hiding this comment.
This close() is documented as "Called when the owning client is closed", but no provider ever calls it.
GcpBlobStore.close() closes the TransferManager and Storage, AliBlobStore.close() closes the OSS client, and AwsBlobStore.close() closes the S3 client — none of them touch the publisher, and neither async store does either. AWS looks covered because AwsMetricsPublisherAdapter.close() delegates here, but the SDK never invokes it: publishers are stored under SdkClientOption.METRIC_PUBLISHERS as a List, and SdkClientConfiguration.close() only closes attribute values that are themselves AutoCloseable, which a List is not. So on all three providers a publisher that buffers and flushes on close() — the exact pattern this javadoc invites, given publish is documented to "return quickly, deferring ... to a background thread" — silently drops its final batch and leaks whatever executor it owns.
Suggested fix: invoke metricsPublisher.close() from each provider's close(), or drop this sentence and state that the caller owns the publisher's lifecycle.
Hi @hkhiri, could you grant me access to this doc? My email address is barry.liu@salesforce.com. |
4 participants
Summary
Connection-pool exhaustion is a leading cause of latency spikes and timeout incidents in production, and today we're flying blind, there's no way to see pool saturation until requests already start failing.
This adds an opt-in withMetricsPublisher(...) hook that surfaces connection-pool metrics across AWS, GCP, and Alibaba (pool saturation, sync and async) so teams can catch capacity problems before they page someone, size pools on evidence instead of guesswork, and cut mean-time-to-diagnose when incidents do hit. The metric names are identical on every cloud, so one dashboard and one set of alerts work everywhere.
Off by default, zero change when unset. AWS bridges the SDK's native metric SPI; GCP samples its Apache pool per request; Ali wraps the SDK's own transport (TLS/reaper/timeouts preserved, pools still released on close). Sampling sits off the request path and never throws. Foundation for the pool/retry tuning next.