718872 - Graph server should generate dashboard dynamically

Status: RESOLVED FIXED

(Webtools Graveyard :: Graph Server, defect)

Description

[Ryosuke Niwa]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.75 Safari/535.7

Comment 1

[Ryosuke Niwa]

Attachment: Initial patch

This patch removes the static html on dashboard.html and generates the table dynamically.

Comment 2

[Ryosuke Niwa]

My only concern is that this patch changes the format of perm links on the dashboard and therefore invalidates the existing perm links. I guess we can add a map to convert the old perm links to new ones?

Comment 3

[Robert Helmer [:rhelmer]]

Comment on attachment 589334 [details] [diff] [review]
Initial patch

First of all, this looks great! Thanks for taking this on, has been on my list todo for a while now.

Only one change I'd like to see (at the very bottom), I'll go ahead and make that and push this to github.

>diff --git a/js/dashboard.js b/js/dashboard.js
>index 2540130..94c3b2d 100644
>--- a/js/dashboard.js
>+++ b/js/dashboard.js
>@@ -3,36 +3,58 @@
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> (function($) {
> 
>-    // FIXME server should store "popular" values
>-    var DEFAULT_BRANCH = 'firefox',
>-        DEFAULT_PLATFORM = ['windows7', 'windowsxp', 'macosx', 'linux'],
>-        DEFAULT_TEST = ['ts', 'tp', 'ss'];
>+    var branchToId;
>+    var platformToId;
>+    var testToId;
>+
>+    function fetchDashboardManifest()
>+    {
>+        defaultBranch = 'Firefox';
>+        branchToId = {'Firefox': 1};
>+        platformToId = {
>+            'Windows 7': 12,
>+            'Windows XP': 1,
>+            'Mac OS X': 13,
>+            'Linux': 14
>+        };
>+        testToId = {
>+            'Ts': 83,
>+            'Tp': 115,
>+            'SunSpider': 104
>+        };
>+    }
>+    fetchDashboardManifest();


I like the way you've done this, so we can easily move it to the server soon without having to change the JS API.


>@@ -135,6 +157,31 @@
>         updateLocation();
>     });
> 
>+
>+    $('#charts').prepend($('<table><thead><tr><td class="spacer"></td></tr></thead><tbody></tbody></table>'))
>+
>+    $.each(branchToId, function (name, id) {
>+        var selected = name == branch ? ' selected' : '';
>+        $('#branch select').append($('<option value="' + name + '"' + selected + '>' + name + '</option>'));
>+    })
>+
>+    $.each(platformToId, function (name, id) {
>+        var cell = document.createElement('td');
>+        cell.appendChild(document.createTextNode(name));
>+        cell.className = 'platform' + id;
>+        $('#charts thead tr').append(cell);
>+        $('#platform select').append($('<option value="' + name + '">' + name + '</option>'));
>+    });
>+
>+    $.each(testToId, function (name, id) {
>+        var cells = '';
>+        $.each(platformToId, function (platformName, platformId) {
>+            cells += '<td class="platform' + platformId + ' test' + id + '">' + name + ':' + platformName + '</td>'
>+        })
>+        $('#charts tbody').append($('<tr><td class="rowhead test' + id + '">' + name + '</td>' + cells + '</tr>'));

There should be a <p> in here, so that the "transform: rotate(90deg)" rule works on the row headers:

        $('#charts tbody').append($('<tr><td class="rowhead test' + id + '"><p>' + name + '</p></td>' + cells + '</tr>'));

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to Ryosuke Niwa from comment #2)
> My only concern is that this patch changes the format of perm links on the
> dashboard and therefore invalidates the existing perm links. I guess we can
> add a map to convert the old perm links to new ones?

Probably not a concern, I doubt anyone really uses it as-is.. I think we will switch to much different and more useful filters soon anyway. 

We'll probably continue to have a default product along with "Top Platforms" and "Top Tests", but instead of restricting to one test or platform at a time, it'd probably be much more useful to provide sets of related tests, or something along those lines.

There was even a proposal in one of the newsgroups to have the dashboard show the most recently loaded graphs, similar to the "speed dial" that most modern browers show on startup: https://groups.google.com/forum/#!msg/mozilla.dev.performance/xLW_o5LGr6U/O1Nzp9Ir0LYJ 

The consensus seems to be that the current dashboard and URL formats (both for dynamic graphs and dashboard) are suboptimal, and I feel ok breaking them if we end up with something better/more supportable. If it becomes an issue I'd rather rewrite them using the server software than drag it along in the graphs code.

Comment 5

[Robert Helmer [:rhelmer]]

Pushed this (and also some formatting fixes to make gjslint happy):
https://github.com/rhelmer/graphs/commit/cb3a2fba6115d9b50e72d9473c342ffdf6c27bb7

This is live on stage (http://graphs.allizom.org) and lgtm so far, thanks again!

Comment 6

[Robert Helmer [:rhelmer]]

Found a small typo that caused regression in "show changesets" button, now fixed:
https://github.com/mozilla/graphs/commit/0b9771fd82b2e51f419cca0514a44ad3088c68b6

Also live on stage: http://graphs.allizom.org

Comment 7

[Ryosuke Niwa]

Thanks for the review & merge!

(In reply to Robert Helmer [:rhelmer] from comment #6)
> Found a small typo that caused regression in "show changesets" button, now
> fixed:
> https://github.com/mozilla/graphs/commit/
> 0b9771fd82b2e51f419cca0514a44ad3088c68b6
> 
> Also live on stage: http://graphs.allizom.org

I get 403 on js/common.js.

Comment 8

[Robert Helmer [:rhelmer]]

(In reply to Ryosuke Niwa from comment #7)
> Thanks for the review & merge!
> 
> (In reply to Robert Helmer [:rhelmer] from comment #6)
> > Found a small typo that caused regression in "show changesets" button, now
> > fixed:
> > https://github.com/mozilla/graphs/commit/
> > 0b9771fd82b2e51f419cca0514a44ad3088c68b6
> > 
> > Also live on stage: http://graphs.allizom.org
> 
> I get 403 on js/common.js.

Oops, look like me running "git clone" on the staging box set things as 600 (so Apache couldn't read it).

Just did a chmod -R, looking better now? That reminds me, I really need to make deployment to stage better.