blob: opt-in connection-pool saturation metrics for AWS and GCP

[iamabhilaksh]

Summary

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. Off by default, zero change when unset.

• multicloudj-common: MetricsPublisher, Metric, ConnectionPoolMetrics
• AWS: AwsMetricsPublisherAdapter bridges the native MetricPublisher SPI
• GCP: GcpConnectionPoolMetricsInterceptor samples the Apache pool per response (installed only when the builder owns its transport; mirrors the default 200/20 pool sizing so enabling metrics never shrinks the pool)
• blob-client: withMetricsPublisher on BlobStoreBuilder

Relationship to #532

Reworks and supersedes @hxe-m's #532 — carries the same feature forward with a common-module abstraction and validation evidence. Retained as Co-authored-by. Alibaba support is deferred to a follow-up (accuracy not yet validated on OSS).

Validation

• 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): full per-response sampling path costs 10.6 ± 0.1 ns/op (~0.000007% of a ~150 ms GCS request).
• Unit tests: 1048 across the four touched modules, 0 failures; checkstyle clean.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 89.81481% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.63%. Comparing base (7d64089) to head (f05830f).

Files with missing lines	Patch %	Lines
.../multicloudj/blob/aws/async/AwsAsyncBlobStore.java	90.00%	2 Missing ⚠️
...lticloudj/blob/async/client/AsyncBucketClient.java	0.00%	2 Missing ⚠️
...lesforce/multicloudj/blob/client/BucketClient.java	0.00%	2 Missing ⚠️
...rce/multicloudj/blob/driver/BlobClientBuilder.java	0.00%	2 Missing ⚠️
.../salesforce/multicloudj/blob/gcp/GcpBlobStore.java	88.88%	1 Missing and 1 partial ⚠️
...icloudj/common/observability/MetricsPublisher.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #561      +/-   ##
============================================
+ Coverage     83.61%   83.63%   +0.02%     
  Complexity      674      674              
============================================
  Files           215      219       +4     
  Lines         15010    15099      +89     
  Branches       2076     2089      +13     
============================================
+ Hits          12550    12628      +78     
- Misses         1636     1646      +10     
- Partials        824      825       +1     

Flag	Coverage Δ
unittests	83.63% <89.81%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hkhiri]

Hi @iamabhilaksh,

Thanks for looking into this. However, since #532 was already covering this exact architecture and feature set (AWS, GCP, Alibaba, and metric abstractions), the standard practice would have been to review #532 or push commits directly onto that branch rather than opening a superseding PR.

Since the core implementation and design stem from #532, let's keep #532 as the primary PR. Feel free to push your JMH benchmarks and the common-module refactoring directly to my branch so we keep the authorship, commit history, and discussion clean in one place.

Thanks!

[kchoy-sfdc]

A few notes from a local review — one functional gap on the GCP path (inline) plus a couple of smaller items. Nice feature overall; the abstraction is clean and off-by-default.

Work-item tag in the title: small thing — could we drop the [W-…] work-item tag from the PR title? Since this repo is public, it's cleaner to keep work-item ↔ PR traceability tracked on our side rather than in the title. I checked and it's only in the title — the commit messages and changed files are all clean — so this is just a quick title edit.

[kchoy-sfdc]

GCP: a metrics-only builder never installs this interceptor.

This block only runs inside buildHttpClient, which is only reached when shouldConfigureHttpClient (line 1618) returns true — and that gate checks proxyEndpoint || maxConnections || socketTimeout || idleConnectionTimeout, but not getMetricsPublisher(). So a builder with only withMetricsPublisher(...) set (no proxy/maxConnections/timeout) never reaches here: buildStorage / buildMultipartUploadClient skip setTransportOptions(...), the interceptor is never registered, and nothing is ever published.

That's the most likely way an operator enables the feature ("I just want pool metrics"), so it's effectively a silent no-op for the common case, and it contradicts the builder javadoc promise that the four counters are emitted on every supported provider.

Suggested fix: add || builder.getMetricsPublisher() != null to shouldConfigureHttpClient. Note the two new GCP tests call buildHttpClient reflectively, so they bypass this gate and won't catch it — worth adding one test that builds a metrics-only client through build() and asserts the interceptor is actually wired onto the transport.

[iamabhilaksh]

Good catch, this was exactly right. Added || builder.getMetricsPublisher() != null to shouldConfigureHttpClient so a metrics-only builder still owns its transport and installs the interceptor.

On the test: I added testShouldConfigureHttpClient_trueWhenOnlyMetricsPublisherSet, but you're right that it goes through the reflective gate helper, so it proves the gate flips, not that the interceptor lands on the transport end-to-end through build(). Fair to call that partial coverage. [Thinking out loud] a true build()-through assertion means reaching into the HttpTransportOptions to confirm the interceptor is registered, which is a bit awkward given how the transport is constructed — let me take a proper look and either add it here or track it as a focused follow-up. Won't claim it's covered until it is.

[iamabhilaksh]

Update: went ahead and added the end-to-end assertion rather than deferring it. testBuildHttpClient_metricsOnlyInstallsPoolMetricsInterceptor builds the client through buildHttpClient(...) for a metrics-only builder and walks the Apache exec chain (execChain → ProtocolExec.httpProcessor → responseInterceptors) to assert GcpConnectionPoolMetricsInterceptor is actually registered on the transport — so it now proves the wiring, not just the gate. Pushed in f05830f.

[kchoy-sfdc]

The CRT-backed async client doesn't wire the metrics publisher.

This wires the adapter for the standard S3AsyncClientBuilder path, but the S3CrtAsyncClientBuilder overload (applyCommonConfig around line 757) has no equivalent — so withParallelDownloadsEnabled(true) + withMetricsPublisher(...) silently emits nothing (the CRT path is taken when parallel downloads are enabled).

This is defensible since CRT uses a native HTTP stack rather than the SDK metric SPI, but the builder javadoc states AWS support for "both the synchronous and asynchronous clients" with no caveat. Could we either add a one-line javadoc note that the CRT-backed async path (parallel downloads) does not emit metrics, or log a debug message when a publisher is set on the CRT path — so it's a documented limitation rather than a silent gap?

[iamabhilaksh]

Went with the javadoc route. Added a caveat on BlobStoreBuilder.withMetricsPublisher noting the CRT-backed async client (parallel downloads) uses a native HTTP stack and doesn't emit connection-pool metrics, plus a matching comment on the CRT applyCommonConfig so it's documented at the code site too. Made the AWS bullet in the javadoc explicit about the exception rather than the unqualified "both sync and async" it promised before.

[kchoy-sfdc]

Can we use import statement instead of FQCN here?

[iamabhilaksh]

Done — imported software.amazon.awssdk.metrics.MetricPublisher and dropped the FQCN.

[LihaoLiuXs]

Feature is unreachable through the public client builders. withMetricsPublisher is added only to the abstract SPI BlobStoreBuilder. Unlike its siblings withRetryConfig and withTracingPolicy — which are re-declared as delegating methods on both BucketClient.BlobBuilder (withRetryConfig ~L1051, withTracingPolicy ~L1100) and BlobClient.BlobClientBuilder (~L145 / ~L156) — no delegating withMetricsPublisher was added to either user-facing builder. A normal caller who goes through BucketClient/BlobClient cannot set a publisher; the only way to reach it is to instantiate new AwsBlobStore.Builder() / new GcpBlobStore.Builder() at the SPI level and cast (which is exactly what the new tests do). As written, the feature ships dead to end users. Please add the delegating withMetricsPublisher(...) to BucketClient.BlobBuilder and BlobClient.BlobClientBuilder (and the async equivalents) to follow the established three-place pattern.

[iamabhilaksh]

Agreed, this was the big one — it shipped dead to anyone going through the public builders. Added the delegating withMetricsPublisher(...) in all three places to match the withRetryConfig/withTracingPolicy pattern: BlobClientBuilder, BucketClient.BlobBuilder, and AsyncBucketClient.Builder (which overrides and calls super). So a normal caller can now set a publisher without dropping to the SPI and casting.

[LihaoLiuXs]

MetricsPublisher.close() is never invoked on any path — the documented lifecycle contract is unfulfilled and buffered publishers leak. This javadoc promises "Called when the owning client is closed," but nothing calls it. GcpBlobStore.close() closes only transferManager and storage; the interceptor's publisher is never closed. On AWS, AwsMetricsPublisherAdapter.close() correctly delegates, but nothing ever closes the adapter — AwsBlobStore.close() / AwsAsyncBlobStore.close() close only the S3 client, and the AWS SDK v2 does not close MetricPublishers registered via addMetricPublisher (ownership stays with the caller). The interface explicitly recommends deferring transmission to a background thread, so a buffered/threaded publisher will never be flushed or shut down when the blob client is closed — leaking a thread/executor per client instance. Either wire close() through both providers' close(), or drop the method from the contract.

[iamabhilaksh]

Fixed on both providers. GcpBlobStore, AwsBlobStore, and AwsAsyncBlobStore now hold the supplied MetricsPublisher and call close() on it in their own close(), with a comment noting neither the GCS SDK nor AWS SDK v2 closes caller-registered publishers. Added AwsBlobStoreTest#testCloseReleasesMetricsPublisher asserting the publisher is closed. So a buffered/threaded publisher gets shut down with the client now instead of leaking.

[LihaoLiuXs]

The public Metric contract over-promises neutrality and will be hard to tighten later. Metric/MetricsPublisher are documented as cloud-agnostic, but value is a bare Object and category is a free-form String, and the two providers populate them asymmetrically: GCP emits exactly the four ConnectionPoolMetrics counters under category "HttpClient", while AwsMetricsPublisherAdapter forwards the SDK's entire native metric tree (raw AWS names like ApiCallDuration/OperationName, values of Duration/String/int, categories ApiCall/ApiCallAttempt/HttpClient). So a consumer written to the "neutral" contract can portably rely on only four names under one category; everything else is AWS-shaped and absent on GCP, with no compile-time help to tell them apart. This is a brand-new public API surface — since value:Object + free-form category can't be narrowed later without a breaking change, please make this a conscious API commitment: document the value type of the four guaranteed pool counters, and consider gating the AWS full-tree passthrough behind an explicit opt-in so the default contract stays genuinely provider-neutral.

[iamabhilaksh]

This is the one I'd like to keep open for a bit. I did the first half: documented the provider-neutral contract on Metric — the four ConnectionPoolMetrics counters under category "HttpClient" with an Integer value are the only cross-provider guarantee, and anything else (the AWS ApiCall/ApiCallAttempt tree, Duration/String values) is an explicitly non-portable superset a consumer must not assume.

On the second half — gating the AWS full-tree passthrough behind an explicit opt-in so the default stays strictly neutral — I think you're right that it's the cleaner API commitment, and since value:Object + free-form category can't be narrowed later without a break, it's worth deciding deliberately rather than in a PR thread. I've written up the tradeoff in a short design doc and am setting up a quick review to lock the call. Happy to go either way (document-only vs. opt-in flag) based on where that lands — will circle back here with the decision.

[hkhiri]

Hi everyone,

Just to give an update @sandeepvinayak requested the design RFC for the connection-pool telemetry architecture on PR #532. I've published the proposal here: https://docs.google.com/document/d/15nN0RBYwVIMOdCAzZeXgRvu73aoLxyO5mcMxYQoXjGU/edit?usp=sharing

To keep the review feedback and maintainer sign-offs easy to track in one place for Sandeep, it would be great if we could consolidate the remaining review on PR #532.

@iamabhilaksh Great catch on the builder delegation and close() lifecycle handling. Would you mind pushing those commits onto branch blob-aws-metrics-publisher on #532? That way we can merge all these improvements together on the primary branch and keep everyone's co-authorship clean.

Thanks!

[LihaoLiuXs]

Why do we need to expose withMetricsPublisher(...)? Was this feature requested by a client, or is it to unblock the benchmark test?

[hkhiri]

Hi @LihaoLiuXs,

Yes, this is a direct production requirement from my team (SeaS/Search).

During heavy Solr indexing bursts, backups, and segment restores, connection pool starvation is one of our main culprits for I/O latency spikes and timeouts. Exposing withMetricsPublisher(...) gives us the visibility we need into transport pool saturation (LeasedConcurrency, queued requests, etc.) to size our connection pools properly and troubleshoot storage issues faster in prod.