DropWizardMetricMaker: Move sanitizeMetricName to MetricMaker base class
The DropWizardMetricMaker#sanitizeMetricName method was added in
change Ic0074b53f and used in change I332cb50b8 to prevent work queue
metrics from being created with names that Dropwizard rejected.
Those changes made the WorkQueue class dependent on the Dropwizard
metrics implementation, which is contrary to the way the metrics system
was designed:
MetricMaker is the metrics interface, and DropWizardMetricMaker is the
concrete implementation of it used in open source Gerrit. The reason
it was done in this way was to allow Google to replace Dropwizard with
their own metrics backend.
Instead of calling the static DropWizardMetricMaker method, classes
should refer to a method on the MetricMaker interface, so that it also
works if the implementation is replaced with something else.
Hoist sanitizeMetricName up to MetricMaker as a public method with a
default implementation that only returns the input, and override it in
the DropWizardMetricMaker.
Change-Id: Iacaed3bbf36f17b87680255de8afb265ab73c06b
diff --git a/gerrit-server/src/main/java/com/google/gerrit/metrics/MetricMaker.java b/gerrit-server/src/main/java/com/google/gerrit/metrics/MetricMaker.java
index 9773869..eee76fd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/metrics/MetricMaker.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/metrics/MetricMaker.java
@@ -167,4 +167,14 @@
}
public abstract RegistrationHandle newTrigger(Set<CallbackMetric<?>> metrics, Runnable trigger);
+
+ /**
+ * Sanitize the given metric name.
+ *
+ * @param name the name to sanitize.
+ * @return sanitized version of the name.
+ */
+ public String sanitizeMetricName(String name) {
+ return name;
+ }
}
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 37efb78..fc53ee7 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
@@ -344,17 +344,18 @@
METRIC_NAME_PATTERN.pattern());
}
- public static String sanitizeMetricName(String input) {
- if (METRIC_NAME_PATTERN.matcher(input).matches()) {
- return input;
+ @Override
+ public String sanitizeMetricName(String name) {
+ if (METRIC_NAME_PATTERN.matcher(name).matches()) {
+ return name;
}
- String first = input.substring(0, 1).replaceFirst("[^\\w-]", "_");
- if (input.length() == 1) {
+ String first = name.substring(0, 1).replaceFirst("[^\\w-]", "_");
+ if (name.length() == 1) {
return first;
}
- String result = first + input.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_");
+ String result = first + name.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_");
if (result.endsWith("/")) {
result = result.substring(0, result.length() - 1);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java
index 4e77874..4e9d937 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.git;
-import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName;
-
import com.google.common.base.CaseFormat;
import com.google.common.base.Supplier;
import com.google.gerrit.extensions.events.LifecycleListener;
@@ -313,7 +311,7 @@
CaseFormat.UPPER_CAMEL.to(
CaseFormat.LOWER_UNDERSCORE,
queueName.replaceFirst("SSH", "Ssh").replaceAll("-", ""));
- return sanitizeMetricName(String.format("queue/%s/%s", name, metricName));
+ return metrics.sanitizeMetricName(String.format("queue/%s/%s", name, metricName));
}
public void unregisterWorkQueue() {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java b/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java
index 26b98c6..9b21bf6 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java
@@ -15,28 +15,30 @@
package com.google.gerrit.metrics.dropwizard;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName;
import org.junit.Test;
public class DropWizardMetricMakerTest {
+ DropWizardMetricMaker metrics =
+ new DropWizardMetricMaker(null /* MetricRegistry unused in tests */);
+
@Test
public void shouldSanitizeUnwantedChars() throws Exception {
- assertThat(sanitizeMetricName("very+confusing$long#metric@net/name^1"))
+ assertThat(metrics.sanitizeMetricName("very+confusing$long#metric@net/name^1"))
.isEqualTo("very_confusing_long_metric_net/name_1");
- assertThat(sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric");
+ assertThat(metrics.sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric");
}
@Test
public void shouldReduceConsecutiveSlashesToOne() throws Exception {
- assertThat(sanitizeMetricName("/metric//submetric1///submetric2/submetric3"))
+ assertThat(metrics.sanitizeMetricName("/metric//submetric1///submetric2/submetric3"))
.isEqualTo("_metric/submetric1/submetric2/submetric3");
}
@Test
public void shouldNotFinishWithSlash() throws Exception {
- assertThat(sanitizeMetricName("metric/")).isEqualTo("metric");
- assertThat(sanitizeMetricName("metric//")).isEqualTo("metric");
- assertThat(sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric");
+ assertThat(metrics.sanitizeMetricName("metric/")).isEqualTo("metric");
+ assertThat(metrics.sanitizeMetricName("metric//")).isEqualTo("metric");
+ assertThat(metrics.sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric");
}
}
