Page MenuHomePhabricator

Enable Gerrit reviewers-by-blame plugin
Closed, DeclinedPublic

Description

Per discussion in T91190, it would be nice to enable reviewers-by-blame which helps inexperienced developers find the right person for code review.
(I'm not sure what the roadmap is for Gerrit -> Phabricator migration - please decline if it's too close for Gerrit improvements to be worth the effort. Closest Phabricator equivalent I have found is phabricator:T1743, still under development.)

Event Timeline

Tgr created this task.Jun 2 2015, 5:32 PM
Tgr updated the task description. (Show Details)
Tgr raised the priority of this task from to Needs Triage.
Tgr added a project: Gerrit.
Tgr added a subscriber: Tgr.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 2 2015, 5:32 PM
Ricordisamoa added a subscriber: Ricordisamoa.
scfc added a subscriber: scfc.Jun 2 2015, 6:40 PM
Paladox set Security to None.Jun 2 2015, 11:07 PM
Paladox added subscribers: Legoktm, QChris.
hashar triaged this task as Low priority.Sep 10 2015, 9:54 AM
mmodell added a subscriber: mmodell.Jun 2 2016, 7:49 PM
demon moved this task from Bugs & stuff to Needs a plugin! on the Gerrit board.Jul 26 2016, 11:25 PM
Dalba awarded a token.Dec 12 2016, 3:14 AM
Dalba added a subscriber: Dalba.
Paladox updated the task description. (Show Details)Jan 28 2017, 2:11 AM
Paladox added a subscriber: Paladox.

This project is selected for the Developer-Wishlist voting round and will be added to a MediaWiki page very soon. To the subscribers, or proposer of this task: please help modify the task description: add a brief summary (10-12 lines) of the problem that this proposal raises, topics discussed in the comments, and a proposed solution (if there is any yet). Remember to add a header with a title "Description," to your content. Please do so before February 5th, 12:00 pm UTC.

This will have to be written for polygerrit's ui since gwtui has begun to be removed upstream. We can still make it for gwtui but it will need to support polygerrit ui too.

Change 422178 had a related patch set uploaded (by Chad; owner: Chad):
[operations/software/gerrit/gerrit@wmf/stable-2.14] Adding reviewers-by-blame plugin

https://gerrit.wikimedia.org/r/422178

Change 422178 merged by Chad:
[operations/software/gerrit/gerrit@wmf/stable-2.14] Adding reviewers-by-blame plugin

https://gerrit.wikimedia.org/r/422178

hashar added a subscriber: hashar.EditedNov 21 2018, 9:05 PM

We have the plugin installed: https://gerrit.wikimedia.org/r/plugins/reviewers-by-blame/Documentation/index.html as well as the reviewer one https://gerrit.wikimedia.org/r/plugins/reviewers/Documentation/index.html

Both can be configured on a per project basis by the project owners. Ideally one would want to craft a tutorial/example, highlight it via a Phame post / wikitech-l then I guess we can call it done.

The plugin is enabled. On All-Projects there are some configuration bits available:

reviewers-by-blame Plugin

Ignore file Regex: ____________
[Ignore files that match the given regular expression when looking for reviewers]

Ignore Regex: ____________
[Ignore commits where the subject matches the given regular expression]

Max Reviewers: 3
[The maximum number of reviewers that should be automatically added to a change based on the git blame computation on the changed files.]


So it should work? Unless there is some magic flag required.

I have send a patch to restore logging of plugins at INFO level with https://gerrit.wikimedia.org/r/#/c/475226/ , that might give us some clues. Then if haven't figured it out yet, we can add a log4j debug logger for com.googlesource.gerrit.plugins.reviewersbyblame.

https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/475226/ enables logging for plugins:

[2018-11-28 16:57:47,182] [main] INFO  : Loaded plugin reviewers-by-blame, version 2c08ef7

Will probably need to be tweaked to use DEBUG level for the reviewersbyblame plugin (see above), will let us figure out whether it works properly.

Change 476343 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/puppet@production] gerrit: log reviewers-by-blame plugin at DEBUG level

https://gerrit.wikimedia.org/r/476343

Change 476343 abandoned by Hashar:
gerrit: log reviewers-by-blame plugin at DEBUG level

Reason:
Bah actually the code only has log entries for warn or error ...

I assume that is already logged by the root logger.

https://gerrit.wikimedia.org/r/476343

Tgr added a comment.Dec 17 2018, 3:59 AM

Does not seem to be working, my list of reviewer suggestions seems to be populated from people I asked for a review in the past, and is not specific to the repo. E.g. here is a random extension: BlueSpiceExtendedFilelist, you can see that the top reviewer is Robert Vogel. But when I click on "add reviewer" I get a list of people who have never even interacted with that repo, and as far as I can see the only common feature is that they have all reviewed patches of mine in the past, and that I have asked all of them for a review at some point in time in some repo (but not BlueSpiceExtendedFilelist).

Mentioned in SAL (#wikimedia-operations) [2019-01-08T09:19:39Z] <hashar> gerrit: resaved configuration for All-Projects by changing "Max Reviewers" from 3 to 4. Might enable adding reviewers automatically based on git blame. See task for config diff # T101131

hashar added a comment.Jan 8 2019, 9:30 AM

Indeed it is definitely not adding reviewers. That puzzled me during the xmas break and eventually had an idea this morning. Based on my previous comment about the plugin configuration ( T101131#4766853 ) I eventually looked at the raw configuration file on the project:

$ git clone https://gerrit.wikimedia.org/r/p/All-Projects.git
$ git fetch refs/meta/config && git checkout FETCH_HEAD
$ grep -i reviewer project.config
$

Ie despite the UI showing some values, there are none listed in the All-Projects configuration. In the web UI I have thus changed Max Reviewers from 3 to 4 (using copy paste since my browser/javascript would not let me add a value). I have saved the config b972f5ea571e7bd15542a6562dd1d0b010681a1a which got me:

$ git fetch refs/meta/config
$ git reset --hard FETCH_HEAD
$ git show
commit b972f5ea571e7bd15542a6562dd1d0b010681a1a
Author: Hashar <hashar@free.fr>
Date:   Tue Jan 8 09:17:04 2019 +0000

    Modified project settings

diff --git a/project.config b/project.config
index fa884eb..85399de 100644
--- a/project.config
+++ b/project.config
@@ -51,6 +51,7 @@
 [submit]
        mergeContent = false
        action = fast forward only
+       matchAuthorToCommitterDate = false
 [dashboard]
        default = refs/meta/dashboards/default:recent
 [label "Verified"]
@@ -90,5 +91,12 @@
        push = group Administrators
        push = group Gerrit Managers
        push = group Project Owners
+[change]
+       privateByDefault = false
+       workInProgressByDefault = false
+[reviewer]
+       enableByEmail = false
+[plugin "reviewers-by-blame"]
+       maxReviewers = 4
 [plugin "its-phabricator"]
        enabled = true

Which is b972f5ea571e7bd15542a6562dd1d0b010681a1a. Some settings are just normalization. The code default to 3 anyway via a getInt("maxReviewers", 3). So I dont see how my change could modify anything.

Note the plugin is definitely installed and loaded based on https://gerrit.wikimedia.org/r/#/admin/plugins/

hashar added a comment.Jan 8 2019, 9:38 AM

Eventually I have found https://groups.google.com/d/msg/repo-discuss/-5kMTN34aqw/zgTAEgReDgAJ reviewers-by-blame on stable-2.15 busted with NoteDb. It points at Change 192110: Find change via ChangeData.Factory rather than ReviewDb.

Git log:

* 0b6233e - (origin/stable-2.15) Find change via ChangeData.Factory rather than ReviewDb (8 weeks ago) <Richard Christie>
| * 5ff127d - (origin/master, origin/HEAD) Adapats plugin to changes done in core (3 months ago) <Paladox none>
| * d2fc6d7 - Adapt to new core package layout (1 year, 2 months ago) <Paladox none>
|/  
* 2c08ef7 - (HEAD -> master) Adapt to removed AccountByEmailCache (1 year, 2 months ago) <Paladox none>
* 85abed9 - Remove support for drafts (1 year, 2 months ago) <Paladox none>
* 7daadf5 - (origin/stable-2.14) Format all Java files with google-java-format (1 year, 2 months ago) <David Pursehouse>

So we need to update our copy of reviewers-by-blame from 2c08ef7 to stable-2.15 and we will get the change.

Change 482775 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/software/gerrit@wmf/stable-2.15] Update plugins reviewers-by-blame to stable-2.15

https://gerrit.wikimedia.org/r/482775

hashar claimed this task.Jan 8 2019, 9:59 AM
hashar added a subscriber: thcipriani.EditedJan 8 2019, 4:09 PM

From @thcipriani to rebuild the plugin one would do something like:

bazel build plugins/reviewers-by-blame
wmf/stable-2.15
bazel build plugins/reviewers-by-blame:reviewers-by-blame

Result in bazel-bin

Upload the resulting .jar to Archiva, update git fat (we have a script to handle that: upload-artifacts script).

Then scap deploy. We don't even need to restart Gerrit.

I went with install bazel from their apt package (which is a huge binary blob bah). Then:

$ cd ~/projects/operations/software/gerrit
$ bazel build plugins/reviewers-by-blame:reviewers-by-blame
INFO: Analysed target //plugins/reviewers-by-blame:reviewers-by-blame (142 packages loaded, 3182 targets configured).
INFO: Found 1 target...
Target //plugins/reviewers-by-blame:reviewers-by-blame up-to-date:
  bazel-genfiles/plugins/reviewers-by-blame/reviewers-by-blame.jar
INFO: Elapsed time: 44.699s, Critical Path: 12.04s
INFO: 126 processes: 121 processwrapper-sandbox, 5 worker.
INFO: Build completed successfully, 140 total actions

Which gets me a ton of .jar files:

$ find bazel-genfiles/ -type f
bazel-genfiles/gerrit-server/prolog-common.srcjar
bazel-genfiles/gerrit-patch-jgit/edit.srcjar
bazel-genfiles/external/bcprov/jar/_ijar/jar/external/bcprov/jar/bcprov-jdk15on-1.57-ijar.jar
bazel-genfiles/external/mina-core/jar/_ijar/jar/external/mina-core/jar/mina-core-2.0.16-ijar.jar
bazel-genfiles/external/automaton/jar/_ijar/jar/external/automaton/jar/automaton-1.11-8-ijar.jar
bazel-genfiles/external/protobuf/jar/_ijar/jar/external/protobuf/jar/protobuf-java-3.0.0-beta-2-ijar.jar
bazel-genfiles/external/commons-validator/jar/_ijar/jar/external/commons-validator/jar/commons-validator-1.6-ijar.jar
bazel-genfiles/external/commons-net/jar/_ijar/jar/external/commons-net/jar/commons-net-3.5-ijar.jar
bazel-genfiles/external/httpcore/jar/_ijar/jar/external/httpcore/jar/httpcore-4.4.1-ijar.jar
bazel-genfiles/external/soy/jar/_ijar/jar/external/soy/jar/soy-2017-04-23-ijar.jar
bazel-genfiles/external/mime-util/jar/_ijar/jar/external/mime-util/jar/mime-util-2.1.3-ijar.jar
bazel-genfiles/external/ow2-asm-tree/jar/_ijar/jar/external/ow2-asm-tree/jar/asm-tree-5.1-ijar.jar
bazel-genfiles/external/guava/jar/_ijar/jar/external/guava/jar/guava-22.0-ijar.jar
bazel-genfiles/external/log-api/jar/_ijar/jar/external/log-api/jar/slf4j-api-1.7.7-ijar.jar
bazel-genfiles/external/joda-time/jar/_ijar/jar/external/joda-time/jar/joda-time-2.9.9-ijar.jar
bazel-genfiles/external/jgit-lib/jar/_ijar/jar/external/jgit-lib/jar/org.eclipse.jgit-4.9.7.201810191756-r-ijar.jar
bazel-genfiles/external/gson/jar/_ijar/jar/external/gson/jar/gson-2.8.0-ijar.jar
bazel-genfiles/external/commons-codec/jar/_ijar/jar/external/commons-codec/jar/commons-codec-1.10-ijar.jar
bazel-genfiles/external/guice-library/jar/_ijar/jar/external/guice-library/jar/guice-4.2.0-ijar.jar
bazel-genfiles/external/ow2-asm-util/jar/_ijar/jar/external/ow2-asm-util/jar/asm-util-5.1-ijar.jar
bazel-genfiles/external/grappa/jar/_ijar/jar/external/grappa/jar/grappa-1.0.4-ijar.jar
bazel-genfiles/external/lucene-analyzers-common/jar/_ijar/jar/external/lucene-analyzers-common/jar/lucene-analyzers-common-5.5.4-ijar.jar
bazel-genfiles/external/pegdown/jar/_ijar/jar/external/pegdown/jar/pegdown-1.6.0-ijar.jar
bazel-genfiles/external/lucene-misc/jar/_ijar/jar/external/lucene-misc/jar/lucene-misc-5.5.4-ijar.jar
bazel-genfiles/external/tukaani-xz/jar/_ijar/jar/external/tukaani-xz/jar/xz-1.4-ijar.jar
bazel-genfiles/external/dropwizard-core/jar/_ijar/jar/external/dropwizard-core/jar/metrics-core-4.0.3-ijar.jar
bazel-genfiles/external/commons-lang/jar/_ijar/jar/external/commons-lang/jar/commons-lang-2.6-ijar.jar
bazel-genfiles/external/velocity/jar/_ijar/jar/external/velocity/jar/velocity-1.7-ijar.jar
bazel-genfiles/external/jgit-servlet/jar/_ijar/jar/external/jgit-servlet/jar/org.eclipse.jgit.http.server-4.9.7.201810191756-r-ijar.jar
bazel-genfiles/external/icu4j/jar/_ijar/jar/external/icu4j/jar/icu4j-57.1-ijar.jar
bazel-genfiles/external/ow2-asm-commons/jar/_ijar/jar/external/ow2-asm-commons/jar/asm-commons-5.1-ijar.jar
bazel-genfiles/external/java-runtime/jar/_ijar/jar/external/java-runtime/jar/antlr-runtime-3.5.2-ijar.jar
bazel-genfiles/external/gwtorm-client/jar/_ijar/jar/external/gwtorm-client/jar/gwtorm-1.18-ijar.jar
bazel-genfiles/external/gwtorm-client/jar/_ijar/src/external/gwtorm-client/jar/gwtorm-1.18-src-ijar.jar
bazel-genfiles/external/guice-servlet/jar/_ijar/jar/external/guice-servlet/jar/guice-servlet-4.2.0-ijar.jar
bazel-genfiles/external/sshd/jar/_ijar/jar/external/sshd/jar/sshd-core-1.6.0-ijar.jar
bazel-genfiles/external/guava-retrying/jar/_ijar/jar/external/guava-retrying/jar/guava-retrying-2.0.0-ijar.jar
bazel-genfiles/external/httpclient/jar/_ijar/jar/external/httpclient/jar/httpclient-4.4.1-ijar.jar
bazel-genfiles/external/jgit-archive/jar/_ijar/jar/external/jgit-archive/jar/org.eclipse.jgit.archive-4.9.7.201810191756-r-ijar.jar
bazel-genfiles/external/user/jar/_ijar/jar/external/user/jar/gwt-user-2.8.2-ijar.jar
bazel-genfiles/external/javax_inject/jar/_ijar/jar/external/javax_inject/jar/javax.inject-1-ijar.jar
bazel-genfiles/external/mime4j-core/jar/_ijar/jar/external/mime4j-core/jar/apache-mime4j-core-0.8.0-ijar.jar
bazel-genfiles/external/juniversalchardet/jar/_ijar/jar/external/juniversalchardet/jar/juniversalchardet-2.0.0-ijar.jar
bazel-genfiles/external/commons-dbcp/jar/_ijar/jar/external/commons-dbcp/jar/commons-dbcp-1.4-ijar.jar
bazel-genfiles/external/jsoup/jar/_ijar/jar/external/jsoup/jar/jsoup-1.9.2-ijar.jar
bazel-genfiles/external/gwtjsonrpc/jar/_ijar/jar/external/gwtjsonrpc/jar/gwtjsonrpc-1.11-ijar.jar
bazel-genfiles/external/gwtjsonrpc/jar/_ijar/src/external/gwtjsonrpc/jar/gwtjsonrpc-1.11-src-ijar.jar
bazel-genfiles/external/ow2-asm/jar/_ijar/jar/external/ow2-asm/jar/asm-5.1-ijar.jar
bazel-genfiles/external/ow2-asm-analysis/jar/_ijar/jar/external/ow2-asm-analysis/jar/asm-analysis-5.1-ijar.jar
bazel-genfiles/external/dev/jar/_ijar/jar/external/dev/jar/gwt-dev-2.8.2-ijar.jar
bazel-genfiles/external/log4j/jar/_ijar/jar/external/log4j/jar/log4j-1.2.17-ijar.jar
bazel-genfiles/external/jsch/jar/_ijar/jar/external/jsch/jar/jsch-0.1.54-ijar.jar
bazel-genfiles/external/guice-assistedinject/jar/_ijar/jar/external/guice-assistedinject/jar/guice-assistedinject-4.2.0-ijar.jar
bazel-genfiles/external/h2/jar/_ijar/jar/external/h2/jar/h2-1.3.176-ijar.jar
bazel-genfiles/external/jsonevent-layout/jar/_ijar/jar/external/jsonevent-layout/jar/jsonevent-layout-1.7-ijar.jar
bazel-genfiles/external/lucene-queryparser/jar/_ijar/jar/external/lucene-queryparser/jar/lucene-queryparser-5.5.4-ijar.jar
bazel-genfiles/external/eddsa/jar/_ijar/jar/external/eddsa/jar/eddsa-0.2.0-ijar.jar
bazel-genfiles/external/auto-value-annotations/jar/_ijar/jar/external/auto-value-annotations/jar/auto-value-annotations-1.6.2-ijar.jar
bazel-genfiles/external/bcpkix/jar/_ijar/jar/external/bcpkix/jar/bcpkix-jdk15on-1.57-ijar.jar
bazel-genfiles/external/auto-value/jar/_ijar/jar/external/auto-value/jar/auto-value-1.6.2-ijar.jar
bazel-genfiles/external/mime4j-dom/jar/_ijar/jar/external/mime4j-dom/jar/apache-mime4j-dom-0.8.0-ijar.jar
bazel-genfiles/external/blame-cache/jar/_ijar/jar/external/blame-cache/jar/blame-cache-0.2-5-ijar.jar
bazel-genfiles/external/prolog-runtime/jar/_ijar/jar/external/prolog-runtime/jar/prolog-runtime-1.4.3-ijar.jar
bazel-genfiles/external/prolog-runtime/jar/_ijar/neverlink/external/prolog-runtime/jar/prolog-runtime-1.4.3-ijar.jar
bazel-genfiles/external/commons-compress/jar/_ijar/jar/external/commons-compress/jar/commons-compress-1.13-ijar.jar
bazel-genfiles/external/args4j/jar/_ijar/jar/external/args4j/jar/args4j-2.0.26-ijar.jar
bazel-genfiles/external/servlet-api-3_1/jar/_ijar/jar/external/servlet-api-3_1/jar/tomcat-servlet-api-8.0.24-ijar.jar
bazel-genfiles/lib/lucene/lucene-core-and-backward-codecs__merged.jar
bazel-genfiles/lib/lucene/_ijar/lucene-core-and-backward-codecs/lib/lucene/lucene-core-and-backward-codecs__merged-ijar.jar
bazel-genfiles/plugins/reviewers-by-blame/reviewers-by-blame.jar
bazel-genfiles/gerrit-index/query_antlr.srcjar

Maybe we just need bazel-genfiles/plugins/reviewers-by-blame/reviewers-by-blame.jar

@hashar correct (for the path to the plugin jar)

Change 482775 abandoned by Hashar:
Update plugins reviewers-by-blame to stable-2.15

Reason:
Superseeded by the upgrade to Gerrit 2.15.8 https://gerrit.wikimedia.org/r/#/c/operations/software/gerrit/ /484437/

https://gerrit.wikimedia.org/r/482775

hashar closed this task as Resolved.Jan 16 2019, 7:52 PM

The plugin version we were using was not compatible yet with Gerrit 2.15. In the previous upgrades we forgot to bump the plugin.

With Gerrit 2.15.8 (T210785) which we upgraded a few minutes ago, the plugin got bumped. Looking at a few recent changes, they seem to have magic reviewers being added to change!

I am assuming the plugin works properly now!

Tgr awarded a token.Jan 17 2019, 2:30 AM

Works great, thank you for fixing it!

Given that action of adding reviewers is done under the name of the uploader, so it's not entirely obvious what's going on, maybe a wikitech-l announcement would be in order?
(Or we could just ask upstream to add an option to use a dedicated user for adding reviers.)

Works great, thank you for fixing it!

Given that action of adding reviewers is done under the name of the uploader, so it's not entirely obvious what's going on, maybe a wikitech-l announcement would be in order?
(Or we could just ask upstream to add an option to use a dedicated user for adding reviers.)

We should do both, Email in the short term, but ideally in the long term we should request automated actions like that are done by a system user and not as a actual user when the user isn't actually performing the action.

I have published a blog explaining the Gerrit review by blame plugin as well as other manual way to find reviewers: J139: Blog Post: Gerrit now automatically adds reviewers

Tgr added a comment.Jan 17 2019, 7:58 PM

ideally in the long term we should request automated actions like that are done by a system user and not as a actual user when the user isn't actually performing the action.

Filed upstream as #10328.

greg added a subscriber: greg.Jan 17 2019, 8:01 PM

ideally in the long term we should request automated actions like that are done by a system user and not as a actual user when the user isn't actually performing the action.

Filed upstream as #10328.

@Paladox had filed https://bugs.chromium.org/p/gerrit/issues/detail?id=10318 which is similar (exempting bot accounts vs using a dedicated account)

Change 485184 had a related patch set uploaded (by Hashar; owner: Hashar):
[All-Projects@refs/meta/config] reviewers-by-blame: default to not adding any reviewers

https://gerrit.wikimedia.org/r/485184

As a follow up (also posted as a comment to J139):

The Gerrit plugin blindly adds reviewers even when in a few cases it might not be appropriate (for a bot account, author of code who is not qualified to do a review, user that is no more active on a repository).

As such, the plugin will no more report by default. The configuration setting maxReviewers has been set to 0 in All-Projects.git(Gerrit #485184) which causes the plugin to early abort. The setting is inherited by all repositories

Project owners can still enable the setting for their repository by editing the project settings and setting maxReviewers to a positive integer.

Change 485184 merged by Thcipriani:
[All-Projects@refs/meta/config] reviewers-by-blame: default to not adding any reviewers

https://gerrit.wikimedia.org/r/485184

Change 485810 had a related patch set uploaded (by Hashar; owner: Thcipriani):
[operations/software/gerrit@deploy/wmf/stable-2.15] Remove reviewers-by-blame from deployment

https://gerrit.wikimedia.org/r/485810

hashar changed the task status from Resolved to Declined.Jan 22 2019, 2:05 PM

Thank you @Tgr and @Paladox . We are going to uninstall the plugin entirely based on the feedback from the thread wikitech-l Gerrit now automatically adds reviewers.

We would need way more features to be implement before considering enabling it again. Something to take in mind for later is to gradually enable it per repository instead of globally.

I am changing this to Declined, I don't think we will enable it anytime soon. But feel free to flip it to stalled state instead :)

Dalba removed a subscriber: Dalba.Jan 22 2019, 2:07 PM

Change 485810 merged by Thcipriani:
[operations/software/gerrit@deploy/wmf/stable-2.15] Remove reviewers-by-blame from deployment

https://gerrit.wikimedia.org/r/485810

Change 486262 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/software/gerrit@wmf/stable-2.15] Remove reviewers-by-blame

https://gerrit.wikimedia.org/r/486262

Change 486262 merged by Thcipriani:
[operations/software/gerrit@wmf/stable-2.15] Remove reviewers-by-blame

https://gerrit.wikimedia.org/r/486262