From f890c5d9e3fe49d9cce171fd275b2428a37f62b0 Mon Sep 17 00:00:00 2001 From: "tim.quinn@oracle.com" Date: Fri, 26 Feb 2021 08:12:51 -0600 Subject: [PATCH] Fix bug where timer's histogram did not use the timer's clock; avoid cases where double NaN can occur and, among other things, spoil JSON formatting as a BigDecimal; add tests Signed-off-by: tim.quinn@oracle.com --- .../ExponentiallyDecayingReservoir.java | 5 +++- .../java/io/helidon/metrics/HelidonTimer.java | 4 +-- .../io/helidon/metrics/WeightedSnapshot.java | 8 ++++-- .../helidon/metrics/HelidonHistogramTest.java | 26 ++++++++++++++++++- .../io/helidon/metrics/HelidonTimerTest.java | 21 ++++++++++++++- 5 files changed, 57 insertions(+), 7 deletions(-) diff --git a/metrics/metrics/src/main/java/io/helidon/metrics/ExponentiallyDecayingReservoir.java b/metrics/metrics/src/main/java/io/helidon/metrics/ExponentiallyDecayingReservoir.java index cd12b304863..e2137fbf842 100644 --- a/metrics/metrics/src/main/java/io/helidon/metrics/ExponentiallyDecayingReservoir.java +++ b/metrics/metrics/src/main/java/io/helidon/metrics/ExponentiallyDecayingReservoir.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2021 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -185,6 +185,9 @@ private void rescale(long now, long next) { final WeightedSnapshot.WeightedSample sample = values.remove(key); final WeightedSnapshot.WeightedSample newSample = new WeightedSnapshot.WeightedSample(sample.getValue(), sample.getWeight() * scalingFactor); + if (Double.compare(newSample.getWeight(), 0) == 0) { + continue; + } values.put(key * scalingFactor, newSample); } } diff --git a/metrics/metrics/src/main/java/io/helidon/metrics/HelidonTimer.java b/metrics/metrics/src/main/java/io/helidon/metrics/HelidonTimer.java index 9afe3f90eb9..eb6f9bc4e05 100644 --- a/metrics/metrics/src/main/java/io/helidon/metrics/HelidonTimer.java +++ b/metrics/metrics/src/main/java/io/helidon/metrics/HelidonTimer.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2021 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -287,7 +287,7 @@ private static class TimerImpl implements Timer { this.histogram = HelidonHistogram.create(repoType, Metadata.builder() .withName(name) .withType(MetricType.HISTOGRAM) - .build()); + .build(), clock); this.clock = clock; } diff --git a/metrics/metrics/src/main/java/io/helidon/metrics/WeightedSnapshot.java b/metrics/metrics/src/main/java/io/helidon/metrics/WeightedSnapshot.java index d25d3c3547d..3433a99d857 100644 --- a/metrics/metrics/src/main/java/io/helidon/metrics/WeightedSnapshot.java +++ b/metrics/metrics/src/main/java/io/helidon/metrics/WeightedSnapshot.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2021 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -65,7 +65,11 @@ class WeightedSnapshot extends Snapshot { for (int i = 0; i < copy.length; i++) { this.values[i] = copy[i].value; - this.normWeights[i] = copy[i].weight / sumWeight; + /* + * A zero denominator could cause the resulting double to be infinity or, if the numerator is also 0, + * NaN. Either causes problems when formatting for JSON output. Just use 0 instead. + */ + this.normWeights[i] = sumWeight != 0 ? copy[i].weight / sumWeight : 0; } for (int i = 1; i < copy.length; i++) { diff --git a/metrics/metrics/src/test/java/io/helidon/metrics/HelidonHistogramTest.java b/metrics/metrics/src/test/java/io/helidon/metrics/HelidonHistogramTest.java index b3e5517335a..503cdcef0fb 100644 --- a/metrics/metrics/src/test/java/io/helidon/metrics/HelidonHistogramTest.java +++ b/metrics/metrics/src/test/java/io/helidon/metrics/HelidonHistogramTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2020 Oracle and/or its affiliates. + * Copyright (c) 2018, 2021 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -32,6 +32,7 @@ import javax.json.JsonObject; import javax.json.JsonObjectBuilder; +import org.eclipse.microprofile.metrics.Histogram; import org.eclipse.microprofile.metrics.Metadata; import org.eclipse.microprofile.metrics.MetricID; import org.eclipse.microprofile.metrics.MetricType; @@ -43,6 +44,8 @@ import static io.helidon.metrics.HelidonMetricsMatcher.withinTolerance; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.fail; @@ -227,6 +230,27 @@ void testStatisticalValues() { testSnapshot(10, "delegating longs", delegatingHistoLong.getSnapshot(), 506.3, 294.389); } + @Test + void testNaNAvoidance() { + Metadata metadata = Metadata.builder() + .withName("long_idle_test") + .withDisplayName("theDisplayName") + .withDescription("Simulates a long idle period") + .withType(MetricType.HISTOGRAM) + .withUnit(MetricUnits.SECONDS) + .build(); + TestClock testClock = TestClock.create(); + Histogram idleHistogram = HelidonHistogram.create("application", metadata, testClock); + + idleHistogram.update(100); + + for (int i = 1; i < 48; i++) { + testClock.add(1, TimeUnit.HOURS); + assertThat("Idle histogram failed after simulating " + i + " hours", idleHistogram.getSnapshot().getMean(), + not(equalTo(Double.NaN))); + } + } + private void testSnapshot(int factor, String description, Snapshot snapshot, double mean, double stddev) { assertAll("Testing statistical values for " + description, () -> assertThat("median", snapshot.getMedian(), is(withinTolerance(factor * 48))), diff --git a/metrics/metrics/src/test/java/io/helidon/metrics/HelidonTimerTest.java b/metrics/metrics/src/test/java/io/helidon/metrics/HelidonTimerTest.java index 646a86e7f8d..4691073f44a 100644 --- a/metrics/metrics/src/test/java/io/helidon/metrics/HelidonTimerTest.java +++ b/metrics/metrics/src/test/java/io/helidon/metrics/HelidonTimerTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2020 Oracle and/or its affiliates. + * Copyright (c) 2018, 2021 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -221,4 +221,23 @@ void testPrometheus() { + "/index.html\n" + "application_response_time_seconds_count 200")); } + + @Test + void testNaNAvoidance() { + TestClock testClock = TestClock.create(); + HelidonTimer helidonTimer = HelidonTimer.create("application", meta, testClock); + MetricID metricID = new MetricID("idleTimer"); + + JsonObjectBuilder builder = MetricImpl.JSON.createObjectBuilder(); + helidonTimer.update(1L, TimeUnit.MILLISECONDS); + + for (int i = 1; i < 48; i++) { + testClock.add(1L, TimeUnit.HOURS); + try { + helidonTimer.jsonData(builder, metricID); + } catch (Throwable t) { + fail("Failed after simulating " + i + " hours"); + } + } + } }