core(fr): add navigation phase + trace gatherer #12098

patrickhulce · 2021-02-17T17:50:28Z

Summary
The primary goal of this PR is to add the trace gatherer and support performance metrics in Fraggle Rock navigation mode (as well as pave the way for upgrades to support timespan metrics). That change is pretty small, but this triggered a number of other changes.

Add new beforeNavigation/afterNavigation methods to distinguish this new category of "navigation-only" gatherers.
Adjust config validation rules to disallow navigation gatherers from depending on snapshot artifacts.
Refactor navigation runner to reduce duplication in collection of artifacts of each phase (there are now 5 instead of just 3)
Refactor default trace categories list to new gatherer, import from legacy driver.
Add more test coverage for these refactored areas.

Primary Point of Discussion

This PR sets the new execution sequence to the following phase order.

beforeNavigation
beforeTimespan
afterTimespan
afterNavigation
snapshot

The primary question IMO is when exactly beforeNavigation and afterNavigation should take place.

There is an argument for moving afterNavigation to happen after snapshot so it can depend on snapshot artifacts, but that means any timespan style recording would include all of the snapshot gatherer work which seems undesirable. I've kept it before snapshot and if anything truly needs the snapshot dependency we can think of something like afterSnapshot to fill that gap in the future.

Implications for Gatherer Authoring

There are 5 potential phases, and this is where that array of supported modes starts enter the territory that concerned me. Really we only have 3 types of valid gatherers. Timespan, Snapshot, or Navigation. We can enforce that fact in config validation if we like. The gatherer of each type should only truly implement the set of methods that are related to it but all the methods of the supported modes will be invoked.

Example: A timespan gatherer says it works for both timespans and navigations, but it only implements beforeTimespan and afterTimespan, leaving the rest as noops. When run in navigation mode, beforeNavigation and afterNavigation will still be executed.

Related Issues/PRs
ref #11313

patrickhulce · 2021-02-17T17:50:55Z

lighthouse-core/fraggle-rock/gather/base-gatherer.js

   */
-  snapshot(passContext) { }


moved this just to reflect the order in which they're invoked

patrickhulce · 2021-02-17T17:51:56Z

lighthouse-core/gather/driver.js

@@ -139,43 +137,7 @@ class Driver {
  }

  static get traceCategories() {


this is part of our public API, so didn't remove

should we mark deprecated?

ya let's do it

patrickhulce · 2021-02-17T17:52:24Z

lighthouse-core/gather/driver.js

@@ -816,15 +778,7 @@ class Driver {
  async beginTrace(settings) {
    const additionalCategories = (settings && settings.additionalTraceCategories &&
        settings.additionalTraceCategories.split(',')) || [];
-    const traceCategories = this._traceCategories.concat(additionalCategories);
-
-    // In Chrome <71, gotta use the chatty 'toplevel' cat instead of our own.


with Chrome stable up to 88 this feels pretty safe now :)

patrickhulce · 2021-02-17T17:53:19Z

lighthouse-core/gather/gatherers/trace-compat.js

+class TraceCompat extends FRGatherer {
+  /** @type {LH.Gatherer.GathererMeta<'Trace'>} */
+  meta = {
+    supportedModes: ['navigation'],


this is actually a fairly important signal that we shouldn't try to compute all the metrics over an arbitrary timespan

longer term I think this evolves into a Trace and NavigationTrace artifact or something where the latter is guaranteed to have been collected over a navigation

patrickhulce · 2021-02-17T17:54:39Z

lighthouse-core/test/fraggle-rock/config/config-test.js

+        .toThrow(/Dependency.*is invalid/);
+    });
+
+    it('should throw when navigation needs snapshot', () => {


complete coverage is already in validation-test but covering some more important common mistakes seemed useful

patrickhulce · 2021-02-17T17:55:07Z

types/gatherer.d.ts

@@ -100,9 +100,11 @@ declare global {
    export interface FRGathererInstance<TDependencies extends DependencyKey = DefaultDependenciesKey> {
      name: keyof LH.GathererArtifacts; // temporary COMPAT measure until artifact config support is available
      meta: GathererMeta<TDependencies>;
-      snapshot(context: FRTransitionalContext<TDependencies>): PhaseResult;


rearranged to signal the order in which they're invoked

connorjclark · 2021-02-17T20:03:50Z

lighthouse-core/gather/driver.js

@@ -139,43 +137,7 @@ class Driver {
  }

  static get traceCategories() {


should we mark deprecated?

lighthouse-core/gather/gatherers/trace.js

Co-authored-by: Connor Clark <cjamcl@google.com>

patrickhulce · 2021-02-17T21:41:47Z

lighthouse-core/gather/driver.js

@@ -139,43 +137,7 @@ class Driver {
  }

  static get traceCategories() {


ya let's do it

lighthouse-core/gather/driver.js

lighthouse-core/fraggle-rock/gather/navigation-runner.js

patrickhulce · 2021-02-18T18:03:45Z

I take it there are no strong feelings about the order of phase execution yet. I wouldn't really expect there to be without hands-on experience yet, so let's leave this as a good first start and if we run into any issues while converting the remaining gatherers we can revisit the decision if necessary.

core(fr): add navigation phase + trace gatherer

a97b87f

patrickhulce requested a review from a team as a code owner February 17, 2021 17:50

patrickhulce requested review from adamraine and removed request for a team February 17, 2021 17:50

google-cla bot added the cla: yes label Feb 17, 2021

devtools-bot assigned adamraine Feb 17, 2021

devtools-bot added the waiting4reviewer label Feb 17, 2021

patrickhulce commented Feb 17, 2021

View reviewed changes

connorjclark reviewed Feb 17, 2021

View reviewed changes

Update lighthouse-core/gather/gatherers/trace.js

aafe960

Co-authored-by: Connor Clark <cjamcl@google.com>

vercel bot deployed to Preview February 17, 2021 21:40 View deployment

patrickhulce commented Feb 17, 2021

View reviewed changes

Update lighthouse-core/gather/driver.js

c4cee83

vercel bot deployed to Preview February 17, 2021 21:42 View deployment

adamraine approved these changes Feb 17, 2021

View reviewed changes

lighthouse-core/fraggle-rock/gather/navigation-runner.js Show resolved Hide resolved

patrickhulce added 2 commits February 18, 2021 09:32

add jsdoc

3e7706d

Merge branch 'master' into fr_trace

d3429b2

vercel bot deployed to Preview February 18, 2021 15:33 View deployment

update chromium expectations

334a8bf

vercel bot deployed to Preview February 18, 2021 16:05 View deployment

patrickhulce added land-when-ci-is-green and removed waiting4reviewer labels Feb 18, 2021

devtools-bot merged commit f595247 into master Feb 22, 2021

devtools-bot deleted the fr_trace branch February 22, 2021 17:12

devtools-bot removed the land-when-ci-is-green label Feb 22, 2021

This was referenced Mar 14, 2021

Bump lighthouse from 6.3.0 to 7.2.0 KRRISH96/saikrishna.dev#289

Merged

Bump lighthouse from 6.4.1 to 7.2.0 chronark/drownthesun#181

Closed

Bump lighthouse from 6.5.0 to 7.2.0 imoisharma/jekyll-rtd-theme#5

Closed

paulirish pushed a commit that referenced this pull request Mar 23, 2021

core(fr): add navigation phase + trace gatherer (#12098)

5d1512d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(fr): add navigation phase + trace gatherer #12098

core(fr): add navigation phase + trace gatherer #12098

patrickhulce commented Feb 17, 2021

patrickhulce Feb 17, 2021

patrickhulce Feb 17, 2021

connorjclark Feb 17, 2021

patrickhulce Feb 17, 2021

patrickhulce Feb 17, 2021

patrickhulce Feb 17, 2021

patrickhulce Feb 17, 2021

patrickhulce Feb 17, 2021

connorjclark Feb 17, 2021

patrickhulce Feb 17, 2021

patrickhulce commented Feb 18, 2021

		@@ -139,43 +137,7 @@ class Driver {
		}

		static get traceCategories() {

core(fr): add navigation phase + trace gatherer #12098

core(fr): add navigation phase + trace gatherer #12098

Conversation

patrickhulce commented Feb 17, 2021

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

patrickhulce commented Feb 18, 2021