Don't initialize metrics at startup
Start populating metrics only when collected. Avoid defaulting
them all to 0 at startup.
Bug: Issue 16236
Change-Id: I76712db1819c4b8a2fbc15f4b660c357ad42c7dc
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 b3a0fc8..7f36f27 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
@@ -86,14 +86,41 @@
public void setMetrics(Map<GitRepoMetric, Long> newMetrics, String projectName) {
newMetrics.forEach(
(repoMetric, value) -> {
+ String metricsName = getMetricName(repoMetric.getName(), projectName);
logger.atFine().log(
String.format(
"Collected %s for project %s: %d", repoMetric.getName(), projectName, value));
- metrics.put(getMetricName(repoMetric.getName(), projectName), value);
+ metrics.put(metricsName, value);
+
+ if (!metricExists(metricsName)) {
+ createNewCallbackMetric(repoMetric, projectName);
+ }
});
collectedAt.put(projectName, clock.millis());
}
+ private boolean metricExists(String metricName) {
+ return metricRegistry
+ .getMetrics()
+ .containsKey(String.format("%s/%s/%s", "plugins", "git-repo-metrics", metricName));
+ }
+
+ private void createNewCallbackMetric(GitRepoMetric metric, String projectName) {
+ String metricName = getMetricName(metric.getName(), projectName);
+ Supplier<Long> supplier =
+ new Supplier<Long>() {
+ public Long get() {
+ return getMetrics().getOrDefault(metricName, 0L);
+ }
+ };
+
+ metricMaker.newCallbackMetric(
+ metricName,
+ Long.class,
+ new Description(metric.getDescription()).setRate().setUnit(metric.getUnit()),
+ supplier);
+ }
+
public List<GitRepoMetric> getMetricsNames() {
return metricsNames;
}
@@ -107,51 +134,10 @@
return collectedAt;
}
- public void initCache() {
- metricsNames.forEach(
- gitRepoMetric -> {
- projects.forEach(
- projectName -> {
- String name =
- GitRepoMetricsCache.getMetricName(gitRepoMetric.getName(), projectName);
- Supplier<Long> supplier =
- new Supplier<Long>() {
- public Long get() {
- // TODO Blaah! Initializing all the values to zero!? Would be better
- // registering
- // dynamically the metrics
- // TODO add grace period!!
- return getMetrics().getOrDefault(name, 0L);
- }
- };
-
- if (!metricRegistry
- .getMetrics()
- .containsKey(
- GitRepoMetricsCache.getFullyQualifiedMetricName(
- gitRepoMetric.getName(), projectName))) {
- metricMaker.newCallbackMetric(
- name,
- Long.class,
- new Description(gitRepoMetric.getDescription())
- .setRate()
- .setUnit(gitRepoMetric.getUnit()),
- supplier);
- }
- });
- });
- }
-
public static String getMetricName(String metricName, String projectName) {
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/main/java/com/googlesource/gerrit/plugins/gitrepometrics/Module.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/Module.java
index f612533..8428ad8 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/Module.java
@@ -36,7 +36,6 @@
DynamicSet.setOf(binder(), MetricsCollector.class);
DynamicSet.bind(binder(), MetricsCollector.class).to(GitStatsMetricsCollector.class);
DynamicSet.bind(binder(), MetricsCollector.class).to(FSMetricsCollector.class);
- listener().to(PluginStartup.class);
install(new UpdateGitMetricsTaskModule());
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/PluginStartup.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/PluginStartup.java
deleted file mode 100644
index 3d019fe..0000000
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/PluginStartup.java
+++ /dev/null
@@ -1,43 +0,0 @@
-// Copyright (C) 2022 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.google.gerrit.extensions.events.LifecycleListener;
-import com.google.gerrit.metrics.MetricMaker;
-import com.google.inject.Inject;
-
-public class PluginStartup implements LifecycleListener {
- public final MetricMaker metricMaker;
- public final GitRepoMetricsConfig config;
- public final GitRepoMetricsCache gitRepoMetricsCache;
-
- @Inject
- public PluginStartup(
- MetricMaker metricMaker,
- GitRepoMetricsConfig config,
- GitRepoMetricsCache gitRepoMetricsCache) {
- this.metricMaker = metricMaker;
- this.config = config;
- this.gitRepoMetricsCache = gitRepoMetricsCache;
- }
-
- @Override
- public void start() {
- gitRepoMetricsCache.initCache();
- }
-
- @Override
- public void stop() {}
-}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricMaker.java b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricMaker.java
new file mode 100644
index 0000000..6faaf99
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricMaker.java
@@ -0,0 +1,48 @@
+// Copyright (C) 2022 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.Meter;
+import com.codahale.metrics.MetricRegistry;
+import com.google.gerrit.metrics.CallbackMetric0;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.DisabledMetricMaker;
+
+class FakeMetricMaker extends DisabledMetricMaker {
+ Integer callsCounter;
+ private MetricRegistry metricRegistry;
+
+ FakeMetricMaker(MetricRegistry metricRegistry) {
+ callsCounter = 0;
+ this.metricRegistry = metricRegistry;
+ }
+
+ @Override
+ public <V> CallbackMetric0<V> newCallbackMetric(
+ String name, Class<V> valueClass, Description desc) {
+
+ callsCounter += 1;
+ metricRegistry.register(
+ String.format("%s/%s/%s", "plugins", "git-repo-metrics", name), new Meter());
+ return new CallbackMetric0<V>() {
+
+ @Override
+ public void set(V value) {}
+
+ @Override
+ public void remove() {}
+ };
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricsCollector.java b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricsCollector.java
index eee6274..a24707b 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricsCollector.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/FakeMetricsCollector.java
@@ -23,10 +23,9 @@
import org.eclipse.jgit.internal.storage.file.FileRepository;
class FakeMetricsCollector implements MetricsCollector {
- public static final GitRepoMetric fakeMetric1 =
- new GitRepoMetric("fake-metric-1", "Fake metric 1", "Count");
- public static final GitRepoMetric fakeMetric2 =
- new GitRepoMetric("fake-metric-2", "Fake metric 2", "Count");
+ private final String prefix;
+ private final GitRepoMetric fakeMetric1;
+ private final GitRepoMetric fakeMetric2;
@Override
public HashMap<GitRepoMetric, Long> collect(FileRepository repository, String projectName) {
@@ -35,11 +34,21 @@
@Override
public String getMetricsCollectorName() {
- return "fake-metrics-collector";
+ return prefix + "-fake-metrics-collector";
}
@Override
public ImmutableList<GitRepoMetric> availableMetrics() {
return ImmutableList.of(fakeMetric1, fakeMetric2);
}
+
+ protected FakeMetricsCollector(String prefix) {
+ this.prefix = prefix;
+ this.fakeMetric1 = new GitRepoMetric(prefix + "-fake-metric-1", "Fake metric 1", "Count");
+ this.fakeMetric2 = new GitRepoMetric(prefix + "-fake-metric-2", "Fake metric 2", "Count");
+ }
+
+ protected FakeMetricsCollector() {
+ this("defaultPrefix");
+ }
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheIT.java b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheIT.java
index 8882edd..ffa03e5 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheIT.java
@@ -21,12 +21,14 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
+import com.google.gerrit.entities.Project;
import com.google.inject.Inject;
import com.googlesource.gerrit.plugins.gitrepometrics.collectors.FSMetricsCollector;
import com.googlesource.gerrit.plugins.gitrepometrics.collectors.GitStatsMetricsCollector;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;
+import org.junit.Before;
import org.junit.Test;
@TestPlugin(
@@ -37,23 +39,40 @@
@Inject MetricRegistry metricRegistry;
@Inject FSMetricsCollector fsMetricsCollector;
@Inject GitStatsMetricsCollector gitStatsMetricsCollector;
+ GitRepoMetricsCache gitRepoMetricsCache;
+
+ private final Project.NameKey testProject1 = Project.nameKey("testProject1");
+ private final Project.NameKey testProject2 = Project.nameKey("testProject2");
+
+ @Override
+ @Before
+ public void setUpTestPlugin() throws Exception {
+ super.setUpTestPlugin();
+
+ repoManager.createRepository(testProject1);
+ repoManager.createRepository(testProject2);
+ gitRepoMetricsCache = plugin.getSysInjector().getInstance(GitRepoMetricsCache.class);
+ }
@Test
@UseLocalDisk
@GlobalPluginConfig(
pluginName = "git-repo-metrics",
name = "git-repo-metrics.project",
- values = {"test1", "test2"})
+ values = {"testProject1", "testProject2"})
public void shouldRegisterAllMetrics() {
- List<String> availableProjects = Arrays.asList("test1", "test2");
+ List<Project.NameKey> availableProjects = Arrays.asList(testProject1, testProject2);
+ new UpdateGitMetricsTask(gitRepoMetricsCache, repoManager, testProject1.get()).run();
+ new UpdateGitMetricsTask(gitRepoMetricsCache, repoManager, testProject2.get()).run();
+
List<String> repoMetricsCount =
metricRegistry.getMetrics().keySet().stream()
.filter(metricName -> metricName.contains("git-repo-metrics"))
.collect(Collectors.toList());
int expectedMetricsCount =
- gitStatsMetricsCollector.availableMetrics().size()
- + fsMetricsCollector.availableMetrics().size();
+ fsMetricsCollector.availableMetrics().size()
+ + gitStatsMetricsCollector.availableMetrics().size();
assertThat(repoMetricsCount.size()).isEqualTo(availableProjects.size() * expectedMetricsCount);
}
}
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 e7fd9eb..712b744 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
@@ -16,12 +16,8 @@
import static com.google.common.truth.Truth.assertThat;
-import com.codahale.metrics.Metric;
import com.codahale.metrics.MetricRegistry;
import com.google.gerrit.extensions.registration.DynamicSet;
-import com.google.gerrit.metrics.CallbackMetric0;
-import com.google.gerrit.metrics.Description;
-import com.google.gerrit.metrics.DisabledMetricMaker;
import com.googlesource.gerrit.plugins.gitrepometrics.collectors.GitRepoMetric;
import com.googlesource.gerrit.plugins.gitrepometrics.collectors.MetricsCollector;
import java.io.IOException;
@@ -37,6 +33,7 @@
GitRepoMetricsCache gitRepoMetricsCache;
GitRepoMetricsConfig gitRepoMetricsConfig;
FakeMetricMaker fakeMetricMaker;
+ MetricRegistry metricRegistry;
private ConfigSetupUtils configSetupUtils;
private final String enabledRepo = "enabledRepo";
@@ -46,6 +43,8 @@
@Before
public void setupRepo() throws IOException {
configSetupUtils = new ConfigSetupUtils(Collections.singletonList(enabledRepo));
+ metricRegistry = new MetricRegistry();
+ fakeMetricMaker = new FakeMetricMaker(metricRegistry);
fakeStatsCollector = new FakeMetricsCollector();
ds = new DynamicSet<MetricsCollector>();
@@ -55,46 +54,35 @@
@Test
public void shouldRegisterMetrics() {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- fakeMetricMaker = new FakeMetricMaker();
gitRepoMetricsCache =
new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
- gitRepoMetricsCache.initCache();
- assertThat(fakeMetricMaker.callsCounter)
- .isEqualTo(fakeStatsCollector.availableMetrics().size());
+
+ gitRepoMetricsCache.setMetrics(getCollectedMetrics(), "anyRepo");
+
+ assertThat(fakeMetricMaker.callsCounter).isEqualTo(1);
}
@Test
public void shouldRegisterMetricsOnlyOnce() {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- MetricRegistry metricRegistry = new MetricRegistry();
- fakeMetricMaker = new FakeMetricMaker();
gitRepoMetricsCache =
new GitRepoMetricsCache(ds, fakeMetricMaker, metricRegistry, gitRepoMetricsConfig);
- gitRepoMetricsCache.initCache();
- assertThat(fakeMetricMaker.callsCounter)
- .isEqualTo(fakeStatsCollector.availableMetrics().size());
+ gitRepoMetricsCache.setMetrics(getCollectedMetrics(), "anyRepo");
- gitRepoMetricsCache
- .getMetricsNames()
- .forEach(
- gitRepoMetric ->
- metricRegistry.register(
- GitRepoMetricsCache.getFullyQualifiedMetricName(
- gitRepoMetric.getName(), enabledRepo),
- new FakeMetric()));
+ assertThat(fakeMetricMaker.callsCounter).isEqualTo(1);
- gitRepoMetricsCache.initCache();
- assertThat(fakeMetricMaker.callsCounter)
- .isEqualTo(fakeStatsCollector.availableMetrics().size());
+ gitRepoMetricsCache.setMetrics(getCollectedMetrics(), "anyRepo");
+
+ assertThat(fakeMetricMaker.callsCounter).isEqualTo(1);
}
@Test
public void shouldCollectStatsForEnabledRepo() {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
+
gitRepoMetricsCache =
- new GitRepoMetricsCache(
- ds, new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isTrue();
}
@@ -104,8 +92,7 @@
String disabledRepo = "disabledRepo";
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(
- ds, new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
assertThat(gitRepoMetricsCache.shouldCollectStats(disabledRepo)).isFalse();
}
@@ -116,16 +103,9 @@
new ConfigSetupUtils(Collections.singletonList(enabledRepo));
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(
- ds, new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
- gitRepoMetricsCache.setMetrics(
- new HashMap<GitRepoMetric, Long>() {
- {
- put(new GitRepoMetric("anyMetric", "anyMetricDescription", "Count"), 0L);
- }
- },
- enabledRepo);
+ gitRepoMetricsCache.setMetrics(getCollectedMetrics(), enabledRepo);
assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isTrue();
}
@@ -136,16 +116,9 @@
new ConfigSetupUtils(Collections.singletonList(enabledRepo), "5 m");
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(
- ds, new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, new MetricRegistry(), gitRepoMetricsConfig);
- gitRepoMetricsCache.setMetrics(
- new HashMap<GitRepoMetric, Long>() {
- {
- put(new GitRepoMetric("anyMetric", "anyMetricDescription", "Count"), 0L);
- }
- },
- enabledRepo);
+ gitRepoMetricsCache.setMetrics(getCollectedMetrics(), enabledRepo);
assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isFalse();
}
@@ -158,19 +131,13 @@
gitRepoMetricsCache =
new GitRepoMetricsCache(
ds,
- new FakeMetricMaker(),
- new MetricRegistry(),
+ fakeMetricMaker,
+ metricRegistry,
gitRepoMetricsConfig,
Clock.fixed(
Instant.now().minus(2, ChronoUnit.SECONDS), Clock.systemDefaultZone().getZone()));
- gitRepoMetricsCache.setMetrics(
- new HashMap<GitRepoMetric, Long>() {
- {
- put(new GitRepoMetric("anyMetric", "anyMetricDescription", "Count"), 0L);
- }
- },
- enabledRepo);
+ gitRepoMetricsCache.setMetrics(getCollectedMetrics(), enabledRepo);
assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isTrue();
}
@@ -181,46 +148,20 @@
new ConfigSetupUtils(Collections.singletonList(enabledRepo));
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
gitRepoMetricsCache =
- new GitRepoMetricsCache(
- ds, new FakeMetricMaker(), new MetricRegistry(), gitRepoMetricsConfig);
+ new GitRepoMetricsCache(ds, fakeMetricMaker, metricRegistry, gitRepoMetricsConfig);
long currentTimeStamp = System.currentTimeMillis();
- gitRepoMetricsCache.setMetrics(
- new HashMap<GitRepoMetric, Long>() {
- {
- put(new GitRepoMetric("anyMetric", "anyMetricDescription", "Count"), 0L);
- }
- },
- enabledRepo);
+ gitRepoMetricsCache.setMetrics(getCollectedMetrics(), enabledRepo);
assertThat(gitRepoMetricsCache.getCollectedAt().get(enabledRepo)).isAtLeast(currentTimeStamp);
}
- private class FakeMetricMaker extends DisabledMetricMaker {
- Integer callsCounter;
-
- FakeMetricMaker() {
- callsCounter = 0;
- }
-
- @Override
- public <V> CallbackMetric0<V> newCallbackMetric(
- String name, Class<V> valueClass, Description desc) {
-
- callsCounter += 1;
- return new CallbackMetric0<V>() {
-
- @Override
- public void set(V value) {}
-
- @Override
- public void remove() {}
- };
- }
- }
-
- private class FakeMetric implements Metric {
- FakeMetric() {}
+ private HashMap<GitRepoMetric, Long> getCollectedMetrics() {
+ return new HashMap<GitRepoMetric, Long>() {
+ {
+ put(new GitRepoMetric("anyMetrics", "anyMetric description", "Count"), 1L);
+ }
+ };
}
}
