Page MenuHomePhabricator

Herald rules causing delays to task edit saves - getting worse
Open, HighPublic

Description

Herald rules are run on every change to every task. With the current number of herald rules we are already experiencing significant delays to each task edit. The proliferation of herald rules is going to be a problem soon.

Do you have an idea of the scale? How much time is being added by them?

@greg: Currently, about 80ms for 64 herald rules.

That doesn't sound great.....

Updates:

2015-08-0880ms
2016-03-14~150ms
2018-09-20~300ms

Event Timeline

greg raised the priority of this task from to Needs Triage.
greg updated the task description. (Show Details)
greg added a project: Phabricator.
greg added subscribers: greg, mmodell.

What relation are the 80 ms to the overall time it takes to process a change?

@scfc: I believe that it's a significant portion of the time, in the range of 10-50%... I would have to do some profiling to give any more specific numbers.

Luke081515 moved this task from To Triage to Misc on the Phabricator board.
Luke081515 added a subscriber: Luke081515.

I know this task is technical, but to have one social aspect:
Have an SQL query for Herald rules that are only for watching/following projects; contact those Phab users; tell them to migrate to watch projects (which does not require being a member anymore soon) until $deadline?
Won't help in the long run but could be a small cleanup at least.

I'm not sure if this will impact things, but we just fixed a caching issue in the upstream (https://secure.phabricator.com/D15451) which was making Herald rules much more expensive than they should be in some cases.

This may not make the rules on this install much cheaper, and there's plenty of merit to planning ahead for the long-term since Herald rules must run inline and will never be free, but that change may give you at least a bit more headroom.

mmodell raised the priority of this task from Low to High.Mar 14 2016, 1:30 AM
mmodell updated the task description. (Show Details)

Down to 118ms to evaluate 101 rules. Thanks @epriestley for the optimization.

That's still a little slow but it's better for sure.

@Quiddity I've disabled my herald rule like requested.

I know this task is technical, but to have one social aspect:
Have an SQL query for Herald rules that are only for watching/following projects; contact those Phab users; tell them to migrate to watch projects (which does not require being a member anymore soon) until $deadline?
Won't help in the long run but could be a small cleanup at least.

You can start with that as an example, since you're being added to nearly all tasks.

@greg @mmodell Is there any chance to measure time per each rule?

You can start with that as an example, since you're being added to nearly all tasks.

@Danny_B: Do you know a way how I could add myself to watch all projects, become a watcher of all future projects to be created, and get automatically subscribed to all newly created tasks that do not have any projects associated? If that is possible, I could, but I'm currently not aware of such functionality.

You can simply watch all relevant projects.
Creation of projects is not so often, so you can watch newly created projects or T103700: LOG: Phabricator project creation.
You can also help to elaborate the needs in upstream report to have the needed functionality added.
Unassociated tasks: Use {H38}.

You can simply watch all relevant projects.

Practically speaking, how can I add myself to watch nearly all existing projects without spending hours of clicking?

Creation of projects is not so often, so you can watch newly created projects or T103700: LOG: Phabricator project creation.

Can I automatically become a watcher of all future projects to be created, or would I have to spend time and do this manually?

You can also help to elaborate the needs in upstream report to have the needed functionality added.

https://secure.phabricator.com/T11312#187893 describes the upstream position and I don't (yet) see why to not follow that advice.
As requested here I have replied to Evan's questions in https://secure.phabricator.com/T11312#188004

Unassociated tasks: Use {H38}.

For H38 I get "You do not have permission to view this object."

Unassociated tasks: Use {H38}.

For H38 I get "You do not have permission to view this object."

Hm, that's my rule... I haven't been able to figure out how to edit the policies on it, so I hope this is helpful instead:

You can edit policys at private rules, and you can't change the permissions for other rules too.

Update: It's now up to 140ms-150ms for each task (with 91 herald rules)

Do these numbers come from DarkConsole's Services tab? Wondering how/where to compare.

Not sure if this is the original source of the numbers, but the "Herald Transcripts" view shows how long each evaluation took (at least, roughly):

https://phabricator.wikimedia.org/herald/transcript/

For example, "93 ms" means that rule evaluation took 93 milliseconds.

Is it the number of Rules that is the problem, or more the number of Conditions? I.e. would it help if we asked users who have 2 rules that could easily be merged, to do so? E.g. H274 and H275 could easily be merged into a single Rule, but it would have the same number of Conditions so merging might not help...

Can we [easily] add custom text to the top of the page https://phabricator.wikimedia.org/herald/create/ giving a bold warning?
Something like: Please do not use Herald if you can avoid it, because each new rule causes longer delays (see T108586).
and then if merging tasks would help, then also mention, If you must use it, then try to only use a single rule with multiple conditions.

I see some patterns, like

  • "inform me when natural language foo was mentioned" (such as H188, H92, H81),
  • "email me explicitly when tasks in project XYZ get the UBN priority" (H154, H294, H236, H230, H231),
  • to set flags,

Rules like H241 H252 H219 all do the same but don't have a 'shared maintainer'. As we've allowed personal rules (and I do not remember why) it's probably more cumbersome to ping a rule author than jfdi by creating a rule yourself.

I personally don't understand the use cases of some rules such as H133 (can be done by changing the personal notification settings?) or H204 (as I don't believe in single default assignees due to cookie-licking) or H170 (edit the user profile name to make clear that it's a private account?).

Is it the number of Rules that is the problem, or more the number of Conditions? I.e. would it help if we asked users who have 2 rules that could easily be merged, to do so? E.g. H274 and H275 could easily be merged into a single Rule, but it would have the same number of Conditions so merging might not help...

I think the number of conditions is probably the biggest factor so combining them into fewer rules might not help very much.

I was curious how much antivandalism was slowing things down so I disabled the rule and then checked transcripts on transactions that happened while the rule was disabled. It didn't seem to make a significant different in herald runtime. So although antivandalism does a couple of expensive sql queries, it appears that I've optimized that code sufficiently - the difference was only a couple of milliseconds at most.

I see some patterns, like

  • "inform me when natural language foo was mentioned" (such as H188, H92, H81),
  • "email me explicitly when tasks in project XYZ get the UBN priority" (H154, H294, H236, H230, H231),
  • to set flags,

Rules like H241 H252 H219 all do the same but don't have a 'shared maintainer'. As we've allowed personal rules (and I do not remember why) it's probably more cumbersome to ping a rule author than jfdi by creating a rule yourself.

I personally don't understand the use cases of some rules such as H133 (can be done by changing the personal notification settings?) or H204 (as I don't believe in single default assignees due to cookie-licking) or H170 (edit the user profile name to make clear that it's a private account?).

The reason for H133 was, that I set all preferences to notification only, not email, but I still got mails for some types of activity where there was no setting, so this was the only way to change this. I turned it off for now, I hope that I don't get mails now.... I've set all available settings from mail to notify now, let's see if the behaviour of phab changed in the meantime.

Just looking manually at enabled global rules that target tasks, the following rules appear to all do the same thing for different cases

Group 1: add the owner's tag if assigned to the owner[1]

  • {H370}
  • {H336}
  • {H318}
  • {H317}
  • {H311}
  • {H306}
  • {H302}
  • {H268}
  • {H243}
  • {H212}
  • {H182}
  • {H128}
  • {H125}
  • {H91}

Group 2: Add the owner's tag if assigned to or authored by the owner

  • {H322}
  • {H271}
  • {H267}
  • {H261}
  • {H248}
  • {H233}

Group 3: Add tags for spaces

  • {H270}
  • {H264}
  • {H256}
  • {H255}
  • {H237}
  • {H178}
  • {H177}
  • {H171}
  • {H116}

Is there a more efficient way to be doing this?

[1] Owner meaning the specific user; these are global herald rules

Is there a more efficient way to be doing this?

I could probably build a custom action that's something like

"add tag #user-$u when assigned to user: $u" and apply that globally, once, eliminating most of the first group.

Group 4: Umbrella projects: if tagged as one of a few other projects, add an extra project (once):

  • {H337}
  • {H314}
  • {H285}
  • {H216}
  • {H187}
  • {H143}
  • {H131}
  • {H109}

Group 5: Umbrella or mirror projects: if tagged as one of a few other projects (or just one other project), add an extra project (always):

  • {H232}
  • {H193}
  • {H175}
  • {H174}
  • {H24}
  • {H15}
  • {H14}
  • {H10}

Since group 5 rules are activated each time they match, it may be worth investigating if @Maintenance_bot would be able to take over those functions (@Ladsgroup?) Group 4 would be similar, but likely harder to implement

Also, not really a group, but

  • {H315}
  • {H329}

both email a list if a task is UBN and has a specific tag

If you want to move things to the maintenance bot, PR is very welcome: https://github.com/Ladsgroup/Phabricator-maintenance-bot

I made a PR to the maintenance bot that could replace umbrella project adding Herald rules (group 5 in T108586#5669368).

https://github.com/Ladsgroup/Phabricator-maintenance-bot/pull/22

@Aklapper Hey, is it fine if we move the group 4 herald rules to maintenance bot?

@Aklapper Hey, is it fine if we move the group 4 herald rules to maintenance bot?

the pull request is for group 5, which run {always} instead of, as group 4 does, {once}

Thanks. Making it work with group 4 shouldn't be too hard.

@Aklapper Hey, is it fine if we move the group 4 herald rules to maintenance bot?

Is there any general info available, or a dedicated task / thread? I lack context what that bot does and why.

Maybe moving some actions to an external place might be workaround to improve performance, but does this potentially complicate making/reverting changes (which are currently Herald rules) when it comes to how quickly changes get deployed, how many folks can make such changes, etc? I honestly don't know...

Maybe moving some actions to an external place might be workaround to improve performance, but does this potentially complicate making/reverting changes (which are currently Herald rules) when it comes to how quickly changes get deployed, how many folks can make such changes, etc? I honestly don't know...

Yeah IMO it's a trade-off, I don't think we should move all Herald rules but some clearly would benefit from being done by the bot. To mitigate the usability of the bot, I can work on its documentation but also I want to mention that the source code is linked on the bot's profile page, it has seven contributors (so it's not just me) and it's configuration is rather straightforward, for example: https://github.com/Ladsgroup/Phabricator-maintenance-bot/blob/master/column_mover.py#L33

I personally think we should automate even more work (not Herald work that's already automated) to the bot (that's why it has a rather general name) to reduce the toil our tech people has to do (I even worked on machine learning for triaging but that's too early for making it prioritize automatically).

The patch is merged and working as expected. I disabled cases in group 5. It'll be done by @Maintenance_bot

I still think doing group 4 is also pretty easy. You just should go through transactions to make sure the project has not been added before.

now it supports group 4 type of cases as well: https://github.com/Ladsgroup/Phabricator-maintenance-bot/commit/131808cd06078eb4542165450ee2d32b9b32925b

I migrated H30 and I'm disabling it. I also wrote a quick js to make migrating herald rule to maint bot easier. I think we should slowly take the data out of python file into a json for when someone have time.

I migrated and disabled several herald rules of group 4. I'm going to add more later (plus other groups like personal rules).

Do we have some numbers on how much is the median for saving now? its grafana board doesn't have anything like that.