Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plots: make measure script more flexible (CLI args) #2183

Merged
merged 30 commits into from
May 31, 2017

Conversation

wwwillchen
Copy link
Contributor

@wwwillchen wwwillchen commented May 8, 2017

Add flags, help docs, and making it better all around.

@@ -41,10 +41,13 @@ const NEXUS5X_USERAGENT = {
'(KHTML, like Gecko) Chrome/59.0.3033.0 Mobile Safari/537.36'
};

const LATENCY_FACTOR = 3.75;
const THROUGHPUT_FACTOR = 1.1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we model both of these as multipliers so it's just multiply for both?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1.

it's equal to throughput * 90/99 but you might as well simplify it to throughput * 0.9

plots/measure.js Outdated
};
console.log('Running lighthouse with flag\n disableNetworkThrottling: ', FLAGS.disableNetworkThrottling);

const SUBSET = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running plots/ this weekend, it was very handy to split this to loading from file so I could quickly switch between a couple of different sets of test sites. If you're adding yargs anyways, it would be great to support loading a list of URLs from a path passed in as a CLI option :)

plots/measure.js Outdated
@@ -150,20 +178,30 @@ main();
* Returns a promise chain that analyzes all the sites n times.
* @return {!Promise}
*/
function runAnalysis() {
function runAnalysisWithNewChromeInstances() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@wwwillchen wwwillchen changed the title Network analysis - WIP Plots: make measure script more flexible (CLI args) May 15, 2017
@brendankenny
Copy link
Member

is this ready to review? And will need a rebase with recent changes to chromeLauncher

@brendankenny
Copy link
Member

is this ready to review?

was that a yes? :)

@wwwillchen
Copy link
Contributor Author

ping @brendankenny for a review :)

@brendankenny
Copy link
Member

will need a rebase :)

plots/measure.js Outdated
.catch(err => console.error(err))
.then(() => launcher.kill());
if (REUSE_CHROME) {
ChromeLauncher.launch({port: 9222}).then(launcher => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been running multiple plots at a time recently.. Launcher is getting a unique port by default, but you have to get it off launcher.port and pass into lighthouse().. Here's what I did today:
image

Can you support this functionality? You could just pass it along like this or create measure instances that hold this state.. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yar. Done.

plots/measure.js Outdated
// Running it n + 1 times if the first run is deliberately ignored
// because it has different perf characteristics from subsequent runs
// (e.g. DNS cache which can't be easily reset between runs)
const NUMBER_OF_RUNS = (CUSTOM_NUMBER_OF_RUNS || 20) + (KEEP_FIRST_RUN ? 0 : 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't seem like an uppercase const as its super conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

plots/measure.js Outdated
@@ -31,9 +59,29 @@ const ChromeLauncher = require('../chrome-launcher/chrome-launcher.js');
const Printer = require('../lighthouse-cli/printer');
const assetSaver = require('../lighthouse-core/lib/asset-saver.js');

const NUMBER_OF_RUNS = 20;
const DISABLE_DEVICE_EMULATION = args['disable-device-emulation'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh seems kinda odd to establish these all as uppercase consts as they are dependent on CLI input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - I removed them and just inlined their usage

plots/measure.js Outdated
}

if (SUBSET) {
return [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about we just slice the top of the full sites list instead of repeating here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not DRY but I wanted to both have a set of sites that were useful for cpu/network analysis (basically the most visible / meaningful sites) and preserve the original order of the sites list since it explains in comments how they were sourced.

plots/measure.js Outdated
// Running it n + 1 times if the first run is deliberately ignored
// because it has different perf characteristics from subsequent runs
// (e.g. DNS cache which can't be easily reset between runs)
const NUMBER_OF_RUNS = (CUSTOM_NUMBER_OF_RUNS || 20) + (KEEP_FIRST_RUN ? 0 : 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's unexpected that number_of_runs == 1 will provide two runs for each site. Can we make sure the number here will match # of actual runs done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I updated it so there's no +1 run. I added a check in main so users don't accidentally try to only do 1 run whose results are discard.

plots/utils.js Outdated
/**
* @param {string} filePath
*/
function removeRecursive(filePath) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oy - yeah I was using it in a one-off script. I'll remove it.

Copy link
Contributor Author

@wwwillchen wwwillchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take another look! Sorry this took a while :)

plots/measure.js Outdated
@@ -31,9 +59,29 @@ const ChromeLauncher = require('../chrome-launcher/chrome-launcher.js');
const Printer = require('../lighthouse-cli/printer');
const assetSaver = require('../lighthouse-core/lib/asset-saver.js');

const NUMBER_OF_RUNS = 20;
const DISABLE_DEVICE_EMULATION = args['disable-device-emulation'];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - I removed them and just inlined their usage

plots/measure.js Outdated
// Running it n + 1 times if the first run is deliberately ignored
// because it has different perf characteristics from subsequent runs
// (e.g. DNS cache which can't be easily reset between runs)
const NUMBER_OF_RUNS = (CUSTOM_NUMBER_OF_RUNS || 20) + (KEEP_FIRST_RUN ? 0 : 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

plots/measure.js Outdated
// Running it n + 1 times if the first run is deliberately ignored
// because it has different perf characteristics from subsequent runs
// (e.g. DNS cache which can't be easily reset between runs)
const NUMBER_OF_RUNS = (CUSTOM_NUMBER_OF_RUNS || 20) + (KEEP_FIRST_RUN ? 0 : 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I updated it so there's no +1 run. I added a check in main so users don't accidentally try to only do 1 run whose results are discard.

plots/measure.js Outdated
}

if (SUBSET) {
return [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not DRY but I wanted to both have a set of sites that were useful for cpu/network analysis (basically the most visible / meaningful sites) and preserve the original order of the sites list since it explains in comments how they were sourced.

plots/measure.js Outdated
.catch(err => console.error(err))
.then(() => launcher.kill());
if (REUSE_CHROME) {
ChromeLauncher.launch({port: 9222}).then(launcher => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yar. Done.

plots/utils.js Outdated
/**
* @param {string} filePath
*/
function removeRecursive(filePath) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oy - yeah I was using it in a one-off script. I'll remove it.

plots/measure.js Outdated
* then open a fresh Chrome instance for each run.
*/
function getUrls() {
if (args['sites-path']) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we already have this, why not toss the default SITES into a file and have sites-path default to that one.

plots/measure.js Outdated
}

if (args['subset']) {
return [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it seems like we could just extract this into a diff sites_subset.js file?

function analyzeWithLighthouse(url, outputPath, assetsPath, {ignoreRun}) {
const flags = {output: 'json'};
function analyzeWithLighthouse(launcher, url, outputPath, assetsPath, {ignoreRun}) {
const flags = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ignoreRun is true then there's no reason to have throttling on. I'd force disable cpu & network throttling in these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

@wwwillchen wwwillchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yo @paulirish - updated per your comments.

function analyzeWithLighthouse(url, outputPath, assetsPath, {ignoreRun}) {
const flags = {output: 'json'};
function analyzeWithLighthouse(launcher, url, outputPath, assetsPath, {ignoreRun}) {
const flags = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

plots/sites.js Outdated
'use strict';

/* eslint-disable no-unused-expressions */
[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module.exports = [ ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

'use strict';

/* eslint-disable no-unused-expressions */
[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module.exports = [ ...

plots/measure.js Outdated
'https://ask.com',
'https://bbc.com'
];
const keepFirstRun = args['keep-first-run'] || !args['reuse-chrome'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use camelcase instead of this square bracket dance.

args.keepFirstRun etc. yargs does it for you.

plots/measure.js Outdated
return eval(fs.readFileSync(path.resolve(__dirname, 'sites_subset.js'), 'utf-8'));
}

return eval(fs.readFileSync(path.resolve(__dirname, 'sites.js'), 'utf-8'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead. you can set the default on yargs:

.default('sites-path', 'sites.js')

plots/measure.js Outdated
}

if (args['sites-path']) {
const sitesPath = path.resolve(__dirname, args['sites-path']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (args.sitesPath) {
  return require(path.resolve(__dirname, args.sitesPath));
}

Copy link
Contributor Author

@wwwillchen wwwillchen May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - this is nice. I did it the more verbose way because the downside is require() will hold onto these files indefinitely in node's module cache. It's definitely OK now since the sites list are needed for the entire lifetime of the script but if we start loading a lot of different sites files, we'll need to watch out for heap usage.

@wwwillchen
Copy link
Contributor Author

@paulirish - please take another look.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % nits

thanks! excited about this

plots/measure.js Outdated
})
.group(
['disable-device-emulation', 'disable-cpu-throttling', 'disable-network-throttling'],
'Chrome DevTools settings:')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's call this Lighthouse settings:

plots/measure.js Outdated
@@ -17,11 +17,40 @@
'use strict';

/* eslint-disable no-console */

const fs = require('fs');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer neccessary

plots/measure.js Outdated
'https://bbc.com'
];
const keepFirstRun = args.keepFirstRun || !args.reuseChrome;
const numberOfRuns = args.n || 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to yargs default

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

<body>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add this page to the <nav> of the other two html files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -0,0 +1,29 @@
<!--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this to bars-per-site ?

const path = require('path');
const parseURL = require('url').parse;

const mkdirp = require('mkdirp');
const args = require('yargs')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the new style of invoking, can you update the readme?

to me, it makes more sense to use the node script.js invocation rather than the npm run one now that we have a CLI. but no big deal.

Copy link
Contributor Author

@wwwillchen wwwillchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it up!

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

<body>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

# Run lighthouse to collect metrics data
$ yarn measure
$ node measure.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you got a whole CLI for measure.js now!. at the very least you should paste the --help output into the readme

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice stuff all around! Thanks for the back 'n forth!

📉 📈 📊

@paulirish paulirish merged commit 3f7e5a1 into GoogleChrome:master May 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants