1451661 - Highlight significant alerts from alert summary

Status: RESOLVED FIXED

(Tree Management :: Perfherder, enhancement, P1)

Description

[Ionuț Goldan [:igoldan]]

Enable a sheriff to mark any alerts inside an alert summary as "Significant".

We want to use the same mechanics for labeling alerts: select them via the checkbox, then have a new "Significant" button. Alerts marked with it will be easy to spot, maybe ordered on top of other alerts. This is more of a visual utility.

Story behind this:

We are notified about performance regressions via Perfherder's alert summaries.
These summaries usually group multiple perf alerts originating from varying platforms/tests/build types/repos.

Retriggering/backfilling on all of these is too resource consuming for us. And also pointless, as a subset of these alerts is enough to identify the culprit.

Comment 1

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3469

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8969221 [details] [diff] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3469

Review of attachment 8969221 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand how we determine what is significant- is there heuristics we use?  Is this patch just the work to allow it and will be active in the UI and hiding !significant will be all of them?

Overall I think this looks reasonable.

::: ui/js/models/perf/alerts.js
@@ +69,5 @@
> +            const toggledSignificance = !this.significant;
> +            this.modify({
> +                significant: toggledSignificance
> +            }).then(
> +                function () {

nit: couldn't |function () {| be on the same line as then( ?

Comment 3

[Ionuț Goldan [:igoldan]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #2)
> Comment on attachment 8969221 [details] [diff] [review]
> Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3469
> 
> Review of attachment 8969221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand how we determine what is significant- is there heuristics
> we use?  Is this patch just the work to allow it and will be active in the
> UI and hiding !significant will be all of them?

'significant' is just a label sheriffs assign to an alert subset, from an alert summary.
The idea is that retriggers/backfills should be done only on them, not on the other alerts.

Out of a pile of 40 alerts, we would then easily know where to check for new results of perf tests.
I wrote more about this here [1], in the Retrigger/backfill [2] section.


> Overall I think this looks reasonable.
> 
> ::: ui/js/models/perf/alerts.js
> @@ +69,5 @@
> > +            const toggledSignificance = !this.significant;
> > +            this.modify({
> > +                significant: toggledSignificance
> > +            }).then(
> > +                function () {
> 
> nit: couldn't |function () {| be on the same line as then( ?
I used the same style as in ui/js/controllers/perf/alerts.js

[1] https://bug1451655.bmoattachments.org/attachment.cgi?id=8965239

Comment 4

[Ionuț Goldan [:igoldan]]

Sheriffs add the 'significant' label manually.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks :igoldan

Comment 6

[William Lachance (:wlach)]

Comment on attachment 8969221 [details] [diff] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3469

One change requested, see PR

Comment 7

[William Lachance (:wlach)]

Comment on attachment 8969221 [details] [diff] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3469

r+ with comments in github pr addressed

Comment 8

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/9a47ad307cb2cad4d06db45b10402cfe63254c5f
Bug 1451661 - Highlight significant alerts from alert summary (#3469)