diff --git a/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java b/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java index 0a3bc5eb5d..ca0d50a3c1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java +++ b/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 Counter1 newCounter( String name, Description desc, Field field1) { - checkCounterDescription(desc); + checkCounterDescription(name, desc); CounterImpl1 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 Counter2 newCounter( String name, Description desc, Field field1, Field 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 Counter3 newCounter( String name, Description desc, Field field1, Field field2, Field 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 Timer1 newTimer(String name, Description desc, Field field1) { - checkTimerDescription(desc); + checkTimerDescription(name, desc); TimerImpl1 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 Timer2 newTimer(String name, Description desc, Field field1, Field 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 Timer3 newTimer( String name, Description desc, Field field1, Field field2, Field 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 CallbackMetric0 newCallbackMetric( String name, Class 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 CallbackMetric1 newCallbackMetric( String name, Class valueClass, Description desc, Field field1) { + checkMetricName(name); CallbackMetricImpl1 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) { diff --git a/gerrit-server/src/test/java/com/google/gerrit/metrics/proc/ProcMetricModuleTest.java b/gerrit-server/src/test/java/com/google/gerrit/metrics/proc/ProcMetricModuleTest.java index bd650b9031..aa4c4ebf38 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/metrics/proc/ProcMetricModuleTest.java +++ b/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 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 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 Gauge gauge(String name) { return (Gauge) get(name, Gauge.class);