1481815 - generate damp subtest alerts

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

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

the devtools team has been manually sheriffing the subtests for the last year, it would save them a lot of time to use our sheriffing tools and alert management tools.

We should be able to specify in perfherder to generate alerts on the subtests.  If this is too much load we can specify a branch like autoland or ignore pgo.  We also need to make sure that these do not show up in the default talos list of alerts, so they should be in a different "product", maybe talos-subtests, or maybe talos-damp.  That might be tricky, but we could hard code it.

Once this is implemented, I believe the devtools team could save a lot of time immediately.  In addition, it is very likely they would have some suggestions for the sheriffing/alerting interface and workflow.

Comment 1

[William Lachance (:wlach)]

Should be pretty easy to do this by setting the `shouldAlert` property on the subtest to true, and changing the performance framework to something like `damp`. All of these changes could be done inside talos itself, the only perfherder change required would be adding a talos-damp framework.

Comment 2

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

:igoldan, could you add a 'talos-damp' framework to perfherder?  Once that is in, I can work on the talos changes.

Comment 3

[GitHub Bugzilla PR Linker]

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

Comment 4

[William Lachance (:wlach)]

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

Actually, how about we just call this framework `devtools`? Less jargon, and gives us the flexibility in the future if e.g. we're no longer using talos to run damp, or we want other devtools performance suites, or ...

Comment 5

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/a1ce4ff3573cd439a1da9688fbaca3baef3dd3f7
Bug 1481815 - add talos-damp framework to support damp subtest alerts. r=wlach (#3895)

Comment 6

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

Attachment: Bug 1481815 - generate damp subtest alerts.

allow damp to generate alerts for the subtests

Comment 7

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

Comment on attachment 8999567 [details]
Bug 1481815 - generate damp subtest alerts.

I am struggling with the implementation here.  I think getting the subtests to alert is easy, getting them put in 'devtools' will be harder.  I think we either need to:
1) move damp summary to 'devtools' as well
2) post duplicated PERFHERDER_DATA from the 'damp' job with just the subtests and use the overall framework 'devtools'

possibly there are other options and I would like some opinions :)

Comment 8

[William Lachance (:wlach)]

Comment on attachment 8999567 [details]
Bug 1481815 - generate damp subtest alerts.

Moving the summary to devtools is (IMO) obviously the correct solution. It would be confusing to have the results for the same test suite in two different places!

Comment 9

[Ionuț Goldan [:igoldan]]

(In reply to Joel Maher ( :jmaher ) (UTC+2) from comment #7)
> Comment on attachment 8999567 [details]
> Bug 1481815 - generate damp subtest alerts.
> 
> I am struggling with the implementation here.  I think getting the subtests
> to alert is easy, getting them put in 'devtools' will be harder.  I think we
> either need to:
> 1) move damp summary to 'devtools' as well
> 2) post duplicated PERFHERDER_DATA from the 'damp' job with just the
> subtests and use the overall framework 'devtools'
> 
> possibly there are other options and I would like some opinions :)

I'm not a fan for duplication, so yes, I agree with William.

Comment 10

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

in going this route it is likely that we would not catch damp summary regressions properly since perf sheriffs would need to look at devtools and the list of alerts in devtools would be primarily subtests instead of summary.  My understanding is that the summary value that we currently sheriff is lower value in general as the subtests are more interesting.

If we were to sheriff the summary only or do some hybrid sheriffing, then we would need to build some additional features into perfherder alerts to view summary alerts only.

:ochameau, in this model would it be safe to assume that the devtools team would be ok sheriffing 100% of the alerts related to damp and just ask perf sheriffs for help with the tooling, backfill/retrigger, etc. as needed?

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Joel Maher ( :jmaher ) (UTC+2) from comment #10)
> in going this route it is likely that we would not catch damp summary
> regressions properly since perf sheriffs would need to look at devtools and
> the list of alerts in devtools would be primarily subtests instead of
> summary.  My understanding is that the summary value that we currently
> sheriff is lower value in general as the subtests are more interesting.

That's exact, in many cases, summary doesn't catch subtest regressions
and also in some other cases reports a regressions whereas we only added/removed a subtest.
But still, sometimes, summary reports real improvements/regressions.

> If we were to sheriff the summary only or do some hybrid sheriffing, then we
> would need to build some additional features into perfherder alerts to view
> summary alerts only.
> 
> :ochameau, in this model would it be safe to assume that the devtools team
> would be ok sheriffing 100% of the alerts related to damp and just ask perf
> sheriffs for help with the tooling, backfill/retrigger, etc. as needed?

It is hard to say before seeing how many alerts we get and I would have to see with Patrick if we can staff people on DevTools on the long run.
Can we experiment for a couple of weeks and rollback to previous setup if there is too many alerts, or if we don't feel confortable sheriffing 100% on our side?

Comment 12

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

yes, we could easily roll back to the old way, I agree we can experiment and see what makes the most sense based on volume, tooling, staffing.

Comment 13

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

Comment on attachment 8999567 [details]
Bug 1481815 - generate damp subtest alerts.

Here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de39fb8d38104cc12d5cd8cebc5bacdf97d753e9&filter-searchStr=talos

In doing this change we would lose history as the signatures we post to today are in the talos framework and after this change it will be in the devtools framework.

Since we need 25 data points to find a regression, I think this isn't a big problem w.r.t missing regressions, but it seems like it could be confusing when we do not see historical data.  Also any tools that pull data from damp would need to be updated.

Comment 14

[Robert Wood [:rwood]]

Comment on attachment 8999567 [details]
Bug 1481815 - generate damp subtest alerts.

Robert Wood [:rwood] has approved the revision.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41443a067773
generate damp subtest alerts. r=rwood

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/41443a067773