1270524 - Bug reporting templates should have a textual summary of what regressed / improved

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

We currently link to an alert, but it would be nice to automatically generate a textual summary of what regressed and put it into the actual bug report. Quoting :avih from bug 1178222

> 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 1

[William Lachance (:wlach)]

Thinking about this a bit more, I suspect it makes sense to do bug 1260805 before this one, as we'll mostly likely be moving the template generation code to python for that (the JS code we used to generate the alert template was always a bit of a hack).

That said, functionally this bug is pretty simple: just add a simple textual summary like that displayed in the alert view to the template. Taking this as an example:
https://treeherder.mozilla.org/perf.html#/alerts?id=1412

I'd put this text in the summary:

```
Summary of tests that regressed:

tp5o Main_RSS osx-10-10 opt e10s - 2.53% worse
tp5o_scroll summary windows8-64 opt e10s - 5.02% worse
tp5o_scroll summary windowsxp opt - 2.61% worse
tp5o_scroll summary windowsxp opt e10s - 2% worse
tp5o_scroll summary windowsxp pgo - 2.37% worse
```

Comment 2

[William Lachance (:wlach)]

I actually opted to keep most of the logic in JS when I implemented bug 1260805, so this should be a very straightforward modification to alerts.js and the alerts template.

We should just add a template tag like `{{ alertSummary }}` to the template (modify treeherder/perf/fixtures/performance_bug_templates.yaml), and then modify the alerts code to generate a string to substitute in (modify ui/js/controllers/perf/alerts.js). Roy, would you still be interested in working on this?

Comment 3

[crosscent]

(In reply to William Lachance (:wlach) from comment #2)
> 
> We should just add a template tag like `{{ alertSummary }}` to the template
> (modify treeherder/perf/fixtures/performance_bug_templates.yaml), and then
> modify the alerts code to generate a string to substitute in (modify
> ui/js/controllers/perf/alerts.js). Roy, would you still be interested in
> working on this?

I think Vibhor wants to work on the hover graph, so I can definitely work on this.

Comment 4

[GitHub Autolander Bot]

Attachment: [treeherder] crosscent:bug1270524 > mozilla:master

Comment 5

[crosscent]

Will, I have made a PR so it's easier to see my questions.

Right now, my implementation is to display all the alerts that have regressed in the summary report, which is what I think the goal of this bug is.

I'm guessing that you want the ng-repeat be implemented as a scope in alert.js, and be called $scope.alertSummary, then edit the fixture to include alertSummary. I thought fixtures are used to prepopulate the database, and adding alertSummary would be modification to the Django side?

Could you please elaborate more on the modification to performance_bug_templates.yaml, and how it will affect the template?

Comment 6

[Avi Halachmi (:avih)]

I'll leave it up to you, but before actual work gets done, I'd suggest to change the title from " Bug reporting templates should have a textual summary of what regressed" to " Bug reporting templates should have a textual summary of what regressed/improved".

I.e. improvements are important to allow weighting the overall impact before making decisions.

Comment 7

[William Lachance (:wlach)]

(In reply to crosscent from comment #5)
> Will, I have made a PR so it's easier to see my questions.
> 
> Right now, my implementation is to display all the alerts that have
> regressed in the summary report, which is what I think the goal of this bug
> is.
> 
> I'm guessing that you want the ng-repeat be implemented as a scope in
> alert.js, and be called $scope.alertSummary, then edit the fixture to
> include alertSummary. I thought fixtures are used to prepopulate the
> database, and adding alertSummary would be modification to the Django side?
> 
> Could you please elaborate more on the modification to
> performance_bug_templates.yaml, and how it will affect the template?

Actually I was just thinking that you'd put the information in the textual bug report. The place to do that would be around here:

https://github.com/mozilla/treeherder/blob/master/ui/js/controllers/perf/alerts.js#L15

The template would need to be modified to have space for the text. Right now we only have the talos template which you can find here:

https://github.com/mozilla/treeherder/blob/master/treeherder/perf/fixtures/performance_bug_templates.yaml#L10

I'd just put in a parameter like {{ alertTextSummary }} and then pass it in from the JS side. No backend work should be necessary aside from that.

And yeah, Avi's suggestion makes sense to me. Let's include both improvements and regressions. Just make the improvement list a mirror image of the regression text we described in comment 1 (if there were no improvements, omit that part)

Comment 8

[crosscent]

Thank you for your suggestion Avi! I have made the appropriate changes.

Will, I now understand what the bug reporting template is used for, and I think the new commit should do just that! I'm starting new lines with "\n", and I don't dare to click the create bug button, but I think the resulting text should look fine.

Comment 9

[Hannes Verschore [:h4writer]]

This would be very handy! As a side question. Would this also allow me to get the "textual representation" without filing a bug? The workflow I have is to add sort of a summary when big patches land. This gives an idea how big of an improvement that patch speed-wise is. In that case I don't want to open a new bug, just get the summary and be able to paste it in the bug.

Comment 10

[William Lachance (:wlach)]

(In reply to Hannes Verschore [:h4writer] from comment #9)
> This would be very handy! As a side question. Would this also allow me to
> get the "textual representation" without filing a bug? The workflow I have
> is to add sort of a summary when big patches land. This gives an idea how
> big of an improvement that patch speed-wise is. In that case I don't want to
> open a new bug, just get the summary and be able to paste it in the bug.

This should be pretty easy, maybe we could add an option on the right dropdown menu to copy the textual summary to the clipboard?

(this would necessitate showing the dropdown menu even when the user is not logged in, but that's probably the right thing to do anyway)

If that sounds good I can file a bug.

Comment 11

[Hannes Verschore [:h4writer]]

(In reply to William Lachance (:wlach) from comment #10)
> (In reply to Hannes Verschore [:h4writer] from comment #9)
> > This would be very handy! As a side question. Would this also allow me to
> > get the "textual representation" without filing a bug? The workflow I have
> > is to add sort of a summary when big patches land. This gives an idea how
> > big of an improvement that patch speed-wise is. In that case I don't want to
> > open a new bug, just get the summary and be able to paste it in the bug.
> 
> This should be pretty easy, maybe we could add an option on the right
> dropdown menu to copy the textual summary to the clipboard?
> 
> (this would necessitate showing the dropdown menu even when the user is not
> logged in, but that's probably the right thing to do anyway)
> 
> If that sounds good I can file a bug.

Yes, that sounds good!

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8765947 [details] [review]
[treeherder] crosscent:bug1270524 > mozilla:master

This looks good to me! :jmaher, do you have any final thoughts or follow up issues before I resolve the bug?

Comment 13

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

Comment on attachment 8765947 [details] [review]
[treeherder] crosscent:bug1270524 > mozilla:master

I am excited to see this

Comment 14

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/4de5c0a2de6d530c44c400994a9ca055270208fc
Bug 1270524 - Display textual summary of what regressed (#1621)

Comment 15

[William Lachance (:wlach)]

Awesome, thanks Roy!

Comment 16

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #10)
> (In reply to Hannes Verschore [:h4writer] from comment #9)
> > This would be very handy! As a side question. Would this also allow me to
> > get the "textual representation" without filing a bug? The workflow I have
> > is to add sort of a summary when big patches land. This gives an idea how
> > big of an improvement that patch speed-wise is. In that case I don't want to
> > open a new bug, just get the summary and be able to paste it in the bug.
> 
> This should be pretty easy, maybe we could add an option on the right
> dropdown menu to copy the textual summary to the clipboard?
> 
> (this would necessitate showing the dropdown menu even when the user is not
> logged in, but that's probably the right thing to do anyway)
> 
> If that sounds good I can file a bug.

Filed bug 1284325 for this. :)

Comment 17

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1270524-2 > mozilla:master

Comment 18

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/c63895e89fd7d7ad0febd13b3ae78c1a070e586d
Bug 1270524 - Improve formatting of textual description of alert summary (#1659)