add tests for Bucket and Stats timers

Also updates StatsMonitor to have a `computeStats` method to force the update without worrying about timing issues in the tests.
Netflix · May 10, 2018 · 00eea64 · 00eea64
1 parent d255698
commit 00eea64
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 36 deletions.
diff --git a/servo-core/src/main/java/com/netflix/servo/monitor/StatsMonitor.java b/servo-core/src/main/java/com/netflix/servo/monitor/StatsMonitor.java
@@ -368,6 +368,34 @@ public StatsMonitor(final MonitorConfig config,
     }
   }
 
+  void computeStats() {
+    try {
+      if (myFutureRef.get() == null) {
+        return;
+      }
+      final boolean expired = (clock.now() - lastUsed) > EXPIRE_AFTER_MS;
+      if (expired) {
+        final ScheduledFuture<?> future = myFutureRef.getAndSet(null);
+        if (future != null) {
+          LOGGER.debug("Expiring unused StatsMonitor {}", getConfig().getName());
+          future.cancel(true);
+        }
+        return;
+      }
+
+      synchronized (updateLock) {
+        final StatsBuffer tmp = prev;
+        prev = cur;
+        cur = tmp;
+      }
+      prev.computeStats();
+      updateGauges();
+      prev.reset();
+    } catch (Exception e) {
+      handleException(e);
+    }
+  }
+
   /**
    * starts computation.
    * Because of potential race conditions, derived classes may wish
@@ -378,35 +406,8 @@ public void startComputingStats() {
   }
 
   private void startComputingStats(ScheduledExecutorService executor, long frequencyMillis) {
-    Runnable command = () -> {
-      try {
-        if (myFutureRef.get() == null) {
-          return;
-        }
-        final boolean expired = (clock.now() - lastUsed) > EXPIRE_AFTER_MS;
-        if (expired) {
-          final ScheduledFuture<?> future = myFutureRef.getAndSet(null);
-          if (future != null) {
-            LOGGER.debug("Expiring unused StatsMonitor {}", getConfig().getName());
-            future.cancel(true);
-          }
-          return;
-        }
-
-        synchronized (updateLock) {
-          final StatsBuffer tmp = prev;
-          prev = cur;
-          cur = tmp;
-        }
-        prev.computeStats();
-        updateGauges();
-        prev.reset();
-      } catch (Exception e) {
-        handleException(e);
-      }
-    };
-
-    this.myFutureRef.set(executor.scheduleWithFixedDelay(command, frequencyMillis, frequencyMillis,
+    this.myFutureRef.set(executor.scheduleWithFixedDelay(this::computeStats,
+        frequencyMillis, frequencyMillis,
         TimeUnit.MILLISECONDS));
   }
 

diff --git a/servo-core/src/test/java/com/netflix/servo/monitor/SpectatorIntegrationTest.java b/servo-core/src/test/java/com/netflix/servo/monitor/SpectatorIntegrationTest.java
@@ -18,6 +18,7 @@
 import com.netflix.servo.SpectatorContext;
 import com.netflix.servo.annotations.DataSourceType;
 import com.netflix.servo.annotations.Monitor;
+import com.netflix.servo.stats.StatsConfig;
 import com.netflix.servo.util.Clock;
 import com.netflix.spectator.api.DefaultRegistry;
 import com.netflix.spectator.api.Id;
@@ -191,6 +192,41 @@ public void testDynamicTimerRecordSeconds() {
     assertEquals(42, registry.maxGauge(id.withTag(Statistic.max)).value(), 1e-12);
   }
 
+  @Test
+  public void testBucketTimerRecordMillis() {
+    BucketConfig bc = new BucketConfig.Builder()
+        .withBuckets(new long[] {10L, 50L})
+        .withTimeUnit(TimeUnit.MILLISECONDS)
+        .build();
+    BucketTimer d = new BucketTimer(CONFIG, bc);
+    d.record(42, TimeUnit.MILLISECONDS);
+    Id id = ID.withTag("unit", "MILLISECONDS");
+    assertEquals(1, registry.counter(id.withTag(Statistic.count).withTag("servo.bucket", "bucket=50ms")).count());
+    assertEquals(42.0, registry.counter(id.withTag(Statistic.totalTime)).actualCount(), 1e-12);
+    assertEquals(42.0, registry.maxGauge(id.withTag(Statistic.max)).value(), 1e-12);
+  }
+
+  @Test
+  public void testStatsTimerRecordMillis() {
+    StatsConfig sc = new StatsConfig.Builder()
+        .withPercentiles(new double[] {50.0, 95.0})
+        .withPublishCount(true)
+        .withPublishMax(true)
+        .withPublishMean(true)
+        .withSampleSize(10)
+        .build();
+    StatsTimer d = new StatsTimer(CONFIG, sc);
+    d.record(42, TimeUnit.MILLISECONDS);
+    d.computeStats();
+    Id id = ID.withTag("unit", "MILLISECONDS");
+    assertEquals(1, registry.counter(id.withTag(Statistic.count)).count());
+    assertEquals(42.0, registry.counter(id.withTag(Statistic.totalTime)).actualCount(), 1e-12);
+    assertEquals(42.0, registry.maxGauge(id.withTag(Statistic.max)).value(), 1e-12);
+    assertEquals(42.0, registry.gauge(id.withTag("statistic", "percentile_50")).value(), 1e-12);
+    assertEquals(42.0, registry.gauge(id.withTag("statistic", "percentile_95")).value(), 1e-12);
+    assertEquals(42.0, registry.gauge(id.withTag("statistic", "avg")).value(), 1e-12);
+  }
+
   public static class AnnotateExample {
 
     private long count = 0;

diff --git a/servo-core/src/test/java/com/netflix/servo/monitor/StatsMonitorTest.java b/servo-core/src/test/java/com/netflix/servo/monitor/StatsMonitorTest.java
@@ -35,13 +35,11 @@ public void testExpiration() throws Exception {
         Executors.newSingleThreadScheduledExecutor(),
         "total", false, clock);
 
-    monitor.startComputingStats();
-
     clock.set(TimeUnit.MINUTES.toMillis(20));
-    Thread.sleep(20);
+    monitor.computeStats();
     assertTrue(monitor.isExpired());
     monitor.getMonitors();
-    Thread.sleep(20);
+    monitor.computeStats();
     assertFalse(monitor.isExpired());
   }
 }
diff --git a/servo-core/src/test/java/com/netflix/servo/monitor/StatsTimerTest.java b/servo-core/src/test/java/com/netflix/servo/monitor/StatsTimerTest.java
@@ -18,11 +18,13 @@
 import com.netflix.servo.stats.StatsConfig;
 import org.testng.annotations.Test;
 
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
 
 import static org.testng.Assert.assertEquals;
 
@@ -67,7 +69,7 @@ public void testStats() throws Exception {
       timer.record(i);
     }
 
-    Thread.sleep(1000L);
+    timer.computeStats();
     assertStats(timer.getMonitors(), expectedValues);
   }
 
@@ -117,11 +119,16 @@ public void testMultiThreadStats() throws Exception {
     final int poolSize = Runtime.getRuntime().availableProcessors();
     ExecutorService service = Executors.newFixedThreadPool(poolSize);
 
+    List<Future<?>> futures = new ArrayList<>();
     for (int i = 0; i < n; ++i) {
-      service.submit(new TimerTask(timer, i));
+      futures.add(service.submit(new TimerTask(timer, i)));
     }
 
-    Thread.sleep(1000L);
+    // Wait for all submitted tasks to complete
+    for (Future<?> f : futures) {
+      f.get();
+    }
+    timer.computeStats();
     assertStats(timer.getMonitors(), expectedValues);
   }