Skip to content

Commit

Permalink
Prevent NaN values when calculating mean (#1230)
Browse files Browse the repository at this point in the history
* Remove samples with 0 weight from values to prevent NaN values when calculating mean

* Remove unnecessary throws.

* add whitespace after for
  • Loading branch information
philmtd authored and arteam committed Dec 19, 2017
1 parent 68087f9 commit 9b19e31
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.codahale.metrics;

import com.codahale.metrics.Timer.Context;
import org.junit.Test;

import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -269,26 +286,26 @@ 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
for (int i = 0; i < 120*valuesRatePerMinute; i++) {
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();
Expand All @@ -300,7 +317,7 @@ public void quantiliesShouldBeBasedOnWeights() {
}

clock.addSeconds(120);

for (int i = 0; i < 10; i++) {
reservoir.update(9999);
}
Expand All @@ -316,7 +333,7 @@ public void quantiliesShouldBeBasedOnWeights() {
assertThat(reservoir.getSnapshot().get75thPercentile())
.isEqualTo(9999);
}

private static void assertAllValuesBetween(ExponentiallyDecayingReservoir reservoir,
double min,
double max) {
Expand Down

0 comments on commit 9b19e31

Please sign in to comment.