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
This commit is contained in:
David Pursehouse 2018-06-07 11:11:20 +09:00
parent 6020261a2c
commit 7b518731ad
3 changed files with 26 additions and 13 deletions

View File

@ -153,4 +153,14 @@ public abstract class MetricMaker {
} }
public abstract RegistrationHandle newTrigger(Set<CallbackMetric<?>> metrics, Runnable trigger); 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;
}
} }

View File

@ -344,17 +344,18 @@ public class DropWizardMetricMaker extends MetricMaker {
METRIC_NAME_PATTERN.pattern()); METRIC_NAME_PATTERN.pattern());
} }
public static String sanitizeMetricName(String input) { @Override
if (METRIC_NAME_PATTERN.matcher(input).matches()) { public String sanitizeMetricName(String name) {
return input; if (METRIC_NAME_PATTERN.matcher(name).matches()) {
return name;
} }
String first = input.substring(0, 1).replaceFirst("[^\\w-]", "_"); String first = name.substring(0, 1).replaceFirst("[^\\w-]", "_");
if (input.length() == 1) { if (name.length() == 1) {
return first; return first;
} }
String result = first + input.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_"); String result = first + name.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_");
if (result.endsWith("/")) { if (result.endsWith("/")) {
result = result.substring(0, result.length() - 1); result = result.substring(0, result.length() - 1);

View File

@ -15,28 +15,30 @@
package com.google.gerrit.metrics.dropwizard; package com.google.gerrit.metrics.dropwizard;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName;
import org.junit.Test; import org.junit.Test;
public class DropWizardMetricMakerTest { public class DropWizardMetricMakerTest {
DropWizardMetricMaker metrics =
new DropWizardMetricMaker(null /* MetricRegistry unused in tests */);
@Test @Test
public void shouldSanitizeUnwantedChars() throws Exception { 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"); .isEqualTo("very_confusing_long_metric_net/name_1");
assertThat(sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric"); assertThat(metrics.sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric");
} }
@Test @Test
public void shouldReduceConsecutiveSlashesToOne() throws Exception { public void shouldReduceConsecutiveSlashesToOne() throws Exception {
assertThat(sanitizeMetricName("/metric//submetric1///submetric2/submetric3")) assertThat(metrics.sanitizeMetricName("/metric//submetric1///submetric2/submetric3"))
.isEqualTo("_metric/submetric1/submetric2/submetric3"); .isEqualTo("_metric/submetric1/submetric2/submetric3");
} }
@Test @Test
public void shouldNotFinishWithSlash() throws Exception { public void shouldNotFinishWithSlash() throws Exception {
assertThat(sanitizeMetricName("metric/")).isEqualTo("metric"); assertThat(metrics.sanitizeMetricName("metric/")).isEqualTo("metric");
assertThat(sanitizeMetricName("metric//")).isEqualTo("metric"); assertThat(metrics.sanitizeMetricName("metric//")).isEqualTo("metric");
assertThat(sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric"); assertThat(metrics.sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric");
} }
} }