Closed
Bug 1145660
Opened 9 years ago
Closed 9 years ago
perfherder urls are overly long
Categories
(Tree Management :: Perfherder, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: wlach, Assigned: akhileshpillai)
Details
Attachments
(1 file, 2 obsolete files)
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).
Reporter | ||
Comment 1•9 years ago
|
||
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. :)
Flags: needinfo?(vikasmishra95)
Reporter | ||
Comment 2•9 years ago
|
||
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. :)
Assignee: vikasmishra95 → nobody
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(vikasmishra95)
Reporter | ||
Comment 3•9 years ago
|
||
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. :)
Reporter | ||
Comment 4•9 years ago
|
||
Akhil is interested in taking this on
Assignee: nobody → akhileshpillai
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8613640 -
Flags: review?(wlachance)
Reporter | ||
Comment 6•9 years ago
|
||
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.
Attachment #8613640 -
Flags: review?(wlachance) → review-
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8613640 -
Attachment is obsolete: true
Attachment #8614140 -
Flags: review?(wlachance)
Assignee | ||
Comment 8•9 years ago
|
||
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.
Reporter | ||
Comment 9•9 years ago
|
||
Comment on attachment 8614140 [details] [review] Implements suggestions from previous attachment Very close, just a few things left to do. See pull request.
Attachment #8614140 -
Flags: review?(wlachance) → review-
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8614140 -
Attachment is obsolete: true
Attachment #8614373 -
Flags: review?(wlachance)
Comment 11•9 years ago
|
||
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/d6ab7871e694aa3f7bbf296fcd187aa41062ccdb Bug 1145660 - Perfherder urls are overly long
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8614373 [details] [review] FInal rebased code. This looks good, thanks!
Attachment #8614373 -
Flags: review?(wlachance) → review+
Reporter | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 13•9 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 14•9 years ago
|
||
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•9 years ago
|
||
Marking fixed per above merge.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•