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)

defect
Not set
normal

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)
Blocks: e10s-harness
tracking-e10s: --- → +
Assignee: nobody → rwood
Status: NEW → ASSIGNED
Attached image suggested-pulldowns.png (deleted) —
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)
Comment on attachment 8784497 [details] suggested-pulldowns.png hey, I am sold! that is easy to follow.
Attachment #8784497 - Flags: feedback?(jmaher) → feedback+
Comment on attachment 8784497 [details] suggested-pulldowns.png Yup, that LGTM.
Attachment #8784497 - Flags: feedback?(wlachance) → feedback+
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 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+
Comment on attachment 8788513 [details] [treeherder] rwood-moz:perf_data > mozilla:master Thanks :wlach, PR updated
Attachment #8788513 - Flags: feedback+ → review?(wlachance)
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)
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 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 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 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+
(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!
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 on attachment 8792876 [details] [treeherder] rwood-moz:rwood-e10s-trend > mozilla:master Waiting for next iteration.
Attachment #8792876 - Flags: feedback?(wlachance)
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)
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)
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)
(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 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)
Comment on attachment 8792876 [details] [treeherder] rwood-moz:rwood-e10s-trend > mozilla:master Thanks Will! PR updated.
Attachment #8792876 - Flags: review?(wlachance)
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+
great stuff!
If you have suggestions for improvements please file bugs and I'll update. Thanks guys!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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!
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.

Attachment

General

Created:
Updated:
Size: