Add ZooKeeperMetricsImplTest and comments

helios-services/src/main/java/com/spotify/helios/servicescommon/coordination/ReportingZooKeeperClient.java

@@ -35,6 +35,10 @@
 import java.util.Collection;
 import java.util.List;
 
+/**
+ * This class instruments ZooKeeper calls by timing them and reporting exceptions.
+ * Calls are delegated to a {@link ZooKeeperClient}.
+ */
 public class ReportingZooKeeperClient implements ZooKeeperClient {
 
   private final ZooKeeperClient client;

helios-services/src/main/java/com/spotify/helios/servicescommon/coordination/ZooKeeperModelReporter.java

@@ -21,6 +21,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 
+import com.codahale.metrics.Clock;
 import com.spotify.helios.servicescommon.NoOpRiemannClient;
 import com.spotify.helios.servicescommon.RiemannFacade;
 import com.spotify.helios.servicescommon.statistics.NoopZooKeeperMetrics;
@@ -32,6 +33,7 @@
 import org.apache.zookeeper.KeeperException.RuntimeInconsistencyException;
 
 import java.util.List;
+import java.util.concurrent.TimeUnit;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
@@ -43,6 +45,7 @@ public class ZooKeeperModelReporter {
           OperationTimeoutException.class, "timeout",
           ConnectionLossException.class, "connection_loss",
           RuntimeInconsistencyException.class, "inconsistency");
+  private final Clock clock = Clock.defaultClock();
 
   public ZooKeeperModelReporter(final RiemannFacade riemannFacade,
                                 final ZooKeeperMetrics metrics) {
@@ -73,17 +76,25 @@ public void checkException(Exception e, String... tags) {
 
   public <T> T time(final String tag, final String name, ZooKeeperCallable<T> callable)
       throws KeeperException {
+    final long startTime = clock.getTick();
     try {
-      return metrics.timer(name).time(callable::call);
+      return callable.call();
     } catch (KeeperException e) {
       checkException(e, tag, name);
       throw e;
     } catch (Exception e) {
       throw Throwables.propagate(e);
+    } finally {
+      metrics.updateTimer(name, clock.getTick() - startTime, TimeUnit.NANOSECONDS);
     }
   }
 
   public static ZooKeeperModelReporter noop() {
     return new ZooKeeperModelReporter(new NoOpRiemannClient().facade(), new NoopZooKeeperMetrics());
   }
+
+  @FunctionalInterface
+  public interface ZooKeeperCallable<T> {
+    T call() throws KeeperException;
+  }
 }

helios-services/src/main/java/com/spotify/helios/servicescommon/statistics/NoopZooKeeperMetrics.java

@@ -17,14 +17,13 @@
 
 package com.spotify.helios.servicescommon.statistics;
 
-import com.codahale.metrics.Timer;
+import java.util.concurrent.TimeUnit;
 
 public class NoopZooKeeperMetrics implements ZooKeeperMetrics {
   @Override
   public void zookeeperTransientError() {}
 
   @Override
-  public Timer timer(String name) {
-    return new Timer();
+  public void updateTimer(String name, long duration, TimeUnit timeUnit) {
   }
 }

helios-services/src/main/java/com/spotify/helios/servicescommon/statistics/ZooKeeperMetrics.java

@@ -17,11 +17,20 @@
 
 package com.spotify.helios.servicescommon.statistics;
 
-import com.codahale.metrics.Timer;
+import java.util.concurrent.TimeUnit;
 
+/**
+ * This interface lets us report ZooKeeper metrics.
+ */
 public interface ZooKeeperMetrics {
 
+  /**
+   * Call this to report a transient ZooKeeper error.
+   */
   void zookeeperTransientError();
 
-  Timer timer(String name);
+  /**
+   * Call this to update the appropriate {@link Timer}.
+   */
+  void updateTimer(String name, long duration, TimeUnit timeUnit);
 }

helios-services/src/main/java/com/spotify/helios/servicescommon/statistics/ZooKeeperMetricsImpl.java

@@ -20,7 +20,8 @@
 import com.codahale.metrics.Counter;
 import com.codahale.metrics.Meter;
 import com.codahale.metrics.MetricRegistry;
-import com.codahale.metrics.Timer;
+
+import java.util.concurrent.TimeUnit;
 
 public class ZooKeeperMetricsImpl implements ZooKeeperMetrics {
 
@@ -45,7 +46,7 @@ public void zookeeperTransientError() {
   }
 
   @Override
-  public Timer timer(final String name) {
-    return registry.timer(prefix + name);
+  public void updateTimer(final String name, final long duration, final TimeUnit timeUnit) {
+    registry.timer(prefix + name).update(duration, timeUnit);
   }
 }

helios-services/src/test/java/com/spotify/helios/servicescommon/statistics/ZooKeeperMetricsImplTest.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2014 Spotify AB.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.spotify.helios.servicescommon.statistics;
+
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Timer;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import java.util.concurrent.TimeUnit;
+
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class ZooKeeperMetricsImplTest {
+
+  @Mock MetricRegistry registry;
+  @Mock Timer timer;
+
+  @Test
+  public void testTimer() throws Exception {
+    when(registry.timer("group.zookeeper.timer")).thenReturn(timer);
+    final ZooKeeperMetrics zkMetrics = new ZooKeeperMetricsImpl("group", registry);
+    zkMetrics.updateTimer("timer", 100, TimeUnit.NANOSECONDS);
+    verify(registry).timer(eq("group.zookeeper.timer"));
+    verify(timer).update(100, TimeUnit.NANOSECONDS);
+  }
+}