From 5b77464959b3aa7166c30f601b1eead476cc8750 Mon Sep 17 00:00:00 2001 From: David Byron Date: Tue, 2 Nov 2021 11:02:41 -0700 Subject: [PATCH] only log connection pool gauges when there's a handler context value --- spectator-ext-aws/README.md | 4 +++- .../aws/SpectatorRequestMetricCollector.java | 10 ++++++++-- .../aws/SpectatorRequestMetricCollectorTest.java | 15 ++++++++++++--- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/spectator-ext-aws/README.md b/spectator-ext-aws/README.md index a593e6d62..3985da61c 100644 --- a/spectator-ext-aws/README.md +++ b/spectator-ext-aws/README.md @@ -68,7 +68,9 @@ aws.request.RequestSigningTime | Field.RequestSigningTime, aws.request.responseProcessingTime | Field.ResponseProcessingTime, aws.request.retryPauseTime | Field.RetryPauseTime -The following request metrics are captured as gauges: +The following request metrics are captured as gauges, only when the handler +context has a value for the relevant handler context key (see +[below](#tag-via-handler-context)). metric name | SDK metric -----------------------------------------|----------- diff --git a/spectator-ext-aws/src/main/java/com/netflix/spectator/aws/SpectatorRequestMetricCollector.java b/spectator-ext-aws/src/main/java/com/netflix/spectator/aws/SpectatorRequestMetricCollector.java index b498f66e1..cbd070b1b 100644 --- a/spectator-ext-aws/src/main/java/com/netflix/spectator/aws/SpectatorRequestMetricCollector.java +++ b/spectator-ext-aws/src/main/java/com/netflix/spectator/aws/SpectatorRequestMetricCollector.java @@ -174,9 +174,15 @@ public void collectMetrics(Request request, Response response) { .record(t.getEndTimeNano() - t.getStartTimeNano(), TimeUnit.NANOSECONDS)); } - for (Field gauge : GAUGES) { - Optional.ofNullable(timing.getCounter(gauge.name())) + // Only include gauge metrics if there is a value in the context handler + // key. Without that, it's likely that guage metrics from multiple + // connection pools don't make sense. This assumes that all gauge metrics + // are about connection pools. + if (request.getHandlerContext(handlerContextKey) != null) { + for (Field gauge : GAUGES) { + Optional.ofNullable(timing.getCounter(gauge.name())) .ifPresent(v -> registry.gauge(metricId(gauge, allTags)).set(v.doubleValue())); + } } notEmpty(metrics.getProperty(Field.ThrottleException)).ifPresent(throttleExceptions -> { diff --git a/spectator-ext-aws/src/test/java/com/netflix/spectator/aws/SpectatorRequestMetricCollectorTest.java b/spectator-ext-aws/src/test/java/com/netflix/spectator/aws/SpectatorRequestMetricCollectorTest.java index a767acd9c..dbba4205c 100644 --- a/spectator-ext-aws/src/test/java/com/netflix/spectator/aws/SpectatorRequestMetricCollectorTest.java +++ b/spectator-ext-aws/src/test/java/com/netflix/spectator/aws/SpectatorRequestMetricCollectorTest.java @@ -144,7 +144,10 @@ public void testMetricCollection() { List allMetrics = new ArrayList<>(); registry.iterator().forEachRemaining(allMetrics::add); - assertEquals(3, allMetrics.size()); + // We didn't provide a handler context value, so don't expect any gauges. + // That leaves 2 metrics. + assertEquals(2, allMetrics.size()); + Optional expectedTimer = registry.timers().findFirst(); assertTrue(expectedTimer.isPresent()); Timer timer = expectedTimer.get(); @@ -155,9 +158,9 @@ public void testMetricCollection() { assertTrue(expectedCounter.isPresent()); assertEquals(12345L, expectedCounter.get().count()); + // Again, don't expect any gauges. Optional expectedGauge = registry.gauges().findFirst(); - assertTrue(expectedGauge.isPresent()); - assertEquals(-5678d, expectedGauge.get().value()); + assertFalse(expectedGauge.isPresent()); } @Test @@ -203,5 +206,11 @@ public void testDefaultHandlerContextKey() { String handlerContextValue = "some-value"; execRequest("http://monitoring", 503, DEFAULT_HANDLER_CONTEXT_KEY, handlerContextValue); assertEquals(set(handlerContextValue), valueSet(DEFAULT_HANDLER_CONTEXT_KEY.getName())); + + // With a value in the handler context key, make sure there is a gauge + // metric. + Optional expectedGauge = registry.gauges().findFirst(); + assertTrue(expectedGauge.isPresent()); + assertEquals(-5678d, expectedGauge.get().value()); } }