From 9b19e3172bf51800479760b4f09b77780b6e00c2 Mon Sep 17 00:00:00 2001
From: Philip <philmtd@users.noreply.github.com>
Date: Tue, 19 Dec 2017 20:41:52 +0100
Subject: [PATCH] Prevent NaN values when calculating mean (#1230)

* Remove samples with 0 weight from values to prevent NaN values when calculating mean

* Remove unnecessary throws.

* add whitespace after for
---
 .../ExponentiallyDecayingReservoir.java       |  3 ++
 .../ExponentiallyDecayingReservoirTest.java   | 33 ++++++++++++++-----
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/metrics-core/src/main/java/com/codahale/metrics/ExponentiallyDecayingReservoir.java b/metrics-core/src/main/java/com/codahale/metrics/ExponentiallyDecayingReservoir.java
index 4a539f7413..e8ca81e6d6 100644
--- a/metrics-core/src/main/java/com/codahale/metrics/ExponentiallyDecayingReservoir.java
+++ b/metrics-core/src/main/java/com/codahale/metrics/ExponentiallyDecayingReservoir.java
@@ -173,6 +173,9 @@ private void rescale(long now, long next) {
                     for (Double key : keys) {
                         final WeightedSample sample = values.remove(key);
                         final WeightedSample newSample = new WeightedSample(sample.value, sample.weight * scalingFactor);
+                        if (Double.compare(newSample.weight, 0) == 0) {
+                            continue;
+                        }
                         values.put(key * scalingFactor, newSample);
                     }
                 }
diff --git a/metrics-core/src/test/java/com/codahale/metrics/ExponentiallyDecayingReservoirTest.java b/metrics-core/src/test/java/com/codahale/metrics/ExponentiallyDecayingReservoirTest.java
index f6ee7863ee..34b7cbff7c 100644
--- a/metrics-core/src/test/java/com/codahale/metrics/ExponentiallyDecayingReservoirTest.java
+++ b/metrics-core/src/test/java/com/codahale/metrics/ExponentiallyDecayingReservoirTest.java
@@ -1,5 +1,6 @@
 package com.codahale.metrics;
 
+import com.codahale.metrics.Timer.Context;
 import org.junit.Test;
 
 import java.util.concurrent.TimeUnit;
@@ -142,6 +143,22 @@ public void emptyReservoirSnapshot_shouldReturnZeroForAllValues() {
         assertThat(snapshot.size()).isEqualTo(0);
     }
 
+    @Test
+    public void removeZeroWeightsInSamplesToPreventNaNInMeanValues() {
+        final ManualClock clock = new ManualClock();
+        final ExponentiallyDecayingReservoir reservoir = new ExponentiallyDecayingReservoir(1028, 0.015, clock);
+        Timer timer = new Timer(reservoir, clock);
+
+        Context context = timer.time();
+        clock.addMillis(100);
+        context.stop();
+
+        for (int i = 1; i < 48; i++) {
+            clock.addHours(1);
+            assertThat(reservoir.getSnapshot().getMean()).isBetween(0.0, Double.MAX_VALUE);
+        }
+    }
+
     @Test
     public void multipleUpdatesAfterlongPeriodsOfInactivityShouldNotCorruptSamplingState () throws Exception {
         // This test illustrates the potential race condition in rescale that
@@ -251,13 +268,13 @@ public void spotLift() {
             reservoir.update(177);
             clock.addMillis(valuesIntervalMillis);
         }
-        
+
         // switching to mode 2: 10 minutes more with the same rate, but larger value
         for (int i = 0; i < 10*valuesRatePerMinute; i++) {
             reservoir.update(9999);
             clock.addMillis(valuesIntervalMillis);
         }
-        
+
         // expect that quantiles should be more about mode 2 after 10 minutes
         assertThat(reservoir.getSnapshot().getMedian())
                 .isEqualTo(9999);
@@ -269,7 +286,7 @@ public void spotFall() {
         final ExponentiallyDecayingReservoir reservoir = new ExponentiallyDecayingReservoir(1000,
                                                                                             0.015,
                                                                                             clock);
-        
+
         final int valuesRatePerMinute = 10;
         final int valuesIntervalMillis = (int) (TimeUnit.MINUTES.toMillis(1) / valuesRatePerMinute);
         // mode 1: steady regime for 120 minutes
@@ -277,18 +294,18 @@ public void spotFall() {
             reservoir.update(9998);
             clock.addMillis(valuesIntervalMillis);
         }
-        
+
         // switching to mode 2: 10 minutes more with the same rate, but smaller value
         for (int i = 0; i < 10*valuesRatePerMinute; i++) {
             reservoir.update(178);
             clock.addMillis(valuesIntervalMillis);
         }
-        
+
         // expect that quantiles should be more about mode 2 after 10 minutes
         assertThat(reservoir.getSnapshot().get95thPercentile())
                 .isEqualTo(178);
     }
-    
+
     @Test
     public void quantiliesShouldBeBasedOnWeights() {
         final ManualClock clock = new ManualClock();
@@ -300,7 +317,7 @@ public void quantiliesShouldBeBasedOnWeights() {
         }
 
         clock.addSeconds(120);
-        
+
         for (int i = 0; i < 10; i++) {
             reservoir.update(9999);
         }
@@ -316,7 +333,7 @@ public void quantiliesShouldBeBasedOnWeights() {
         assertThat(reservoir.getSnapshot().get75thPercentile())
                 .isEqualTo(9999);
     }
-    
+
     private static void assertAllValuesBetween(ExponentiallyDecayingReservoir reservoir,
                                                double min,
                                                double max) {