-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(scripts): add trace/devtoolslog minification scripts #5237
Conversation
package.json
Outdated
@@ -63,7 +63,8 @@ | |||
"update:sample-json": "node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --throttling-method=devtools --output=json --output-path=./lighthouse-core/test/results/sample_v2.json http://localhost/dobetterweb/dbw_tester.html && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing", | |||
"diff:sample-json": "bash lighthouse-core/scripts/assert-golden-lhr-unchanged.sh", | |||
"update:crdp-typings": "node lighthouse-core/scripts/extract-crdp-mapping.js", | |||
"mixed-content": "./lighthouse-cli/index.js --chrome-flags='--headless' --config-path=./lighthouse-core/config/mixed-content.js" | |||
"mixed-content": "./lighthouse-cli/index.js --chrome-flags='--headless' --config-path=./lighthouse-core/config/mixed-content.js", | |||
"minify-latest-run": "./lighthouse-core/scripts/lantern/minify-trace.js ./latest-run/defaultPass.trace.json ./latest-run/defaultPass.trace.json && ./lighthouse-core/scripts/lantern/minify-devtoolslog.js ./latest-run/defaultPass.devtoolslog.json ./latest-run/defaultPass.devtoolslog.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest-run/defaultPass.trace.json is in there twice. same with the dtlog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's going to do it in place, maybe make the default output just be the input? Would make it a lot easier to read here, at least (otoh you might be sad that it automatically wrote over your saved trace, but I don't foresee many folks running this script independently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to make the default mode be inplace, I can put the latest-run files into different files? defaultPass.trace.min.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fine (either way, really). It was mostly a comment on npm scripts being generally unreadable and this with the doubled-up everything is especially hard to read :)
* @param {LH.TraceEvent[]} events | ||
*/ | ||
function filterTraceEvents(events) { | ||
const startedInPageEvt = events.find(evt => evt.name === 'TracingStartedInPage'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lucky you: #5271 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
speaking of which, is there some way to share all this logic with trace-of-tab
so things like this (and what's considered "important") are always kept in sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I'd want #5271 to move the logic into tracing-process for mainthread identification and then I can just reuse it :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so...future TODO/file an issue/wait until something breaks? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO it is!
}); | ||
|
||
// Filter down the screenshots to key moments + 2fps animations | ||
const screenshotTimestamps = filtered.filter(evt => evt.name === 'Screenshot').map(evt => evt.ts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you break each of these techniques into its own fn. i might want to use just the screenshots one in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 done
'x-robots-tag', | ||
]); | ||
|
||
/** @param {any} headers */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LH.Crdp.Network.Headers
(which is just Record<string, string>
)? Or I guess could be Partial<LH.Crdp.Network.Headers>
since setting to undefined below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
obj.url = obj.url.replace(/^(data:.*?base64,).*/, '$1FILLER'); | ||
} | ||
|
||
/** @param {LH.Crdp.Network.ResponseReceivedEvent['response']} [response] */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LH.Crdp.Network.Response
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/** @type {any} */ | ||
const timing = response.timing || {}; | ||
for (const [k, v] of Object.entries(timing)) { | ||
if (v === -1) timing[k] = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how much does this end up mattering since the values will be just -1, not some unconstrained payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just eliminates most of the useless keys here, are you saying you'd rather keep them?
* @param {LH.TraceEvent[]} events | ||
*/ | ||
function filterTraceEvents(events) { | ||
const startedInPageEvt = events.find(evt => evt.name === 'TracingStartedInPage'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
speaking of which, is there some way to share all this logic with trace-of-tab
so things like this (and what's considered "important") are always kept in sync?
package.json
Outdated
@@ -63,7 +63,8 @@ | |||
"update:sample-json": "node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --throttling-method=devtools --output=json --output-path=./lighthouse-core/test/results/sample_v2.json http://localhost/dobetterweb/dbw_tester.html && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing", | |||
"diff:sample-json": "bash lighthouse-core/scripts/assert-golden-lhr-unchanged.sh", | |||
"update:crdp-typings": "node lighthouse-core/scripts/extract-crdp-mapping.js", | |||
"mixed-content": "./lighthouse-cli/index.js --chrome-flags='--headless' --config-path=./lighthouse-core/config/mixed-content.js" | |||
"mixed-content": "./lighthouse-cli/index.js --chrome-flags='--headless' --config-path=./lighthouse-core/config/mixed-content.js", | |||
"minify-latest-run": "./lighthouse-core/scripts/lantern/minify-trace.js ./latest-run/defaultPass.trace.json ./latest-run/defaultPass.trace.json && ./lighthouse-core/scripts/lantern/minify-devtoolslog.js ./latest-run/defaultPass.devtoolslog.json ./latest-run/defaultPass.devtoolslog.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's going to do it in place, maybe make the default output just be the input? Would make it a lot easier to read here, at least (otoh you might be sad that it automatically wrote over your saved trace, but I don't foresee many folks running this script independently)
|
||
/* eslint-disable no-console */ | ||
|
||
const fs = require('fs'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fileoverview for both of these scripts would be 👍 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PTAL :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
phase 1 of lantern accuracy checks per commit
does a pretty awesome job of minifying traces from light/medium sites for inclusion, OK job of minifying the devtoolslog
example.com: 526KB -> 28KB, 7KB -> 4KB
paulirish.com: 2.3MB -> 151KB, 147KB -> 47KB
does okish on heavy sites like CNN
cnn.com: 30MB ->3.8MB, 2.9MB -> 1.3MB