Fix metric registration check for existing metrics
Checking for the presence of existing metrics cannot be done anymore by
full match but should only consider the prefix, excluding the project
name that is now part of the metric field.
Enable the shouldRegisterMetricsOnlyOnce() test again now that the
metrics detection logic has been fixed and it is not possible anymore
to trigger the registration of the same metric twice.
Change-Id: Ia43962025f43c00c9c3acc06649580619199230b
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 5577bc2..07d5333 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
@@ -16,8 +16,6 @@
import static com.google.gerrit.metrics.Field.ofProjectName;
-import com.codahale.metrics.Metric;
-import com.codahale.metrics.MetricRegistry;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.registration.DynamicSet;
@@ -37,9 +35,9 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final ConcurrentHashMap<String, ConcurrentHashMap<String, Long>> metrics;
private final MetricMaker metricMaker;
- private final MetricRegistry metricRegistry;
private final Set<String> projects;
private final boolean collectAllRepositories;
+ private final ProjectlessMetricsTracker metricsTracker;
private final DynamicSet<MetricsCollector> collectors;
private final Set<String> staleStatsProjects;
@@ -47,11 +45,11 @@
GitRepoMetricsCache(
DynamicSet<MetricsCollector> collectors,
MetricMaker metricMaker,
- MetricRegistry metricRegistry,
+ ProjectlessMetricsTracker metricsTracker,
GitRepoMetricsConfig config) {
this.collectors = collectors;
this.metricMaker = metricMaker;
- this.metricRegistry = metricRegistry;
+ this.metricsTracker = metricsTracker;
this.projects = new HashSet<>(config.getRepositoryNames());
this.metrics = new ConcurrentHashMap<>();
this.collectAllRepositories = config.collectAllRepositories();
@@ -70,20 +68,12 @@
metrics
.computeIfAbsent(metricsName, (m) -> new ConcurrentHashMap<>())
.put(projectName.toLowerCase(Locale.ROOT), value);
- if (!metricExists(metricsName)) {
+ if (!metricsTracker.metricExists(metricsName)) {
createNewCallbackMetric(repoMetric);
}
});
}
- private boolean metricExists(String metricName) {
- Map<String, Metric> currMetrics = metricRegistry.getMetrics();
- return currMetrics.containsKey(
- String.format("%s/%s/%s/", "plugins", "git-repo-metrics", metricName))
- || currMetrics.containsKey(
- String.format("%s/%s/%s", "plugins", "git-repo-metrics", metricName));
- }
-
private void createNewCallbackMetric(GitRepoMetric metric) {
String metricName = metric.getName();
CallbackMetric1<String, Long> cb =
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/ProjectlessMetricsTracker.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/ProjectlessMetricsTracker.java
new file mode 100644
index 0000000..2e70fcd
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/ProjectlessMetricsTracker.java
@@ -0,0 +1,116 @@
+// Copyright (C) 2025 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.gitrepometrics;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.MetricRegistryListener;
+import com.codahale.metrics.Timer;
+import com.google.common.base.Splitter;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.inject.Inject;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+class ProjectlessMetricsTracker implements MetricRegistryListener {
+ private final Set<String> projectLessMetricNames = ConcurrentHashMap.newKeySet();
+ private final String metricsPrefix;
+
+ @Inject
+ ProjectlessMetricsTracker(@PluginName String pluginName, MetricRegistry metricRegistry) {
+ this.metricsPrefix = "plugins/" + pluginName.toLowerCase(Locale.ROOT);
+ metricRegistry.addListener(this);
+ }
+
+ @Override
+ public void onGaugeAdded(String name, Gauge<?> gauge) {
+ addProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onGaugeRemoved(String name) {
+ removeProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onCounterAdded(String name, Counter counter) {
+ addProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onCounterRemoved(String name) {
+ removeProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onHistogramAdded(String name, Histogram histogram) {
+ addProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onHistogramRemoved(String name) {
+ removeProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onMeterAdded(String name, Meter meter) {
+ addProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onMeterRemoved(String name) {
+ removeProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onTimerAdded(String name, Timer timer) {
+ addProjectLessMetricName(name);
+ }
+
+ @Override
+ public void onTimerRemoved(String name) {
+ removeProjectLessMetricName(name);
+ }
+
+ boolean metricExists(String metricName) {
+ return projectLessMetricNames.contains(metricName.toLowerCase(Locale.ROOT));
+ }
+
+ private void addProjectLessMetricName(String name) {
+ if (name.toLowerCase(Locale.ROOT).startsWith(metricsPrefix)) {
+ projectLessMetricNames.add(dropProjectName(name));
+ }
+ }
+
+ private void removeProjectLessMetricName(String name) {
+ if (name.toLowerCase(Locale.ROOT).startsWith(metricsPrefix)) {
+ projectLessMetricNames.remove(dropProjectName(name));
+ }
+ }
+
+ private String dropProjectName(String fullMetricName) {
+ List<String> metricParts =
+ Splitter.on('/')
+ .trimResults()
+ .omitEmptyStrings()
+ .splitToList(fullMetricName.toLowerCase(Locale.ROOT));
+ return metricParts.get(metricParts.size() - 2);
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricMaker.java b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricMaker.java
index a370dbe..0134747 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricMaker.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricMaker.java
@@ -24,12 +24,14 @@
import com.google.gerrit.metrics.Field;
class FakeMetricMaker extends DisabledMetricMaker {
+ private final ProjectlessMetricsTracker metricTracker;
+ private final MetricRegistry metricRegistry;
Integer callsCounter;
- private MetricRegistry metricRegistry;
FakeMetricMaker(MetricRegistry metricRegistry) {
callsCounter = 0;
this.metricRegistry = metricRegistry;
+ this.metricTracker = new ProjectlessMetricsTracker("git-repo-metrics", metricRegistry);
}
@Override
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 402e08f..344412e 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
@@ -27,7 +27,6 @@
import java.util.HashMap;
import java.util.List;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Test;
public class GitRepoMetricsCacheTest {
@@ -56,7 +55,7 @@
public void shouldRegisterMetrics() {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, newMetricsTracker(), gitRepoMetricsConfig);
gitRepoMetricsCache.setMetrics(getCollectedMetrics(), "anyRepo");
@@ -64,14 +63,14 @@
}
@Test
- @Ignore /* The fix of FakeMetricMaker makes this test failing, which is correct because the implementation
- of the logic for avoiding the double-registration of metrics is broken and should have already failed before;
- however, it was compensated by the wrong implementation of the FakeMetricMaker which did not create the project
- name in the generated metrics. */
public void shouldRegisterMetricsOnlyOnce() {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(ds, fakeMetricMaker, metricRegistry, gitRepoMetricsConfig);
+ new GitRepoMetricsCache(
+ ds,
+ fakeMetricMaker,
+ new ProjectlessMetricsTracker("git-repo-metrics", metricRegistry),
+ gitRepoMetricsConfig);
gitRepoMetricsCache.setMetrics(getCollectedMetrics(), "anyRepo");
@@ -87,7 +86,7 @@
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, newMetricsTracker(), gitRepoMetricsConfig);
assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isTrue();
}
@@ -97,7 +96,7 @@
gitRepoMetricsConfig = new ConfigSetupUtils(List.of(), "0", true).getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, newMetricsTracker(), gitRepoMetricsConfig);
assertThat(gitRepoMetricsCache.shouldCollectStats("new-repo")).isTrue();
}
@@ -107,7 +106,7 @@
String disabledRepo = "disabledRepo";
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, newMetricsTracker(), gitRepoMetricsConfig);
assertThat(gitRepoMetricsCache.shouldCollectStats(disabledRepo)).isFalse();
}
@@ -118,7 +117,7 @@
new ConfigSetupUtils(Collections.singletonList(enabledRepo));
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, newMetricsTracker(), gitRepoMetricsConfig);
gitRepoMetricsCache.setMetrics(getCollectedMetrics(), enabledRepo);
@@ -129,4 +128,8 @@
return Maps.newHashMap(
ImmutableMap.of(new GitRepoMetric("anyMetrics", "anyMetric description", "Count"), 1L));
}
+
+ private static ProjectlessMetricsTracker newMetricsTracker() {
+ return new ProjectlessMetricsTracker("git-repo-metrics", new MetricRegistry());
+ }
}
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 e8aa086..572810e 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitUpdateListenerTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitUpdateListenerTest.java
@@ -74,7 +74,7 @@
new GitRepoMetricsCache(
new DynamicSet<>(),
new DisabledMetricMaker(),
- new MetricRegistry(),
+ new ProjectlessMetricsTracker("git-repo-metrics", new MetricRegistry()),
configSetupUtils.getGitRepoMetricsConfig());
AbstractModule m =
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 09b4461..39f7522 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTaskTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTaskTest.java
@@ -61,7 +61,7 @@
new GitRepoMetricsCache(
ds,
new DisabledMetricMaker(),
- new MetricRegistry(),
+ new ProjectlessMetricsTracker("git-repo-metrics", new MetricRegistry()),
configSetupUtils.getGitRepoMetricsConfig());
AbstractModule m =
