Page MenuHomePhabricator

Reduce task notification noise/frequency of changes to associated open patchsets
Open, NormalPublic

Description

In T89940 we replicated open patchsets from Gerrit to Diffusion as a way of keeping feature parity with gitblit.

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.

Workaround:

For the purpose of this functionality, see this exchange on the referenced task:

The one use case I have for refs/changes in phabricator is for converting gerrit unmerged changes into differential diffs.

There's a difference between having the refs on-disk and importing and displaying them though :)

Having them in diffusion makes importing them a lot easier thanks to DifferentialDiffExtractionEngine.php

Event Timeline

greg created this task.Aug 16 2016, 9:54 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 16 2016, 9:54 PM

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?

greg added a comment.Aug 17 2016, 4:30 PM

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

greg moved this task from INBOX to Later / Need volunteer on the Release-Engineering-Team-TODO board.

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 updated the task description. (Show Details)Sun, Oct 20, 9:28 PM
mmodell claimed this task.Wed, Oct 23, 5:33 PM
mmodell triaged this task as Normal priority.

@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.

Krinkle added a comment.EditedFri, Oct 25, 12:06 AM

@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:

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.

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?

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.