Add a metric to count rebase requests
We want to know how often changes are rebased, how often rebase on
behalf of the uploader is done and how often chains are rebased.
This data is useful to decide:
* How important is it to support rebase on behalf of the uploader when
rebasing chains (how often is rebase on behalf of the uploader used
for rebasing single changes vs. how often are chains rebased).
* Can we disallow rebasing on behalf of the rebaser, aka rebases where
the rebaser takes over the change (number of rebases that are not done
on behalf of the uploader, e.g. number of rebases where the user
explicitly deselects 'on behalf of the uploader', once this option is
added in the UI and set by default).
Release-Notes: skip
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I70ca4ebab08f030b560e18814be64c35d0129b94
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index 6923129..e6494e6 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -199,6 +199,11 @@
=== Change
+* `change/count_rebases`: Total number of rebases
+** `on_behalf_of_uploader`:
+ Whether the rebase was done on behalf of the uploader.
+** `rebase_chain`:
+ Whether a chain was rebased.
* `change/submitted_with_rebaser_approval`: Number of rebased changes that were
submitted with a Code-Review approval of the rebaser that would not have been
submittable if the rebase was not done on behalf of the uploader.
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index fd51fbc..5368c75 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -84,6 +84,7 @@
private final PatchSetUtil patchSetUtil;
private final IdentifiedUser.GenericFactory userFactory;
private final ChangeResource.Factory changeResourceFactory;
+ private final RebaseMetrics rebaseMetrics;
@Inject
public Rebase(
@@ -96,7 +97,8 @@
ProjectCache projectCache,
PatchSetUtil patchSetUtil,
IdentifiedUser.GenericFactory userFactory,
- ChangeResource.Factory changeResourceFactory) {
+ ChangeResource.Factory changeResourceFactory,
+ RebaseMetrics rebaseMetrics) {
this.serverIdent = serverIdent;
this.updateFactory = updateFactory;
this.repoManager = repoManager;
@@ -107,6 +109,7 @@
this.patchSetUtil = patchSetUtil;
this.userFactory = userFactory;
this.changeResourceFactory = changeResourceFactory;
+ this.rebaseMetrics = rebaseMetrics;
}
@Override
@@ -147,6 +150,8 @@
bu.addOp(change.getId(), rebaseOp);
bu.execute();
+ rebaseMetrics.countRebase(input.onBehalfOfUploader);
+
ChangeInfo changeInfo = json.create(OPTIONS).format(change.getProject(), change.getId());
changeInfo.containsGitConflicts =
!rebaseOp.getRebasedCommit().getFilesWithGitConflicts().isEmpty() ? true : null;
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 34a2623..1949c89 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -86,6 +86,7 @@
private final ProjectCache projectCache;
private final PatchSetUtil patchSetUtil;
private final ChangeJson.Factory json;
+ private final RebaseMetrics rebaseMetrics;
@Inject
RebaseChain(
@@ -99,7 +100,8 @@
ChangeNotes.Factory notesFactory,
ProjectCache projectCache,
PatchSetUtil patchSetUtil,
- ChangeJson.Factory json) {
+ ChangeJson.Factory json,
+ RebaseMetrics rebaseMetrics) {
this.repoManager = repoManager;
this.getRelatedChangesUtil = getRelatedChangesUtil;
this.changeDataFactory = changeDataFactory;
@@ -111,6 +113,7 @@
this.projectCache = projectCache;
this.patchSetUtil = patchSetUtil;
this.json = json;
+ this.rebaseMetrics = rebaseMetrics;
}
@Override
@@ -194,6 +197,8 @@
}
}
+ rebaseMetrics.countRebaseChain(input.onBehalfOfUploader);
+
RebaseChainInfo res = new RebaseChainInfo();
res.rebasedChanges = new ArrayList<>();
ChangeJson changeJson = json.create(OPTIONS);
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java b/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java
new file mode 100644
index 0000000..114a112
--- /dev/null
+++ b/java/com/google/gerrit/server/restapi/change/RebaseMetrics.java
@@ -0,0 +1,54 @@
+// Copyright (C) 2023 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.server.restapi.change;
+
+import com.google.gerrit.metrics.Counter2;
+import com.google.gerrit.metrics.Description;
+import com.google.gerrit.metrics.Field;
+import com.google.gerrit.metrics.MetricMaker;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+/** Metrics for the rebase REST endpoints ({@link Rebase} and {@link RebaseChain}). */
+@Singleton
+public class RebaseMetrics {
+ private final Counter2<Boolean, Boolean> countRebases;
+
+ @Inject
+ public RebaseMetrics(MetricMaker metricMaker) {
+ this.countRebases =
+ metricMaker.newCounter(
+ "change/count_rebases",
+ new Description("Total number of rebases").setRate(),
+ Field.ofBoolean("on_behalf_of_uploader", (metadataBuilder, isOnBehalfOfUploader) -> {})
+ .description("Whether the rebase was done on behalf of the uploader.")
+ .build(),
+ Field.ofBoolean("rebase_chain", (metadataBuilder, isRebaseChain) -> {})
+ .description("Whether a chain was rebased.")
+ .build());
+ }
+
+ public void countRebase(boolean isOnBehalfOfUploader) {
+ countRebase(isOnBehalfOfUploader, /* isRebaseChain= */ false);
+ }
+
+ public void countRebaseChain(boolean isOnBehalfOfUploader) {
+ countRebase(isOnBehalfOfUploader, /* isRebaseChain= */ true);
+ }
+
+ private void countRebase(boolean isOnBehalfOfUploader, boolean isRebaseChain) {
+ countRebases.increment(/* field1= */ isOnBehalfOfUploader, /* field2= */ isRebaseChain);
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
index b1fb575..3531234 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
@@ -35,6 +35,7 @@
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestMetricMaker;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.RawInputUtil;
@@ -89,6 +90,7 @@
@Inject protected RequestScopeOperations requestScopeOperations;
@Inject protected ProjectOperations projectOperations;
@Inject protected ExtensionRegistry extensionRegistry;
+ @Inject protected TestMetricMaker testMetricMaker;
@FunctionalInterface
protected interface RebaseCall {
@@ -771,6 +773,28 @@
assertThat(gApi.changes().id(id3.get()).revision(ri3._number).related().changes).isEmpty();
}
+
+ @Test
+ public void testCountRebasesMetric() throws Exception {
+ // Create two changes both with the same parent
+ PushOneCommit.Result r = createChange();
+ testRepo.reset("HEAD~1");
+ PushOneCommit.Result r2 = createChange();
+
+ // Approve and submit the first change
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+ revision.review(ReviewInput.approve());
+ revision.submit();
+
+ // Rebase the second change
+ testMetricMaker.reset();
+ rebaseCallWithInput.call(r2.getChangeId(), new RebaseInput());
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(0);
+ }
}
public static class RebaseViaRevisionApi extends Rebase {
@@ -1125,6 +1149,36 @@
assertThat(thrown).hasMessageThat().contains("recursion not allowed");
}
+ @Test
+ public void testCountRebasesMetric() throws Exception {
+ // Create changes with the following hierarchy:
+ // * HEAD
+ // * r1
+ // * r2
+ // * r3
+ // * r4
+ PushOneCommit.Result r = createChange();
+ testRepo.reset("HEAD~1");
+ PushOneCommit.Result r2 = createChange();
+ PushOneCommit.Result r3 = createChange();
+ PushOneCommit.Result r4 = createChange();
+
+ // Approve and submit the first change
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+ revision.review(ReviewInput.approve());
+ revision.submit();
+
+ // Rebase the chain.
+ testMetricMaker.reset();
+ verifyRebaseChainResponse(
+ gApi.changes().id(r4.getChangeId()).rebaseChain(), false, r2, r3, r4);
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(0);
+ }
+
private void verifyRebaseChainResponse(
Response<RebaseChainInfo> res,
boolean shouldHaveConflicts,
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
index a1d3c14..5a5402b 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
@@ -1079,6 +1079,39 @@
assertThat(testMetricMaker.getCount("change/submitted_with_rebaser_approval")).isEqualTo(1);
}
+ @Test
+ public void testCountRebasesMetric() throws Exception {
+ allowPermissionToAllUsers(Permission.REBASE);
+
+ Account.Id uploader = accountOperations.newAccount().create();
+ Account.Id approver = admin.id();
+ Account.Id rebaser = accountOperations.newAccount().create();
+
+ // Create two changes both with the same parent
+ requestScopeOperations.setApiUser(uploader);
+ Change.Id changeToBeTheNewBase =
+ changeOperations.newChange().project(project).owner(uploader).create();
+ Change.Id changeToBeRebased =
+ changeOperations.newChange().project(project).owner(uploader).create();
+
+ // Approve and submit the change that will be the new base for the change that will be rebased.
+ requestScopeOperations.setApiUser(approver);
+ gApi.changes().id(changeToBeTheNewBase.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
+
+ // Rebase it on behalf of the uploader
+ testMetricMaker.reset();
+ requestScopeOperations.setApiUser(rebaser);
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.onBehalfOfUploader = true;
+ gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
+ // field1 is on_behalf_of_uploader, field2 is rebase_chain
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, false)).isEqualTo(1);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, false)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", true, true)).isEqualTo(0);
+ assertThat(testMetricMaker.getCount("change/count_rebases", false, true)).isEqualTo(0);
+ }
+
private void allowPermissionToAllUsers(String permission) {
allowPermission(permission, REGISTERED_USERS);
}
