bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

perfherder urls are overly long

RESOLVED FIXED

Status

Tree Management
Perfherder
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: wlach, Assigned: Akhilesh Pillai)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

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).
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)
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
Flags: needinfo?(vikasmishra95)
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. :)
Akhil is interested in taking this on
Assignee: nobody → akhileshpillai
(Assignee)

Comment 5

3 years ago
Created attachment 8613640 [details] [review]
Bug 1145660 - Perfherder urls are overly long
Attachment #8613640 - Flags: review?(wlachance)
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

3 years ago
Created attachment 8614140 [details] [review]
Implements suggestions from previous attachment
Attachment #8613640 - Attachment is obsolete: true
Attachment #8614140 - Flags: review?(wlachance)
(Assignee)

Comment 8

3 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.
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

3 years ago
Created attachment 8614373 [details] [review]
FInal rebased code.
Attachment #8614140 - Attachment is obsolete: true
Attachment #8614373 - Flags: review?(wlachance)
Comment on attachment 8614373 [details] [review]
FInal rebased code.

This looks good, thanks!
Attachment #8614373 - Flags: review?(wlachance) → review+
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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 → ---
Marking fixed per above merge.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.