Closed
Bug 1288007
Opened 8 years ago
Closed 8 years ago
modify e10s dashboard (or build a new one) to show the delta over time
Categories
(Tree Management :: Perfherder, defect)
Tree Management
Perfherder
Tracking
(e10s+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: jmaher, Assigned: rwood)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
the e10s dashboard is great and served a great purpose to compare e10s vs non e10s performance numbers. In using it to assess differences on each branch, I ended up with a lot of differences to look at:
https://treeherder.mozilla.org/perf.html#/e10s?showOnlyBlockers=1&repo=mozilla-beta&timerange=604800
I found that looking on the graph for the last 90 days yielded 2 sets of data following the same pattern, just slightly off. This is data that we accepted as ok for releasing e10s- I want to know if things have changed since then.
To do this, I am proposing looking at data over time. Obviously it would be great if we could chart this out on a perf signature, but we can probably do it simpler.
Given a specific branch (above I used mozilla-beta), input 2 revisions (or dates) and use the 7 day timerange. To display the data we would:
for each test in rev1:
test_diff1 = e10s - non-e10s
for each test in rev2:
test_diff2 = e10s - non-e10s
for each test:
print test \t (test_diff2 - test_diff1)
This will ideally be run on a weekly basis, in fact it could default to today and the previous week, with options for the previous 2 weeks, 4 weeks and 6 weeks!
now if we have a change here, we will see a positive number for a regression, and a negative number for an improvement.
This should ideally filter as we do for the existing e10s dashboard where we show only significant changes, I believe this would be >abs(1.0)
Updated•8 years ago
|
Blocks: e10s-harness
tracking-e10s:
--- → +
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rwood
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
I was thinking of something like this for the selection of the base and new dates, and the sample range. What do you think?
Attachment #8784497 -
Flags: feedback?(wlachance)
Attachment #8784497 -
Flags: feedback?(jmaher)
Reporter | ||
Comment 2•8 years ago
|
||
Comment on attachment 8784497 [details]
suggested-pulldowns.png
hey, I am sold! that is easy to follow.
Attachment #8784497 -
Flags: feedback?(jmaher) → feedback+
Comment 3•8 years ago
|
||
Comment on attachment 8784497 [details]
suggested-pulldowns.png
Yup, that LGTM.
Attachment #8784497 -
Flags: feedback?(wlachance) → feedback+
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8788513 [details]
[treeherder] rwood-moz:perf_data > mozilla:master
First step is to add support to the perf data apis for getting data for a specific data range.
My logic is off somewhere when getting signatures using the start date - my unit test test_filter_signatures_by_range is failing with:
> assert len(resp.json.keys()) == 1
E assert 0 == 1
E + where 0 = len([])
E + where [] = <built-in method keys of dict object at 0x7fb1b2cf0280>()
E + where <built-in method keys of dict object at 0x7fb1b2cf0280> = {}.keys
E + where {} = <200 OK application/json body='{}'>.json
tests/webapp/api/test_performance_data_api.py:263: AssertionError
:wlach, would you mind having a look and see if you can spot where my logic is off? Thanks :)
Attachment #8788513 -
Flags: feedback?(wlachance)
Comment 6•8 years ago
|
||
Comment on attachment 8788513 [details]
[treeherder] rwood-moz:perf_data > mozilla:master
Left some feedback in the PR! Needs some adjustment but coming together well.
Attachment #8788513 -
Flags: feedback?(wlachance) → feedback+
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8788513 [details]
[treeherder] rwood-moz:perf_data > mozilla:master
Thanks :wlach, PR updated
Attachment #8788513 -
Flags: feedback+ → review?(wlachance)
Comment 8•8 years ago
|
||
Comment on attachment 8788513 [details]
[treeherder] rwood-moz:perf_data > mozilla:master
Requested a few changes, please re-r? when addressed. :)
Attachment #8788513 -
Flags: review?(wlachance)
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8788513 [details]
[treeherder] rwood-moz:perf_data > mozilla:master
Thanks for the review, some good points there! PR updated.
Attachment #8788513 -
Flags: review?(wlachance)
Comment 10•8 years ago
|
||
Commit pushed to master at https://github.com/mozilla/treeherder
https://github.com/mozilla/treeherder/commit/4fa0599d71843e93ef16c628d110725f1d9b853c
Bug 1288007 - Allow perf series and data apis to accept date range (#1831)
Comment 11•8 years ago
|
||
Comment on attachment 8788513 [details]
[treeherder] rwood-moz:perf_data > mozilla:master
I'm pretty happy with this updated PR.
Attachment #8788513 -
Flags: review?(wlachance) → review+
Comment 12•8 years ago
|
||
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8792876 [details]
[treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
First pass, needs work but would appreciate feedback. No support for sub-tests (yet) and also I can't seem to get any data unless setting both the base and new dates both to 'today', not sure why so any pointers would be appreciated! Thanks.
Attachment #8792876 -
Flags: feedback?(wlachance)
Comment 14•8 years ago
|
||
Comment on attachment 8792876 [details]
[treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
Great start, left some preliminary feedback. Let's try refactoring the series downloading code to reduce repetition -- hopefully things will just work after that (or at least it will become obvious why things aren't working).
Attachment #8792876 -
Flags: feedback?(wlachance) → feedback+
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to William Lachance (:wlach) from comment #14)
> Comment on attachment 8792876 [details]
> [treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
>
> Great start, left some preliminary feedback. Let's try refactoring the
> series downloading code to reduce repetition -- hopefully things will just
> work after that (or at least it will become obvious why things aren't
> working).
Sounds good, thanks for the feedback!
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8792876 [details]
[treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
Thanks for the feedback! PR updated. A couple of notes...
a) I prefer to have a separate 'trendtable.html' instead of using one combined 'comparetable.html', I tried combining them however there are a lot of differences and the resulting html code becomes super messy with so many 'ng-if' statements. Also potential to easily break the regular e10s dashboard and compare dashboard. The 'PhTrendTable' and 'PhCompareTable' components are however mostly duplicate code but I'm not sure if it's possible to get around that.
b) I still cannot get any data unless using 'today' for the base/new dates. When debugging it looks like the API itself is just not returning any data even though the correct dates are being provided. Not sure what I'm missing here.
c) Do we want subtest support for this? I'm assuming so, but haven't added it yet.
Attachment #8792876 -
Flags: feedback+ → feedback?(wlachance)
Comment 17•8 years ago
|
||
Comment on attachment 8792876 [details]
[treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
Waiting for next iteration.
Attachment #8792876 -
Flags: feedback?(wlachance)
Assignee | ||
Comment 18•8 years ago
|
||
From :jmaher's description:
To display the data we would:
for each test in rev1:
test_diff1 = e10s - non-e10s
for each test in rev2:
test_diff2 = e10s - non-e10s
for each test:
print test \t (test_diff2 - test_diff1)
Question...
i.e.
base delta is 11.55% (value 47.82)
new delta is 11.94% (50.44)
change is 5.47% (2.62)
Do we want all of these listed as percentages? Or do we want the first two deltas listed as just values but the change in percentage? Or do we want them all as straight values? Thanks!
Flags: needinfo?(wlachance)
Flags: needinfo?(jmaher)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8792876 [details]
[treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
Data issue fixed (thanks for your guidance :wlach), filtering via checkboxes added, and also added subtest support!
Dashboard currently displays results in percentages, but this may be changed depending on feedback in comment 18.
Ready for first round of review please, thanks!
Attachment #8792876 -
Flags: review?(wlachance)
Reporter | ||
Comment 20•8 years ago
|
||
I like raw data better than derived data. Displaying a percentage to me is derived. With that said, it is easier to understand a %change vs what 2.62 means.
We can solve this by putting thresholds, highlighting, and coloring the data and/or background of the cell, more data on the screen, or mouseover/click through to get all the data.
Personally I like to see a percentage based on the final raw data
Flags: needinfo?(jmaher)
Comment 21•8 years ago
|
||
(In reply to Robert Wood [:rwood] from comment #18)
> From :jmaher's description:
>
> To display the data we would:
> for each test in rev1:
> test_diff1 = e10s - non-e10s
> for each test in rev2:
> test_diff2 = e10s - non-e10s
>
> for each test:
> print test \t (test_diff2 - test_diff1)
>
> Question...
>
> i.e.
>
> base delta is 11.55% (value 47.82)
> new delta is 11.94% (50.44)
> change is 5.47% (2.62)
>
> Do we want all of these listed as percentages? Or do we want the first two
> deltas listed as just values but the change in percentage? Or do we want
> them all as straight values? Thanks!
Percentages seem clear enough to me from an overview perspective, I figure you would look at the graphs if you wanted more detail about a particular number. I gather that Joel agrees. :)
Flags: needinfo?(wlachance)
Comment 22•8 years ago
|
||
Comment on attachment 8792876 [details]
[treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
A few minor changes I'd like to see, but this seems very close.
Attachment #8792876 -
Flags: review?(wlachance)
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8792876 [details]
[treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
Thanks Will! PR updated.
Attachment #8792876 -
Flags: review?(wlachance)
Comment 24•8 years ago
|
||
Comment on attachment 8792876 [details]
[treeherder] rwood-moz:rwood-e10s-trend > mozilla:master
This looks good to go in. I imagine jmaher and others might have further requests once they see it, but I see no reason not to land this.
Attachment #8792876 -
Flags: review?(wlachance) → review+
Comment 25•8 years ago
|
||
Commit pushed to master at https://github.com/mozilla/treeherder
https://github.com/mozilla/treeherder/commit/ce16cabce311763a20ef2c7b42576fc5d680a6e3
Bug 1288007 - Add e10s-trend dashboard (#1863)
Reporter | ||
Comment 26•8 years ago
|
||
great stuff!
Assignee | ||
Comment 27•8 years ago
|
||
If you have suggestions for improvements please file bugs and I'll update. Thanks guys!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 28•8 years ago
|
||
oh,I see the dashboard:
https://treeherder.allizom.org/perf.html#/e10s-trend?basedate=3628800
I see that all 3 checkboxes do not yield any results- I am not sure why that is the case, but it seems oddly suspicious. I am surprised there are so many data points of concern- I looked at a few and it is accurate.
overall, this is really cool!
Assignee | ||
Comment 29•8 years ago
|
||
Thanks :jmaher, looks like I somehow broke them last minute, I filed Bug 1306601
You need to log in
before you can comment on or make changes to this bug.
Description
•