Add buckets for reject count metric
Change I3649ae187 added a metric to count rejected pushes. This metric
recorded the rejection message as reason, but the messages contain
resource IDs such as change numbers, change URLs, branch names, account
identifiers, label names etc. so that there are too many different
reasons and the metric is not that useful. Fix this by defining a
constant number of metric buckets.
Bug: Google b/151127672
Release-Notes: skip
Change-Id: Ib8003a655ae208b5715121afcf963d92081a67e3
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 15f566b..0c3fb8b 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -122,6 +122,7 @@
import com.google.gerrit.server.RequestListener;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.cancellation.RequestCancelledException;
import com.google.gerrit.server.cancellation.RequestStateContext;
@@ -146,6 +147,7 @@
import com.google.gerrit.server.git.ReceivePackInitializer;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.ValidationError;
+import com.google.gerrit.server.git.receive.RejectionReason.MetricBucket;
import com.google.gerrit.server.git.validators.CommentCountValidator;
import com.google.gerrit.server.git.validators.CommentSizeValidator;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
@@ -708,9 +710,12 @@
.addRequestStateProvider(
deadlineCheckerFactory.create(start, requestInfo, clientProvidedDeadlineValue))) {
processCommandsUnsafe(commands, progress);
- rejectRemaining(commands, INTERNAL_SERVER_ERROR);
+ rejectRemaining(
+ commands,
+ RejectionReason.create(MetricBucket.INTERNAL_SERVER_ERROR, INTERNAL_SERVER_ERROR));
} catch (InvalidDeadlineException e) {
- rejectRemaining(commands, e.getMessage());
+ rejectRemaining(
+ commands, RejectionReason.create(MetricBucket.INVALID_DEADLINE, e.getMessage()));
} catch (RuntimeException e) {
Optional<RequestCancelledException> requestCancelledException =
RequestCancelledException.getFromCausalChain(e);
@@ -726,7 +731,8 @@
String.format(
" (%s)", requestCancelledException.get().getCancellationMessage().get()));
}
- rejectRemaining(commands, msg.toString());
+ rejectRemaining(
+ commands, RejectionReason.create(MetricBucket.REQUEST_CANCELLED, msg.toString()));
}
// This sends error messages before the 'done' string of the progress monitor is sent.
@@ -752,7 +758,11 @@
if (!projectState.getProject().getState().permitsWrite()) {
for (ReceiveCommand cmd : commands) {
- reject(cmd, "prohibited by Gerrit: project state does not permit write");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.PROJECT_NOT_WRITABLE,
+ "prohibited by Gerrit: project state does not permit write"));
}
return;
}
@@ -774,7 +784,11 @@
}
if (!magicCommands.isEmpty() && !regularCommands.isEmpty()) {
- rejectRemaining(commands, "cannot combine normal pushes and magic pushes");
+ rejectRemaining(
+ commands,
+ RejectionReason.create(
+ MetricBucket.CANNOT_COMBINE_NORMAL_AND_MAGIC_PUSHES,
+ "cannot combine normal pushes and magic pushes"));
return;
}
@@ -807,7 +821,8 @@
if (first) {
first = false;
} else {
- reject(cmd, "duplicate request");
+ reject(
+ cmd, RejectionReason.create(MetricBucket.DUPLICATE_REQUEST, "duplicate request"));
}
}
} catch (PermissionBackendException | NoSuchProjectException | IOException err) {
@@ -1096,10 +1111,15 @@
}
} catch (ResourceConflictException e) {
addError(e.getMessage());
- reject(magicBranchCmd, "conflict");
+ reject(magicBranchCmd, RejectionReason.create(MetricBucket.CONFLICT, "conflict"));
+ } catch (UnresolvableAccountException e) {
+ logger.atFine().log("Rejecting because account cannot be resolved: %s", e.getMessage());
+ reject(
+ magicBranchCmd,
+ RejectionReason.create(MetricBucket.ACCOUNT_NOT_FOUND, e.getMessage()));
} catch (BadRequestException | UnprocessableEntityException | AuthException e) {
logger.atFine().withCause(e).log("Rejecting due to client error");
- reject(magicBranchCmd, e.getMessage());
+ reject(magicBranchCmd, RejectionReason.create(MetricBucket.CLIENT_ERROR, e.getMessage()));
} catch (RestApiException | IOException | UpdateException e) {
throw new StorageException("Can't insert change/patch set for " + project.getName(), e);
}
@@ -1109,7 +1129,7 @@
submit(newChanges, replaceByChange.values());
} catch (ResourceConflictException e) {
addError(e.getMessage());
- reject(magicBranchCmd, "conflict");
+ reject(magicBranchCmd, RejectionReason.create(MetricBucket.CONFLICT, "conflict"));
} catch (RestApiException
| StorageException
| UpdateException
@@ -1117,7 +1137,9 @@
| ConfigInvalidException
| PermissionBackendException e) {
logger.atSevere().withCause(e).log("Error submitting changes to %s", project.getName());
- reject(magicBranchCmd, "error during submit");
+ reject(
+ magicBranchCmd,
+ RejectionReason.create(MetricBucket.SUBMIT_ERROR, "error during submit"));
}
}
}
@@ -1194,8 +1216,8 @@
magicBranchCmd.setResult(OK);
}
for (ReplaceRequest replace : replaceByChange.values()) {
- String rejectMessage = replace.getRejectMessage();
- if (rejectMessage == null) {
+ Optional<RejectionReason> rejectionReason = replace.getRejectionReason();
+ if (!rejectionReason.isPresent()) {
if (replace.inputCommand.getResult() == NOT_ATTEMPTED) {
// Not necessarily the magic branch, so need to set OK on the original
// value.
@@ -1203,7 +1225,7 @@
}
} else {
logger.atFine().log("Rejecting due to message from ReplaceOp");
- reject(replace.inputCommand, rejectMessage);
+ reject(replace.inputCommand, rejectionReason.get());
}
}
}
@@ -1308,7 +1330,7 @@
}
if (!Repository.isValidRefName(cmd.getRefName()) || cmd.getRefName().contains("//")) {
- reject(cmd, "not valid ref");
+ reject(cmd, RejectionReason.create(MetricBucket.INVALID_REF, "not valid ref"));
return;
}
if (RefNames.isNoteDbMetaRef(cmd.getRefName())) {
@@ -1326,14 +1348,20 @@
// NoteDb refs.
reject(
cmd,
- "NoteDb update requires -o "
- + NoteDbPushOption.OPTION_NAME
- + "="
- + NoteDbPushOption.ALLOW.value());
+ RejectionReason.create(
+ MetricBucket.NOTEDB_UPDATE_WITHOUT_ALLOW_OPTION,
+ "NoteDb update requires -o "
+ + NoteDbPushOption.OPTION_NAME
+ + "="
+ + NoteDbPushOption.ALLOW.value()));
return;
}
if (!permissionBackend.user(user).test(GlobalPermission.ACCESS_DATABASE)) {
- reject(cmd, "NoteDb update requires access database permission");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.NOTEDB_UPDATE_WITHOUT_ACCESS_DATABASE_PERMISSION,
+ "NoteDb update requires access database permission"));
return;
}
}
@@ -1356,7 +1384,11 @@
break;
default:
- reject(cmd, "prohibited by Gerrit: unknown command type " + cmd.getType());
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.UNKNOWN_COMMAND_TYPE,
+ "prohibited by Gerrit: unknown command type " + cmd.getType()));
return;
}
@@ -1378,9 +1410,11 @@
if (!permissions.test(ProjectPermission.WRITE_CONFIG)) {
reject(
cmd,
- String.format(
- "must be either project owner or have %s permission",
- ProjectPermission.WRITE_CONFIG.describeForException()));
+ RejectionReason.create(
+ MetricBucket.PROJECT_CONFIG_UPDATE_NOT_ALLOWED,
+ String.format(
+ "must be either project owner or have %s permission",
+ ProjectPermission.WRITE_CONFIG.describeForException())));
return;
}
@@ -1396,7 +1430,11 @@
for (ValidationError err : cfg.getValidationErrors()) {
addError(" " + err.getMessage());
}
- reject(cmd, "invalid project configuration");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.INVALID_PROJECT_CONFIGURATION_UPDATE,
+ "invalid project configuration"));
logger.atSevere().log(
"User %s tried to push invalid project configuration %s for %s",
user.getLoggableName(), cmd.getNewId().name(), project.getName());
@@ -1407,7 +1445,11 @@
if (oldParent == null) {
// update of the 'All-Projects' project
if (newParent != null) {
- reject(cmd, "invalid project configuration: root project cannot have parent");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.INVALID_PROJECT_CONFIGURATION_UPDATE,
+ "invalid project configuration: root project cannot have parent"));
return;
}
} else {
@@ -1418,25 +1460,40 @@
.project(project.getNameKey())
.test(ProjectPermission.WRITE_CONFIG)) {
reject(
- cmd, "invalid project configuration: only project owners can set parent");
+ cmd,
+ RejectionReason.create(
+ MetricBucket.PROJECT_CONFIG_UPDATE_NOT_ALLOWED,
+ "invalid project configuration: only project owners can set parent"));
return;
}
} else {
if (!permissionBackend.user(user).test(GlobalPermission.ADMINISTRATE_SERVER)) {
- reject(cmd, "invalid project configuration: only Gerrit admin can set parent");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.PROJECT_CONFIG_UPDATE_NOT_ALLOWED,
+ "invalid project configuration: only Gerrit admin can set parent"));
return;
}
}
}
if (!projectCache.get(newParent).isPresent()) {
- reject(cmd, "invalid project configuration: parent does not exist");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.INVALID_PROJECT_CONFIGURATION_UPDATE,
+ "invalid project configuration: parent does not exist"));
return;
}
}
validatePluginConfig(cmd, cfg);
} catch (Exception e) {
- reject(cmd, "invalid project configuration");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.INVALID_PROJECT_CONFIGURATION_UPDATE,
+ "invalid project configuration"));
logger.atSevere().withCause(e).log(
"User %s tried to push invalid project configuration %s for %s",
user.getLoggableName(), cmd.getNewId().name(), project.getName());
@@ -1450,8 +1507,10 @@
default:
reject(
cmd,
- "prohibited by Gerrit: don't know how to handle config update of type "
- + cmd.getType());
+ RejectionReason.create(
+ MetricBucket.UNKNOWN_COMMAND_TYPE,
+ "prohibited by Gerrit: don't know how to handle config update of type "
+ + cmd.getType()));
}
}
}
@@ -1480,10 +1539,12 @@
&& !configEntry.isEditable(projectState)) {
reject(
cmd,
- String.format(
- "invalid project configuration: Not allowed to set parameter"
- + " '%s' of plugin '%s' on project '%s'.",
- e.getExportName(), e.getPluginName(), project.getName()));
+ RejectionReason.create(
+ MetricBucket.INVALID_PROJECT_CONFIGURATION_UPDATE,
+ String.format(
+ "invalid project configuration: Not allowed to set parameter"
+ + " '%s' of plugin '%s' on project '%s'.",
+ e.getExportName(), e.getPluginName(), project.getName())));
continue;
}
@@ -1492,10 +1553,12 @@
&& !configEntry.getPermittedValues().contains(value)) {
reject(
cmd,
- String.format(
- "invalid project configuration: The value '%s' is "
- + "not permitted for parameter '%s' of plugin '%s'.",
- value, e.getExportName(), e.getPluginName()));
+ RejectionReason.create(
+ MetricBucket.INVALID_PROJECT_CONFIGURATION_UPDATE,
+ String.format(
+ "invalid project configuration: The value '%s' is "
+ + "not permitted for parameter '%s' of plugin '%s'.",
+ value, e.getExportName(), e.getPluginName())));
}
}
}
@@ -1506,7 +1569,10 @@
if (repo.resolve(cmd.getRefName()) != null) {
reject(
cmd,
- String.format("Cannot create ref '%s' because it already exists.", cmd.getRefName()));
+ RejectionReason.create(
+ MetricBucket.CANNOT_CREATE_REF_BECAUSE_IT_ALREADY_EXISTS,
+ String.format(
+ "Cannot create ref '%s' because it already exists.", cmd.getRefName())));
return;
}
RevObject obj;
@@ -1534,7 +1600,10 @@
rejectProhibited(cmd, denied);
return;
} catch (ResourceConflictException denied) {
- reject(cmd, "prohibited by Gerrit: " + denied.getMessage());
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.CONFLICT, "prohibited by Gerrit: " + denied.getMessage()));
return;
}
@@ -1552,7 +1621,8 @@
Optional<AuthException> err = checkRefPermission(cmd, RefPermission.UPDATE);
if (!err.isPresent()) {
if (isHead(cmd) && !isCommit(globalRevWalk, cmd)) {
- reject(cmd, "head must point to commit");
+ reject(
+ cmd, RejectionReason.create(MetricBucket.INVALID_HEAD, "head must point to commit"));
return;
}
if (validRefOperation(cmd)) {
@@ -1582,7 +1652,7 @@
if (obj instanceof RevCommit) {
return true;
}
- reject(cmd, "not a commit");
+ reject(cmd, RejectionReason.create(MetricBucket.NOT_A_COMMIT, "not a commit"));
return false;
}
@@ -1591,10 +1661,16 @@
logger.atFine().log("Deleting %s", cmd);
if (cmd.getRefName().startsWith(REFS_CHANGES)) {
errors.put(CANNOT_DELETE_CHANGES, cmd.getRefName());
- reject(cmd, "cannot delete changes");
+ reject(
+ cmd,
+ RejectionReason.create(MetricBucket.CANNOT_DELETE_CHANGES, "cannot delete changes"));
} else if (isConfigRef(cmd.getRefName())) {
errors.put(CANNOT_DELETE_CONFIG, cmd.getRefName());
- reject(cmd, "cannot delete project configuration");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.CANNOT_DELETE_PROJECT_CONFIGURATION,
+ "cannot delete project configuration"));
}
Optional<AuthException> err = checkRefPermission(cmd, RefPermission.DELETE);
@@ -1611,7 +1687,11 @@
// here.
// Without this check, such delete always fails with the "internal error" message, caused
// by the checkArgument in the ChainedReceiveCommands#add.
- reject(cmd, String.format("The ref %s doesn't exist", cmd.getRefName()));
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.REF_NOT_FOUND,
+ String.format("The ref %s doesn't exist", cmd.getRefName())));
}
}
}
@@ -1662,7 +1742,7 @@
private void rejectProhibited(ReceiveCommand cmd, AuthException err) {
err.getAdvice().ifPresent(a -> errors.put(a, cmd.getRefName()));
- reject(cmd, prohibited(err, cmd.getRefName()));
+ reject(cmd, RejectionReason.create(MetricBucket.PROHIBITED, prohibited(err, cmd.getRefName())));
}
private static String prohibited(AuthException e, String alreadyDisplayedResource) {
@@ -2026,7 +2106,7 @@
} catch (CmdLineException e) {
if (!magicBranch.cmdLineParser.wasHelpRequestedByOption()) {
logger.atFine().log("Invalid branch syntax");
- reject(cmd, e.getMessage());
+ reject(cmd, RejectionReason.create(MetricBucket.INVALID_BRANCH_SYNTAX, e.getMessage()));
return;
}
ref = null; // never happens
@@ -2035,14 +2115,20 @@
if (magicBranch.skipValidation) {
reject(
cmd,
- String.format(
- "\"--%s\" option is only supported for direct push", PUSH_OPTION_SKIP_VALIDATION));
+ RejectionReason.create(
+ MetricBucket.CANNOT_SKIP_VALIDATION_FOR_MAGIC_PUSH,
+ String.format(
+ "\"--%s\" option is only supported for direct push",
+ PUSH_OPTION_SKIP_VALIDATION)));
return;
}
if (magicBranch.topic != null && magicBranch.topic.length() > ChangeUtil.TOPIC_MAX_LENGTH) {
reject(
- cmd, String.format("topic length exceeds the limit (%d)", ChangeUtil.TOPIC_MAX_LENGTH));
+ cmd,
+ RejectionReason.create(
+ MetricBucket.TOPIC_TOO_LARGE,
+ String.format("topic length exceeds the limit (%d)", ChangeUtil.TOPIC_MAX_LENGTH)));
}
if (magicBranch.cmdLineParser.wasHelpRequestedByOption()) {
@@ -2064,7 +2150,7 @@
}
addMessage(w.toString());
- reject(cmd, "see help");
+ reject(cmd, RejectionReason.create(MetricBucket.HELP_REQUESTED, "see help"));
return;
}
if (projectState.isAllUsers() && RefNames.REFS_USERS_SELF.equals(ref)) {
@@ -2082,9 +2168,11 @@
logger.atFine().log("Ref %s not found", ref);
if (ref.startsWith(Constants.R_HEADS)) {
String n = ref.substring(Constants.R_HEADS.length());
- reject(cmd, "branch " + n + " not found");
+ reject(
+ cmd,
+ RejectionReason.create(MetricBucket.BRANCH_NOT_FOUND, "branch " + n + " not found"));
} else {
- reject(cmd, ref + " not found");
+ reject(cmd, RejectionReason.create(MetricBucket.REF_NOT_FOUND, ref + " not found"));
}
return;
}
@@ -2102,7 +2190,11 @@
}
if (magicBranch.isPrivate && magicBranch.removePrivate) {
- reject(cmd, "the options 'private' and 'remove-private' are mutually exclusive");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.INVALID_OPTION,
+ "the options 'private' and 'remove-private' are mutually exclusive"));
return;
}
@@ -2115,17 +2207,26 @@
magicBranch.isPrivate || (privateByDefault && !magicBranch.removePrivate);
if (receiveConfig.disablePrivateChanges && setChangeAsPrivate) {
- reject(cmd, "private changes are disabled");
+ reject(
+ cmd,
+ RejectionReason.create(MetricBucket.INVALID_OPTION, "private changes are disabled"));
return;
}
if (magicBranch.workInProgress && magicBranch.ready) {
- reject(cmd, "the options 'wip' and 'ready' are mutually exclusive");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.INVALID_OPTION,
+ "the options 'wip' and 'ready' are mutually exclusive"));
return;
}
if (magicBranch.publishComments && magicBranch.noPublishComments) {
reject(
- cmd, "the options 'publish-comments' and 'no-publish-comments' are mutually exclusive");
+ cmd,
+ RejectionReason.create(
+ MetricBucket.INVALID_OPTION,
+ "the options 'publish-comments' and 'no-publish-comments' are mutually exclusive"));
return;
}
@@ -2152,17 +2253,25 @@
try {
if (magicBranch.merged) {
if (magicBranch.base != null) {
- reject(cmd, "cannot use merged with base");
+ reject(
+ cmd,
+ RejectionReason.create(MetricBucket.INVALID_OPTION, "cannot use merged with base"));
return;
}
Ref refTip = receivePackRefCache.exactRef(magicBranch.dest.branch());
if (refTip == null) {
- reject(cmd, magicBranch.dest.branch() + " not found");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.BRANCH_NOT_FOUND, magicBranch.dest.branch() + " not found"));
return;
}
RevCommit branchTip = globalRevWalk.parseCommit(refTip.getObjectId());
if (!globalRevWalk.isMergedInto(tip, branchTip)) {
- reject(cmd, "not merged into branch");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.NOT_MERGED_INTO_BRANCH, "not merged into branch"));
return;
}
}
@@ -2184,10 +2293,11 @@
try {
magicBranch.baseCommit.add(globalRevWalk.parseCommit(id));
} catch (IncorrectObjectTypeException notCommit) {
- reject(cmd, "base must be a commit");
+ reject(
+ cmd, RejectionReason.create(MetricBucket.INVALID_BASE, "base must be a commit"));
return;
} catch (MissingObjectException e) {
- reject(cmd, "base not found");
+ reject(cmd, RejectionReason.create(MetricBucket.INVALID_BASE, "base not found"));
return;
} catch (IOException e) {
throw new StorageException(
@@ -2209,7 +2319,10 @@
// branch does not exist yet. This allows to push initial code for review to an empty
// repository and to review an initial project configuration.
if (!ref.equals(readHEAD(repo)) && !ref.equals(RefNames.REFS_CONFIG)) {
- reject(cmd, magicBranch.dest.branch() + " not found");
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.BRANCH_NOT_FOUND, magicBranch.dest.branch() + " not found"));
return;
}
}
@@ -2263,7 +2376,8 @@
globalRevWalk.markStart(tip);
globalRevWalk.markStart(h);
if (globalRevWalk.next() == null) {
- reject(cmd, "no common ancestry");
+ reject(
+ cmd, RejectionReason.create(MetricBucket.NO_COMMON_ANCESTRY, "no common ancestry"));
return false;
}
} finally {
@@ -2306,15 +2420,17 @@
if (change.isClosed()) {
reject(
cmd,
- changeFormatter.changeClosed(
- ChangeReportFormatter.Input.builder().setChange(change).build()));
+ RejectionReason.create(
+ MetricBucket.CHANGE_IS_CLOSED,
+ changeFormatter.changeClosed(
+ ChangeReportFormatter.Input.builder().setChange(change).build())));
return false;
}
ReplaceRequest req =
new ReplaceRequest(globalRevWalk, change.getId(), newCommit, cmd, checkMergedInto);
if (replaceByChange.containsKey(req.ontoChange)) {
- reject(cmd, "duplicate request");
+ reject(cmd, RejectionReason.create(MetricBucket.DUPLICATE_REQUEST, "duplicate request"));
return false;
}
@@ -2447,7 +2563,10 @@
logger.atFine().log("%d changes exceeds limit of %d", n, maxBatchChanges);
reject(
magicBranch.cmd,
- "the number of pushed changes in a batch exceeds the max limit " + maxBatchChanges);
+ RejectionReason.create(
+ MetricBucket.TOO_MANY_CHANGES,
+ "the number of pushed changes in a batch exceeds the max limit "
+ + maxBatchChanges));
return ImmutableList.of();
}
@@ -2490,8 +2609,10 @@
if (newChangeForAllNotInTarget && c.getParentCount() > 1) {
reject(
magicBranch.cmd,
- "Pushing merges in commit chains with 'all not in target' is not allowed,\n"
- + "to override please set the base manually");
+ RejectionReason.create(
+ MetricBucket.CANNOT_PUSH_MERGE_WITH_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
+ "Pushing merges in commit chains with 'all not in target' is not allowed,\n"
+ + "to override please set the base manually"));
logger.atFine().log("Rejecting merge commit %s with newChangeForAllNotInTarget", name);
// TODO(dborowitz): Should we early return here?
}
@@ -2519,7 +2640,10 @@
if (newChangeIds.contains(p.changeKey)) {
logger.atFine().log("Multiple commits with Change-Id %s", p.changeKey);
- reject(magicBranch.cmd, SAME_CHANGE_ID_IN_MULTIPLE_CHANGES);
+ reject(
+ magicBranch.cmd,
+ RejectionReason.create(
+ MetricBucket.DUPLICATE_CHANGE_ID, SAME_CHANGE_ID_IN_MULTIPLE_CHANGES));
return ImmutableList.of();
}
@@ -2535,7 +2659,10 @@
// a different Change-Id. In practice, we should never see
// this error message as Change-Id should be unique per branch.
//
- reject(magicBranch.cmd, p.changeKey.get() + " has duplicates");
+ reject(
+ magicBranch.cmd,
+ RejectionReason.create(
+ MetricBucket.DUPLICATE_CHANGE, p.changeKey.get() + " has duplicates"));
return ImmutableList.of();
}
@@ -2549,7 +2676,11 @@
if (pending.size() == 1) {
// There are no commits left to check, all commits in pending were already
// current PatchSet of the corresponding target changes.
- reject(magicBranch.cmd, "commit(s) already exists (as current patchset)");
+ reject(
+ magicBranch.cmd,
+ RejectionReason.create(
+ MetricBucket.COMMIT_ALREADY_EXISTS_IN_CHANGE,
+ "commit(s) already exists (as current patchset)"));
} else {
// Commit is already current PatchSet.
// Remove from pending and try next commit.
@@ -2566,7 +2697,9 @@
if (changes.isEmpty()) {
if (!isValidChangeId(p.changeKey.get())) {
- reject(magicBranch.cmd, "invalid Change-Id");
+ reject(
+ magicBranch.cmd,
+ RejectionReason.create(MetricBucket.INVALID_CHANGE_ID, "invalid Change-Id"));
return ImmutableList.of();
}
@@ -2574,7 +2707,11 @@
// double check against the existing refs
if (foundInExistingPatchSets(receivePackRefCache.patchSetIdsFromObjectId(p.commit))) {
if (pending.size() == 1) {
- reject(magicBranch.cmd, "commit(s) already exists (as current patchset)");
+ reject(
+ magicBranch.cmd,
+ RejectionReason.create(
+ MetricBucket.COMMIT_ALREADY_EXISTS_IN_CHANGE,
+ "commit(s) already exists (as current patchset)"));
return ImmutableList.of();
}
itr.remove();
@@ -2594,11 +2731,15 @@
}
if (newChanges.isEmpty() && replaceByChange.isEmpty()) {
- reject(magicBranch.cmd, "no new changes");
+ reject(
+ magicBranch.cmd, RejectionReason.create(MetricBucket.NO_NEW_CHANGES, "no new changes"));
return ImmutableList.of();
}
if (!newChanges.isEmpty() && magicBranch.edit) {
- reject(magicBranch.cmd, "edit is not supported for new changes");
+ reject(
+ magicBranch.cmd,
+ RejectionReason.create(
+ MetricBucket.CANNOT_EDIT_NEW_CHANGE, "edit is not supported for new changes"));
return ImmutableList.copyOf(newChanges);
}
@@ -2704,7 +2845,9 @@
+ c.getShortMessage(),
ValidationMessage.Type.ERROR));
}
- reject(magicBranch.cmd, "implicit merges detected");
+ reject(
+ magicBranch.cmd,
+ RejectionReason.create(MetricBucket.IMPLICIT_MERGE, "implicit merges detected"));
}
}
}
@@ -3063,7 +3206,10 @@
throws IOException, PermissionBackendException {
try (TraceTimer traceTimer = newTimer("validateNewPatchSetNoteDb")) {
if (notes == null) {
- reject(inputCommand, "change " + ontoChange + " not found");
+ reject(
+ inputCommand,
+ RejectionReason.create(
+ MetricBucket.CHANGE_NOT_FOUND, "change " + ontoChange + " not found"));
return false;
}
@@ -3081,7 +3227,10 @@
.limit(100) // Enough for "normal" changes.
.map(PatchSet.Id::getId)
.collect(Collectors.toList())));
- reject(inputCommand, "change " + ontoChange + " missing revisions");
+ reject(
+ inputCommand,
+ RejectionReason.create(
+ MetricBucket.MISSING_REVISION, "change " + ontoChange + " missing revisions"));
return false;
}
@@ -3089,24 +3238,36 @@
// Not allowed to create a new patch set if the current patch set is locked.
if (psUtil.isPatchSetLocked(notes)) {
- reject(inputCommand, "cannot add patch set to " + ontoChange + ".");
+ reject(
+ inputCommand,
+ RejectionReason.create(
+ MetricBucket.PATCH_SET_LOCKED, "cannot add patch set to " + ontoChange + "."));
return false;
}
if (!permissions.change(notes).test(ChangePermission.ADD_PATCH_SET)) {
- reject(inputCommand, "cannot add patch set to " + ontoChange + ".");
+ reject(
+ inputCommand,
+ RejectionReason.create(
+ MetricBucket.CANNOT_ADD_PATCH_SET,
+ "cannot add patch set to " + ontoChange + "."));
return false;
}
if (change.isClosed()) {
- reject(inputCommand, "change " + ontoChange + " closed");
+ reject(
+ inputCommand,
+ RejectionReason.create(
+ MetricBucket.CHANGE_IS_CLOSED, "change " + ontoChange + " closed"));
return false;
} else if (revisions.containsKey(newCommit)) {
reject(
inputCommand,
- String.format(
- "commit %s already exists in change %s",
- newCommit.name().substring(0, 10), change.getId()));
+ RejectionReason.create(
+ MetricBucket.COMMIT_ALREADY_EXISTS_IN_CHANGE,
+ String.format(
+ "commit %s already exists in change %s",
+ newCommit.name().substring(0, 10), change.getId())));
return false;
}
@@ -3117,8 +3278,10 @@
// without the option to turn that off.
reject(
inputCommand,
- "commit already exists (in the project): "
- + existingPatchSetsWithSameCommit.get(0).toRefName());
+ RejectionReason.create(
+ MetricBucket.COMMIT_ALREADY_EXISTS_IN_PROJECT,
+ "commit already exists (in the project): "
+ + existingPatchSetsWithSameCommit.get(0).toRefName()));
return false;
}
@@ -3128,7 +3291,10 @@
// very common error due to users making a new commit rather than
// amending when trying to address review comments.
if (globalRevWalk.isMergedInto(prior, newCommit)) {
- reject(inputCommand, SAME_CHANGE_ID_IN_MULTIPLE_CHANGES);
+ reject(
+ inputCommand,
+ RejectionReason.create(
+ MetricBucket.DUPLICATE_CHANGE_ID, SAME_CHANGE_ID_IN_MULTIPLE_CHANGES));
return false;
}
}
@@ -3146,7 +3312,11 @@
&& !user.getAccountId().equals(change.getOwner())) {
if (!permissions.test(ProjectPermission.WRITE_CONFIG)) {
if (!permissions.change(notes).test(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE)) {
- reject(inputCommand, ONLY_USERS_WITH_TOGGLE_WIP_STATE_PERM_CAN_MODIFY_WIP);
+ reject(
+ inputCommand,
+ RejectionReason.create(
+ MetricBucket.CANNOT_TOGGLE_WIP,
+ ONLY_USERS_WITH_TOGGLE_WIP_STATE_PERM_CAN_MODIFY_WIP));
}
}
}
@@ -3314,9 +3484,8 @@
}
}
- @Nullable
- String getRejectMessage() {
- return replaceOp != null ? replaceOp.getRejectMessage() : null;
+ Optional<RejectionReason> getRejectionReason() {
+ return replaceOp != null ? replaceOp.getRejectionReason() : Optional.empty();
}
Optional<String> getOutdatedApprovalsMessage() {
@@ -3411,7 +3580,7 @@
messages.addAll(refValidators.validateForRefOperation());
} catch (RefOperationValidationException e) {
messages.addAll(e.getMessages());
- reject(cmd, e.getMessage());
+ reject(cmd, RejectionReason.create(MetricBucket.REJECTED_BY_VALIDATOR, e.getMessage()));
return false;
}
@@ -3436,7 +3605,11 @@
&& pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION);
if (skipValidation) {
if (projectState.is(BooleanProjectConfig.USE_SIGNED_OFF_BY)) {
- reject(cmd, "requireSignedOffBy prevents option " + PUSH_OPTION_SKIP_VALIDATION);
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.SIGNED_OFF_BY_REQUIRED,
+ "requireSignedOffBy prevents option " + PUSH_OPTION_SKIP_VALIDATION));
return;
}
@@ -3447,7 +3620,11 @@
return;
}
if (!Iterables.isEmpty(rejectCommits)) {
- reject(cmd, "reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION);
+ reject(
+ cmd,
+ RejectionReason.create(
+ MetricBucket.BANNED_COMMIT,
+ "reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION));
}
}
@@ -3471,8 +3648,11 @@
logger.atFine().log("Number of new commits exceeds limit of %d", limit);
reject(
cmd,
- String.format(
- "more than %d commits, and %s not set", limit, PUSH_OPTION_SKIP_VALIDATION));
+ RejectionReason.create(
+ MetricBucket.TOO_MANY_COMMITS,
+ String.format(
+ "more than %d commits, and %s not set",
+ limit, PUSH_OPTION_SKIP_VALIDATION)));
return;
}
if (!receivePackRefCache.patchSetIdsFromObjectId(c).isEmpty()) {
@@ -3713,19 +3893,20 @@
return TraceContext.newTimer(clazz.getSimpleName() + "#" + name, metadataBuilder.build());
}
- private void reject(ReceiveCommand cmd, String why) {
- logger.atFine().log("Rejecting command '%s': %s", cmd, why);
+ private void reject(ReceiveCommand cmd, RejectionReason reason) {
+ logger.atFine().log("Rejecting command '%s': %s", cmd, reason.why());
metrics.rejectCount.increment(
- MagicBranch.isMagicBranch(cmd.getRefName()) ? "magic" : "direct", why);
- cmd.setResult(REJECTED_OTHER_REASON, why);
+ MagicBranch.isMagicBranch(cmd.getRefName()) ? "magic" : "direct",
+ reason.metricBucket().name());
+ cmd.setResult(REJECTED_OTHER_REASON, reason.why());
}
- private void rejectRemaining(Collection<ReceiveCommand> commands, String why) {
- rejectRemaining(commands.stream(), why);
+ private void rejectRemaining(Collection<ReceiveCommand> commands, RejectionReason reason) {
+ rejectRemaining(commands.stream(), reason);
}
- private void rejectRemaining(Stream<ReceiveCommand> commands, String why) {
- commands.filter(cmd -> cmd.getResult() == NOT_ATTEMPTED).forEach(cmd -> reject(cmd, why));
+ private void rejectRemaining(Stream<ReceiveCommand> commands, RejectionReason reason) {
+ commands.filter(cmd -> cmd.getResult() == NOT_ATTEMPTED).forEach(cmd -> reject(cmd, reason));
}
private static boolean isHead(ReceiveCommand cmd) {
diff --git a/java/com/google/gerrit/server/git/receive/RejectionReason.java b/java/com/google/gerrit/server/git/receive/RejectionReason.java
new file mode 100644
index 0000000..063c5c1
--- /dev/null
+++ b/java/com/google/gerrit/server/git/receive/RejectionReason.java
@@ -0,0 +1,83 @@
+// Copyright (C) 2024 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.git.receive;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class RejectionReason {
+ public enum MetricBucket {
+ ACCOUNT_NOT_FOUND,
+ CANNOT_ADD_PATCH_SET,
+ CANNOT_COMBINE_NORMAL_AND_MAGIC_PUSHES,
+ CANNOT_CREATE_REF_BECAUSE_IT_ALREADY_EXISTS,
+ CANNOT_DELETE_CHANGES,
+ CANNOT_DELETE_PROJECT_CONFIGURATION,
+ CANNOT_EDIT_NEW_CHANGE,
+ CANNOT_PUSH_MERGE_WITH_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
+ CANNOT_SKIP_VALIDATION_FOR_MAGIC_PUSH,
+ CANNOT_TOGGLE_WIP,
+ CHANGE_IS_CLOSED,
+ CHANGE_NOT_FOUND,
+ CLIENT_ERROR,
+ COMMIT_ALREADY_EXISTS_IN_CHANGE,
+ COMMIT_ALREADY_EXISTS_IN_PROJECT,
+ CONFLICT,
+ BANNED_COMMIT,
+ BRANCH_NOT_FOUND,
+ DUPLICATE_CHANGE,
+ DUPLICATE_CHANGE_ID,
+ DUPLICATE_REQUEST,
+ HELP_REQUESTED,
+ IMPLICIT_MERGE,
+ INTERNAL_SERVER_ERROR,
+ INVALID_BASE,
+ INVALID_BRANCH_SYNTAX,
+ INVALID_CHANGE_ID,
+ INVALID_DEADLINE,
+ INVALID_HEAD,
+ INVALID_OPTION,
+ INVALID_PROJECT_CONFIGURATION_UPDATE,
+ INVALID_REF,
+ MISSING_REVISION,
+ NO_COMMON_ANCESTRY,
+ NO_NEW_CHANGES,
+ NOT_A_COMMIT,
+ NOT_MERGED_INTO_BRANCH,
+ NOTEDB_UPDATE_WITHOUT_ACCESS_DATABASE_PERMISSION,
+ NOTEDB_UPDATE_WITHOUT_ALLOW_OPTION,
+ PATCH_SET_LOCKED,
+ PROHIBITED,
+ PROJECT_CONFIG_UPDATE_NOT_ALLOWED,
+ PROJECT_NOT_WRITABLE,
+ REF_NOT_FOUND,
+ REJECTED_BY_VALIDATOR,
+ REQUEST_CANCELLED,
+ SIGNED_OFF_BY_REQUIRED,
+ SUBMIT_ERROR,
+ TOPIC_TOO_LARGE,
+ TOO_MANY_CHANGES,
+ TOO_MANY_COMMITS,
+ UNKNOWN_COMMAND_TYPE;
+ }
+
+ static RejectionReason create(MetricBucket metricBucket, String why) {
+ return new AutoValue_RejectionReason(metricBucket, why);
+ }
+
+ public abstract MetricBucket metricBucket();
+
+ public abstract String why();
+}
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 2036078..e31f3ac 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -64,6 +64,7 @@
import com.google.gerrit.server.extensions.events.RevisionCreated;
import com.google.gerrit.server.git.MergedByPushOp;
import com.google.gerrit.server.git.receive.ReceiveCommits.MagicBranchInput;
+import com.google.gerrit.server.git.receive.RejectionReason.MetricBucket;
import com.google.gerrit.server.git.validators.TopicValidator;
import com.google.gerrit.server.mail.MailUtil.MailRecipients;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -158,7 +159,7 @@
private ChangeKind changeKind;
private String mailMessage;
private ApprovalCopier.Result approvalCopierResult;
- private String rejectMessage;
+ private RejectionReason rejectionReason;
private MergedByPushOp mergedByPushOp;
private ReviewerModificationList reviewerAdditions;
private MailRecipients oldRecipients;
@@ -262,7 +263,7 @@
notes = ctx.getNotes();
Change change = notes.getChange();
if (change == null || change.isClosed()) {
- rejectMessage = CHANGE_IS_CLOSED;
+ rejectionReason = RejectionReason.create(MetricBucket.CHANGE_IS_CLOSED, CHANGE_IS_CLOSED);
return false;
}
if (groups.isEmpty()) {
@@ -590,8 +591,8 @@
return notes.getChange();
}
- public String getRejectMessage() {
- return rejectMessage;
+ public Optional<RejectionReason> getRejectionReason() {
+ return Optional.ofNullable(rejectionReason);
}
public Optional<String> getOutdatedApprovalsMessage() {
