Add metrics to record latency for adding/extending change messages
We are observing a latency increase for posting reviews, likely caused
by extending the change message on code owner approvals. We do have an
idea how to speed this up and with this new metric we will be able to
verify how much the implemented improvement helped.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I421c99fb80546bded5218c515bdc58fb6483e11c
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index a7c2f27..9352e97 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -25,8 +25,10 @@
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.events.ReviewerAddedListener;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.CurrentUser;
@@ -65,6 +67,7 @@
private final ChangeNotes.Factory changeNotesFactory;
private final AccountCache accountCache;
private final ChangeMessagesUtil changeMessageUtil;
+ private final CodeOwnerMetrics codeOwnerMetrics;
@Inject
CodeOwnersOnAddReviewer(
@@ -74,7 +77,8 @@
RetryHelper retryHelper,
ChangeNotes.Factory changeNotesFactory,
AccountCache accountCache,
- ChangeMessagesUtil changeMessageUtil) {
+ ChangeMessagesUtil changeMessageUtil,
+ CodeOwnerMetrics codeOwnerMetrics) {
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerApprovalCheck = codeOwnerApprovalCheck;
this.userProvider = userProvider;
@@ -82,6 +86,7 @@
this.changeNotesFactory = changeNotesFactory;
this.accountCache = accountCache;
this.changeMessageUtil = changeMessageUtil;
+ this.codeOwnerMetrics = codeOwnerMetrics;
}
@Override
@@ -96,7 +101,7 @@
return;
}
- try {
+ try (Timer0.Context ctx = codeOwnerMetrics.addChangeMessageOnAddReviewer.start()) {
retryHelper
.changeUpdate(
"addCodeOwnersMessageOnAddReviewer",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index 7f2cd2b..101cedd 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -20,9 +20,11 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -54,13 +56,16 @@
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
+ private final CodeOwnerMetrics codeOwnerMetrics;
@Inject
OnCodeOwnerApproval(
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
- CodeOwnerApprovalCheck codeOwnerApprovalCheck) {
+ CodeOwnerApprovalCheck codeOwnerApprovalCheck,
+ CodeOwnerMetrics codeOwnerMetrics) {
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerApprovalCheck = codeOwnerApprovalCheck;
+ this.codeOwnerMetrics = codeOwnerMetrics;
}
@Override
@@ -90,8 +95,10 @@
return Optional.empty();
}
- return buildMessageForCodeOwnerApproval(
- user, changeNotes, patchSet, oldApprovals, approvals, requiredApproval);
+ try (Timer0.Context ctx = codeOwnerMetrics.extendChangeMessageOnPostReview.start()) {
+ return buildMessageForCodeOwnerApproval(
+ user, changeNotes, patchSet, oldApprovals, approvals, requiredApproval);
+ }
}
private Optional<String> buildMessageForCodeOwnerApproval(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
index c06e760..db3f936 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
@@ -21,9 +21,11 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.restapi.change.OnPostReview;
@@ -45,10 +47,14 @@
@Singleton
class OnCodeOwnerOverride implements OnPostReview {
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
+ private final CodeOwnerMetrics codeOwnerMetrics;
@Inject
- OnCodeOwnerOverride(CodeOwnersPluginConfiguration codeOwnersPluginConfiguration) {
+ OnCodeOwnerOverride(
+ CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
+ CodeOwnerMetrics codeOwnerMetrics) {
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
+ this.codeOwnerMetrics = codeOwnerMetrics;
}
@Override
@@ -69,27 +75,32 @@
return Optional.empty();
}
- ImmutableList<RequiredApproval> overrideApprovals =
+ ImmutableList<RequiredApproval> appliedOverrideApprovals =
codeOwnersConfig.getOverrideApproval().stream()
.sorted(comparing(RequiredApproval::toString))
+ // If oldApprovals doesn't contain the label or if the labels value in it is null, the
+ // label was not changed.
+ .filter(
+ overrideApproval ->
+ oldApprovals.get(overrideApproval.labelType().getName()) != null)
.collect(toImmutableList());
- List<String> messages = new ArrayList<>();
- for (RequiredApproval overrideApproval : overrideApprovals) {
- if (oldApprovals.get(overrideApproval.labelType().getName()) == null) {
- // If oldApprovals doesn't contain the label or if the labels value in it is null, the label
- // was not changed.
- continue;
- }
-
- buildMessageForCodeOwnerOverride(user, patchSet, oldApprovals, approvals, overrideApproval)
- .ifPresent(messages::add);
- }
-
- if (messages.isEmpty()) {
+ if (appliedOverrideApprovals.isEmpty()) {
return Optional.empty();
}
- return Optional.of(Joiner.on("\n\n").join(messages));
+
+ try (Timer0.Context ctx = codeOwnerMetrics.extendChangeMessageOnPostReview.start()) {
+ List<String> messages = new ArrayList<>();
+ appliedOverrideApprovals.forEach(
+ overrideApproval ->
+ buildMessageForCodeOwnerOverride(
+ user, patchSet, oldApprovals, approvals, overrideApproval)
+ .ifPresent(messages::add));
+ if (messages.isEmpty()) {
+ return Optional.empty();
+ }
+ return Optional.of(Joiner.on("\n\n").join(messages));
+ }
}
private Optional<String> buildMessageForCodeOwnerOverride(
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index 5d523a6..22de56d 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -29,11 +29,13 @@
@Singleton
public class CodeOwnerMetrics {
// latency metrics
+ public final Timer0 addChangeMessageOnAddReviewer;
public final Timer0 computeChangedFilesAgainstAutoMerge;
public final Timer0 computeChangedFilesAgainstFirstParent;
public final Timer0 computeFileStatus;
public final Timer0 computeFileStatuses;
public final Timer0 computeOwnedPaths;
+ public final Timer0 extendChangeMessageOnPostReview;
public final Timer0 prepareFileStatusComputation;
public final Timer0 prepareFileStatusComputationForAccount;
public final Timer0 resolveCodeOwnerConfig;
@@ -61,6 +63,11 @@
this.metricMaker = metricMaker;
// latency metrics
+ this.addChangeMessageOnAddReviewer =
+ createLatencyTimer(
+ "add_change_message_on_add_reviewer",
+ "Latency for adding a change message with the owned path when a code owner is added as"
+ + " a reviewer");
this.computeChangedFilesAgainstAutoMerge =
createLatencyTimer(
"compute_changed_files_against_auto_merge",
@@ -80,6 +87,11 @@
createLatencyTimer(
"compute_owned_paths",
"Latency for computing the files in a change that are owned by a user");
+ this.extendChangeMessageOnPostReview =
+ createLatencyTimer(
+ "extend_change_message_on_post_review",
+ "Latency for extending the change message with the owned path when a code owner"
+ + " approval is applied");
this.prepareFileStatusComputation =
createLatencyTimer(
"prepare_file_status_computation", "Latency for preparing the file status computation");
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 8e6ee6d..b4512f2 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -5,6 +5,9 @@
## <a id="latencyMetrics"> Latency Metrics
+* `add_change_message_on_add_reviewer`:
+ Latency for adding a change message with the owned path when a code owner is
+ added as a reviewer.
* `compute_changed_files_against_auto_merge`:
Latency for computing changed files against auto merge.
* `compute_changed_files_against_first_parent`:
@@ -17,6 +20,9 @@
Latency for computing file statuses.
* `compute_owned_paths`:
Latency for computing the files in a change that are owned by a user.
+* `extend_change_message_on_post_review`:
+ Latency for extending the change message with the owned path when a code owner
+ approval is applied.
* `prepare_file_status_computation`:
Latency for preparing the file status computation.
* `prepare_file_status_computation_for_account`:
