feat: save devtools log #1665
feat: save devtools log #1665
Conversation
There was a problem hiding this comment.
what does WPT use the log for? It can just consume arbitrary chatter between a client and the browser, or is it only interested in a subset of the log?
Also somewhat concerned about size here...if it's just events this is interested in we e.g. won't get the trace in there, but not sure how well the log size will be bounded
| @@ -108,6 +129,11 @@ class Connection { | |||
| return object.result; | |||
| })); | |||
| } | |||
|
|
|||
| if (this._recordMessages) { | |||
There was a problem hiding this comment.
is this only for events, not command responses?
There was a problem hiding this comment.
yes, figured that would be distracting and not make very much sense without the request payload
There was a problem hiding this comment.
yup, we dont need command reqs/responses.
| const traceFilename = `${pathWithBasename}-${index}.trace.json`; | ||
| fs.writeFileSync(traceFilename, JSON.stringify(traceData, null, 2)); |
There was a problem hiding this comment.
you mentioned the WPT trace parser doesn't do pretty JSON.
Should we start saving it in the right format? (or another PR i suppose)
There was a problem hiding this comment.
I'm planning on handling the pretty printed file in WPT since I don't think its an excuse to be human-hostile here
| this._messageLog = []; | ||
| } | ||
|
|
||
| beginRecording() { |
There was a problem hiding this comment.
i think we should have a whitelist filter to only record given domains. that way we could only do Page/Network which is all that WPT needs (and same for Lighthouse in its artifact-driven run).
honestly the API doesn't really need to be configurable.. maybe just set the filter as a property on this class?
| /** | ||
| * @return {!Array<Object>} | ||
| */ | ||
| get messageLog() { |
There was a problem hiding this comment.
maybe bump this stuff into it's own class and instantiate it in the connection constructor?
| it('devtools log file saved to disk with data', () => { | ||
| const filename = 'the_file-0.devtools.json'; | ||
| const fileContents = fs.readFileSync(filename, 'utf8'); | ||
| assert.ok(fileContents.includes('"message": "first"')); |
There was a problem hiding this comment.
are there any constraints around the format for this file? (aka not pretty print?)
There was a problem hiding this comment.
no constraints on this one
lighthouse-core/lib/asset-saver.js
Outdated
| log.log('trace file saved to disk', traceFilename); | ||
|
|
||
| const devtoolsLogFilename = `${pathWithBasename}-${index}.devtools.json`; |
There was a problem hiding this comment.
can we use "*devtoolslog.json" rather?
|
WPT uses the message log to generate most of what you see in a report, network waterfalls, request breakdowns, timing information, etc. For that it needs the page and network events, and from the samples I've done the trace is far larger (~5-30x) than the devtools log. |
Got it. I do like @paulirish's idea of filtering messages saved.
Right, but we don't stream in the trace until after we're done tracing :) If it's just events and just for the two domains it should probably be fine |
|
Filtering to just |
|
I didn't get to this fast enough, but some drive by thoughts when future changes touch the code:
|
fixes #1662