Merge changes Ia166d6d7,Ica5b5b7e
* changes:
Add metric to record the latency of computing the file status for one file
Fix latency metrics for computing file statuses
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index ae354de..6c36cd1 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -128,7 +128,13 @@
public ImmutableList<Path> getOwnedPaths(
ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
throws ResourceConflictException {
- try {
+ try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
+ logger.atFine().log(
+ "compute owned paths for account %d (project = %s, change = %d, patch set = %d)",
+ accountId.get(),
+ changeNotes.getProjectName(),
+ changeNotes.getChangeId().get(),
+ patchSet.id().get());
return getFileStatusesForAccount(changeNotes, patchSet, accountId)
.flatMap(
fileCodeOwnerStatus ->
@@ -181,6 +187,22 @@
/**
* Gets the code owner statuses for all files/paths that were changed in the current revision of
+ * the given change as a set.
+ *
+ * @see #getFileStatuses(ChangeNotes)
+ */
+ public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(ChangeNotes changeNotes)
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
+ logger.atFine().log(
+ "compute file statuses (project = %s, change = %d)",
+ changeNotes.getProjectName(), changeNotes.getChangeId().get());
+ return getFileStatuses(changeNotes).collect(toImmutableSet());
+ }
+ }
+
+ /**
+ * Gets the code owner statuses for all files/paths that were changed in the current revision of
* the given change.
*
* <p>The code owner statuses tell the user which code owner approvals are still missing in order
@@ -208,9 +230,9 @@
public Stream<FileCodeOwnerStatus> getFileStatuses(ChangeNotes changeNotes)
throws ResourceConflictException, IOException, PatchListNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
- try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
+ try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputation.start()) {
logger.atFine().log(
- "compute file statuses (project = %s, change = %d)",
+ "prepare stream to compute file statuses (project = %s, change = %d)",
changeNotes.getProjectName(), changeNotes.getChangeId().get());
if (codeOwnersPluginConfiguration.arePureRevertsExempted(changeNotes.getProjectName())
@@ -298,9 +320,10 @@
requireNonNull(changeNotes, "changeNotes");
requireNonNull(patchSet, "patchSet");
requireNonNull(accountId, "accountId");
- try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatusesForAccount.start()) {
+ try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputationForAccount.start()) {
logger.atFine().log(
- "compute file statuses for account %d (project = %s, change = %d, patch set = %d)",
+ "prepare stream to compute file statuses for account %d (project = %s, change = %d,"
+ + " patch set = %d)",
accountId.get(),
changeNotes.getProjectName(),
changeNotes.getChangeId().get(),
@@ -387,50 +410,53 @@
ImmutableSet<Account.Id> approverAccountIds,
boolean hasOverride,
ChangedFile changedFile) {
- logger.atFine().log("computing file status for %s", changedFile);
+ try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatus.start()) {
+ logger.atFine().log("computing file status for %s", changedFile);
- // Compute the code owner status for the new path, if there is a new path.
- Optional<PathCodeOwnerStatus> newPathStatus =
- changedFile
- .newPath()
- .map(
- newPath ->
- getPathCodeOwnerStatus(
- branch,
- revision,
- globalCodeOwners,
- enableImplicitApprovalFromUploader,
- patchSetUploader,
- reviewerAccountIds,
- approverAccountIds,
- hasOverride,
- newPath));
+ // Compute the code owner status for the new path, if there is a new path.
+ Optional<PathCodeOwnerStatus> newPathStatus =
+ changedFile
+ .newPath()
+ .map(
+ newPath ->
+ getPathCodeOwnerStatus(
+ branch,
+ revision,
+ globalCodeOwners,
+ enableImplicitApprovalFromUploader,
+ patchSetUploader,
+ reviewerAccountIds,
+ approverAccountIds,
+ hasOverride,
+ newPath));
- // Compute the code owner status for the old path, if the file was deleted or renamed.
- Optional<PathCodeOwnerStatus> oldPathStatus = Optional.empty();
- if (changedFile.isDeletion() || changedFile.isRename()) {
- checkState(changedFile.oldPath().isPresent(), "old path must be present for deletion/rename");
- logger.atFine().log(
- "file was %s (old path = %s)",
- changedFile.isDeletion() ? "deleted" : "renamed", changedFile.oldPath().get());
- oldPathStatus =
- Optional.of(
- getPathCodeOwnerStatus(
- branch,
- revision,
- globalCodeOwners,
- enableImplicitApprovalFromUploader,
- patchSetUploader,
- reviewerAccountIds,
- approverAccountIds,
- hasOverride,
- changedFile.oldPath().get()));
+ // Compute the code owner status for the old path, if the file was deleted or renamed.
+ Optional<PathCodeOwnerStatus> oldPathStatus = Optional.empty();
+ if (changedFile.isDeletion() || changedFile.isRename()) {
+ checkState(
+ changedFile.oldPath().isPresent(), "old path must be present for deletion/rename");
+ logger.atFine().log(
+ "file was %s (old path = %s)",
+ changedFile.isDeletion() ? "deleted" : "renamed", changedFile.oldPath().get());
+ oldPathStatus =
+ Optional.of(
+ getPathCodeOwnerStatus(
+ branch,
+ revision,
+ globalCodeOwners,
+ enableImplicitApprovalFromUploader,
+ patchSetUploader,
+ reviewerAccountIds,
+ approverAccountIds,
+ hasOverride,
+ changedFile.oldPath().get()));
+ }
+
+ FileCodeOwnerStatus fileCodeOwnerStatus =
+ FileCodeOwnerStatus.create(changedFile, newPathStatus, oldPathStatus);
+ logger.atFine().log("fileCodeOwnerStatus = %s", fileCodeOwnerStatus);
+ return fileCodeOwnerStatus;
}
-
- FileCodeOwnerStatus fileCodeOwnerStatus =
- FileCodeOwnerStatus.create(changedFile, newPathStatus, oldPathStatus);
- logger.atFine().log("fileCodeOwnerStatus = %s", fileCodeOwnerStatus);
- return fileCodeOwnerStatus;
}
private PathCodeOwnerStatus getPathCodeOwnerStatus(
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index e1ce3bf..be681e6 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -25,8 +25,11 @@
@Singleton
public class CodeOwnerMetrics {
public final Timer0 computeChangedFiles;
+ public final Timer0 computeFileStatus;
public final Timer0 computeFileStatuses;
- public final Timer0 computeFileStatusesForAccount;
+ public final Timer0 computeOwnedPaths;
+ public final Timer0 prepareFileStatusComputation;
+ public final Timer0 prepareFileStatusComputationForAccount;
public final Timer0 resolveCodeOwnerConfig;
public final Timer0 resolveCodeOwnerConfigImport;
public final Timer0 resolveCodeOwnerConfigImports;
@@ -41,9 +44,21 @@
this.computeChangedFiles =
createLatencyTimer("compute_changed_files", "Latency for computing changed files");
+ this.computeFileStatus =
+ createLatencyTimer(
+ "compute_file_status", "Latency for computing the file status of one file");
this.computeFileStatuses =
- createLatencyTimer("compute_file_statuses", "Latency for computing file statuses");
- this.computeFileStatusesForAccount =
+ createLatencyTimer(
+ "compute_file_statuses",
+ "Latency for computing file statuses for all files in a change");
+ this.computeOwnedPaths =
+ createLatencyTimer(
+ "compute_owned_paths",
+ "Latency for computing the files in a change that are owned by a user");
+ this.prepareFileStatusComputation =
+ createLatencyTimer(
+ "prepare_file_status_computation", "Latency for preparing the file status computation");
+ this.prepareFileStatusComputationForAccount =
createLatencyTimer(
"compute_file_statuses_for_account",
"Latency for computing file statuses for an account");
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
index 005f3e7..fdf7bd7 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnerStatus.java
@@ -14,8 +14,6 @@
package com.google.gerrit.plugins.codeowners.restapi;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
-
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -64,7 +62,7 @@
throws RestApiException, IOException, PermissionBackendException,
PatchListNotAvailableException {
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatuses(changeResource.getNotes()).collect(toImmutableSet());
+ codeOwnerApprovalCheck.getFileStatusesAsSet(changeResource.getNotes());
return Response.ok(
CodeOwnerStatusInfoJson.format(
changeResource.getNotes().getCurrentPatchSet().id(), fileCodeOwnerStatuses));
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 2e63c23..55bb736 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -7,10 +7,18 @@
* `compute_changed_files`:
Latency for computing changed files.
+* `compute_file_status`:
+ Latency for computing the file status for one file.
* `compute_file_statuses`:
+ Latency for computing file statuses for all files in a change.
+* `compute_owned_paths`:
Latency for computing file statuses.
-* `compute_file_statuses_for_account`:
- Latency for computing file statuses for an account.
+* `compute_owned_paths`:
+ Latency for computing the files in a change that are owned by a user.
+* `prepare_file_status_computation`:
+ Latency for preparing the file status computation.
+* `prepare_file_status_computation_for_account`:
+ Latency for preparing the file status computation for an account.
* `resolve_code_owner_config`:
Latency for resolving a code owner config file.
* `resolve_code_owner_config_import`:
