-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
feat: save devtools log #1665
feat: save devtools log #1665
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this only for events, not command responses?
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.
yes, figured that would be distracting and not make very much sense without the request 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
will do
/** | ||
* @return {!Array<Object>} | ||
*/ | ||
get messageLog() { |
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.
maybe bump this stuff into it's own class and instantiate it in the connection constructor?
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.
will do
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any constraints around the format for this file? (aka not pretty print?)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use "*devtoolslog.json" rather?
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.
will do
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