Allow plugin to be reloaded
The plugin tries to register metrics every time is reloaded.
This raises an exception since metrics might already be registered in
the MetricRegistry.
Avoid re-registering a metric if already present.
Bug: Issue 16082
Change-Id: I616ec0f01cdf87efef15927401944e3473d21620
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
index f1d46b2..ced3ab2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
@@ -14,6 +14,7 @@
package com.googlesource.gerrit.plugins.gitrepometrics;
+import com.codahale.metrics.MetricRegistry;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Supplier;
import com.google.common.collect.Maps;
@@ -33,6 +34,7 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private Map<String, Long> metrics;
private final MetricMaker metricMaker;
+ private final MetricRegistry metricRegistry;
private final List<String> projects;
private Map<String, Long> collectedAt;
private final long gracePeriodMs;
@@ -42,8 +44,13 @@
public static List<GitRepoMetric> metricsNames = new ArrayList<>(GitStats.availableMetrics());
@VisibleForTesting
- GitRepoMetricsCache(MetricMaker metricMaker, GitRepoMetricsConfig config, Clock clock) {
+ GitRepoMetricsCache(
+ MetricMaker metricMaker,
+ MetricRegistry metricRegistry,
+ GitRepoMetricsConfig config,
+ Clock clock) {
this.metricMaker = metricMaker;
+ this.metricRegistry = metricRegistry;
this.projects = config.getRepositoryNames();
this.metrics = Maps.newHashMap();
this.collectedAt = Maps.newHashMap();
@@ -52,8 +59,9 @@
}
@Inject
- GitRepoMetricsCache(MetricMaker metricMaker, GitRepoMetricsConfig config) {
- this(metricMaker, config, Clock.systemDefaultZone());
+ GitRepoMetricsCache(
+ MetricMaker metricMaker, MetricRegistry metricRegistry, GitRepoMetricsConfig config) {
+ this(metricMaker, metricRegistry, config, Clock.systemDefaultZone());
}
public Map<String, Long> getMetrics() {
@@ -88,13 +96,19 @@
}
};
- metricMaker.newCallbackMetric(
- name,
- Long.class,
- new Description(gitRepoMetric.getDescription())
- .setRate()
- .setUnit(gitRepoMetric.getUnit()),
- supplier);
+ if (!metricRegistry
+ .getMetrics()
+ .containsKey(
+ GitRepoMetricsCache.getFullyQualifiedMetricName(
+ gitRepoMetric.getName(), projectName))) {
+ metricMaker.newCallbackMetric(
+ name,
+ Long.class,
+ new Description(gitRepoMetric.getDescription())
+ .setRate()
+ .setUnit(gitRepoMetric.getUnit()),
+ supplier);
+ }
});
});
}
@@ -103,6 +117,12 @@
return String.format("%s_%s", metricName, projectName).toLowerCase(Locale.ROOT);
}
+ @VisibleForTesting
+ static String getFullyQualifiedMetricName(String metricName, String projectName) {
+ return String.format(
+ "%s/%s/%s", "plugins", "git-repo-metrics", getMetricName(metricName, projectName));
+ }
+
public boolean shouldCollectStats(String projectName) {
long lastCollectionTime = collectedAt.getOrDefault(projectName, 0L);
long currentTimeMs = System.currentTimeMillis();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
index 70c2dd9..dcd9a88 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
@@ -16,6 +16,8 @@
import static com.google.common.truth.Truth.assertThat;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricRegistry;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.metrics.CallbackMetric0;
import com.google.gerrit.metrics.Description;
@@ -45,7 +47,30 @@
public void shouldRegisterMetrics() throws IOException {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
fakeMetricMaker = new FakeMetricMaker();
- gitRepoMetricsCache = new GitRepoMetricsCache(fakeMetricMaker, gitRepoMetricsConfig);
+ gitRepoMetricsCache =
+ new GitRepoMetricsCache(fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
+ gitRepoMetricsCache.initCache();
+ assertThat(fakeMetricMaker.callsCounter).isEqualTo(GitStats.availableMetrics().size());
+ }
+
+ @Test
+ public void shouldRegisterMetricsOnlyOnce() throws IOException {
+ gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
+ MetricRegistry metricRegistry = new MetricRegistry();
+ fakeMetricMaker = new FakeMetricMaker();
+ gitRepoMetricsCache =
+ new GitRepoMetricsCache(fakeMetricMaker, metricRegistry, gitRepoMetricsConfig);
+
+ gitRepoMetricsCache.initCache();
+ assertThat(fakeMetricMaker.callsCounter).isEqualTo(GitStats.availableMetrics().size());
+
+ GitRepoMetricsCache.metricsNames.forEach(
+ gitRepoMetric ->
+ metricRegistry.register(
+ GitRepoMetricsCache.getFullyQualifiedMetricName(
+ gitRepoMetric.getName(), enabledRepo),
+ new FakeMetric()));
+
gitRepoMetricsCache.initCache();
assertThat(fakeMetricMaker.callsCounter).isEqualTo(GitStats.availableMetrics().size());
}
@@ -53,7 +78,8 @@
@Test
public void shouldCollectStatsForEnabledRepo() throws IOException {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+ gitRepoMetricsCache =
+ new GitRepoMetricsCache(new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isTrue();
}
@@ -62,7 +88,8 @@
public void shouldNotCollectStatsForDisabledRepo() throws IOException {
String disabledRepo = "disabledRepo";
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+ gitRepoMetricsCache =
+ new GitRepoMetricsCache(new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
assertThat(gitRepoMetricsCache.shouldCollectStats(disabledRepo)).isFalse();
}
@@ -72,7 +99,8 @@
ConfigSetupUtils configSetupUtils =
new ConfigSetupUtils(Collections.singletonList(enabledRepo));
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+ gitRepoMetricsCache =
+ new GitRepoMetricsCache(new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
gitRepoMetricsCache.setMetrics(ImmutableMap.of("anyMetric", 0L), enabledRepo);
@@ -84,7 +112,8 @@
ConfigSetupUtils configSetupUtils =
new ConfigSetupUtils(Collections.singletonList(enabledRepo), "5 m");
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+ gitRepoMetricsCache =
+ new GitRepoMetricsCache(new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
gitRepoMetricsCache.setMetrics(ImmutableMap.of("anyMetric", 0L), enabledRepo);
@@ -99,6 +128,7 @@
gitRepoMetricsCache =
new GitRepoMetricsCache(
new FakeMetricMaker(),
+ new MetricRegistry(),
gitRepoMetricsConfig,
Clock.fixed(
Instant.now().minus(2, ChronoUnit.SECONDS), Clock.systemDefaultZone().getZone()));
@@ -113,7 +143,8 @@
ConfigSetupUtils configSetupUtils =
new ConfigSetupUtils(Collections.singletonList(enabledRepo));
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+ gitRepoMetricsCache =
+ new GitRepoMetricsCache(new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
long currentTimeStamp = System.currentTimeMillis();
@@ -144,4 +175,8 @@
};
}
}
+
+ private class FakeMetric implements Metric {
+ FakeMetric() {}
+ }
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitUpdateListenerTest.java b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitUpdateListenerTest.java
index 0747b23..f6cdb11 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitUpdateListenerTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitUpdateListenerTest.java
@@ -19,6 +19,7 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.reset;
+import com.codahale.metrics.MetricRegistry;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -64,7 +65,9 @@
new ConfigSetupUtils(Collections.singletonList(testProject));
gitRepoMetricsCache =
new GitRepoMetricsCache(
- new DisabledMetricMaker(), configSetupUtils.getGitRepoMetricsConfig());
+ new DisabledMetricMaker(),
+ new MetricRegistry(),
+ configSetupUtils.getGitRepoMetricsConfig());
AbstractModule m =
new AbstractModule() {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTaskTest.java b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTaskTest.java
index d27c22f..45fb988 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTaskTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTaskTest.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static java.nio.file.Files.delete;
+import com.codahale.metrics.MetricRegistry;
import com.google.gerrit.entities.Project;
import com.google.gerrit.metrics.DisabledMetricMaker;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -49,7 +50,9 @@
ConfigSetupUtils configSetupUtils = new ConfigSetupUtils(Collections.singletonList("repo1"));
gitRepoMetricsCache =
new GitRepoMetricsCache(
- new DisabledMetricMaker(), configSetupUtils.getGitRepoMetricsConfig());
+ new DisabledMetricMaker(),
+ new MetricRegistry(),
+ configSetupUtils.getGitRepoMetricsConfig());
AbstractModule m =
new AbstractModule() {
