Merge "Add metrics for timeouts in diff cache"
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index a8eb946..8f36cfb 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -77,6 +77,8 @@
* `caches/disk_cached`: Disk entries used by persistent cache.
* `caches/disk_hit_ratio`: Disk hit ratio for persistent cache.
* `caches/refresh_count`: The number of refreshes per cache with an indicator if a reload was necessary.
+* `caches/diff/timeouts`: The number of git file diff computations that resulted in timeouts.
+* `caches/diff/legacy/timeouts`: The number of git file diff computations (using the legacy cache) that resulted in timeouts.
Cache disk metrics are expensive to compute on larger installations and are not
computed by default. They can be enabled via the
diff --git a/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java b/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java
index 017e276..031f3db 100644
--- a/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java
+++ b/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java
@@ -35,6 +35,9 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
+import com.google.gerrit.metrics.Counter0;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -52,6 +55,7 @@
import com.google.gerrit.server.patch.Text;
import com.google.gerrit.server.patch.filediff.EditTransformer.ContextAwareEdit;
import com.google.inject.Inject;
+import com.google.inject.Singleton;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.util.ArrayList;
@@ -99,12 +103,30 @@
PatchListLoader create(PatchListKey key, Project.NameKey project);
}
+ @Singleton
+ static class Metrics {
+ final Counter0 timeouts;
+
+ @Inject
+ Metrics(MetricMaker metricMaker) {
+ // TODO(ghareeb): Remove this metric from documentation once this class is deprecated.
+ timeouts =
+ metricMaker.newCounter(
+ "caches/diff/legacy/timeouts",
+ new Description(
+ "Total number of git file diff computations that resulted in timeouts.")
+ .setRate()
+ .setUnit("count"));
+ }
+ }
+
private final GitRepositoryManager repoManager;
private final PatchListCache patchListCache;
private final ThreeWayMergeStrategy mergeStrategy;
private final ExecutorService diffExecutor;
private final AutoMerger autoMerger;
private final PatchListKey key;
+ private final Metrics metrics;
private final Project.NameKey project;
private final long timeoutMillis;
@@ -115,16 +137,18 @@
@GerritServerConfig Config cfg,
@DiffExecutor ExecutorService de,
AutoMerger am,
+ Metrics metrics,
@Assisted PatchListKey k,
@Assisted Project.NameKey p) {
- repoManager = mgr;
- patchListCache = plc;
- mergeStrategy = MergeUtil.getMergeStrategy(cfg);
- diffExecutor = de;
- autoMerger = am;
- key = k;
- project = p;
- timeoutMillis =
+ this.repoManager = mgr;
+ this.patchListCache = plc;
+ this.mergeStrategy = MergeUtil.getMergeStrategy(cfg);
+ this.diffExecutor = de;
+ this.autoMerger = am;
+ this.metrics = metrics;
+ this.key = k;
+ this.project = p;
+ this.timeoutMillis =
ConfigUtil.getTimeUnit(
cfg,
"cache",
@@ -529,6 +553,7 @@
try {
return result.get(timeoutMillis, TimeUnit.MILLISECONDS);
} catch (InterruptedException | TimeoutException e) {
+ metrics.timeouts.increment();
logger.atWarning().log(
"%s ms timeout reached for Diff loader in project %s"
+ " on commit %s on path %s comparing %s..%s",
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
index be2709f..6f93f2e 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
@@ -31,6 +31,9 @@
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
+import com.google.gerrit.metrics.Counter0;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -92,6 +95,22 @@
};
}
+ @Singleton
+ static class Metrics {
+ final Counter0 timeouts;
+
+ @Inject
+ Metrics(MetricMaker metricMaker) {
+ timeouts =
+ metricMaker.newCounter(
+ "caches/diff/timeouts",
+ new Description(
+ "Total number of git file diff computations that resulted in timeouts.")
+ .setRate()
+ .setUnit("count"));
+ }
+ }
+
/** Enum for the supported diff algorithms for the file diff computation. */
public enum DiffAlgorithm {
HISTOGRAM_WITH_FALLBACK_MYERS,
@@ -150,12 +169,14 @@
private final GitRepositoryManager repoManager;
private final ExecutorService diffExecutor;
private final long timeoutMillis;
+ private final Metrics metrics;
@Inject
public Loader(
@GerritServerConfig Config cfg,
GitRepositoryManager repoManager,
- @DiffExecutor ExecutorService de) {
+ @DiffExecutor ExecutorService de,
+ Metrics metrics) {
this.repoManager = repoManager;
this.diffExecutor = de;
this.timeoutMillis =
@@ -166,6 +187,7 @@
"timeout",
TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS),
TimeUnit.MILLISECONDS);
+ this.metrics = metrics;
}
@Override
@@ -344,6 +366,7 @@
return GitFileDiff.create(diffEntry, fileHeader);
} catch (InterruptedException | TimeoutException e) {
// If timeout happens, create a negative result
+ metrics.timeouts.increment();
return GitFileDiff.createNegative(
AbbreviatedObjectId.fromObjectId(key.oldTree()),
AbbreviatedObjectId.fromObjectId(key.newTree()),
