Closed
Bug 1451661
Opened 7 years ago
Closed 7 years ago
Highlight significant alerts from alert summary
Categories
(Tree Management :: Perfherder, enhancement, P1)
Tree Management
Perfherder
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: igoldan, Assigned: igoldan)
References
Details
Attachments
(1 file)
47 bytes,
patch
|
wlach
:
review+
jmaher
:
feedback+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8969221 -
Flags: review?(wlachance)
Attachment #8969221 -
Flags: feedback?(jmaher)
Comment 2•7 years ago
|
||
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( ?
Attachment #8969221 -
Attachment is patch: true
Attachment #8969221 -
Attachment mime type: text/x-github-pull-request → text/plain
Attachment #8969221 -
Flags: feedback?(jmaher) → feedback+
Assignee | ||
Comment 3•7 years ago
|
||
(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
Assignee | ||
Comment 4•7 years ago
|
||
Sheriffs add the 'significant' label manually.
Comment 5•7 years ago
|
||
Thanks :igoldan
Comment 6•7 years ago
|
||
Comment on attachment 8969221 [details] [diff] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3469
One change requested, see PR
Attachment #8969221 -
Flags: review?(wlachance)
Assignee | ||
Updated•7 years ago
|
Attachment #8969221 -
Flags: review?(wlachance)
Comment 7•7 years ago
|
||
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
Attachment #8969221 -
Flags: review?(wlachance) → review+
Comment 8•7 years ago
|
||
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)
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•