Add validation for metric name convention

Require names created by developers in source code be restricted to a
subset that is not likely to confuse external metric reporting systems
that use / or . to separate metric names.

Change-Id: I5950d7340811923315d0292f6040722beadf2b6f

gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java

@@ -52,6 +52,7 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
+import java.util.regex.Pattern;
 
 /**
  * Connects Gerrit metric package onto DropWizard.
@@ -109,7 +110,7 @@ public class DropWizardMetricMaker extends MetricMaker {
 
   @Override
   public synchronized Counter0 newCounter(String name, Description desc) {
-    checkCounterDescription(desc);
+    checkCounterDescription(name, desc);
     define(name, desc);
     return newCounterImpl(name, desc.isRate());
   }
@@ -118,7 +119,7 @@ public class DropWizardMetricMaker extends MetricMaker {
   public synchronized <F1> Counter1<F1> newCounter(
       String name, Description desc,
       Field<F1> field1) {
-    checkCounterDescription(desc);
+    checkCounterDescription(name, desc);
     CounterImpl1<F1> m = new CounterImpl1<>(this, name, desc, field1);
     define(name, desc);
     bucketed.put(name, m);
@@ -129,7 +130,7 @@ public class DropWizardMetricMaker extends MetricMaker {
   public synchronized <F1, F2> Counter2<F1, F2> newCounter(
       String name, Description desc,
       Field<F1> field1, Field<F2> field2) {
-    checkCounterDescription(desc);
+    checkCounterDescription(name, desc);
     CounterImplN m = new CounterImplN(this, name, desc, field1, field2);
     define(name, desc);
     bucketed.put(name, m);
@@ -140,14 +141,15 @@ public class DropWizardMetricMaker extends MetricMaker {
   public synchronized <F1, F2, F3> Counter3<F1, F2, F3> newCounter(
       String name, Description desc,
       Field<F1> field1, Field<F2> field2, Field<F3> field3) {
-    checkCounterDescription(desc);
+    checkCounterDescription(name, desc);
     CounterImplN m = new CounterImplN(this, name, desc, field1, field2, field3);
     define(name, desc);
     bucketed.put(name, m);
     return m.counter3();
   }
 
-  private static void checkCounterDescription(Description desc) {
+  private static void checkCounterDescription(String name, Description desc) {
+    checkMetricName(name);
     checkArgument(!desc.isConstant(), "counters must not be constant");
     checkArgument(!desc.isGauge(), "counters must not be gauge");
   }
@@ -176,14 +178,14 @@ public class DropWizardMetricMaker extends MetricMaker {
 
   @Override
   public synchronized Timer0 newTimer(String name, Description desc) {
-    checkTimerDescription(desc);
+    checkTimerDescription(name, desc);
     define(name, desc);
     return newTimerImpl(name);
   }
 
   @Override
   public synchronized <F1> Timer1<F1> newTimer(String name, Description desc, Field<F1> field1) {
-    checkTimerDescription(desc);
+    checkTimerDescription(name, desc);
     TimerImpl1<F1> m = new TimerImpl1<>(this, name, desc, field1);
     define(name, desc);
     bucketed.put(name, m);
@@ -193,7 +195,7 @@ public class DropWizardMetricMaker extends MetricMaker {
   @Override
   public synchronized <F1, F2> Timer2<F1, F2> newTimer(String name, Description desc,
       Field<F1> field1, Field<F2> field2) {
-    checkTimerDescription(desc);
+    checkTimerDescription(name, desc);
     TimerImplN m = new TimerImplN(this, name, desc, field1, field2);
     define(name, desc);
     bucketed.put(name, m);
@@ -204,14 +206,15 @@ public class DropWizardMetricMaker extends MetricMaker {
   public synchronized <F1, F2, F3> Timer3<F1, F2, F3> newTimer(
       String name, Description desc,
       Field<F1> field1, Field<F2> field2, Field<F3> field3) {
-    checkTimerDescription(desc);
+    checkTimerDescription(name, desc);
     TimerImplN m = new TimerImplN(this, name, desc, field1, field2, field3);
     define(name, desc);
     bucketed.put(name, m);
     return m.timer3();
   }
 
-  private static void checkTimerDescription(Description desc) {
+  private static void checkTimerDescription(String name, Description desc) {
+    checkMetricName(name);
     checkArgument(!desc.isConstant(), "timer must not be constant");
     checkArgument(!desc.isGauge(), "timer must not be a gauge");
     checkArgument(!desc.isRate(), "timer must not be a rate");
@@ -226,6 +229,7 @@ public class DropWizardMetricMaker extends MetricMaker {
   @Override
   public <V> CallbackMetric0<V> newCallbackMetric(
       String name, Class<V> valueClass, Description desc) {
+    checkMetricName(name);
     define(name, desc);
     return new CallbackMetricImpl0<>(this, registry, name, valueClass);
   }
@@ -233,6 +237,7 @@ public class DropWizardMetricMaker extends MetricMaker {
   @Override
   public <F1, V> CallbackMetric1<F1, V> newCallbackMetric(
       String name, Class<V> valueClass, Description desc, Field<F1> field1) {
+    checkMetricName(name);
     CallbackMetricImpl1<F1, V> m = new CallbackMetricImpl1<>(this, registry,
         name, valueClass, desc, field1);
     define(name, desc);
@@ -274,6 +279,15 @@ public class DropWizardMetricMaker extends MetricMaker {
     descriptions.put(name, desc.getAnnotations());
   }
 
+  private static final Pattern METRIC_NAME_PATTERN = Pattern
+      .compile("[a-zA-Z0-9_-]+(/[a-zA-Z0-9_-]+)*");
+
+  private static void checkMetricName(String name) {
+    checkArgument(
+        METRIC_NAME_PATTERN.matcher(name).matches(),
+        "metric name must match %s", METRIC_NAME_PATTERN.pattern());
+  }
+
   static String name(Description.FieldOrdering ordering,
       String codeName,
       String fieldValues) {

gerrit-server/src/test/java/com/google/gerrit/metrics/proc/ProcMetricModuleTest.java

@@ -22,6 +22,7 @@ import com.google.gerrit.metrics.CallbackMetric0;
 import com.google.gerrit.metrics.Counter0;
 import com.google.gerrit.metrics.Counter1;
 import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Description.FieldOrdering;
 import com.google.gerrit.metrics.Field;
 import com.google.gerrit.metrics.MetricMaker;
 import com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker;
@@ -35,12 +36,17 @@ import com.codahale.metrics.Metric;
 import com.codahale.metrics.MetricRegistry;
 
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 
 public class ProcMetricModuleTest {
+  @Rule
+  public ExpectedException exception = ExpectedException.none();
+
   @Inject
   MetricMaker metrics;
 
@@ -103,6 +109,30 @@ public class ProcMetricModuleTest {
     assertThat(failed.getCount()).isEqualTo(5);
   }
 
+  @Test
+  public void testCounterPrefixFields() {
+    Counter1<String> cntr = metrics.newCounter(
+        "test/count",
+        new Description("simple test")
+          .setCumulative()
+          .setFieldOrdering(FieldOrdering.PREFIX_FIELDS_BASENAME),
+        Field.ofString("action"));
+
+    Counter total = get("test/count_total", Counter.class);
+    assertThat(total.getCount()).isEqualTo(0);
+
+    cntr.increment("passed");
+    Counter passed = get("test/passed/count", Counter.class);
+    assertThat(total.getCount()).isEqualTo(1);
+    assertThat(passed.getCount()).isEqualTo(1);
+
+    cntr.incrementBy("failed", 5);
+    Counter failed = get("test/failed/count", Counter.class);
+    assertThat(total.getCount()).isEqualTo(6);
+    assertThat(passed.getCount()).isEqualTo(1);
+    assertThat(failed.getCount()).isEqualTo(5);
+  }
+
   @Test
   public void testCallbackMetric0() {
     final CallbackMetric0<Long> cntr = metrics.newCallbackMetric(
@@ -130,6 +160,18 @@ public class ProcMetricModuleTest {
     assertThat(invocations.get()).isEqualTo(1);
   }
 
+  @Test
+  public void testInvalidName1() {
+    exception.expect(IllegalArgumentException.class);
+    metrics.newCounter("invalid name", new Description("fail"));
+  }
+
+  @Test
+  public void testInvalidName2() {
+    exception.expect(IllegalArgumentException.class);
+    metrics.newCounter("invalid/ name", new Description("fail"));
+  }
+
   @SuppressWarnings({"unchecked", "cast"})
   private <V> Gauge<V> gauge(String name) {
     return (Gauge<V>) get(name, Gauge.class);