From f825323de8b6797527558efd5435af1ddb52f48e Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 20 Feb 2018 08:17:38 +0100 Subject: [PATCH 1/2] CurrentUser: Add method for getting a loggable name This is useful for including a CurrentUser into log messages. Change-Id: I25631902e5fb7b69960b3f53d62b51de3c70e9e2 Signed-off-by: Edwin Kempin --- java/com/google/gerrit/server/CurrentUser.java | 5 +++++ java/com/google/gerrit/server/IdentifiedUser.java | 1 + 2 files changed, 6 insertions(+) diff --git a/java/com/google/gerrit/server/CurrentUser.java b/java/com/google/gerrit/server/CurrentUser.java index 6b74d1f009..ace06c529e 100644 --- a/java/com/google/gerrit/server/CurrentUser.java +++ b/java/com/google/gerrit/server/CurrentUser.java @@ -95,6 +95,11 @@ public abstract class CurrentUser { return Optional.empty(); } + /** @return unique name of the user for logging, never {@code null} */ + public String getLoggableName() { + return getUserName().orElseGet(() -> getClass().getSimpleName()); + } + /** Check if user is the IdentifiedUser */ public boolean isIdentifiedUser() { return false; diff --git a/java/com/google/gerrit/server/IdentifiedUser.java b/java/com/google/gerrit/server/IdentifiedUser.java index 1a64abd06b..8379a7cfc8 100644 --- a/java/com/google/gerrit/server/IdentifiedUser.java +++ b/java/com/google/gerrit/server/IdentifiedUser.java @@ -323,6 +323,7 @@ public class IdentifiedUser extends CurrentUser { } /** @return unique name of the user for logging, never {@code null} */ + @Override public String getLoggableName() { return getUserName() .orElseGet( From 3f8ec20bb5533a0f5ec64d8895bd52618fe10506 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 19 Feb 2018 16:39:49 +0100 Subject: [PATCH 2/2] Add debug logs for reviewer suggestion Frequently users complain about unexpected reviewer suggestions (e.g. if they type X on change Y then they expect that user Z is suggested, but user Z is missing in the result). Add some debug logs to make it easier to investigate such problems. Change-Id: I383a3c209f2f1eb3567eba44b05a2d2ee9cf56e8 Signed-off-by: Edwin Kempin --- .../server/restapi/change/ReviewersUtil.java | 47 ++++++++++++++++--- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java index 7a2a148612..38ed756a46 100644 --- a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java +++ b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java @@ -65,6 +65,8 @@ import java.util.List; import java.util.Objects; import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class ReviewersUtil { @Singleton @@ -110,6 +112,8 @@ public class ReviewersUtil { } } + private static final Logger log = LoggerFactory.getLogger(ReviewersUtil.class); + // Generate a candidate list at 2x the size of what the user wants to see to // give the ranking algorithm a good set of candidates it can work with private static final int CANDIDATE_LIST_MULTIPLIER = 2; @@ -163,20 +167,29 @@ public class ReviewersUtil { VisibilityControl visibilityControl, boolean excludeGroups) throws IOException, OrmException, ConfigInvalidException, PermissionBackendException { + CurrentUser currentUser = self.get(); + log.debug( + "Suggesting reviewers for change {} to user {}.", + changeNotes.getChangeId().get(), + currentUser.getLoggableName()); String query = suggestReviewers.getQuery(); + log.debug("Query: {}", query); int limit = suggestReviewers.getLimit(); if (!suggestReviewers.getSuggestAccounts()) { + log.debug("Reviewer suggestion is disabled."); return Collections.emptyList(); } List candidateList = new ArrayList<>(); if (!Strings.isNullOrEmpty(query)) { candidateList = suggestAccounts(suggestReviewers); + log.debug("Candidate list: {}", candidateList); } List sortedRecommendations = recommendAccounts(changeNotes, suggestReviewers, projectState, candidateList); + log.debug("Sorted recommendations: {}", sortedRecommendations); // Filter accounts by visibility and enforce limit List filteredRecommendations = new ArrayList<>(); @@ -192,20 +205,40 @@ public class ReviewersUtil { } } } + log.debug("Filtered recommendations: {}", filteredRecommendations); - List suggestedReviewer = loadAccounts(filteredRecommendations); - if (!excludeGroups && suggestedReviewer.size() < limit && !Strings.isNullOrEmpty(query)) { + List suggestedReviewers = loadAccounts(filteredRecommendations); + if (!excludeGroups && suggestedReviewers.size() < limit && !Strings.isNullOrEmpty(query)) { // Add groups at the end as individual accounts are usually more // important. - suggestedReviewer.addAll( + suggestedReviewers.addAll( suggestAccountGroups( - suggestReviewers, projectState, visibilityControl, limit - suggestedReviewer.size())); + suggestReviewers, + projectState, + visibilityControl, + limit - suggestedReviewers.size())); } - if (suggestedReviewer.size() <= limit) { - return suggestedReviewer; + if (suggestedReviewers.size() > limit) { + suggestedReviewers = suggestedReviewers.subList(0, limit); + log.debug("Limited suggested reviewers to {} accounts.", limit); } - return suggestedReviewer.subList(0, limit); + log.debug( + "Suggested reviewers: {}", + suggestedReviewers + .stream() + .map( + r -> { + if (r.account != null) { + return "a/" + r.account._accountId; + } else if (r.group != null) { + return "g/" + r.group.id; + } else { + return ""; + } + }) + .collect(toList())); + return suggestedReviewers; } private List suggestAccounts(SuggestReviewers suggestReviewers) throws OrmException {