-
Notifications
You must be signed in to change notification settings - Fork 26
blob: opt-in connection-pool saturation metrics for AWS and GCP #561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
iamabhilaksh
wants to merge
7
commits into
salesforce:main
from
iamabhilaksh:blob-connection-pool-metrics
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f9684bd
blob: add opt-in connection-pool saturation metrics for AWS and GCP
iamabhilaksh 8bb064f
Merge branch 'main' into blob-connection-pool-metrics
iamabhilaksh 8f8e11d
blob: align metrics-publisher docs with behavior and cover GCP pool swap
iamabhilaksh 7fb0cf2
Merge branch 'main' into blob-connection-pool-metrics
iamabhilaksh c87e453
Merge branch 'main' into blob-connection-pool-metrics
iamabhilaksh 126e9fc
blob: address PR #561 review — metrics builder delegation, lifecycle,…
iamabhilaksh f05830f
blob: gcp: assert metrics-only client actually installs the pool-metr…
iamabhilaksh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
63 changes: 63 additions & 0 deletions
63
...lob-aws/src/main/java/com/salesforce/multicloudj/blob/aws/AwsMetricsPublisherAdapter.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package com.salesforce.multicloudj.blob.aws; | ||
|
|
||
| import com.salesforce.multicloudj.common.observability.Metric; | ||
| import com.salesforce.multicloudj.common.observability.MetricsPublisher; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import software.amazon.awssdk.metrics.MetricCollection; | ||
| import software.amazon.awssdk.metrics.MetricPublisher; | ||
| import software.amazon.awssdk.metrics.MetricRecord; | ||
|
|
||
| /** | ||
| * Bridges the AWS SDK's {@link software.amazon.awssdk.metrics.MetricPublisher} SPI to the | ||
| * cloud-agnostic {@link MetricsPublisher}. | ||
| * | ||
| * <p>The AWS SDK reports metrics as a tree of {@link MetricCollection}s: a root collection for the | ||
| * API call with nested child collections for lower layers such as the HTTP client (where | ||
| * connection-pool metrics like {@code MaxConcurrency}, {@code LeasedConcurrency}, {@code | ||
| * PendingConcurrencyAcquires}, and {@code ConcurrencyAcquireDuration} live). This adapter flattens | ||
| * the entire tree into neutral {@link Metric} instances, tagging each with the name of the | ||
| * collection that produced it, and forwards them to the configured {@link MetricsPublisher}. | ||
| * | ||
| * <p>Forwarding the whole tree is intentional: the connection-pool counters that every provider | ||
| * emits are a guaranteed subset (found under the {@code HttpClient} collection), and AWS callers | ||
| * additionally receive the SDK's native request- and attempt-level metrics as a provider-specific | ||
| * superset. Consumers that only care about pool saturation can filter by the {@code HttpClient} | ||
| * category; the metric names in that category match the cloud-agnostic {@link | ||
| * com.salesforce.multicloudj.common.observability.ConnectionPoolMetrics} vocabulary. | ||
| */ | ||
| public class AwsMetricsPublisherAdapter implements MetricPublisher { | ||
|
|
||
| private final MetricsPublisher delegate; | ||
|
|
||
| public AwsMetricsPublisherAdapter(MetricsPublisher delegate) { | ||
| this.delegate = delegate; | ||
| } | ||
|
|
||
| @Override | ||
| public void publish(MetricCollection metricCollection) { | ||
| List<Metric> metrics = new ArrayList<>(); | ||
| flatten(metricCollection, metrics); | ||
| delegate.publish(metrics); | ||
| } | ||
|
|
||
| private void flatten(MetricCollection collection, List<Metric> out) { | ||
| String category = collection.name(); | ||
| for (MetricRecord<?> record : collection) { | ||
| out.add( | ||
| Metric.builder() | ||
| .name(record.metric().name()) | ||
| .value(record.value()) | ||
| .category(category) | ||
| .build()); | ||
| } | ||
| for (MetricCollection child : collection.children()) { | ||
| flatten(child, out); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| delegate.close(); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CRT-backed async client doesn't wire the metrics publisher.
This wires the adapter for the standard
S3AsyncClientBuilderpath, but theS3CrtAsyncClientBuilderoverload (applyCommonConfigaround line 757) has no equivalent — sowithParallelDownloadsEnabled(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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with the javadoc route. Added a caveat on
BlobStoreBuilder.withMetricsPublishernoting 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 CRTapplyCommonConfigso 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.