1451657 - Add new 'confirming' status to alert summaries

Status: RESOLVED FIXED

(Tree Management :: Perfherder, enhancement, P1)

Description

[Ionuț Goldan [:igoldan]]

Basically, we want to enable a sheriff to mark an alert summary as "confirming" soon after he does some basic retriggering/backfilling on the relevant alerts.

You can read more about this need in the document attachment from bug 1451655, at Retrigger/backfill section.

We'll add a "Mark as 'confirming'" option under the top-right dropdown from the alert summary. Then we want to be able to filter summaries based on this new state.

Comment 1

[GitHub Bugzilla PR Linker]

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

Comment 2

[Ionuț Goldan [:igoldan]]

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

This bug is based continues work from bug 1451661, which will be deployed soon.
The way I view this: the code sheriff stars the significant alerts. Then he retriggers/backfills the starred alerts. And then he marks the alert summary as "confirming", using the top-right dropdown.
The I come in and filter the confirming alerts and continue investigating them, as usual.

I want to know if you're ok with the new mechanics.

Comment 3

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

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

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

I like the approach in general.  A few questions:
1) I assume you will go to https://treeherder.mozilla.org/perf.html#/alerts?status=9 by default
2) How will this affect all other alerts (non talos)?  will it create more work for the perf sheriffs (i.e. you)?
3) does this apply to alert summaries or individual alerts?  it looks like individual alerts which I think will be confusing when looking at 'confirming' only alerts

would it work better in the UI to not have a different state, but a filter checkbox (as we already have one for 'show improvements') ?

I know you have put much more thought into this approach- I don't expect to get this perfect on round 1, but I don't want there to be too much confusion that we have to revisit this again next week :)

Comment 4

[Ionuț Goldan [:igoldan]]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #3)
> Comment on attachment 8973162 [details] [diff] [review]
> Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3522
> 
> Review of attachment 8973162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the approach in general.  A few questions:
> 1) I assume you will go to
> https://treeherder.mozilla.org/perf.html#/alerts?status=9 by default

Yes, you are right.

> 2) How will this affect all other alerts (non talos)?  will it create more
> work for the perf sheriffs (i.e. you)?

No, not really. There are indeed new steps to triage alerts, but they are not mandatory.
They are the same steps I described in comment 2.
I can still do my usual perf sheriffing, exactly as I did so far. I want to enforce the new steps onto
the code sheriffs.


> 3) does this apply to alert summaries or individual alerts?  it looks like
> individual alerts which I think will be confusing when looking at
> 'confirming' only alerts

I don't think I understood your question here. 'confirming' state applies only to alert summaries; simple alerts don't
have this state.

> would it work better in the UI to not have a different state, but a filter
> checkbox (as we already have one for 'show improvements') ?

I order to do this, we have to add a new "Mark as confirming" button for when we select alerts.
Now that I think about this suggestion, it makes more sense because it ties well with the way the state machine was implemented.

> I know you have put much more thought into this approach- I don't expect to
> get this perfect on round 1, but I don't want there to be too much confusion
> that we have to revisit this again next week :)

Comment 5

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

thanks, if this is alert summaries, then I am happy.

Comment 6

[GitHub Bugzilla PR Linker]

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

Comment 7

[Ionuț Goldan [:igoldan]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #4)
> (In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #3)
>
> > would it work better in the UI to not have a different state, but a filter
> > checkbox (as we already have one for 'show improvements') ?
> 
> I order to do this, we have to add a new "Mark as confirming" button for
> when we select alerts.
> Now that I think about this suggestion, it makes more sense because it ties
> well with the way the state machine was implemented.
> 
> > I know you have put much more thought into this approach- I don't expect to
> > get this perfect on round 1, but I don't want there to be too much confusion
> > that we have to revisit this again next week :)

I tried this second approach, because it fits better into the current implementation and makes more sense. I added the confirming state to both alert and alert summaries.
Basically, the way an alert can switch states is roughly this: untriaged -> confirming -> acknowledge/downstream/invalid/reassign.
The new state will act as an intermediary.

Will, can you review this?

Comment 8

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/e3c9c9ed99057f2a7d34c86d2627ccf41a12432e
Bug 1451657 - Integrate confirming state into current logic flow (#3547)

Comment 9

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

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

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

thanks, I don't see anything wrong here

Comment 10

[Ionuț Goldan [:igoldan]]

:emorley I think this should be closed now, am I right?