plots: dashboard - identify variance over lighthouse versions #2520

wwwillchen · 2017-06-16T23:24:25Z

Updated code: please review the code :)

Example:

If you need some dummy data, you can copy this file into plots/out/dashboard-results.js and then open dashboard/index.html or I can show you the demo in person.

addresses part of: #2413

…nto plots-better-out

…nto plots-dashboard

patrickhulce · 2017-06-16T23:27:33Z

…nto plots-dashboard

codecov · 2017-06-21T00:44:34Z

Codecov Report

Merging #2520 into master will decrease coverage by 0.83%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2520      +/-   ##
==========================================
- Coverage   84.52%   83.68%   -0.84%     
==========================================
  Files         169      177       +8     
  Lines        5614     5780     +166     
  Branches      774      801      +27     
==========================================
+ Hits         4745     4837      +92     
- Misses        855      923      +68     
- Partials       14       20       +6

Impacted Files	Coverage Δ
lighthouse-core/lib/network-recorder.js	`100% <0%> (ø)`	⬆️
lighthouse-cli/commands/list-audits.ts	`100% <0%> (ø)`
lighthouse-cli/cli-flags.ts	`90% <0%> (ø)`
lighthouse-cli/bin.ts	`29.03% <0%> (ø)`
lighthouse-cli/commands/commands.ts	`100% <0%> (ø)`
lighthouse-cli/shim-modules.ts	`50% <0%> (ø)`
lighthouse-cli/index.js	`100% <0%> (ø)`
lighthouse-cli/printer.ts	`94.28% <0%> (ø)`
lighthouse-cli/commands/list-trace-categories.ts	`100% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfcbc30...4c18ec9. Read the comment docs.

wwwillchen · 2017-06-21T01:24:45Z

@patrickhulce @paulirish - please take a look.

patrickhulce · 2017-06-21T17:58:00Z

plots/dashboard/dashboard.css

+  color: white;
+  padding: 15px;
+  font-size: 16px;
+  background-color: #3367d6;


wanna put any of these colors in same vars? :)

patrickhulce · 2017-06-21T18:13:58Z

plots/dashboard/dashboard.js

+let currentMetric = metrics[0];
+let numberOfBatchesToShow = 0;
+
+function main() {


this file is kinda hard to follow and grok, wdyt about grouping some of these into classes/objects/sections by usage? i.e. event handlers, rendering, init, etc

patrickhulce · 2017-06-21T18:17:55Z

plots/dashboard/index.html

+<html>
+
+<head>
+  <script src="https://cdn.plot.ly/plotly-1.25.2.min.js"></script>


blocking script in head! 😱 :)

bumped down to body.

patrickhulce · 2017-06-21T18:19:15Z

plots/package.json

-{
-  "main": "index.js",
-  "scripts": {
-    "measure": "node measure.js",


aw I liked yarn measure

Yeah, the reason why I removed it is because passing flags through yarn or npm can be unintuitive (e.g. yarn measure -- --flag) and I'm guessing most of the time we're using these scripts with flags. In a f/u PR, I can look into using npm link and then we could have something like (plots measure --out out-hi) but I think we should cross that bridge later (e.g. when plots becomes useful outside of the core team).

patrickhulce · 2017-06-22T01:41:59Z

plots/dashboard/dashboard.js

+
+  const renderingScheduler = new RenderingScheduler();
+  const charts = new Charts(renderingScheduler);
+  const dashboard = new Dashboard(metrics, charts);


niiiiice muuuuch easier to follow!

minor nit, personally I like to separate the rendering logic from a constructor to an explicit render() method so it's clear what attaches to the DOM and what does basic setup and whatnot (if there's no need to keep instance variables at all some could even be static and the classes just used for logical grouping but totally up to you)

thanks, done. I'll keep the instance variables (otherwise I think I'd need to use singletons or global variables).

oh yeah wasn't implying that you should make them static, just that if it shook out without the need for instance variables didn't have to keep em that way

wwwillchen

Thanks, please take another look.

wwwillchen · 2017-06-22T00:07:41Z

plots/dashboard/dashboard.css

+  color: white;
+  padding: 15px;
+  font-size: 16px;
+  background-color: #3367d6;


wwwillchen · 2017-06-22T00:09:49Z

plots/dashboard/index.html

+<html>
+
+<head>
+  <script src="https://cdn.plot.ly/plotly-1.25.2.min.js"></script>


bumped down to body.

wwwillchen · 2017-06-22T00:14:07Z

plots/package.json

-{
-  "main": "index.js",
-  "scripts": {
-    "measure": "node measure.js",


Yeah, the reason why I removed it is because passing flags through yarn or npm can be unintuitive (e.g. yarn measure -- --flag) and I'm guessing most of the time we're using these scripts with flags. In a f/u PR, I can look into using npm link and then we could have something like (plots measure --out out-hi) but I think we should cross that bridge later (e.g. when plots becomes useful outside of the core team).

wwwillchen · 2017-06-22T01:31:17Z

plots/dashboard/dashboard.js

+let currentMetric = metrics[0];
+let numberOfBatchesToShow = 0;
+
+function main() {


wwwillchen · 2017-06-22T21:45:32Z

plots/dashboard/dashboard.js

+
+  const renderingScheduler = new RenderingScheduler();
+  const charts = new Charts(renderingScheduler);
+  const dashboard = new Dashboard(metrics, charts);


thanks, done. I'll keep the instance variables (otherwise I think I'd need to use singletons or global variables).

brendankenny

looking good.

I haven't looked carefully enough to answer this for myself: That Note: null results are treated as 0 note, does that include when calculating percentiles?

brendankenny · 2017-06-22T02:00:17Z

plots/dashboard/index.html

@@ -0,0 +1,41 @@
+<!doctype html>


license header

brendankenny · 2017-06-22T02:00:21Z

plots/dashboard/index.html

+  <script src="./dashboard.js"></script>
+</body>
+
+</html>


brendankenny · 2017-06-26T22:38:24Z

plots/dashboard/dashboard.css

@@ -0,0 +1,119 @@
+* {


paulirish · 2017-06-26T22:59:59Z

plots/README.md

+$ node generate-dashboard.js --input-root dir-with-subdirectories
+
+# Or you can specify each input directory explicitly
+$ node generated-dashboard.js --inputs out-1 out-2 out-3


generated => generate?

paulirish · 2017-06-26T23:05:50Z

plots/generate-dashboard.js

+  .example('node $0 --input-root dir-with-subdirectories')
+  .example('node $0 --inputs out-1 out-2 out-3')
+  .describe({
+    'inputs': 'paths to out directory generated by measure.js',


could we instead have a single --input? (And tbh, at that point, you might as well skip the arg).

It'd look like this:

node generate-dashboard out-a out-b out-x node generate-dashboard outParentFolder

Take the directory or its children, depending on the contents

paulirish · 2017-06-26T23:07:40Z

plots/generate-dashboard.js

+  for (const inputPath of inputPaths) {
+    childProcess.execSync(`node analyze.js ${inputPath}`, {
+      env: Object.assign({}, process.env, {
+        CI: '1'


add a comment that this is to prevent opn() from running.
and collapse this Obj.assign into one line?

wwwillchen

@brendankenny - yeah, null is treated as 0 in when calculating the percentiles.

@paulirish - ptal. thanks!

wwwillchen · 2017-06-27T23:06:53Z

plots/README.md

+$ node generate-dashboard.js --input-root dir-with-subdirectories
+
+# Or you can specify each input directory explicitly
+$ node generated-dashboard.js --inputs out-1 out-2 out-3


wwwillchen · 2017-06-27T23:07:41Z

plots/dashboard/dashboard.css

@@ -0,0 +1,119 @@
+* {


wwwillchen · 2017-06-27T23:08:08Z

plots/dashboard/index.html

@@ -0,0 +1,41 @@
+<!doctype html>


wwwillchen · 2017-06-27T23:08:17Z

plots/dashboard/index.html

+  <script src="./dashboard.js"></script>
+</body>
+
+</html>


wwwillchen · 2017-06-27T23:13:02Z

plots/generate-dashboard.js

+  for (const inputPath of inputPaths) {
+    childProcess.execSync(`node analyze.js ${inputPath}`, {
+      env: Object.assign({}, process.env, {
+        CI: '1'


brendankenny · 2017-06-28T03:11:50Z

null is treated as 0 in when calculating the percentiles.

can they be filtered out before calculating? Adding 0s to the sample will bring the median downward when usually (but not always) missing results come from what would be much higher values.

For the time being we could filter nulls out and then in the future add something like a warning that there were too many missing values in a plots/ run and either it needs to be rerun or there's a serious regression we let slip in that needs to be fixed :)

wwwillchen · 2017-06-28T19:02:41Z

@brendankenny I tried out your suggestion and ran into an issue when all the runs for a particular batch of results for a site have a null value. Essentially plotly gets confused about where to plot. I think we can go with treating null as 0 for now and then figuring out a better solution in a f/u PR.

Before (treating null as 0)

After (filtering null)

brendankenny · 2017-06-28T19:47:16Z

hmm, if nulls are filtered out of the array before calling calculatePercentile on it and the array ends empty, shouldn't calculatePercentile already return 0 for the median, 20%, and 80%? I'm ok with having those runs drop to 0 on the graph (it'll be really clear that something was wrong), I just meant nulls should be removed from the array before calculating a percentile.

(unless that's what you did and I'm missing the step that's messing up the graph)

wwwillchen · 2017-06-29T21:43:17Z

@brendankenny updated it so it retuns 0 if the array is empty (before if the array was empty it was returning undefined).

brendankenny · 2017-06-29T22:49:12Z

plots/dashboard/dashboard.js

+    if (array.length === 0) {
+      return 0;
+    }
+    if (percentile <= 0) {


doesn't it need to be sorted before these?

yep - fixed.

brendankenny · 2017-06-29T22:49:33Z

plots/dashboard/dashboard.js

+    if (percentile >= 1) {
+      return array[array.length - 1];
+    }
+    const sorted = array.slice().sort((a, b) => a - b).filter(x => x !== null);


nit: if it does filter() first it can ditch the slice()

brendankenny · 2017-06-29T23:21:43Z

plots/dashboard/dashboard.js

+      return sorted[lower] || 0;
+    }
+    return sorted[lower] * (1 - weight) + sorted[upper] * weight;
+  }


how do you feel about

calculatePercentile(array, percentile) { const sorted = array.filter(x => x !== null).sort((a, b) => a - b); if (sorted.length === 0) { return 0; } if (sorted.length === 1 || percentile <= 0) { return sorted[0]; } if (percentile >= 1) { return sorted[sorted.length - 1]; } const index = (sorted.length - 1) * percentile; const lower = Math.floor(index); const upper = lower + 1; const weight = index % 1; return sorted[lower] * (1 - weight) + sorted[upper] * weight; }

looks good.

…nto plots-dashboard

wwwillchen

@brendankenny thanks - updated per your comments.

wwwillchen · 2017-07-05T23:18:27Z

plots/dashboard/dashboard.js

+    if (array.length === 0) {
+      return 0;
+    }
+    if (percentile <= 0) {


yep - fixed.

wwwillchen · 2017-07-05T23:18:56Z

plots/dashboard/dashboard.js

+    if (percentile >= 1) {
+      return array[array.length - 1];
+    }
+    const sorted = array.slice().sort((a, b) => a - b).filter(x => x !== null);


wwwillchen · 2017-07-05T23:24:14Z

plots/dashboard/dashboard.js

+      return sorted[lower] || 0;
+    }
+    return sorted[lower] * (1 - weight) + sorted[upper] * weight;
+  }


looks good.

brendankenny

LGTM!

wwwillchen added 8 commits June 7, 2017 14:51

up

8b5ec74

Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…

7813584

…nto plots-better-out

fix lint errors but not working

0ca2b21

fix

f364a20

pilot dashboard

cebb277

pilot

c880a95

Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…

317ffc7

…nto plots-dashboard

fix merge issue

353a4de

wwwillchen changed the title ~~plots: dashboard [prototype]~~ plots: dashboard - identify variance over lighthouse versions [prototype] Jun 16, 2017

wwwillchen added 4 commits June 20, 2017 14:24

Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…

a6e1f4a

…nto plots-dashboard

fixup

49e1fa1

fmt

7b8f7c9

fixups

019ea6c

wwwillchen added 2 commits June 20, 2017 18:16

clean up

6a7abdb

fixit

4c18ec9

wwwillchen changed the title ~~plots: dashboard - identify variance over lighthouse versions [prototype]~~ plots: dashboard - identify variance over lighthouse versions Jun 21, 2017

patrickhulce reviewed Jun 21, 2017

View reviewed changes

refactor

810efb0

patrickhulce reviewed Jun 22, 2017

View reviewed changes

fixup

f80adde

wwwillchen commented Jun 22, 2017

View reviewed changes

brendankenny reviewed Jun 26, 2017

View reviewed changes

paulirish reviewed Jun 26, 2017

View reviewed changes

wwwillchen added 4 commits June 27, 2017 16:17

cl feedback

9584dd1

cl fb

a8428a1

fix

59b19c6

fixup

8ebdbe1

wwwillchen commented Jun 28, 2017

View reviewed changes

wwwillchen added 2 commits June 29, 2017 14:37

filter out nulls

8a0b56e

update text

77d41a4

brendankenny reviewed Jun 29, 2017

View reviewed changes

wwwillchen added 2 commits July 5, 2017 16:28

done

1d6b1c2

Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…

811993c

…nto plots-dashboard

wwwillchen commented Jul 5, 2017

View reviewed changes

brendankenny approved these changes Jul 6, 2017

View reviewed changes

wwwillchen merged commit 9561330 into GoogleChrome:master Jul 6, 2017

plots: dashboard - identify variance over lighthouse versions #2520

plots: dashboard - identify variance over lighthouse versions #2520

Conversation

wwwillchen commented Jun 16, 2017 • edited Loading

patrickhulce commented Jun 16, 2017

codecov bot commented Jun 21, 2017 • edited Loading

Codecov Report

wwwillchen commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwwillchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwwillchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jun 28, 2017

wwwillchen commented Jun 28, 2017

Before (treating null as 0)

After (filtering null)

brendankenny commented Jun 28, 2017

wwwillchen commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwwillchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

wwwillchen commented Jun 16, 2017 •

edited

Loading

codecov bot commented Jun 21, 2017 •

edited

Loading