Add grace period to avoid collecting stats too often
Some stats might be too heavy to compute.
Added a grace period to avoid too frequent collection.
Bug: Issue 15971
Change-Id: I54adfe04e8aec8f7fd1b7d8bf12367422b5e04e1
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 dfabe10..f1d46b2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCache.java
@@ -14,42 +14,62 @@
package com.googlesource.gerrit.plugins.gitrepometrics;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Supplier;
import com.google.common.collect.Maps;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.MetricMaker;
import com.google.inject.Inject;
-import com.google.inject.Singleton;
import com.googlesource.gerrit.plugins.gitrepometrics.collectors.GitRepoMetric;
import com.googlesource.gerrit.plugins.gitrepometrics.collectors.GitStats;
+import java.time.Clock;
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
-@Singleton
public class GitRepoMetricsCache {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private Map<String, Long> metrics;
private final MetricMaker metricMaker;
private final List<String> projects;
+ private Map<String, Long> collectedAt;
+ private final long gracePeriodMs;
+
+ private final Clock clock;
public static List<GitRepoMetric> metricsNames = new ArrayList<>(GitStats.availableMetrics());
- @Inject
- GitRepoMetricsCache(MetricMaker metricMaker, GitRepoMetricsConfig config) {
+ @VisibleForTesting
+ GitRepoMetricsCache(MetricMaker metricMaker, GitRepoMetricsConfig config, Clock clock) {
this.metricMaker = metricMaker;
this.projects = config.getRepositoryNames();
this.metrics = Maps.newHashMap();
+ this.collectedAt = Maps.newHashMap();
+ this.clock = clock;
+ this.gracePeriodMs = config.getGracePeriodMs();
+ }
+
+ @Inject
+ GitRepoMetricsCache(MetricMaker metricMaker, GitRepoMetricsConfig config) {
+ this(metricMaker, config, Clock.systemDefaultZone());
}
public Map<String, Long> getMetrics() {
return metrics;
}
- public void setMetrics(Map<String, Long> metrics) {
+ public void setMetrics(Map<String, Long> metrics, String projectName) {
+ this.collectedAt.put(projectName, clock.millis());
this.metrics = metrics;
}
+ @VisibleForTesting
+ public Map<String, Long> getCollectedAt() {
+ return collectedAt;
+ }
+
public void initCache() {
metricsNames.forEach(
gitRepoMetric -> {
@@ -84,6 +104,16 @@
}
public boolean shouldCollectStats(String projectName) {
+ long lastCollectionTime = collectedAt.getOrDefault(projectName, 0L);
+ long currentTimeMs = System.currentTimeMillis();
+ boolean doCollectStats = lastCollectionTime + gracePeriodMs <= currentTimeMs;
+ if (!doCollectStats) {
+ logger.atFine().log(
+ "Skip stats collection for %s (grace period: %d, last collection time: %d, current time: %d",
+ projectName, gracePeriodMs, lastCollectionTime, currentTimeMs);
+ return false;
+ }
+
return projects.stream().anyMatch(p -> p.equals(projectName));
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsConfig.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsConfig.java
index a9937cf..a588a73 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsConfig.java
@@ -22,6 +22,7 @@
import com.google.inject.Singleton;
import java.util.Arrays;
import java.util.List;
+import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.lib.Config;
@Singleton
@@ -38,4 +39,8 @@
public List<String> getRepositoryNames() {
return Arrays.stream(config.getStringList(pluginName, null, "project")).collect(toList());
}
+
+ public Long getGracePeriodMs() {
+ return config.getTimeUnit("git-repo-metrics", null, "gracePeriod", 0L, TimeUnit.MILLISECONDS);
+ }
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoUpdateListener.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoUpdateListener.java
index 1411c61..e9e4d98 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoUpdateListener.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoUpdateListener.java
@@ -23,7 +23,7 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final ExecutorService executor;
private final UpdateGitMetricsTask.Factory updateGitMetricsTaskFactory;
- private GitRepoMetricsCache gitRepoMetricsCache;
+ private final GitRepoMetricsCache gitRepoMetricsCache;
@Inject
GitRepoUpdateListener(
@@ -40,11 +40,9 @@
String projectName = event.getProjectName();
logger.atFine().log("Got an update for project %s", projectName);
- if (!gitRepoMetricsCache.shouldCollectStats(projectName)) {
- return;
+ if (gitRepoMetricsCache.shouldCollectStats(projectName)) {
+ UpdateGitMetricsTask updateGitMetricsTask = updateGitMetricsTaskFactory.create(projectName);
+ executor.execute(updateGitMetricsTask);
}
-
- UpdateGitMetricsTask updateGitMetricsTask = updateGitMetricsTaskFactory.create(projectName);
- executor.execute(updateGitMetricsTask);
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTask.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTask.java
index 2196a06..ed120e5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTask.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/UpdateGitMetricsTask.java
@@ -61,7 +61,7 @@
Map<String, Long> newMetrics = gitStats.get();
logger.atInfo().log(
"Here all the metrics for %s - %s", project.getName(), getStringFromMap(newMetrics));
- gitRepoMetricsCache.setMetrics(newMetrics);
+ gitRepoMetricsCache.setMetrics(newMetrics, projectName);
} catch (RepositoryNotFoundException e) {
logger.atSevere().withCause(e).log("Cannot find repository for %s", projectName);
} catch (IOException e) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitStats.java b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitStats.java
index 5a32a5b..2eab2f9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitStats.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/gitrepometrics/collectors/GitStats.java
@@ -81,4 +81,8 @@
private void putMetric(Map<String, Long> metrics, String metricName, long value) {
metrics.put(GitRepoMetricsCache.getMetricName(metricName, p.getName()), value);
}
+
+ public String getStatName() {
+ return "git-statistics";
+ }
}
diff --git a/src/resources/Documentation/config.md b/src/resources/Documentation/config.md
index 3ba47a9..26d162e 100644
--- a/src/resources/Documentation/config.md
+++ b/src/resources/Documentation/config.md
@@ -30,4 +30,8 @@
[git-repo-metrics]
project = test-project
project = another-repo
-```
\ No newline at end of file
+ gracePeriod = 5m
+```
+
+_git-repo-metrics.gracePeriod_: Grace period between samples collection. Used to avoid aggressive
+metrics collection. By default, 0.
\ No newline at end of file
diff --git a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/ConfigSetupUtils.java b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/ConfigSetupUtils.java
index fc74cef..2872ea9 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/ConfigSetupUtils.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/ConfigSetupUtils.java
@@ -27,15 +27,21 @@
import org.eclipse.jgit.lib.Config;
public class ConfigSetupUtils {
- final static String pluginName = "git-repo-metrics";
+ static final String pluginName = "git-repo-metrics";
private final Path basePath;
private final Path gitBasePath;
private final List<String> projects;
+ private final String gracePeriod;
public ConfigSetupUtils(List<String> projects) throws IOException {
+ this(projects, "0");
+ }
+
+ public ConfigSetupUtils(List<String> projects, String gracePeriod) throws IOException {
this.basePath = Files.createTempDirectory("git_repo_metrics_");
this.gitBasePath = new File(basePath.toFile(), "git").toPath();
this.projects = projects;
+ this.gracePeriod = gracePeriod;
}
public GitRepoMetricsConfig getGitRepoMetricsConfig() {
@@ -50,6 +56,7 @@
Config c = new Config();
c.setStringList(pluginName, null, "project", projects);
+ c.setString(pluginName, null, "gracePeriod", gracePeriod);
c.setString("gerrit", null, "basePath", gitBasePath.toString());
return c;
}
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 7a3cf5a..70c2dd9 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/gitrepometrics/GitRepoMetricsCacheTest.java
@@ -16,17 +16,21 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.common.collect.ImmutableMap;
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.GitStats;
import java.io.IOException;
+import java.time.Clock;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
import java.util.Collections;
import org.junit.Before;
import org.junit.Test;
public class GitRepoMetricsCacheTest {
- GitRepoMetricsCache gitRepoMetricsCacheModule;
+ GitRepoMetricsCache gitRepoMetricsCache;
GitRepoMetricsConfig gitRepoMetricsConfig;
FakeMetricMaker fakeMetricMaker;
private ConfigSetupUtils configSetupUtils;
@@ -41,28 +45,81 @@
public void shouldRegisterMetrics() throws IOException {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
fakeMetricMaker = new FakeMetricMaker();
- gitRepoMetricsCacheModule = new GitRepoMetricsCache(fakeMetricMaker, gitRepoMetricsConfig);
- gitRepoMetricsCacheModule.initCache();
+ gitRepoMetricsCache = new GitRepoMetricsCache(fakeMetricMaker, gitRepoMetricsConfig);
+ gitRepoMetricsCache.initCache();
assertThat(fakeMetricMaker.callsCounter).isEqualTo(GitStats.availableMetrics().size());
}
@Test
public void shouldCollectStatsForEnabledRepo() throws IOException {
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- gitRepoMetricsCacheModule =
- new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+ gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
- assertThat(gitRepoMetricsCacheModule.shouldCollectStats(enabledRepo)).isTrue();
+ assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isTrue();
}
@Test
public void shouldNotCollectStatsForDisabledRepo() throws IOException {
String disabledRepo = "disabledRepo";
gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
- gitRepoMetricsCacheModule =
- new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+ gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
- assertThat(gitRepoMetricsCacheModule.shouldCollectStats(disabledRepo)).isFalse();
+ assertThat(gitRepoMetricsCache.shouldCollectStats(disabledRepo)).isFalse();
+ }
+
+ @Test
+ public void shouldCollectStatsWhenGracePeriodNotSet() throws IOException {
+ ConfigSetupUtils configSetupUtils =
+ new ConfigSetupUtils(Collections.singletonList(enabledRepo));
+ gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
+ gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+
+ gitRepoMetricsCache.setMetrics(ImmutableMap.of("anyMetric", 0L), enabledRepo);
+
+ assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isTrue();
+ }
+
+ @Test
+ public void shouldSkipCollectionWhenGracePeriodIsNotExpired() throws IOException {
+ ConfigSetupUtils configSetupUtils =
+ new ConfigSetupUtils(Collections.singletonList(enabledRepo), "5 m");
+ gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
+ gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+
+ gitRepoMetricsCache.setMetrics(ImmutableMap.of("anyMetric", 0L), enabledRepo);
+
+ assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isFalse();
+ }
+
+ @Test
+ public void shouldCollectStatsWhenGracePeriodIsExpired() throws IOException {
+ ConfigSetupUtils configSetupUtils =
+ new ConfigSetupUtils(Collections.singletonList(enabledRepo), "1 s");
+ gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
+ gitRepoMetricsCache =
+ new GitRepoMetricsCache(
+ new FakeMetricMaker(),
+ gitRepoMetricsConfig,
+ Clock.fixed(
+ Instant.now().minus(2, ChronoUnit.SECONDS), Clock.systemDefaultZone().getZone()));
+
+ gitRepoMetricsCache.setMetrics(ImmutableMap.of("anyMetric", 0L), enabledRepo);
+
+ assertThat(gitRepoMetricsCache.shouldCollectStats(enabledRepo)).isTrue();
+ }
+
+ @Test
+ public void shouldSetCollectionTime() throws IOException {
+ ConfigSetupUtils configSetupUtils =
+ new ConfigSetupUtils(Collections.singletonList(enabledRepo));
+ gitRepoMetricsConfig = configSetupUtils.getGitRepoMetricsConfig();
+ gitRepoMetricsCache = new GitRepoMetricsCache(new FakeMetricMaker(), gitRepoMetricsConfig);
+
+ long currentTimeStamp = System.currentTimeMillis();
+
+ gitRepoMetricsCache.setMetrics(ImmutableMap.of("anyMetric", 0L), enabledRepo);
+
+ assertThat(gitRepoMetricsCache.getCollectedAt().get(enabledRepo)).isAtLeast(currentTimeStamp);
}
private class FakeMetricMaker extends DisabledMetricMaker {
