From 48e8d1c12f780c5e815b12abf9e08d36e6f5d261 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Thu, 26 Sep 2024 11:38:42 -0400 Subject: [PATCH] Add comments and constants to clarify the structure of metric collections --- .../MicrometerAwsSdkMetricPublisher.java | 49 +++++++++++++++++-- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MicrometerAwsSdkMetricPublisher.java b/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MicrometerAwsSdkMetricPublisher.java index 54c411533..99341c8f7 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MicrometerAwsSdkMetricPublisher.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/metrics/MicrometerAwsSdkMetricPublisher.java @@ -34,6 +34,41 @@ public class MicrometerAwsSdkMetricPublisher implements MetricPublisher, Managed private final String awsClientName; private final AtomicInteger mostRecentMaxConcurrency; + // Note that these metric collection type names don't seem to appear anywhere in the AWS SDK documentation. The docs + // also hint at, but don't explicitly call out, the structure of a `MetricCollection` passed to the `#publish(...)` + // method. Empirically, for each AWS SDK operation, the SDK will call `#publish(...)` with a collection structured as + // follows: + // + // MetricCollection { name = "ApiCall" } + // +-- MetricRecord { name = "ApiCallDuration" } + // +-- MetricRecord { name = "ApiCallSuccessful" } + // +-- ... + // +-- MetricCollection { name = "ApiCallAttempt" } + // +-- MetricRecord { name = "AwsExtendedRequestId" } + // +-- MetricRecord { name = "AwsRequestId" } + // +-- ... + // +-- MetricCollection {name = "HttpClient" } + // +-- MetricRecord { name = "AvailableConcurrency" } + // +-- MetricRecord { name = "ConcurrencyAcquireDuration" } + // +-- ... + // +-- MetricCollection { name = "ApiCallAttempt" } + // +-- MetricRecord { name = "AwsExtendedRequestId" } + // +-- MetricRecord { name = "AwsRequestId" } + // +-- ... + // +-- MetricCollection {name = "HttpClient" } + // +-- MetricRecord { name = "AvailableConcurrency" } + // +-- MetricRecord { name = "ConcurrencyAcquireDuration" } + // +-- ... + // +-- ... + // + // Every `MetricCollection` passed to `#publish(...)` should have a name of "ApiCall," and the "ApiCall" collection + // will have a fixed collection of named metrics (which ARE documented!). The "ApiCall" collection should have one or + // more "ApiCallAttempt" `MetricCollection` as children, and each "ApiCallAttempt" collection should have exactly one + // "HttpClient" `MetricCollection` as a child. + private static final String METRIC_COLLECTION_TYPE_API_CALL = "ApiCall"; + private static final String METRIC_COLLECTION_TYPE_API_CALL_ATTEMPT = "ApiCallAttempt"; + private static final String METRIC_COLLECTION_TYPE_HTTP_METRICS = "HttpClient"; + private static final String API_CALL_COUNTER_NAME = MetricsUtil.name(MicrometerAwsSdkMetricPublisher.class, "apiCall"); @@ -74,7 +109,7 @@ public class MicrometerAwsSdkMetricPublisher implements MetricPublisher, Managed @Override public void publish(final MetricCollection metricCollection) { - if ("ApiCall".equals(metricCollection.name())) { + if (METRIC_COLLECTION_TYPE_API_CALL.equals(metricCollection.name())) { try { recordMetricsExecutorService.submit(() -> recordApiCallMetrics(metricCollection)); } catch (final RejectedExecutionException ignored) { @@ -84,7 +119,7 @@ public class MicrometerAwsSdkMetricPublisher implements MetricPublisher, Managed } private void recordApiCallMetrics(final MetricCollection apiCallMetricCollection) { - if (!apiCallMetricCollection.name().equals("ApiCall")) { + if (!apiCallMetricCollection.name().equals(METRIC_COLLECTION_TYPE_API_CALL)) { throw new IllegalArgumentException("Unexpected API call metric collection name: " + apiCallMetricCollection.name()); } @@ -96,6 +131,8 @@ public class MicrometerAwsSdkMetricPublisher implements MetricPublisher, Managed final Optional maybeOperationName = Optional.ofNullable(metricsByName.get("OperationName")) .map(metricRecord -> (String) metricRecord.value()); + // Both the service ID and operation name SHOULD always be present, but since the metric collection is unstructured, + // we don't have any compile-time guarantees and so check that they're both present as a pedantic safety check. if (maybeAwsServiceId.isPresent() && maybeOperationName.isPresent()) { final String awsServiceId = maybeAwsServiceId.get(); final String operationName = maybeOperationName.get(); @@ -122,18 +159,20 @@ public class MicrometerAwsSdkMetricPublisher implements MetricPublisher, Managed .register(Metrics.globalRegistry) .record(retryCount); - apiCallMetricCollection.childrenWithName("ApiCallAttempt") + apiCallMetricCollection.childrenWithName(METRIC_COLLECTION_TYPE_API_CALL_ATTEMPT) .forEach(callAttemptMetricCollection -> recordAttemptMetrics(callAttemptMetricCollection, tags)); } } private void recordAttemptMetrics(final MetricCollection apiCallAttemptMetricCollection, final Tags callTags) { - if (!apiCallAttemptMetricCollection.name().equals("ApiCallAttempt")) { + if (!apiCallAttemptMetricCollection.name().equals(METRIC_COLLECTION_TYPE_API_CALL_ATTEMPT)) { throw new IllegalArgumentException("Unexpected API call attempt metric collection name: " + apiCallAttemptMetricCollection.name()); } - apiCallAttemptMetricCollection.childrenWithName("HttpClient").findFirst().ifPresent(httpMetricCollection -> { + // A "call attempt" metric collection should always have exactly one HTTP metrics child collection, but we have no + // compiler-level guarantees and so do a pedantic check here just to be safe. + apiCallAttemptMetricCollection.childrenWithName(METRIC_COLLECTION_TYPE_HTTP_METRICS).findFirst().ifPresent(httpMetricCollection -> { final Map> callAttemptMetricsByName = toMetricMap(apiCallAttemptMetricCollection); final Map> httpMetricsByName = toMetricMap(httpMetricCollection);