Merge changes Id72def08,Id2d9c0f2
* changes:
Load each code owner config only once for computing code owner statuses
Add a metric to count the number of code owner config reads
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index a582589..4e10e8f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -89,7 +89,7 @@
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final ChangedFiles changedFiles;
private final PureRevertCache pureRevertCache;
- private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy;
+ private final Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider;
private final Provider<CodeOwnerResolver> codeOwnerResolver;
private final ApprovalsUtil approvalsUtil;
private final CodeOwnerMetrics codeOwnerMetrics;
@@ -101,7 +101,7 @@
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
ChangedFiles changedFiles,
PureRevertCache pureRevertCache,
- CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
+ Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider,
Provider<CodeOwnerResolver> codeOwnerResolver,
ApprovalsUtil approvalsUtil,
CodeOwnerMetrics codeOwnerMetrics) {
@@ -110,7 +110,7 @@
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.changedFiles = changedFiles;
this.pureRevertCache = pureRevertCache;
- this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy;
+ this.codeOwnerConfigHierarchyProvider = codeOwnerConfigHierarchyProvider;
this.codeOwnerResolver = codeOwnerResolver;
this.approvalsUtil = approvalsUtil;
this.codeOwnerMetrics = codeOwnerMetrics;
@@ -285,12 +285,14 @@
FallbackCodeOwners fallbackCodeOwners =
codeOwnersPluginConfiguration.getFallbackCodeOwners(branch.project());
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
return changedFiles
.compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
.stream()
.map(
changedFile ->
getFileStatus(
+ codeOwnerConfigHierarchy,
branch,
revision,
globalCodeOwners,
@@ -350,10 +352,12 @@
return getAllPathsAsApproved(changeNotes, patchSet);
}
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
return changedFiles.compute(changeNotes.getProjectName(), patchSet.commitId()).stream()
.map(
changedFile ->
getFileStatus(
+ codeOwnerConfigHierarchy,
branch,
revision,
/* globalCodeOwners= */ CodeOwnerResolverResult.createEmpty(),
@@ -406,6 +410,7 @@
}
private FileCodeOwnerStatus getFileStatus(
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@@ -426,6 +431,7 @@
.map(
newPath ->
getPathCodeOwnerStatus(
+ codeOwnerConfigHierarchy,
branch,
revision,
globalCodeOwners,
@@ -448,6 +454,7 @@
oldPathStatus =
Optional.of(
getPathCodeOwnerStatus(
+ codeOwnerConfigHierarchy,
branch,
revision,
globalCodeOwners,
@@ -468,6 +475,7 @@
}
private PathCodeOwnerStatus getPathCodeOwnerStatus(
+ CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index cecd46d..6b4da27 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -23,7 +23,6 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
-import com.google.inject.Singleton;
import java.io.IOException;
import java.nio.file.Path;
import java.util.Optional;
@@ -44,18 +43,21 @@
* using {@code set noparent} in the root code owner config if the {@code find-owners} backend is
* used).
*/
-@Singleton
public class CodeOwnerConfigHierarchy {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final GitRepositoryManager repoManager;
private final PathCodeOwners.Factory pathCodeOwnersFactory;
+ private final TransientCodeOwnerConfigCache transientCodeOwnerConfigCache;
@Inject
CodeOwnerConfigHierarchy(
- GitRepositoryManager repoManager, PathCodeOwners.Factory pathCodeOwnersFactory) {
+ GitRepositoryManager repoManager,
+ PathCodeOwners.Factory pathCodeOwnersFactory,
+ TransientCodeOwnerConfigCache transientCodeOwnerConfigCache) {
this.repoManager = repoManager;
this.pathCodeOwnersFactory = pathCodeOwnersFactory;
+ this.transientCodeOwnerConfigCache = transientCodeOwnerConfigCache;
}
/**
@@ -153,7 +155,8 @@
CodeOwnerConfig.Key codeOwnerConfigKey =
CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfigKey, revision, absolutePath);
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCache, codeOwnerConfigKey, revision, absolutePath);
if (pathCodeOwners.isPresent()) {
logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
@@ -218,7 +221,8 @@
}
RevCommit metaRevision = rw.parseCommit(ref.getObjectId());
Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(metaCodeOwnerConfigKey, metaRevision, absolutePath);
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCache, metaCodeOwnerConfigKey, metaRevision, absolutePath);
if (pathCodeOwners.isPresent()) {
logger.atFine().log("visit code owner config %s", metaCodeOwnerConfigKey);
pathCodeOwnersVisitor.visit(pathCodeOwners.get());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigLoader.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigLoader.java
new file mode 100644
index 0000000..b263c20
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigLoader.java
@@ -0,0 +1,40 @@
+// Copyright (C) 2021 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.google.gerrit.plugins.codeowners.backend;
+
+import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
+
+/** API to load {@link CodeOwnerConfig}s. */
+public interface CodeOwnerConfigLoader {
+ /**
+ * Retrieves the code owner config for the given key from the given branch revision.
+ *
+ * @param codeOwnerConfigKey the key of the code owner config that should be retrieved
+ * @param revision the branch revision from which the code owner config should be loaded
+ * @return the code owner config for the given key if it exists, otherwise {@link
+ * Optional#empty()}
+ */
+ public Optional<CodeOwnerConfig> get(CodeOwnerConfig.Key codeOwnerConfigKey, ObjectId revision);
+
+ /**
+ * Retrieves the code owner config for the given key from the current revision of the branch.
+ *
+ * @param codeOwnerConfigKey the key of the code owner config that should be retrieved
+ * @return the code owner config for the given key if it exists, otherwise {@link
+ * Optional#empty()}
+ */
+ public Optional<CodeOwnerConfig> getFromCurrentRevision(CodeOwnerConfig.Key codeOwnerConfigKey);
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index a16714c..1fece0d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -155,7 +155,8 @@
requireNonNull(codeOwnerConfig, "codeOwnerConfig");
requireNonNull(absolutePath, "absolutePath");
checkState(absolutePath.isAbsolute(), "path %s must be absolute", absolutePath);
- return resolvePathCodeOwners(pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath));
+ return resolvePathCodeOwners(
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, absolutePath));
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
index 843a054..9315523 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
@@ -18,6 +18,7 @@
import com.google.common.base.Throwables;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.nio.file.Path;
@@ -35,39 +36,32 @@
* that we avoid code repetition in the code owner backends.
*/
@Singleton
-public class CodeOwners {
+public class CodeOwners implements CodeOwnerConfigLoader {
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
+ private final CodeOwnerMetrics codeOwnerMetrics;
@Inject
- CodeOwners(CodeOwnersPluginConfiguration codeOwnersPluginConfiguration) {
+ CodeOwners(
+ CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
+ CodeOwnerMetrics codeOwnerMetrics) {
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
+ this.codeOwnerMetrics = codeOwnerMetrics;
}
- /**
- * Retrieves the code owner config for the given key from the given branch revision.
- *
- * @param codeOwnerConfigKey the key of the code owner config that should be retrieved
- * @param revision the branch revision from which the code owner config should be loaded
- * @return the code owner config for the given key if it exists, otherwise {@link
- * Optional#empty()}
- */
+ @Override
public Optional<CodeOwnerConfig> get(CodeOwnerConfig.Key codeOwnerConfigKey, ObjectId revision) {
requireNonNull(codeOwnerConfigKey, "codeOwnerConfigKey");
requireNonNull(revision, "revision");
+ codeOwnerMetrics.countCodeOwnerConfigReads.increment();
CodeOwnerBackend codeOwnerBackend =
codeOwnersPluginConfiguration.getBackend(codeOwnerConfigKey.branchNameKey());
return codeOwnerBackend.getCodeOwnerConfig(codeOwnerConfigKey, revision);
}
- /**
- * Retrieves the code owner config for the given key from the current revision of the branch.
- *
- * @param codeOwnerConfigKey the key of the code owner config that should be retrieved
- * @return the code owner config for the given key if it exists, otherwise {@link
- * Optional#empty()}
- */
+ @Override
public Optional<CodeOwnerConfig> getFromCurrentRevision(CodeOwnerConfig.Key codeOwnerConfigKey) {
requireNonNull(codeOwnerConfigKey, "codeOwnerConfigKey");
+ codeOwnerMetrics.countCodeOwnerConfigReads.increment();
CodeOwnerBackend codeOwnerBackend =
codeOwnersPluginConfiguration.getBackend(codeOwnerConfigKey.branchNameKey());
return codeOwnerBackend.getCodeOwnerConfig(codeOwnerConfigKey, /* revision= */ null);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index 78e315a..c382801 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -24,6 +24,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
import com.google.gerrit.metrics.Timer0;
@@ -73,11 +74,12 @@
this.codeOwners = codeOwners;
}
- public PathCodeOwners create(CodeOwnerConfig codeOwnerConfig, Path absolutePath) {
+ public PathCodeOwners createWithoutCache(CodeOwnerConfig codeOwnerConfig, Path absolutePath) {
requireNonNull(codeOwnerConfig, "codeOwnerConfig");
return new PathCodeOwners(
codeOwnerMetrics,
projectCache,
+ /* transientCodeOwnerConfigCache= */ null,
codeOwners,
codeOwnerConfig,
absolutePath,
@@ -85,14 +87,21 @@
}
public Optional<PathCodeOwners> create(
- CodeOwnerConfig.Key codeOwnerConfigKey, ObjectId revision, Path absolutePath) {
- return codeOwners
+ TransientCodeOwnerConfigCache transientCodeOwnerConfigCache,
+ CodeOwnerConfig.Key codeOwnerConfigKey,
+ ObjectId revision,
+ Path absolutePath) {
+ requireNonNull(transientCodeOwnerConfigCache, "transientCodeOwnerConfigCache");
+ requireNonNull(codeOwnerConfigKey, "codeOwnerConfigKey");
+ requireNonNull(revision, "revision");
+ return transientCodeOwnerConfigCache
.get(codeOwnerConfigKey, revision)
.map(
codeOwnerConfig ->
new PathCodeOwners(
codeOwnerMetrics,
projectCache,
+ transientCodeOwnerConfigCache,
codeOwners,
codeOwnerConfig,
absolutePath,
@@ -127,6 +136,7 @@
private final CodeOwnerMetrics codeOwnerMetrics;
private final ProjectCache projectCache;
+ private final CodeOwnerConfigLoader codeOwnerConfigLoader;
private final CodeOwners codeOwners;
private final CodeOwnerConfig codeOwnerConfig;
private final Path path;
@@ -137,12 +147,15 @@
private PathCodeOwners(
CodeOwnerMetrics codeOwnerMetrics,
ProjectCache projectCache,
+ @Nullable TransientCodeOwnerConfigCache transientCodeOwnerConfigCache,
CodeOwners codeOwners,
CodeOwnerConfig codeOwnerConfig,
Path path,
PathExpressionMatcher pathExpressionMatcher) {
this.codeOwnerMetrics = requireNonNull(codeOwnerMetrics, "codeOwnerMetrics");
this.projectCache = requireNonNull(projectCache, "projectCache");
+ this.codeOwnerConfigLoader =
+ transientCodeOwnerConfigCache != null ? transientCodeOwnerConfigCache : codeOwners;
this.codeOwners = requireNonNull(codeOwners, "codeOwners");
this.codeOwnerConfig = requireNonNull(codeOwnerConfig, "codeOwnerConfig");
this.path = requireNonNull(path, "path");
@@ -356,8 +369,8 @@
Optional<CodeOwnerConfig> mayBeImportedCodeOwnerConfig =
revision.isPresent()
- ? codeOwners.get(keyOfImportedCodeOwnerConfig, revision.get())
- : codeOwners.getFromCurrentRevision(keyOfImportedCodeOwnerConfig);
+ ? codeOwnerConfigLoader.get(keyOfImportedCodeOwnerConfig, revision.get())
+ : codeOwnerConfigLoader.getFromCurrentRevision(keyOfImportedCodeOwnerConfig);
if (!mayBeImportedCodeOwnerConfig.isPresent()) {
unresolvedImports.add(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
new file mode 100644
index 0000000..96e0a31
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
@@ -0,0 +1,131 @@
+// Copyright (C) 2021 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.google.gerrit.plugins.codeowners.backend;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * Class to load and cache {@link CodeOwnerConfig}s within a request.
+ *
+ * <p>This cache is transient, which means the code owner configs stay cached only for the lifetime
+ * of the {@code TransientCodeOwnerConfigCache} instance.
+ *
+ * <p><strong>Note</strong>: This class is not thread-safe.
+ */
+public class TransientCodeOwnerConfigCache implements CodeOwnerConfigLoader {
+ private final GitRepositoryManager repoManager;
+ private final CodeOwners codeOwners;
+ private final CodeOwnerMetrics codeOwnerMetrics;
+ private final HashMap<CacheKey, Optional<CodeOwnerConfig>> cache = new HashMap<>();
+
+ @Inject
+ TransientCodeOwnerConfigCache(
+ GitRepositoryManager repoManager, CodeOwners codeOwners, CodeOwnerMetrics codeOwnerMetrics) {
+ this.repoManager = repoManager;
+ this.codeOwners = codeOwners;
+ this.codeOwnerMetrics = codeOwnerMetrics;
+ }
+
+ /**
+ * Gets the specified code owner config from the cache, if it was previously retrieved. Otherwise
+ * loads and returns the code owner config.
+ */
+ @Override
+ public Optional<CodeOwnerConfig> get(
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
+ CacheKey cacheKey = CacheKey.create(codeOwnerConfigKey, revision);
+ Optional<CodeOwnerConfig> cachedCodeOwnerConfig = cache.get(cacheKey);
+ if (cachedCodeOwnerConfig != null) {
+ codeOwnerMetrics.countCodeOwnerConfigCacheReads.increment();
+ return cachedCodeOwnerConfig;
+ }
+ return loadAndCache(cacheKey);
+ }
+
+ /**
+ * Gets the specified code owner config from the cache, if it was previously retrieved. Otherwise
+ * loads and returns the code owner config.
+ */
+ @Override
+ public Optional<CodeOwnerConfig> getFromCurrentRevision(CodeOwnerConfig.Key codeOwnerConfigKey) {
+ return get(codeOwnerConfigKey, /* revision= */ null);
+ }
+
+ /** Load a code owner config and puts it into the cache. */
+ private Optional<CodeOwnerConfig> loadAndCache(CacheKey cacheKey) {
+ Optional<CodeOwnerConfig> codeOwnerConfig;
+ if (cacheKey.revision().isPresent()) {
+ codeOwnerConfig = codeOwners.get(cacheKey.codeOwnerConfigKey(), cacheKey.revision().get());
+ } else {
+ Optional<ObjectId> revision = getRevision(cacheKey.codeOwnerConfigKey().branchNameKey());
+ if (revision.isPresent()) {
+ codeOwnerConfig = codeOwners.get(cacheKey.codeOwnerConfigKey(), revision.get());
+ } else {
+ // branch does not exists, hence the code owner config also doesn't exist
+ codeOwnerConfig = Optional.empty();
+ }
+ }
+ cache.put(cacheKey, codeOwnerConfig);
+ return codeOwnerConfig;
+ }
+
+ /**
+ * Gets the revision for the given branch.
+ *
+ * <p>Returns {@link Optional#empty()} if the branch doesn't exist.
+ */
+ private Optional<ObjectId> getRevision(BranchNameKey branchNameKey) {
+ try (Repository repo = repoManager.openRepository(branchNameKey.project())) {
+ Ref ref = repo.exactRef(branchNameKey.branch());
+ if (ref == null) {
+ // branch does not exist
+ return Optional.empty();
+ }
+ return Optional.of(ref.getObjectId());
+ } catch (IOException e) {
+ throw new CodeOwnersInternalServerErrorException(
+ String.format(
+ "failed to get revision of branch %s in project %s",
+ branchNameKey.shortName(), branchNameKey.project()),
+ e);
+ }
+ }
+
+ @AutoValue
+ abstract static class CacheKey {
+ /** The key of the code owner config. */
+ public abstract CodeOwnerConfig.Key codeOwnerConfigKey();
+
+ /** The revision from which the code owner config was loaded. */
+ public abstract Optional<ObjectId> revision();
+
+ public static CacheKey create(
+ CodeOwnerConfig.Key codeOwnerConfigKey, @Nullable ObjectId revision) {
+ return new AutoValue_TransientCodeOwnerConfigCache_CacheKey(
+ codeOwnerConfigKey, Optional.ofNullable(revision));
+ }
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index be681e6..e5a3c58 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.metrics;
+import com.google.gerrit.metrics.Counter0;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
import com.google.gerrit.metrics.MetricMaker;
@@ -24,6 +25,7 @@
/** Metrics of the code-owners plugin. */
@Singleton
public class CodeOwnerMetrics {
+ // latency metrics
public final Timer0 computeChangedFiles;
public final Timer0 computeFileStatus;
public final Timer0 computeFileStatuses;
@@ -36,12 +38,17 @@
public final Timer0 resolvePathCodeOwners;
public final Timer0 runCodeOwnerSubmitRule;
+ // counter metrics
+ public final Counter0 countCodeOwnerConfigReads;
+ public final Counter0 countCodeOwnerConfigCacheReads;
+
private final MetricMaker metricMaker;
@Inject
CodeOwnerMetrics(MetricMaker metricMaker) {
this.metricMaker = metricMaker;
+ // latency metrics
this.computeChangedFiles =
createLatencyTimer("compute_changed_files", "Latency for computing changed files");
this.computeFileStatus =
@@ -79,6 +86,16 @@
this.runCodeOwnerSubmitRule =
createLatencyTimer(
"run_code_owner_submit_rule", "Latency for running the code owner submit rule");
+
+ // counter metrics
+ this.countCodeOwnerConfigReads =
+ createCounter(
+ "count_code_owner_config_reads",
+ "Total number of code owner config reads from backend");
+ this.countCodeOwnerConfigCacheReads =
+ createCounter(
+ "count_code_owner_config_cache_reads",
+ "Total number of code owner config reads from cache");
}
private Timer0 createLatencyTimer(String name, String description) {
@@ -86,4 +103,8 @@
"code_owners/" + name,
new Description(description).setCumulative().setUnit(Units.MILLISECONDS));
}
+
+ private Counter0 createCounter(String name, String description) {
+ return metricMaker.newCounter("code_owners/" + name, new Description(description).setRate());
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
index d01d0fc..d326da6 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -145,7 +145,9 @@
String.format(
"checking code owner config file %s", codeOwnerConfig.key().format(codeOwners)));
OptionalResultWithMessages<PathCodeOwnersResult> pathCodeOwnersResult =
- pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).resolveCodeOwnerConfig();
+ pathCodeOwnersFactory
+ .createWithoutCache(codeOwnerConfig, absolutePath)
+ .resolveCodeOwnerConfig();
messages.addAll(pathCodeOwnersResult.messages());
pathCodeOwnersResult
.get()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
index 5490e2e..8929a72 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
@@ -41,6 +41,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.inject.Inject;
import com.google.inject.Key;
+import com.google.inject.Provider;
import com.google.inject.util.Providers;
import java.nio.file.Path;
import java.nio.file.Paths;
@@ -58,6 +59,7 @@
private CodeOwnerConfigOperations codeOwnerConfigOperations;
private PathCodeOwners.Factory pathCodeOwnersFactory;
private DynamicMap<CodeOwnerBackend> codeOwnerBackends;
+ private Provider<TransientCodeOwnerConfigCache> transientCodeOwnerConfigCacheProvider;
@Before
public void setUpCodeOwnersPlugin() throws Exception {
@@ -66,12 +68,14 @@
pathCodeOwnersFactory = plugin.getSysInjector().getInstance(PathCodeOwners.Factory.class);
codeOwnerBackends =
plugin.getSysInjector().getInstance(new Key<DynamicMap<CodeOwnerBackend>>() {});
+ transientCodeOwnerConfigCacheProvider =
+ plugin.getSysInjector().getInstance(new Key<Provider<TransientCodeOwnerConfigCache>>() {});
}
@Test
public void createPathCodeOwnersForCodeOwnerConfig() throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder().build(), Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners).isNotNull();
}
@@ -82,7 +86,7 @@
assertThrows(
NullPointerException.class,
() ->
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
/* codeOwnerConfig= */ null, Paths.get("/foo/bar/baz.md")));
assertThat(npe).hasMessageThat().isEqualTo("codeOwnerConfig");
}
@@ -93,7 +97,9 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> pathCodeOwnersFactory.create(codeOwnerConfig, /* absolutePath= */ null));
+ () ->
+ pathCodeOwnersFactory.createWithoutCache(
+ codeOwnerConfig, /* absolutePath= */ null));
assertThat(npe).hasMessageThat().isEqualTo("path");
}
@@ -104,7 +110,8 @@
IllegalStateException exception =
assertThrows(
IllegalStateException.class,
- () -> pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get(relativePath)));
+ () ->
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get(relativePath)));
assertThat(exception)
.hasMessageThat()
.isEqualTo(String.format("path %s must be absolute", relativePath));
@@ -123,6 +130,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
codeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -130,12 +138,28 @@
}
@Test
+ public void cannotCreatePathCodeOwnersForNullCache() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ pathCodeOwnersFactory.create(
+ /* transientCodeOwnerConfigCache= */ null,
+ CodeOwnerConfig.Key.create(
+ BranchNameKey.create(project, "master"), Paths.get("/")),
+ projectOperations.project(project).getHead("master"),
+ Paths.get("/foo/bar/baz.md")));
+ assertThat(npe).hasMessageThat().isEqualTo("transientCodeOwnerConfigCache");
+ }
+
+ @Test
public void cannotCreatePathCodeOwnersForNullCodeOwnerConfigKey() throws Exception {
NullPointerException npe =
assertThrows(
NullPointerException.class,
() ->
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
/* codeOwnerConfigKey= */ null,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md")));
@@ -149,6 +173,7 @@
NullPointerException.class,
() ->
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
CodeOwnerConfig.Key.create(
BranchNameKey.create(project, "master"), Paths.get("/")),
/* revision= */ null,
@@ -172,6 +197,7 @@
NullPointerException.class,
() ->
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
codeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
/* absolutePath= */ null));
@@ -195,6 +221,7 @@
IllegalStateException.class,
() ->
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
codeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get(relativePath)));
@@ -207,7 +234,8 @@
public void getEmptyPathCodeOwners() throws Exception {
CodeOwnerConfig emptyCodeOwnerConfig = createCodeOwnerBuilder().build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(emptyCodeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(
+ emptyCodeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners()).isEmpty();
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().hasUnresolvedImports()).isFalse();
}
@@ -219,7 +247,7 @@
.addCodeOwnerSet(CodeOwnerSet.createWithoutPathExpressions(admin.email(), user.email()))
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email(), user.email());
@@ -257,7 +285,7 @@
.addCodeOwnerSet(nonMatchingCodeOwnerSet)
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email(), user.email());
@@ -278,7 +306,7 @@
.build())
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners()).isEmpty();
}
}
@@ -310,7 +338,7 @@
.addCodeOwnerSet(globalCodeOwnerSet)
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email());
@@ -344,7 +372,7 @@
.addCodeOwnerSet(globalCodeOwnerSet)
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email(), user.email());
@@ -379,7 +407,7 @@
.addCodeOwnerSet(perFileCodeOwnerSet2)
.build();
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
+ pathCodeOwnersFactory.createWithoutCache(codeOwnerConfig, Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().getPathCodeOwners())
.comparingElementsUsing(hasEmail())
.containsExactly(admin.email(), user.email());
@@ -390,7 +418,7 @@
public void checkThatParentCodeOwnersAreIgnoredIfCodeOwnerConfigIgnoresParentCodeOwners()
throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder().setIgnoreParentCodeOwners().build(),
Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().ignoreParentCodeOwners()).isTrue();
@@ -400,7 +428,7 @@
public void checkThatParentCodeOwnersAreNotIgnoredIfCodeOwnerConfigDoesNotIgnoreParentCodeOwners()
throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder().setIgnoreParentCodeOwners(false).build(),
Paths.get("/foo/bar/baz.md"));
assertThat(pathCodeOwners.resolveCodeOwnerConfig().get().ignoreParentCodeOwners()).isFalse();
@@ -410,7 +438,7 @@
public void checkThatParentCodeOwnersAreIgnoredIfMatchingCodeOwnerSetIgnoresParentCodeOwners()
throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder()
.addCodeOwnerSet(
CodeOwnerSet.builder()
@@ -427,7 +455,7 @@
checkThatParentCodeOwnersAreNotIgnoredIfNonMatchingCodeOwnerSetIgnoresParentCodeOwners()
throws Exception {
PathCodeOwners pathCodeOwners =
- pathCodeOwnersFactory.create(
+ pathCodeOwnersFactory.createWithoutCache(
createCodeOwnerBuilder()
.addCodeOwnerSet(
CodeOwnerSet.builder()
@@ -456,6 +484,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -502,6 +531,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -549,6 +579,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -596,6 +627,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -645,6 +677,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -693,6 +726,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -745,6 +779,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -796,6 +831,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -839,6 +875,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -877,6 +914,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -936,6 +974,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -995,6 +1034,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1048,6 +1088,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1086,6 +1127,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1134,7 +1176,11 @@
.update();
Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(rootCodeOwnerConfigKey, oldRevision, Paths.get("/foo.md"));
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
+ rootCodeOwnerConfigKey,
+ oldRevision,
+ Paths.get("/foo.md"));
assertThat(pathCodeOwners).isPresent();
// Expectation: we get the global owners from the importing and the imported code owner config
@@ -1169,6 +1215,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1200,6 +1247,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1243,6 +1291,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1276,6 +1325,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1317,6 +1367,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1366,6 +1417,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead(branchName),
Paths.get("/foo.md"));
@@ -1411,6 +1463,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1458,6 +1511,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo/bar/baz.md"));
@@ -1493,6 +1547,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1545,6 +1600,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
importingCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1608,6 +1664,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1668,6 +1725,7 @@
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1743,6 +1801,7 @@
// *.txt
Optional<PathCodeOwners> pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.xyz"));
@@ -1752,6 +1811,7 @@
// Expectation for foo.md file: code owners contains only user since foo.md only matches *.md
pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.md"));
@@ -1764,6 +1824,7 @@
// *.txt
pathCodeOwners =
pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
rootCodeOwnerConfigKey,
projectOperations.project(project).getHead("master"),
Paths.get("/foo.txt"));
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 55bb736..d236b3e 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -28,6 +28,13 @@
* `run_code_owner_submit_rule`:
Latency for running the code owner submit rule.
+## <a id="counterMetrics"> Counter Metrics
+
+* `count_code_owner_config_reads`:
+ Total number of code owner config reads from backend.
+* `count_code_owner_config_cache_reads`:
+ Total number of code owner config reads from cache.
+
---
Back to [@PLUGIN@ documentation index](index.html)
