Traces & Timespans: A Preview #12595

patrickhulce · 2021-06-01T21:36:49Z

Summary
With 8.0 edging out of the harbor, it's time to start discussing the next phase of FR, traces in timespan mode! 🎉

This encompasses several efforts...

Updating trace processor to handle a timespan specific time origin (lots of prework here already, see Improve handling of redirects, navStart #8984)
Updating metrics that can work in timespan mode to handle the new begin/end marks.
Marking audits notApplicable in timespan mode when their artifacts are available, but the audit doesn't make sense in the observed context (e.g. an audit that looks at network records and reports savings on the main document when none was requested).

This PR is a preview of a few of these efforts on a few audits to get a feel for what the future looks like. If anything feels horribly wrong, let's talk now to find an alternate path :)

I'll let this marinate for a few days and proceed breaking it up if there are no objections.

Related Issues/PRs
ref #11313

adamraine

Flushing some initial thoughts

adamraine · 2021-06-01T22:29:50Z

lighthouse-core/audits/metrics/speed-index.js

+    } catch (err) {
+      // SpeedIndex can't be computed on every timespan. If it errored, just mark as notApplicable.
+      if (artifacts.GatherContext.gatherMode === 'timespan') return {notApplicable: true, score: 1};
+      throw err;
+    }


I mentioned this in the UX sync already, but I'm concerned that SI doesn't make sense in timespan because expected UI events which visually update the page would punish the score.

yes definitely! speed index only makes sense for the narrow SPA navigation use case of timespans. we can work on refining how to treat SPA navigations once more of this has taken shape. largely just a proof of concept that we can compute these sorts of metrics on the trace in different modes

adamraine · 2021-06-01T22:32:14Z

lighthouse-core/computed/metrics/total-blocking-time.js

-        interactiveTimeMs
-      ),
-    };
+    if (TraceOfTab.isNavigation(data.traceOfTab)) {


Would normalizing TBT at some point be a good idea? Longer timespans are generally going to have a higher TBT.

for sure, I expect before we launch MVP we'll use a normalized version of TBT (similar to CLS windows) for timespans

adamraine · 2021-06-02T15:25:31Z

lighthouse-core/computed/trace-of-tab.js

+   * @return {traceOfTab is LH.Artifacts.NavigationTraceOfTab}
+   */
+  static isNavigation(traceOfTab) {
+    return Boolean(traceOfTab.firstContentfulPaintEvt);


Is this our only requirement for a navigation trace?

no the real PR will have more requirements (likely checking the use of our custom timespan marker), just needed something quickly to show the concept of detecting the trace type for use in audits

adamraine · 2021-06-02T15:46:59Z

lighthouse-core/computed/metrics/total-blocking-time.js

-        interactiveTimeMs
-      ),
-    };
+    if (TraceOfTab.isNavigation(data.traceOfTab)) {


I was expecting this to split on gatherMode === 'navigation' and then use TraceOfTab.assertIsNavigation. Is this so we can still get TBT in navigation mode if FCP is not present in the trace?

good question, so isNavigation is meant to effectively capture gatherMode === 'navigation'. I like your suggestion of two separate methods that explicitly divide the two concepts and asserts that they match 👍

adamraine · 2021-06-02T15:49:26Z

lighthouse-core/computed/trace-of-tab.js

-    if (
-      !firstContentfulPaintEvt ||
-      firstContentfulPaintTiming === undefined ||
-      firstContentfulPaintTs === undefined ||
-      // FCP-AF will only be undefined if FCP is also undefined.
-      // These conditions are for enforcing types and should never actually trigger.
-      !firstContentfulPaintAllFramesEvt ||
-      firstContentfulPaintAllFramesTiming === undefined ||
-      firstContentfulPaintAllFramesTs === undefined
-    ) {
-      throw new LHError(LHError.errors.NO_FCP);
-    }


Glad to see this go

adamraine · 2021-06-03T15:47:22Z

LGTM to start flushing this out

patrickhulce · 2021-06-03T20:59:22Z

@connorjclark @paulirish @brendankenny any major showstoppers for you? I'll plan to start breaking this up on Monday if not.

brendankenny

@connorjclark @paulirish @brendankenny any major showstoppers for you? I'll plan to start breaking this up on Monday if not.

I think this looks good.

As I mentioned earlier, I'd like for us to also start looking for larger structural changes we could make to either avoid shipping complexity down to leaf files or at least to help keep things simple for inspection, e.g. I don't have to travel far code-wise to figure out what code path a particular line is on and the invariants I can assume there because I could only have gotten here by one or two paths.

brendankenny · 2021-06-03T22:08:18Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

    return NetworkRecords.request(devtoolsLog, context)
      .then(networkRecords =>
        Promise.all([
          this.audit_(artifacts, networkRecords, context),
-          PageDependencyGraph.request({trace, devtoolsLog}, context),
-          LoadSimulator.request(simulatorOptions, context),
+          PageDependencyGraph.request({trace, devtoolsLog}, context).catch(err => {


as a style thing, it would be nice to put a lot of this stuff closer to top level where appropriate so you don't have to dig to see where mode differentiation is happening

brendankenny · 2021-06-03T22:26:15Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

@@ -133,6 +133,7 @@ class RenderBlockingResources extends Audit {
    const traceOfTab = await TraceOfTab.request(trace, context);
    const simulator = await LoadSimulator.request(simulatorData, context);
    const wastedCssBytes = await RenderBlockingResources.computeWastedCSSBytes(artifacts, context);
+    TraceOfTab.assertIsNavigation(traceOfTab);


part of why we made FCP required was because (at the time) everything had to check if FCP was actually in the trace to make sure we weren't auditing nonsense. We've mitigated that in some other ways since, but I worry that we'd be regressing that by going back to assertions in every audit. In an ideal world everything works only if it's being done correctly. What if an audit forgets to call this but only implicitly relies on FCP being defined? Would we notice the somewhat nonsense numbers coming out? (this kind of gets back to my worry about test coverage * audits * mode).

You're in the thick of this so you'd know the feasibility better than me just handwaving here, but what if we changed a little bit more about the model here? e.g. rather than conditionals in trace-of-tab/trace-processor we split trace-processor so we have the thing everyone uses to identify the main process, frame tree, etc. then navigation auditing can pull in the timing/timestamp stuff from a separate module with that as input if/when needed? Almost all of that isn't needed for timespans, and when we get to the point where timespans can have an lcp as well, I'm ok with using a different model than coming in on a giant timestamps object :)

What if an audit forgets to call this but only implicitly relies on FCP being defined?

I'm not sure this is solvable with any method :/ If there is an implicit relationship that FCP is defined but the audit never references it, then that could always be forgotten with supportedMode/dedicated types or classes/favorite method X, right? Or am I misunderstanding that specific concern?

For the case where we're relying on FCP existing because the audit uses it, that's exactly what the types are doing for us here.

rather than conditionals in trace-of-tab/trace-processor we split trace-processor so we have the thing everyone uses to identify the main process, frame tree, etc. then navigation auditing can pull in the timing/timestamp stuff from a separate module with that as input if/when needed?

A total fork of trace-processor is certainly possible, but I'm a little fuzzy on the primary benefit. Is the goal for the type signature of trace output in timespan audits to not have a key at all for firstContentfulPaint instead of an optional one?

A total fork of trace-processor is certainly possible, but I'm a little fuzzy on the primary benefit. Is the goal for the type signature of trace output in timespan audits to not have a key at all for firstContentfulPaint instead of an optional one?

splitting it up, not forking, but yes :) timespan's portion of the trace-processor's output is a subset of what navigations need, so lets just only fetch timings/timestamps when they're needed, rather than always fetching them and asserting they're defined when needed.

brendankenny · 2021-06-03T22:29:42Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

@@ -272,6 +273,8 @@ class RenderBlockingResources extends Audit {
   * @return {Promise<LH.Audit.Product>}
   */
  static async audit(artifacts, context) {
+    if (artifacts.GatherContext.gatherMode !== 'navigation') return {score: 0, notApplicable: true};


in an ideal world it seems like we wouldn't have to remember to do this either, but I don't know a way around it other than audit annotations (a la meta.supportedModes) and have audit-runner take care of the check, but I don't know if that's any better. Curious about your thoughts on it

adding meta.supportedModes to audits is one option, and might be the most straightforward to avoid a ton of notApplicable audits. my primary hesitation is keeping up with the potential mismatch with gatherers and the common case not deviating from the inferred. perhaps it could just be optional though?

@adamraine thoughts?

I like the idea of having it as an optinal dependency. I was imagining the supported modes would be inferred by the available artifacts. Maybe we could use meta.applicableModes or something to avoid mismatch.

brendankenny · 2021-06-03T22:31:39Z

lighthouse-core/audits/metrics/speed-index.js

+        displayValue: str_(i18n.UIStrings.seconds, {timeInMs: metricResult.timing}),
+      };
+    } catch (err) {
+      // SpeedIndex can't be computed on every timespan. If it errored, just mark as notApplicable.


is there more to this? no screenshots in the trace for some timespans? seems kind of loosey goosey for a metric :)

brendankenny · 2021-06-03T22:39:46Z

lighthouse-core/gather/gatherers/trace.js

    // TODO(FR-COMPAT): read additional trace categories from overall settings?
    // TODO(FR-COMPAT): check if CSS/DOM domains have been enabled in another session and warn?
    await driver.defaultSession.sendCommand('Page.enable');
    await driver.defaultSession.sendCommand('Tracing.start', {
      categories: Trace.getDefaultTraceCategories().join(','),
      options: 'sampling-frequency=10000', // 1000 is default and too slow.
    });
+
+    // Only inject the trace processor timespan marker, for timespan traces.
+    if (gatherMode !== 'timespan') return;


total nit but early return seems weird in this case?

is this a if (gatherMode === 'timespan') { request, or something more?

is this a if (gatherMode === 'timespan') { request

yes

brendankenny · 2021-06-03T22:40:03Z

lighthouse-core/gather/gatherers/trace.js

+    await driver.executionContext.evaluate(id => performance.mark(id), {
+      args: [TraceProcessor.TIMESPAN_MARKER_ID],
+    });


surely there's some better way of doing this, haha

what do you have in mind? :)

what do you have in mind? :)

just expressing incredulity there's no Tracing.insertEvent or Page.performanceMark() or whatever :)

you mean like Tracing.recordClockSyncMarker 😄

brendankenny · 2021-06-03T22:42:14Z

lighthouse-core/lib/tracehouse/trace-processor.js

@@ -513,7 +522,7 @@ class TraceProcessor {
   *
   * @param {LH.TraceEvent[]} events
   * @param {LH.TraceEvent} timeOriginEvent
-   * @return {{lcp: LCPEvent | undefined, invalidated: boolean}}
+   * @return {{lcp: LCPEvent | undefined}}


it's unclear if this (and fmpFellBack) is a necessary change for some purpose or if it's just a clearing out of stuff no one actually uses

mostly just clearing stuff out, it simplified some of the logic and type gymnastics that would've been necessary to keep it.

brendankenny · 2021-06-03T22:43:37Z

lighthouse-core/lib/tracehouse/trace-processor.js

@@ -579,7 +586,7 @@ class TraceProcessor {
   * @return {TraceOfTabWithoutFCP}
  */
  static computeTraceOfTab(trace, options) {
-    const {timeOriginDeterminationMethod = 'lastNavigationStart'} = options || {};
+    const {timeOriginDeterminationMethod = 'auto'} = options || {};


in what situations would 'auto' be used?

all situations, it's the default :)

in what situations would 'auto' be used?

all situations, it's the default :)

It could cause some difficult to debug surprises (e.g. something's not using navStart, you have to know the user's trace has a marker in it). Is the intention that most ~~audits~~ of the time traceOfTab will just work™ doing it this way, though?

brendankenny · 2021-06-03T22:45:39Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -182,6 +182,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
    const metricsBoxesEl = this.dom.createChildOf(metricAuditsEl, 'div', 'lh-metrics-container');

    metricAudits.forEach(item => {
+      if (item.result.scoreDisplayMode === 'notApplicable') return;


does this not cause other weird things? or since scoreDisplayMode: 'error' also has null scores we already handle it ok?

I'm not sure what you mean. This line just hides the notApplicable metrics from the metrics section of the category.

Or were you asking if metrics with notApplicable work with the score? If so, the answer is yes, just like others their weight just disappears (though separate weights for timespan is completely different bag of worms!)

Or were you asking if metrics with notApplicable work with the score? If so, the answer is yes, just like others their weight just disappears (though separate weights for timespan is completely different bag of worms!)

yes, I was surprised notApplicable just works because we've never had not-applicable metrics before, but it was a pleasant surprise :)

patrickhulce

thanks folks! I'll try to start breaking this up

patrickhulce · 2021-06-07T21:12:23Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

@@ -133,6 +133,7 @@ class RenderBlockingResources extends Audit {
    const traceOfTab = await TraceOfTab.request(trace, context);
    const simulator = await LoadSimulator.request(simulatorData, context);
    const wastedCssBytes = await RenderBlockingResources.computeWastedCSSBytes(artifacts, context);
+    TraceOfTab.assertIsNavigation(traceOfTab);


What if an audit forgets to call this but only implicitly relies on FCP being defined?

I'm not sure this is solvable with any method :/ If there is an implicit relationship that FCP is defined but the audit never references it, then that could always be forgotten with supportedMode/dedicated types or classes/favorite method X, right? Or am I misunderstanding that specific concern?

For the case where we're relying on FCP existing because the audit uses it, that's exactly what the types are doing for us here.

rather than conditionals in trace-of-tab/trace-processor we split trace-processor so we have the thing everyone uses to identify the main process, frame tree, etc. then navigation auditing can pull in the timing/timestamp stuff from a separate module with that as input if/when needed?

A total fork of trace-processor is certainly possible, but I'm a little fuzzy on the primary benefit. Is the goal for the type signature of trace output in timespan audits to not have a key at all for firstContentfulPaint instead of an optional one?

patrickhulce · 2021-06-07T21:13:09Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

@@ -272,6 +273,8 @@ class RenderBlockingResources extends Audit {
   * @return {Promise<LH.Audit.Product>}
   */
  static async audit(artifacts, context) {
+    if (artifacts.GatherContext.gatherMode !== 'navigation') return {score: 0, notApplicable: true};


adding meta.supportedModes to audits is one option, and might be the most straightforward to avoid a ton of notApplicable audits. my primary hesitation is keeping up with the potential mismatch with gatherers and the common case not deviating from the inferred. perhaps it could just be optional though?

@adamraine thoughts?

patrickhulce · 2021-06-07T21:15:41Z

lighthouse-core/gather/gatherers/trace.js

    // TODO(FR-COMPAT): read additional trace categories from overall settings?
    // TODO(FR-COMPAT): check if CSS/DOM domains have been enabled in another session and warn?
    await driver.defaultSession.sendCommand('Page.enable');
    await driver.defaultSession.sendCommand('Tracing.start', {
      categories: Trace.getDefaultTraceCategories().join(','),
      options: 'sampling-frequency=10000', // 1000 is default and too slow.
    });
+
+    // Only inject the trace processor timespan marker, for timespan traces.
+    if (gatherMode !== 'timespan') return;


is this a if (gatherMode === 'timespan') { request, or something more?

patrickhulce · 2021-06-07T21:16:05Z

lighthouse-core/gather/gatherers/trace.js

+    await driver.executionContext.evaluate(id => performance.mark(id), {
+      args: [TraceProcessor.TIMESPAN_MARKER_ID],
+    });


what do you have in mind? :)

patrickhulce · 2021-06-07T21:17:04Z

lighthouse-core/lib/tracehouse/trace-processor.js

@@ -513,7 +522,7 @@ class TraceProcessor {
   *
   * @param {LH.TraceEvent[]} events
   * @param {LH.TraceEvent} timeOriginEvent
-   * @return {{lcp: LCPEvent | undefined, invalidated: boolean}}
+   * @return {{lcp: LCPEvent | undefined}}


mostly just clearing stuff out, it simplified some of the logic and type gymnastics that would've been necessary to keep it.

patrickhulce · 2021-06-07T21:17:46Z

lighthouse-core/lib/tracehouse/trace-processor.js

@@ -579,7 +586,7 @@ class TraceProcessor {
   * @return {TraceOfTabWithoutFCP}
  */
  static computeTraceOfTab(trace, options) {
-    const {timeOriginDeterminationMethod = 'lastNavigationStart'} = options || {};
+    const {timeOriginDeterminationMethod = 'auto'} = options || {};


all situations, it's the default :)

patrickhulce · 2021-06-07T23:23:46Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -182,6 +182,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
    const metricsBoxesEl = this.dom.createChildOf(metricAuditsEl, 'div', 'lh-metrics-container');

    metricAudits.forEach(item => {
+      if (item.result.scoreDisplayMode === 'notApplicable') return;


I'm not sure what you mean. This line just hides the notApplicable metrics from the metrics section of the category.

Or were you asking if metrics with notApplicable work with the score? If so, the answer is yes, just like others their weight just disappears (though separate weights for timespan is completely different bag of worms!)

patrickhulce · 2021-07-08T16:00:20Z

This is mostly obsolete and implemented at this point 🎉

core(fr): usable traces in timespan mode

175a697

patrickhulce requested a review from adamraine June 1, 2021 21:36

google-cla bot added the cla: yes label Jun 1, 2021

devtools-bot assigned adamraine Jun 1, 2021

devtools-bot added the waiting4reviewer label Jun 1, 2021

adamraine reviewed Jun 2, 2021

View reviewed changes

brendankenny reviewed Jun 3, 2021

View reviewed changes

patrickhulce commented Jun 7, 2021

View reviewed changes

patrickhulce mentioned this pull request Jun 8, 2021

core(fr): split traceOfTab into timespan/navigation types #12633

Merged

adamraine mentioned this pull request Jun 10, 2021

core(fr): add audit mode filter #12649

Merged

This was referenced Jun 23, 2021

core(fr): add lighthouseMarker timeOrigin support #12688

Merged

core(fr): enable traces artifact in timespan mode #12695

Merged

patrickhulce closed this Jul 8, 2021

patrickhulce deleted the fr_timespan_trace branch July 8, 2021 16:00

Traces & Timespans: A Preview #12595

Traces & Timespans: A Preview #12595

Conversation

patrickhulce commented Jun 1, 2021

adamraine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine commented Jun 3, 2021

patrickhulce commented Jun 3, 2021

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jul 8, 2021

brendankenny Jun 7, 2021 •

edited

Loading

brendankenny Jun 7, 2021 •

edited

Loading