1178222 - when filing performance regressions we need to have a clear template to make the bug actionable for developers

Status: RESOLVED INVALID

(Testing :: Talos, defect)

Description

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

developers are confused by our existing templates from alert manager.  We should have a consistent set of information that we require for a bug to be filed.

Comment 1

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

Ideally we would collect 6 data points for each test/platform on the offending revision and the previous revision (i.e. no need for a large set of historical data).

Ideally a textual snapshot of the values before/after the change as well as a link to compare view in perfherder.

Comment 2

[William Lachance (:wlach)]

For discussion purposes, here's an instance of the current template:

"Talos has detected a Firefox performance regression from your commit d71995d0ccd2 in bug 1167090.  We need you to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=d71995d0ccd2&showAll=1

On the page above you can see Talos alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#DAMP

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p linux64,win32 -u none -t g2  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e <path>/firefox -a damp

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Friday, or the offending patch will be backed out! ***

Our wiki page oulines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling"

Comment 3

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #1)
> Ideally we would collect 6 data points for each test/platform on the
> offending revision and the previous revision (i.e. no need for a large set
> of historical data).
> 
> Ideally a textual snapshot of the values before/after the change as well as
> a link to compare view in perfherder.

I'd also add to this list of proposed changes:

1. Add `--rebuild 6` to the try syntax
2. Include a note about retriggering enough runs on a branch (e.g. mozilla-central) to generate a baseline to compare against the try revision?
3. Include instructions on using compareperf to evaluate changes on try

If that's sufficient, I can attempt to write something up.

Comment 4

[Geoff Brown [:gbrown]]

This may be a personal issue, but I'll mention it in case it can help: 

It is not immediately clear to me who is "you" -- who is it that is being addressed and needs to read further. If you are not the patch author but are cc'd on a bug and you read "Talos has detected a Firefox performance regression from YOUR commit d71995d0ccd2 in bug 1167090.  We need YOU to address this regression" you may wonder if that is your commit/bug and check hg/bugzilla to verify; if it isn't your commit, you might not check the next time you receive that templated bugmail.

I would prefer to see:

  Talos has detected a Firefox performance regression from commit d71995d0ccd2 in bug 1167090. 

  :gbrown -- As patch author, we need you to address this regression...

Comment 5

[Avi Halachmi (:avih)]

(In reply to William Lachance (:wlach) from comment #3)
> 1. Add `--rebuild 6` to the try syntax
> 2. Include a note about retriggering enough runs on a branch (e.g.
> mozilla-central) to generate a baseline to compare against the try revision?
> 3. Include instructions on using compareperf to evaluate changes on try


Yup. Maybe change 'rebuild' to 'retrigger' or 'rerun' or 'retest' or `testruns' or 'perfruns' ?

'rebuild' sounds like we want more builds of Firefox, while in fact we want to run the performance tests more times on the same build.

In general though when we say "actionable by developers" I think this summarizes what it should mean:

1. (gbrown) - understand who's being addressed.

2. understand what are the numbers referencing (a diff between rev X and rev Y).

3. be able to reproduce our results (via try pushes of rev X and rev Y and some retriggers).

4. a link to the compare-view, but IMO also a summary of the comparison as text at the bug itself (because it's easier to view as part of the bug and because the data is not kept forever on the servers).


Is that a good summary?

Comment 6

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

do we need raw data in the bug report?  The rest of this looks good so far!

As for rebuild vs retrigger, etc. this is already implemented as of bug 1163698, we can take that discussion there.

Comment 7

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #6)
> do we need raw data in the bug report?

IMO raw enough to make it clear what all the perf-changes are (including improvements).

I think it can either be a formatted textual dump of the entire compare-view page, or maybe just the lines which show meaningful difference. If we take the "meaningful difference only" path, then IMO it should be clear what lines are not present at the summary because the perf diff is negligible vs which lines don't show at the summary because we have no results for those.

But we can fine tune this in a different bug. The point here is to make it clear at the bug itself what are all the perf changes - and do that better than we do currently (minimal info at the bug title).

Comment 8

[Vladan Djeric (:vladan)]

Let's work on the wording in here https://bugzilla.mozilla.org/show_bug.cgi?id=1178222

Comment 9

[Vladan Djeric (:vladan)]

Oops
https://etherpad.mozilla.org/24-hour-backout-template

Comment 10

[William Lachance (:wlach)]

We've updated the template some since this bug was filed (now used in Perfherder). I think I'm just going to mark this one as invalid, the current one works well enough for our purposes (and we can file specific bugs to improve as needed). I'll file a specific bug in perfherder for avih's suggestion in comment 7.

Incidentally, in retrospect I'm not sure if 24 hour backouts were ever really the right goal to shoot for. It turns out that filing informative bugs and giving developers tools to resolve them seems to have the sort of impact we want (developers taking these things seriously).