-
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
core(metrics): consumable metrics audit output #5101
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,13 +8,15 @@ | |
|
||
const log = require('lighthouse-logger'); | ||
|
||
function findValueInMetricsAuditFn(metricName, timingOrTimestamp) { | ||
// TODO: rework this file to not need this function | ||
// see https://github.com/GoogleChrome/lighthouse/pull/5101/files#r186168840 | ||
function findValueInMetricsAuditFn(metricName) { | ||
return auditResults => { | ||
const metricsAudit = auditResults.metrics; | ||
if (!metricsAudit || !metricsAudit.details || !metricsAudit.details.items) return; | ||
|
||
const values = metricsAudit.details.items.find(item => item.metricName === metricName); | ||
return values && values[timingOrTimestamp]; | ||
const values = metricsAudit.details.items[0]; | ||
return values && values[metricName]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can be a TODO, but seems like we can just get rid of this whole ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure TODO added |
||
}; | ||
} | ||
|
||
|
@@ -33,68 +35,68 @@ class Metrics { | |
{ | ||
name: 'Navigation Start', | ||
id: 'navstart', | ||
getTs: findValueInMetricsAuditFn('observedNavigationStart', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedNavigationStart', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedNavigationStartTs'), | ||
getTiming: findValueInMetricsAuditFn('observedNavigationStart'), | ||
}, | ||
{ | ||
name: 'First Contentful Paint', | ||
id: 'ttfcp', | ||
getTs: findValueInMetricsAuditFn('observedFirstContentfulPaint', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedFirstContentfulPaint', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedFirstContentfulPaintTs'), | ||
getTiming: findValueInMetricsAuditFn('observedFirstContentfulPaint'), | ||
}, | ||
{ | ||
name: 'First Meaningful Paint', | ||
id: 'ttfmp', | ||
getTs: findValueInMetricsAuditFn('observedFirstMeaningfulPaint', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedFirstMeaningfulPaint', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedFirstMeaningfulPaintTs'), | ||
getTiming: findValueInMetricsAuditFn('observedFirstMeaningfulPaint'), | ||
}, | ||
{ | ||
name: 'Speed Index', | ||
id: 'si', | ||
getTs: findValueInMetricsAuditFn('observedSpeedIndex', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedSpeedIndex', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedSpeedIndexTs'), | ||
getTiming: findValueInMetricsAuditFn('observedSpeedIndex'), | ||
}, | ||
{ | ||
name: 'First Visual Change', | ||
id: 'fv', | ||
getTs: findValueInMetricsAuditFn('observedFirstVisualChange', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedFirstVisualChange', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedFirstVisualChangeTs'), | ||
getTiming: findValueInMetricsAuditFn('observedFirstVisualChange'), | ||
}, | ||
{ | ||
name: 'Visually Complete 100%', | ||
id: 'vc100', | ||
getTs: findValueInMetricsAuditFn('observedLastVisualChange', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedLastVisualChange', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedLastVisualChangeTs'), | ||
getTiming: findValueInMetricsAuditFn('observedLastVisualChange'), | ||
}, | ||
{ | ||
name: 'First CPU Idle', | ||
id: 'ttfi', | ||
getTs: findValueInMetricsAuditFn('firstCPUIdle', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('firstCPUIdle', 'timing'), | ||
getTs: findValueInMetricsAuditFn('firstCPUIdleTs'), | ||
getTiming: findValueInMetricsAuditFn('firstCPUIdle'), | ||
}, | ||
{ | ||
name: 'Interactive', | ||
id: 'tti', | ||
getTs: findValueInMetricsAuditFn('interactive', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('interactive', 'timing'), | ||
getTs: findValueInMetricsAuditFn('interactiveTs'), | ||
getTiming: findValueInMetricsAuditFn('interactive'), | ||
}, | ||
{ | ||
name: 'End of Trace', | ||
id: 'eot', | ||
getTs: findValueInMetricsAuditFn('observedTraceEnd', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedTraceEnd', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedTraceEndTs'), | ||
getTiming: findValueInMetricsAuditFn('observedTraceEnd'), | ||
}, | ||
{ | ||
name: 'On Load', | ||
id: 'onload', | ||
getTs: findValueInMetricsAuditFn('observedLoad', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedLoad', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedLoadTs'), | ||
getTiming: findValueInMetricsAuditFn('observedLoad'), | ||
}, | ||
{ | ||
name: 'DOM Content Loaded', | ||
id: 'dcl', | ||
getTs: findValueInMetricsAuditFn('observedDomContentLoaded', 'timestamp'), | ||
getTiming: findValueInMetricsAuditFn('observedDomContentLoaded', 'timing'), | ||
getTs: findValueInMetricsAuditFn('observedDomContentLoadedTs'), | ||
getTiming: findValueInMetricsAuditFn('observedDomContentLoaded'), | ||
}, | ||
]; | ||
} | ||
|
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 copy defined values to a new
metrics
object to return rather than delete from the original?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.
any particular reason for this? I'll end up just creating an empty object with no type and casting at the end which seems sad 😢
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 guess we can do it for now.
delete
is just almost always a bad idea :(On the compiler side, this isn't really type checked either, since
key
has to be cast andvalue
ends up asany
. We just know thatundefined
and a number that can be rounded are the two possible cases. I don't know a great solution, though. We'd need anObject.entries()
that doesn't widen, anArray.filter()
that really filters types, and anObject.fromEntries()
from TC39 :)The other option is to leave them defined as undefined. Was that bad? They'll get dropped from the JSON and we're testing against
undefined
anywaysThere 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 guess it's not the end of the world to leave them undefined, will do