1301129 - in perfherder alerts reassigned are marked as investigating

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

with the large volume of alerts summaries that we have, most of them end up as downstream or reassigned.  In trying to be a good steward and cleanup old alerts, I found that we had so many alerts which are marked as 'investigating' status, but they were just reassigned.  When I went in and marked the main alert as 'fixed', the reassigned alerts remain as 'investigating'.  We should allow alert summaries to have a status of 'resolved' or 'reassigned' such that we know that nothing else needs to be done.

Comment 1

[William Lachance (:wlach)]

I suspect this is probably trivial to fix. Do you have an example handy?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

sadly I have been cleaning them all up by hand- but I did find another one:
https://treeherder.mozilla.org/perf.html#/alerts?id=1992

this has an improvement and a downstream alert, it is downstreamed to a 'wontfix' status :)

Comment 3

[William Lachance (:wlach)]

(In reply to Joel Maher ( :jmaher) from comment #2)
> sadly I have been cleaning them all up by hand- but I did find another one:
> https://treeherder.mozilla.org/perf.html#/alerts?id=1992
> 
> this has an improvement and a downstream alert, it is downstreamed to a
> 'wontfix' status :)

Ok in this case I would expect the alert summary to be marked as an "improvement" - reassigned or downstream alerts shouldn't count towards the status, unless *all* the alerts are reassigned or downstream. Are there any cases where that's happening (all alerts marked reassigned or downstream, but summary is "investigating"?).

Comment 4

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

it is usually mismarked when there are a mix of improvements and regressions.  We still have the above alert as a status of 'investigating', not 'improvement'

Comment 5

[William Lachance (:wlach)]

(In reply to Joel Maher ( :jmaher) from comment #4)
> it is usually mismarked when there are a mix of improvements and
> regressions.  We still have the above alert as a status of 'investigating',
> not 'improvement'

Ok, but are the any other types of cases where you're seeing this, aside from this one (mix of improvement + downstream/reassigned?). Just want to make sure we cover all the bases with any fix.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

I believe it is only a mixed environment- I have seen some 'reassigned' status alerts which are when all the alerts are reassigned.

Comment 7

[William Lachance (:wlach)]

Hey Akhil, are you interested in taking this one on? I think this bug should be a relatively simple reintroduction to perfherder. :) So basically the business logic for our performance alerting system in one case (see above) is wrong and needs to be fixed. I think there are two steps to this bug:

1. Updating the business logic in this file: https://github.com/mozilla/treeherder/blob/master/treeherder/perf/models.py#L160
2. Adding a unit test to make sure we handle this case: https://github.com/mozilla/treeherder/blob/master/tests/perfalert/test_alert_modification.py

As a refresher, basic information on perfherder (and instructions for getting setup) are here: https://wiki.mozilla.org/Auto-tools/Projects/Perfherder

Please let me or Joel know if you have questions. irc.mozilla.org #treeherder or #perfherder is probably the fastest route to reaching us. :)

Comment 8

[Akhilesh Pillai]

Will, Sounds good. I took a cursory look at it. Will start looking further into it later today and let you know if I have questions. Thanks

Comment 9

[Akhilesh Pillai]

Hi Will, 

I pinged the channel, however, I forgot you guys are on EST. I am on Pacific Time. 

I was wondering if there is a doc that helps me understand what PerformanceAlertSummary.STATUSES and PerformanceAlert.STATUSES are and how one should affect the another. I am looking at the code and do see  rules however not sure what new rule to add to fix this bug. 
1)I do see that https://treeherder.mozilla.org/perf.html#/alerts?id=1992 is a summary alert whose status is marked investigating. 
2)Further when I click downstream to alert #2016 I am directed to https://treeherder.mozilla.org/perf.html#/alerts?id=2016 whose summary status is "won't fix"
3) On alert id 2016 when I click on (reassigned from alert #1979) I am directed to https://treeherder.mozilla.org/perf.html#/alerts?id=1979 whose summary status and individual alert status is "reassigned"

Questions: 
So is the summary alert status of id=2016 ("won't fix")correct?
What should have been the alert status of alert id = 1992 ? given alert id=2016 is wont fix and further alert id=1979 is reassigned.
My understanding is if the alerts in a summary has 
   1) at least acknowledged and no untriaged it should be marked improvement.
I do not understand Downstream status and Reassigned status. Can you elaborate on this more? 
What are the different usecases that we are adding new rules for?

Comment 10

[William Lachance (:wlach)]

Yeah, we probably ought to document this somewhere. We have https://wiki.mozilla.org/Buildbot/Talos/Sheriffing#What_is_an_alert but it's not very helpful.

So we have a set of "alerts" and "alert summaries". An alert is a notification that a particular performance series changed. Alert summaries represent a group of alerts against a particular revision. Both types can have their own status, the alert status represents the particular alert's status, the alert summary's status represents that status of the group. 

A "downstream" status for an alert means that this alert occurred earlier on another development branch i.e. it's not unique
A "reassigned" status for an alert means that there is another alert summary whose revision range more closely matches the event that this alert represents.

We're just adding some extra business logic for a very specific case, where some alerts are marked as reassigned/downstream, but there are other alerts in the summary which are improvements. In that case the reassigned/downstream alerts aren't relevant for determining the summary's status, and we just want to look at the improvements.

Does that help? I'd be happy to over this on skype or whatever if you'd like. :)

Comment 11

[Akhilesh Pillai]

Thanks Will,  that helps will start coding it up.

Comment 12

[Akhilesh Pillai]

Attachment: Fix for bug 1301129

Comment 13

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 8792559 [details] [review]
Fix for bug 1301129

great stuff!

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8792559 [details] [review]
Fix for bug 1301129

I'd like to see a few adjustments, but this is close! Please re-r? when ready.

Comment 15

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/69b81c772fd9a9686ac11bcd87c9b212ad2abd58
Bug 1301129 - Ignore reassigned and downstream alerts if acknowledged (#1856)



The tests are passing locally.

	modified:   tests/perfalert/test_alert_modification.py
	modified:   treeherder/perf/models.py

Comment 16

[William Lachance (:wlach)]

Comment on attachment 8792559 [details] [review]
Fix for bug 1301129

This looks great, thank you Akhilesh!

Note for the future: if you could make your commit messages follow the format:

"Bug XXXXX - <short description of change>"

It would help me merge faster. :)