-
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): move TTCI to computed artifact #4943
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.
I'm liking this approach to automating requests for simulated vs observed metrics. So far mostly just questions about where that abstraction should be invoked
|
||
const ComputedArtifact = require('../computed-artifact'); | ||
|
||
class MetricArtifact extends ComputedArtifact { |
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.
bikeshed: I don't want to suggest ComputedMetricArtifact
:) but maybe ComputedMetric
is better here?
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
|
||
/** | ||
* @param {LH.Gatherer.Artifact.MetricComputationData} data | ||
* @param {Object} artifacts |
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 we land after #4944 these can become LH.Artifacts
...the only downside is that there would need to be changes in a bunch of other places too :)
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.
🏁 -------- 🐎 💨
🏁 ---- 🐎 💨
} | ||
|
||
/** | ||
* @param {LH.Gatherer.Artifact.MetricComputationData} data |
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 benefits to doing this on the behalf of the computed artifact instead of having them do it? It makes sense now, I'm just wondering if it's going to grow over time to include so many other artifacts that data
and artifacts
will start to be the same thing :)
This does allow them to select which pass's data to work on, but the pass name could be part of data
instead, I guess. Not sure of other benefits...
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.
The primary benefits:
- All callers of
compute*
should pass in the same set of properties to ensure that caching works since it's based on deep equality (maybe we should split this into separate typedef?) - Achieve 1 while still allowing one-off computations for alternative settings, for example, regardless of the throttling specified, load-fast-enough-4-pwa will always use lantern to compute the 10s TTI for consistency (and enable the audit to not stick the ugly toplevel warning when called without DT throttling)
- All artifacts would need to do the networkRecords traceOfTab as their first two lines
* number of allowed concurrent requests (2). | ||
* @param {Array<LH.WebInspector.NetworkRequest>} networkRecords | ||
* @param {{timestamps: {traceEnd: number}}} traceOfTab | ||
* @return {!Array<!TimePeriod>} |
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.
delete all these !
s :)
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
|
||
const ComputedArtifact = require('../computed-artifact'); | ||
|
||
class MetricArtifact extends ComputedArtifact { |
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.
a class-level doc string would be good too, just to give a sense that this is for computed artifacts calculating metrics, supporting simulated and observed. To implement, override these methods...
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
static findOverlappingQuietPeriods(longTasks, networkRecords, traceOfTab) { | ||
const FMPTsInMs = traceOfTab.timestamps.firstMeaningfulPaint / 1000; | ||
|
||
/** @type {function(TimePeriod):boolean} */ |
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 should be able to get away with just /** @param {TimePeriod} period */
(or you might prefer the current form :)
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.
meh I like the explicitness of this one if I'm gonna bother with the typedef anyhow :)
* @return {Promise<LH.Gatherer.Artifact.LanternMetric>} | ||
*/ | ||
computeSimulatedMetric(data, artifacts) { | ||
return artifacts[`requestLantern${this.name}`](data); |
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.
this is kind of unfortunate, though I don't know a great way around it besides making computed metrics copy and paste basically this line. Benefits would be easier typing (more useful if lantern metrics wanted to start returning metric-specific details) and computed artifacts could do things like alter and combine simulated results, but it would be kind of ugly in the default case
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 about we turn this into an unimplemented method the first case we have that needs either of those? :)
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.
lol fair but I'm holding you to it :P
traceOfTab.timestamps.domContentLoaded / 1000 | ||
) * 1000; | ||
const timeInMs = (timestamp - traceOfTab.timestamps.navigationStart) / 1000; | ||
const extendedInfo = Object.assign(quietPeriodInfo, {timestamp, timeInMs}); |
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 to call out, this PR does remove quietPeriodInfo
from the extInfo, but i think that's just fine. ;)
aight @brendankenny all rebased, are we GTG? |
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!
will follow up with others if this looks good
if possible, can we do one at a time? Fine if in parallel, though. Presumably more annoying for you, but I think reviewing will go a lot quicker (especially now that this lands the base classes) assuming the regular review thoroughness :)
progress toward #4872, starting with just one to make sure we like how it looks. will follow up with others if this looks good
review timer according to #4934 has this deadline at 4/10 10:05am :)