Closed Bug 1285368 Opened 4 years ago Closed 4 years ago

Performance alert summary-copy-as-text could be even more useful with some minor changes

Categories

(Tree Management :: Perfherder, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wlach, Assigned: rwood)

Details

Attachments

(1 file)

avih had some suggestions for improving the summary-copy-as-text, which sound useful:

https://bugzilla.mozilla.org/show_bug.cgi?id=1272433#c5

Looks great. Two things which I'd have said earlier if I saw an example of the summary-copy-as-text before it landed:

1. The date would be useful to, because the values can change over time at the alerts manager, so at least know that the view is NN days/weeks old.

2. It would be useful to have also the before/after numbers, and not only the change in %, because on some tests the absolute value is as important as the change percentage. E.g. copy "<test name>: before: X   After: Y   Change: NN% improvement/regression"
(In reply to William Lachance (:wlach) from comment #0)
> avih had some suggestions for improving the summary-copy-as-text, which
> sound useful:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1272433#c5
> 
> Looks great. Two things which I'd have said earlier if I saw an example of
> the summary-copy-as-text before it landed:
> 
> 1. The date would be useful to, because the values can change over time at
> the alerts manager, so at least know that the view is NN days/weeks old.
> 
> 2. It would be useful to have also the before/after numbers, and not only
> the change in %, because on some tests the absolute value is as important as
> the change percentage. E.g. copy "<test name>: before: X   After: Y  
> Change: NN% improvement/regression"

Experimenting a bit locally, I think I like this way of formatting it: "<test name>: X -> Y (NN% improvement/regression)"

Here's what a recent regression would look like with this schema:

  ts_paint linux64 opt e10s: 606.92 → 624.42 (2.88% worse)

vs.

  ts_paint linux64 opt e10s: Before: 606.92 After: 624.42 (2.88% worse)

--

Including the date, I think something like this might work well

```
== Change summary for alert #1234 (as of July 24th 2016 15:42 UTC) ==

Summary of tests that regressed:
  tabpaint summary linux64 opt e10s: 55.7 → 59.97 (7.66% worse)

Summary of tests that improved:
  damp summary linux64 pgo: 222.76 → 212.37 (4.66% better)
  damp summary linux64 pgo: 211.8 e10s → 212.37 (5.02% better)

For up to date results, see: https://treeherder.allizom.org/perf.html#/alerts?id=2891
```

What do you think?
Flags: needinfo?(jmaher)
Flags: needinfo?(avihpit)
Good idea, I also think it's better, but I think plain ASCII instead of the unicode arrow would be easier to pass around. E.g. '-->' would work equally well IMO.
Flags: needinfo?(avihpit)
agree with Avi here, and this looks very useful!
Flags: needinfo?(jmaher)
Assignee: nobody → rwood
Status: NEW → ASSIGNED
Quick question, the text copied to the clipboard is generated by AlertSummary.getTextualSummary():

https://github.com/mozilla/treeherder/blob/master/ui/js/models/perf/alerts.js#L78

Which is used not only for the "Copy Summary" text, but also for the text used filing a new bug:

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

Just to clarify, do we want the changes in comment 1 to be applied to both the clipboard text and bug template? Or is this change just for the clipboard text only? Thanks!
Flags: needinfo?(wlachance)
This change is for the clipboard text only. Some refactoring of the code may be required. :)
Flags: needinfo?(wlachance)
Attachment #8775310 - Flags: review?(wlachance)
Comment on attachment 8775310 [details] [review]
[treeherder] rwood-moz:rwood-bug1285368 > mozilla:master

This looks great, just a few adjustments needed.
Attachment #8775310 - Flags: review?(wlachance) → review-
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.