-
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
refactor(trace-of-tab): return timestamps in microseconds #2454
Conversation
meta comment: where should we document this? It's good to know (and important to know when you do anything with these :) Probably trace-of-tab, but maybe also in https://github.com/GoogleChrome/lighthouse/blob/master/docs/architecture.md? |
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
does plots/
need a heads up on this change? It looks like some (most? all?) of these metrics were being corrected back to microseconds for when they're picked up by pwmetrics-events
, so it may be fine, but are there any that are going ms -> μs?
timestamps[metric] = metrics[metric] && metrics[metric].ts / 1000; | ||
timings[metric] = timestamps[metric] - timestamps.navigationStart; | ||
timestamps[metric] = metrics[metric] && metrics[metric].ts; | ||
timings[metric] = (timestamps[metric] - navigationStart.ts) / 1000; |
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 do wish we had a better name for these than timings
, but may be too late :)
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.
ha a battle for another day :)
@@ -54,7 +54,7 @@ class ConsistentlyInteractiveMetric extends Audit { | |||
* @return {!Array<!TimePeriod>} | |||
*/ | |||
static _findNetworkQuietPeriods(networkRecords, traceOfTab) { | |||
const traceEnd = traceOfTab.timestamps.traceEnd; | |||
const traceEndTsInMs = traceOfTab.timestamps.traceEnd / 1000; |
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.
nit: move this down to where it's used?
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.
aw I liked that each function in this file got this aliasing out of the way at the beginning :(
did it make you look for its use too much right afterwards?
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.
ha, yeah, exactly. I figured that's what this was doing, but then my next thought was, "wait, what is this even used for?"
got another PR comin' with some more groovy docs 📜 📝 |
fixes #2116