1145660 - perfherder urls are overly long

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

[William Lachance (:wlach)]

The JSON encoding scheme we use for encoding state generates a lot of redundancy, especially since we url encode stuff (bug 1134875). Let's move to a model more like graphserver, where we store state as [x,y,z]. It's ok if the structures in the url are a bit magical (i.e. arrays as opposed to objects).

Comment 1

[William Lachance (:wlach)]

Hi Vikas, are you going to be able to look at this soon? I'd like to get this done before too many people start sharing perfherder urls. If you don't have time at the moment, that's totally fine but I do need to know. :)

Comment 2

[William Lachance (:wlach)]

I'm going to unassign from Vikas as we want this fixed sooner than later and it seems as if he's away. I'm sure there will be new stuff waiting for him to hack on if/when he returns. :)

Comment 3

[William Lachance (:wlach)]

So, just to be a bit more specific, the aim of this bug is to move away from URL-encoded json for storing the series information in the uri to just use a standard array and parsing that. It's probably clearest to use an example. Here's an example of a url for a performance series on mozilla-inbound:

https://treeherder.allizom.org/perf.html#/graphs?timerange=604800&series=%257B%2522project%2522%253A%2522mozilla-inbound%2522%252C%2522signature%2522%253A%2522cc1873c9de0539ea1c6b21cb04b66dbdb1440590%2522%252C%2522visible%2522%253Atrue%257D&zoom=%7B%22x%22:%5B1432409269873.7864,1432488402592.233%5D,%22y%22:%5B289.1851806640625,320%5D%7D

Note the crazy series property (and the somewhat less crazy zoom property). Instead of that, let's just have a url that looks like this:

https://treeherder.allizom.org/perf.html#/graphs?timerange=604800&series=[mozilla-inbound,cc1873c9de0539ea1c6b21cb04b66dbdb1440590]&zoom=[1432409269873.7864,1432488402592.233,289.1851806640625,320]

(we also persist highlighted datapoints, but I omitted them from this example for simplicity)

Fixing this will involve changing the way we parse out $stateParams. We currently use JSON.parse (e.g. https://github.com/mozilla/treeherder/blob/master/ui/js/graphs.js#L580), instead we'll need to convert a stringified array into a javascript data structure. I'm not 100% sure of the best way of doing this offhand but I'm sure it's not too hard. :)

Comment 4

[William Lachance (:wlach)]

Akhil is interested in taking this on

Comment 5

[Akhilesh Pillai]

Attachment: Bug 1145660 - Perfherder urls are overly long

Comment 6

[William Lachance (:wlach)]

Comment on attachment 8613640 [details] [review]
Bug 1145660 - Perfherder urls are overly long

This is a great start! In fact I'd say it's almost there. There are a few points that needed to be addressed first. Let me know either in the PR or on irc if you have questions.

Comment 7

[Akhilesh Pillai]

Attachment: Implements suggestions from previous attachment

Comment 8

[Akhilesh Pillai]

Will, I updated the code to make visible setting work properly and rectify the weird object in the URI. Please have a look when you get a chance. pull request in the attachment is valid. Thanks.

Comment 9

[William Lachance (:wlach)]

Comment on attachment 8614140 [details] [review]
Implements suggestions from previous attachment

Very close, just a few things left to do. See pull request.

Comment 10

[Akhilesh Pillai]

Attachment: FInal rebased code.

Comment 11

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/d6ab7871e694aa3f7bbf296fcd187aa41062ccdb
Bug 1145660 - Perfherder urls are overly long

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8614373 [details] [review]
FInal rebased code.

This looks good, thanks!

Comment 13

[Jonathan French (:jfrench)]

A minor regression during a moztest sweep on stage: clicking on any 'graph' link in a Compare, ends up landing on our groovey new (and intentionally empty) Graph page, rather than landing on the graphed data from that compare. Will is looking at it now.

Comment 14

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/329c6f5a37dd2a087efa97b21b15a914ae393218
Bug 1145660 - Fix links to graphs in perfherder compare

Comment 15

[Jonathan French (:jfrench)]

Marking fixed per above merge.