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 {