PROPOSAL: new structure for metrics + lantern #4676
Conversation
| * @return {Object} | ||
| */ | ||
| async compute_(data, artifacts) { | ||
| if (data.throttling.method !== 'lantern') { |
There was a problem hiding this comment.
perhaps we should move this to the gatherer runner? this way we don't need to explicitly do if statements inside the compute_.
const throttlingMethod = data.throttling.method;
const computedGathererFunc = throttlingMethod.charAt(0).toUpperCase() + throttlingMethod.slice(1);
if (gatherer[computedGathererFunc]) {
gatherer[computedGathererFunc]();
} else {
gatherer.compute_();
}
There was a problem hiding this comment.
Hm I feel like that introduces a decent amount of extra complexity to where the computed artifacts are coming from when they are already pretty hard to follow. what's the primary concern with having the ifs here in the compute?
There was a problem hiding this comment.
no real reason :) just thought it was better to not think about putting ifs everywhere but maybe it's better to make it explicit 😄
| /** | ||
| * @typedef MetricResult | ||
| * @property {number} timing | ||
| * @property {number|undefined} timestamp |
There was a problem hiding this comment.
i know it's just an example but shouldn't this be @property {number=} timestamp
There was a problem hiding this comment.
nah it should be more like @property {number} [timestamp] docs but point taken yes will spend more time on the real typedef when trying to land 😛
|
@brendankenny @paulirish any other broader feedback on this structure? I'll need to get moving on all of this ASAP |
| onlyAudits: ['first-contentful-paint'], | ||
| throttling: { | ||
| // method: 'devtools', | ||
| // requestLatency: emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.latency, |
There was a problem hiding this comment.
i'd like these key names to be more in-line with the ones used for lantern, but i think you already know that. :)
There was a problem hiding this comment.
I named them differently specifically because they operate at different levels and the amount you'd want to apply to one should be very different from the amount you'd apply to the other to get the same experience :)
| const Node = require('../../lib/dependency-graph/node'); | ||
| const Simulator = require('../../lib/dependency-graph/simulator/simulator'); | ||
|
|
||
| const COEFFICIENTS = { |
There was a problem hiding this comment.
comment on where this come from plx
| static getScriptUrls(dependencyGraph, condition) { | ||
| const scriptUrls = new Set(); | ||
|
|
||
| dependencyGraph.traverse(node => { |
There was a problem hiding this comment.
it feels like it'd make sense for a common lib/dependency-graph-predicates.js at some point
| } | ||
|
|
||
| /** | ||
| * @param {!Node} dependencyGraph |
There was a problem hiding this comment.
not relevant to the architecture, but we should document how we compute this estimated FCP.
| * @param {!TraceOfTabArtifact} traceOfTab | ||
| * @return {!Node} | ||
| */ | ||
| static getPessimisticGraph(dependencyGraph, traceOfTab) { |
There was a problem hiding this comment.
theres so much similarity between these two fns (optimistic & pessimistic). could we DRY it up? dunno if adding && optionalPred(node) to the end of both would be terrible or not.
| const optimisticGraph = FirstContentfulPaint.getOptimisticGraph(graph, traceOfTab); | ||
| const pessimisticGraph = FirstContentfulPaint.getPessimisticGraph(graph, traceOfTab); | ||
|
|
||
| const options = {...networkAnalysis, ...data.throttling}; |
There was a problem hiding this comment.
looks like data.throttling will override the rtt/throughput provided by networkAnalysis, right?
would you ever want just rtt from data.throttling and take throughput from networkAnalysis?
i wonder if networkAnalysis should take data.throttling as a parameter instead? We don't really need to compute throughput if we're using a provided value, right?
There was a problem hiding this comment.
right, and not that I can think of. Network analysis artifact is meant to provide you a glimpse into what was observed while throttling is what you want it to be.
In lantern's case we don't really care about what was observed, other than the additional latency observed per origin
UPDATE: closing this now that separate PRs to land have been opened #4764 #4766
This PR is an example of a couple proposed changes we've been discussing that, if still acceptable to everyone, I'll make in separate PRs, but thought it'd be easier to see the whole thing come together.
I've demo'd the following changes with how it would look for
first-contentful-paintsettings.throttlingobject in the config controls whether the throttling method isdevtools,lantern, orprovidedand the relevant settings.devtoolsandprovidedwill make audits surface things based on what was observed (current method), while lantern will make audits surface things based on the lantern graph with specified settings (what we need for LR, default, etc)requiredNumberOfArtifactsnow that we have deep equality