Merge changes Ic887a27f,I421c99fb
* changes:
Stop resolving all users by default when listing code owners
Add metrics to record latency for adding/extending change messages
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/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 6fb8c15..0644b4c 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -86,7 +86,7 @@
private int limit = DEFAULT_LIMIT;
private Optional<Long> seed = Optional.empty();
- private boolean resolveAllUsers = true;
+ private boolean resolveAllUsers;
@Option(
name = "-o",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
index 0826b9e..743bef0 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -631,7 +631,8 @@
public void getAllUsersAsGlobalCodeOwners() throws Exception {
TestAccount user2 = accountCreator.user2();
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -640,7 +641,9 @@
// Query code owners with a limit.
requestScopeOperations.setApiUser(user.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(2), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(2);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -932,7 +935,8 @@
.addCodeOwnerEmail("*")
.create();
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -941,7 +945,9 @@
// Query code owners with a limit.
requestScopeOperations.setApiUser(user.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(2), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(2);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -975,7 +981,8 @@
// user can only see itself
requestScopeOperations.setApiUser(user.id());
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -984,7 +991,8 @@
// user2 can see user3 and itself
requestScopeOperations.setApiUser(user2.id());
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -993,7 +1001,8 @@
// admin can see all users
requestScopeOperations.setApiUser(admin.id());
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1002,7 +1011,9 @@
// Query code owners with a limit, user2 can see user3 and itself
requestScopeOperations.setApiUser(user2.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(1), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(1);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -1039,7 +1050,8 @@
// user can only see itself, user2 (because user is owner of a group that contains user2) and
// user3 (because user3 is member of a group that is visible to all users)
requestScopeOperations.setApiUser(user.id());
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1048,7 +1060,8 @@
// user2 can see user3 and itself
requestScopeOperations.setApiUser(user2.id());
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1057,7 +1070,8 @@
// admin can see all users
requestScopeOperations.setApiUser(admin.id());
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1066,7 +1080,9 @@
// Query code owners with a limit, user2 can see user3 and itself
requestScopeOperations.setApiUser(user2.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(1), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(1);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -1115,7 +1131,8 @@
// Since accounts.visibility = NONE, no account is visible and hence the list of code owners is
// empty.
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty();
assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue();
@@ -1126,7 +1143,8 @@
.update();
// If VIEW_ALL_ACCOUNTS is assigned, all accounts are visible now.
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1182,14 +1200,17 @@
// Query code owners with limits, "*" is resolved to random users, but user2 should always be
// included since this user is set explicitly as code owner
CodeOwnersInfo codeOwnersInfo =
- queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(2), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(2);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
.contains(user2.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(1), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1252,7 +1273,8 @@
long seed = (new Random()).nextLong();
CodeOwnersInfo codeOwnersInfo =
- queryCodeOwners(getCodeOwnersApi().query().withSeed(seed), "/foo/bar/baz.md");
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withSeed(seed), "/foo/bar/baz.md");
// all code owners have the same score, hence their order is random
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -1265,7 +1287,10 @@
.map(info -> Account.id(info.account._accountId))
.collect(toList());
for (int i = 0; i < 10; i++) {
- assertThat(queryCodeOwners(getCodeOwnersApi().query().withSeed(seed), "/foo/bar/baz.md"))
+ assertThat(
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withSeed(seed),
+ "/foo/bar/baz.md"))
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
.containsExactlyElementsIn(expectedAccountIds)
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`:
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index b977f3c..9d4d3a8 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -458,7 +458,7 @@
| `O` | optional | [Account option](../../../Documentation/rest-api-accounts.html#query-options) in hex. For the explanation see `o` parameter.
| `limit`\|`n` | optional | Limit defining how many code owners should be returned at most. By default 10.
| `seed` | optional | Seed, as a long value, that should be used to shuffle code owners that have the same score. Can be used to make the sort order stable across several requests, e.g. to get the same set of random code owners for different file paths that have the same code owners. Important: the sort order is only stable if the requests use the same seed **and** the same limit. In addition, the sort order is not guaranteed to be stable if new accounts are created in between the requests, or if the account visibility is changed.
-| `resolve-all-users` | optional | Whether code ownerships that are assigned to all users should be resolved to random users. If not set, `true` by default. Also see the [sorting example](#sortingExample) below to see how this parameter affects the returned code owners.
+| `resolve-all-users` | optional | Whether code ownerships that are assigned to all users should be resolved to random users. If not set, `false` by default. Also see the [sorting example](#sortingExample) below to see how this parameter affects the returned code owners.
| `revision` | optional | Revision from which the code owner configs should be read as commit SHA1. Can be used to read historic code owners from this branch, but imports from other branches or repositories as well as default and global code owners from `refs/meta/config` are still read from the current revisions. If not specified the code owner configs are read from the HEAD revision of the branch. Not supported for getting code owners for a path in a change.
As a response a [CodeOwnersInfo](#code-owners-info) entity is returned that
