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

misc(compare-runs): report metrics, add gather/audit modes, etc #10296

Merged
merged 18 commits into from
Feb 28, 2020

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Feb 4, 2020

Some upgrades to the collect-timings script!

Some new features:

--gather and --audit modes, and visual progress bar

image

had to save all artifacts into timings-data/ as well.

metrics are reported

image

Coefficient of variation is reported

i prefer this over reading the stddev in a big way.

other

  • rename --measure-filter to --filter
  • strict yargs

Split out from #9920

@@ -23,28 +25,32 @@ const argv = yargs
// common flags
'name': 'Unique identifier, makes the folder for storing LHRs. Not a path',
'report-exclude': 'Regex of properties to exclude. Set to "none" to disable default',
// --collect
'collect': 'Saves LHRs to disk',
// collection
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// collection
// --gather

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you group the params for the gather stage right after gather?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

mkdirp.sync(gatherDir);
// TODO include multiple urls too.
const progressBar = new Array(Math.round(i * 40/ argv.n)).fill('▄').join('').padEnd(40);
console.log(`Gathering: `, '|', progressBar, '|')
Copy link
Collaborator

@connorjclark connorjclark Feb 4, 2020

Choose a reason for hiding this comment

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

this would be so much better if you grabbed the logger from the lantern collect scripts (handles clearing the linefeed for you)

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

'node',
`${LH_ROOT}/lighthouse-cli`,
url,
`--audit-mode=${gatherDir}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

-A?

Copy link
Member Author

Choose a reason for hiding this comment

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

i like being verbose when possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

funny b.c. I had to look this up :)

if (argv.collect) collect();
if (argv.gather) gather();
if (argv.audit) audit();
if (argv.collect) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

backwards compatibility.

--collect did the full run before. now it does the same thing.

happy to do whatever tho.

@@ -23,28 +25,32 @@ const argv = yargs
// common flags
'name': 'Unique identifier, makes the folder for storing LHRs. Not a path',
'report-exclude': 'Regex of properties to exclude. Set to "none" to disable default',
// --collect
'collect': 'Saves LHRs to disk',
// collection
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you group the params for the gather stage right after gather?

// collection
'gather': 'Just gathers',
'audit': 'Audits from the artifacts on disk',
'collect': 'Gathers, audits and saves LHRs to disk',
Copy link
Collaborator

Choose a reason for hiding this comment

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

move this to under common flags

@@ -188,22 +238,24 @@ function filter(results) {
}

/**
* @param {number=} value
* @param {number|string|undefined} value
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was string added here? it makes the type guard not make sense. maybe change body to typeof value === 'number'

Copy link
Member Author

Choose a reason for hiding this comment

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

mostly because i wanted to render CV as a percentage instead of a 0.XXX number.

but i did another solution which is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

i do like using typeof number tho. did that.

const durations = durationsByName[entry.name] = durationsByName[entry.name] || [];
durations.push(entry.duration);
const durations = durationsByName[name] = durationsByName[name] || [];
durations.push(Number(timimg));
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

Copy link
Member Author

Choose a reason for hiding this comment

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

ah thx. renaming to duration anyway.

const results = aggregateResults(argv.name);
filter(results);
print(results);
for (const resultType of ['timings', 'metrics']) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a fan of printing both of these unconditionally. seems you'd only want to look at timings or metrics, not both.

Copy link
Member Author

Choose a reason for hiding this comment

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

k done and documented.

@connorjclark
Copy link
Collaborator

connorjclark commented Feb 4, 2020

seems a rename is in order. just collect?

Copy link
Member Author

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

seem a rename is in order.

renamed to compare-runs.js

mkdirp.sync(gatherDir);
// TODO include multiple urls too.
const progressBar = new Array(Math.round(i * 40/ argv.n)).fill('▄').join('').padEnd(40);
console.log(`Gathering: `, '|', progressBar, '|')
Copy link
Member Author

Choose a reason for hiding this comment

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

done!

const durations = durationsByName[entry.name] = durationsByName[entry.name] || [];
durations.push(entry.duration);
const durations = durationsByName[name] = durationsByName[name] || [];
durations.push(Number(timimg));
Copy link
Member Author

Choose a reason for hiding this comment

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

ah thx. renaming to duration anyway.

if (argv.collect) collect();
if (argv.gather) gather();
if (argv.audit) audit();
if (argv.collect) {
Copy link
Member Author

Choose a reason for hiding this comment

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

backwards compatibility.

--collect did the full run before. now it does the same thing.

happy to do whatever tho.

'node',
`${LH_ROOT}/lighthouse-cli`,
url,
`--audit-mode=${gatherDir}`,
Copy link
Member Author

Choose a reason for hiding this comment

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

i like being verbose when possible.

@@ -23,28 +25,32 @@ const argv = yargs
// common flags
'name': 'Unique identifier, makes the folder for storing LHRs. Not a path',
'report-exclude': 'Regex of properties to exclude. Set to "none" to disable default',
// --collect
'collect': 'Saves LHRs to disk',
// collection
Copy link
Member Author

Choose a reason for hiding this comment

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

sure

@@ -188,22 +238,24 @@ function filter(results) {
}

/**
* @param {number=} value
* @param {number|string|undefined} value
Copy link
Member Author

Choose a reason for hiding this comment

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

mostly because i wanted to render CV as a percentage instead of a 0.XXX number.

but i did another solution which is better.

@@ -188,22 +238,24 @@ function filter(results) {
}

/**
* @param {number=} value
* @param {number|string|undefined} value
Copy link
Member Author

Choose a reason for hiding this comment

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

i do like using typeof number tho. did that.

const results = aggregateResults(argv.name);
filter(results);
print(results);
for (const resultType of ['timings', 'metrics']) {
Copy link
Member Author

Choose a reason for hiding this comment

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

k done and documented.

@paulirish paulirish changed the title misc(collect-timings): report metrics, add gather/audit modes, etc. misc(compare-runs): report metrics, add gather/audit modes, etc. Feb 5, 2020
@paulirish paulirish changed the title misc(compare-runs): report metrics, add gather/audit modes, etc. misc(compare-runs): report metrics, add gather/audit modes, etc Feb 5, 2020

// Push the aggregate time of each unique (by name) entry.
for (const [name, durationsForSingleRun] of Object.entries(durationsByName)) {
const key = `${lhr.requestedUrl}@@@${kind}@@@${name}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants