The downside of this is that for gerrit changes that go through many iterations (which is not uncommon and a good thing) will 'spam' the associated task(s) with the updates.

Isn't that the same as gerritbot now? what is the frequency difference between the two for example?

In T143162#2559832, @Peachey88 wrote:

Isn't that the same as gerritbot now?

No, Gerritbot only notifies on new associated patch creation (or the adding of the correct Bug: Twhatever line) then either merge or abandonment.

I've done this for perf team repos, and mediawiki/core. But it still happens for lots of other repositories, such as at T224224#5211032 for every iteration of a patch for extension-GrowthExperiments.

Could we turn these off for all Diffusion repos at once with a DB query or bot of some kind?

Mentioned in SAL (#wikimedia-releng) [2019-06-13T15:33:38Z] <Krinkle> Disable "Publish/Notify" for quibble.git in diffusion, T143162, T196347

Mentioned in SAL (#wikimedia-releng) [2019-06-15T00:17:57Z] <Krinkle> Disable "Publish/Notify" for mediawiki/libs/ObjectFactory.git in Diffusion – T222409, T143162

Do we still have a good use case for these to be mirrored to Diffusion?

Mentioned in SAL (#wikimedia-releng) [2019-09-11T22:31:37Z] <Krinkle> Disable "publishing" for mediawiki/skins/Vector in Phabricator Diffusion mirror - T143162

@greg: not really but currently refs/changes are not, by default, "published". See Diffusion User Guide: Permanent Refs. Shortly after this behavior landed in Phabricator, I ran a script to update the settings for every diffusion repository and exclude refs/changes from being "published" which means that although the refs exist, they should not generate notifications or any other activity in phabricator. Once a patch merges into a "real" branch, then phabricator treats it as permanent and publishes appropriate notifications (if any.)

Mentioned in SAL (#wikimedia-releng) [2019-10-11T17:57:59Z] <Krinkle> Click "Disable Publishing" on extension-Elastica Phab mirror, T235233, T143162

@mmodell That does not appear to have worked. I still every week see this noise in various tasks on new commits and have to manually disable it ad-hoc on each repo it happens with..

Mentioned in SAL (#wikimedia-releng) [2019-10-20T21:28:00Z] <Krinkle> Click "Disable Publishing" for extensions/OAuth and extensions/RevisionSlider in Phabricator, T143162 ,T219604

@Krinkle: can you link to an example of the noise you are seeing? I looked at one of the repos where you disabled publishing, https://phabricator.wikimedia.org/diffusion/EOAU/manage/branches/ and it's only set to fetch refs/heads/* and refs/tags/* ... so it shouldn't have been fetching any unmerged patches from gerrit even with publishing enabled.

@mmodell For most of the these I tend to link to two tasks. This one, and the task where the noise was encountered.

For example:

From T219604#5589317:

Jdforrester-WMF mentioned this in rEOAUcaf27989e2a0: Remove use of jquery.ui module aliases.
Jdforrester-WMF mentioned this in rERSLf0720b6eaf50: Remove use of jquery.ui module aliases.

In T143162#5599939, @mmodell wrote:

so it shouldn't have been fetching any unmerged patches from gerrit even with publishing enabled.

These were merged patches. I don't think those should result in noise either, given we already have gerritbot reporting when tagged patches are created, merged or abandoned. Could we disable this feature en-mass somehow?

In T143162#5605053, @Krinkle wrote:

These were merged patches. I don't think those should result in noise either, given we already have gerritbot reporting when tagged patches are created, merged or abandoned. Could we disable this feature en-mass somehow?

Here's what the docs[1] say about publishing:

When a commit is published, Phabricator acts on it and:

• sends email;
• delivers notifications;
• publishes a feed story;
• triggers Audits;
• runs Herald rules;
• updates mentioned objects;
• closes referenced tasks; and
• closes associated revisions.

It is definitely possible to disable publishing en-mass by using the conduit diffusion.repository.edit method. This has drawbacks though: I don't think it's possible (without hacking the code) to selectively disable some of the above list without disabling all of them.

I agree that there are a lot of redundant notifications, however, to me it seems pretty clear that it is gerritbot that's broken and redundant, not phabricator. It is annoying that phabricator treats publishing as all-or-nothing per repository (actually per branch, see[1]) and It may be worth my time to add more configurability in phabricator so that we can disable individual publishing activities rather than all or nothing.

I really don't find the gerritbot comments in phabricator to be very valuable in their current format. And further, I find the phabricator object relationships (mentions, related commits, etc) highly valuable. So I'd rather improve Phabricator's behavior / functionality and disable redundant comments in gerritbot.

References:

1. Diffusion docs: Permanent References
2. Conduit API method: diffusion.repository.edit

I realized I probably need to clarify further and make a couple of points explicit:

From the task description:

for gerrit changes that go through many iterations (which is not uncommon and a good thing) will 'spam' the associated task(s) with the updates.

This problem is resolved already. Phabricator added the permanent refs configuration which gives us the ability to ignore gerrit's refs/changes/* and github's refs/pull/*. We currently take advantage of this since I mass-edited all the repositories to remove refs/changes from the list of "permanent" refs. Now phabricator will not "publish" the intermediate iterations of a gerrit change. Only the final merged patch is published once it hits a "permanent" ref, which is master in most cases.

The problem at hand is that we have noise on tasks. The most common type of notification (change merged) is being communicated on the same task by two different systems. This task is about solving that in the short term.

Given the history of Phabricator initially also leaving noise about unmerged patchset revisions, the status quo has been to disable this system in Phabricator, which was done years ago for most repos (where most = repos responsible for most number of changes, not number of repos).

Right now, there is only a long tail of lesser active repos where Phabricator's notif system is still active.

Until and unless another solution is presented to the problem at hand, we will continue to disable Phabricator's system for more repos to solve the immediate problem of two systems both notifying users of the same event on tasks.

Improving Gerritbot or making it obsolete sounds good to me, but is imho out of scope for this task.

So how useful is the notification from gerritbot? I don't rely on it for anything but presumably some people are relying on the notifications? Tasks now include a list of open changes but that is only discoverable once you've loaded the task page, that feature does not send any sort of notifications.

@mmodell: Does "notification" only refer to creation of a comment in a Phab task, or also to adding the Patch-For-Review tag? I'd say the former might not be needed anymore, while some teams (CPT?) might depend on the latter in their workflows.

Mentioned in SAL (#wikimedia-releng) [2019-11-26T20:00:10Z] <Krinkle> Click "Disable Publishing" for extension-CollaborationKit, T143162, T219604

Bottom line is we should not have Phabricator /and/ Gerritbot adding noise to the timeline about merged patches.

Until one of those changes or until we intentionally decide to turn this back on for all repos, should we turn it off for the long tail of less-frequently updated repos as well until we have a solid idea of how we want this to work mid-long term?

Mentioned in SAL (#wikimedia-releng) [2020-03-20T22:43:41Z] <Krinkle> Click "Disable Publishing" for extension-TitleBlacklist, T243175, T143162

+1 to reducing noise from gerritbot, especially nowadays that gerrit patches are (very helpfully!) shown in "related changes in gerrit". IMHO we could completely get rid of gerritbot task updates

Ping/bump ? Looking forward to stop gerritbot messages for patches uploaded now that phabricator/gerrit integration is much better (thanks!)

In T143162#6152069, @fgiunchedi wrote:

nowadays that gerrit patches are (very helpfully!) shown in "related changes in gerrit". IMHO we could completely get rid of gerritbot task updates

That might collide with the workflow of Platform Engineering (as GerritBot adds Patch-For-Review tags) (but not sure how exactly that data is pulled).

In T143162#6353539, @Aklapper wrote:

In T143162#6152069, @fgiunchedi wrote:

nowadays that gerrit patches are (very helpfully!) shown in "related changes in gerrit". IMHO we could completely get rid of gerritbot task updates

That might collide with the workflow of Platform Engineering (as GerritBot adds Patch-For-Review tags) (but not sure how exactly that data is pulled).

Could we maybe still have something add Patch-For-Review but not make the comment if that's something preferred more?

Yeah adding Patch-For-Review only sounds good to me, less visually noisy for sure (albeit still spammy via email I believe?).

Is there an update/progress on stopping gerritbot messages?

In T143162#6559276, @fgiunchedi wrote:

(albeit still spammy via email I believe?).

Isn't that desired tho?

If nothing triggers a email, no one would know a patch has been submitted, unless they constantly view a task.

This task is about the Phabricator's Diffusion mirror notifying tasks about patches.

At the time of the task's creation four years ago in 2016, this was causing noise through duplicate messaging on tasks.

Now, this is actually one of not two but three signals since as of T229934 we also have the "Related Gerrit patches" query panel from our Wikimedia-specific Phab extension. I've already disabled this on the biggest repos years ago, it's just a matter of someone turning it off everywhere either by default and/or through a db query.

Mentioned in SAL (#wikimedia-releng) [2020-11-24T02:22:28Z] <Krinkle> click "Disable publishing" for extensions-Scribunto ref T267587#6624926, T143162

@Krinkle: From my reading, the consensus now seems to be leaning towards reducing / eliminating the comments from gerritbot. Do we need to make a new task for that?

From my perspective it's helpful to have one comment from gerritbot but not a new one for every single patchset iteration in gerrit.

In T143162#7252768, @mmodell wrote:

@Krinkle: From my reading, the consensus now seems to be leaning towards reducing / eliminating the comments from gerritbot. Do we need to make a new task for that?

I've not heard anything like that.

From my perspective it's helpful to have one comment from gerritbot but not a new one for every single patchset iteration in gerrit.

As I understand it, that is exactly what Gerritbot does today. It comments once on patch creation (to invite code review), and once upon patch closing (to queue QA or expedite post-merge review ahead of deployment). Both of these actions can also send notifications (to people that have that enabled, and don't filter them out). The Phab extension you built for live querying Gerrit is a nice additional feature, especially for summarising them in one spot, and for security tasks where the bot can't comment, to see the current live status (incl to retroactively remove things that were wrongly tagged).

The Differential mirror ping after the fact, though, is what this task is about and afaik doesn't add any value. It's redundant, doesn't notify (afaik), points to a non-canonical view with out-of-bound commenting ability that is neither Gitiles nor Gerrit. I've not actually heard much talk about those pings either, and that's probably not surprising as for active repos, we've already had them disabled for many years now. It's just that less active repos (and maybe new repos?) have that publishing feature on by default.

In T143162#7252814, @Krinkle wrote:

In T143162#7252768, @mmodell wrote:

@Krinkle: From my reading, the consensus now seems to be leaning towards reducing / eliminating the comments from gerritbot. Do we need to make a new task for that?

I've not heard anything like that.

My apologies. I'm conflating diffusion emails for commits and gerritbot messages and this task.

From my perspective it's helpful to have one comment from gerritbot but not a new one for every single patchset iteration in gerrit.

As I understand it, that is exactly what Gerritbot does today. It comments once on patch creation (to invite code review), and once upon patch closing (to queue QA or expedite post-merge review ahead of deployment). Both of these actions can also send notifications (to people that have that enabled, and don't filter them out). The Phab extension you built for live querying Gerrit is a nice additional feature, especially for summarising them in one spot, and for security tasks where the bot can't comment, to see the current live status (incl to retroactively remove things that were wrongly tagged).

Indeed by looking at https://phabricator.wikimedia.org/p/gerritbot/, message on patch creation + tag patch-for-review + message on merge

The Differential mirror ping after the fact, though, is what this task is about and afaik doesn't add any value. It's redundant, doesn't notify (afaik), points to a non-canonical view with out-of-bound commenting ability that is neither Gitiles nor Gerrit. I've not actually heard much talk about those pings either, and that's probably not surprising as for active repos, we've already had them disabled for many years now. It's just that less active repos (and maybe new repos?) have that publishing feature on by default.

I can confirm I am receiving diffusion notifications for commits on new repos, case in point https://phabricator.wikimedia.org/rOALEdf38ea571fc4 for operations/alerts.

Could we disable all diffusion pings for new and existing repos?

In T143162#7255358, @fgiunchedi wrote:

Could we disable all diffusion pings for new and existing repos?

Thoughts on this @mmodell / @Krinkle ? I'm happy to do it if you point me in the right direction, thank you!

How do you have the settings for "Audit" on https://phabricator.wikimedia.org/settings/user/fgiunchedi/page/emailpreferences/ ?

I have it set like so (I think the key is ignoring "a commit is created"):

Thank you @mmodell that did it! I think we might as well disable all diffusion notifications globally but Good Enough™ for me for now

Mentioned in SAL (#wikimedia-releng) [2022-07-27T03:48:09Z] <Krinkle> Click "Disable publishing" for a dozen repos created recently, including OAuthRateLimiter, ref T143162, T193565

Mentioned in SAL (#wikimedia-releng) [2023-01-03T19:12:10Z] <Krinkle> Disable publishing of rPMSM, ref T143162, T326112

I recently also tried to remember why I received an email notification for rPHDEP5225aea35e51: https://phabricator.wikimedia.org/source/PHDEP/manage/branches/ defines Permanent Refs: wmf/stable and publishing is enabled.

Quoting https://we.phorge.it/book/phorge/article/diffusion_permanent/ ,

When a commit is published, Phorge acts on it and:

• sends email;
• delivers notifications;
• publishes a feed story;
• triggers Audits;
• runs Herald rules;
• updates mentioned objects;
• closes referenced tasks; and
• closes associated revisions.

In my current understanding none of this feels very relevant to us (at least I cannot come up with use cases), however there is a small number of related Herald rules to first tackle (note that these are unrelated to Differential Herald rules as that is T351585).

I guess the way to go would be either unsetting "Permanent Refs" on all repositories, or disabling publishing on all repositories entirely.
I expect that from next week on we will only have mirrored repository in Diffusion and no repositories canonically hosted in Diffusion due to T191182.