Add TTI < 10s audit for PWA #1840

paulirish · 2017-03-09T00:23:05Z

Basic audit that just checks TTI value against a threshold.

Current UI is

Note: I have this audit included in default.json here, but I will remove it before landing this PR. It's just there to make testing a bit easier. :)

Once landed, I will move the other perf metrics from PWA to Perf, leaving only this one in PWA.

Fixes #1831

patrickhulce · 2017-03-09T00:37:07Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+      if (!isFast) {
+        return LoadFastEnough4Pwa.generateAuditResult({
+          rawValue: false,
+          debugString: `Under mobile conditions, the TTI was at ${ttiResult.displayValue}. ` +


do we need a debug string here?
also feels a little weird we're asserting here that everything was under mobile conditions when we don't really know it was :) thoughts on adding some fancy check for network throughput AND TTI matching certain conditions?

Very good point. I'd hate for people to setup a CI, leave off emulation, and always pass this audit.

Should there be a unit after ${ttiResult.displayValue}?

I like this idea. SG.

patrickhulce · 2017-03-09T00:38:42Z

lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js

+
+    return FastPWAAudit.audit(generateArtifactsWithTrace(traceEvents)).then(result => {
+      assert.equal(result.rawValue, false, 'not failing a long TTI value');
+      TTIAudit.audit = origTTI;


will this mess anything else up if it fails? should we restore original method in catch too?

Was thinking about that! Maybe move to before each etc..

another reason why we need randomized test order :)

ebidel · 2017-03-09T00:58:43Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+      if (!isFast) {
+        return LoadFastEnough4Pwa.generateAuditResult({
+          rawValue: false,
+          debugString: `Under mobile conditions, the TTI was at ${ttiResult.displayValue}. ` +


Should there be a unit after ${ttiResult.displayValue}?

ebidel · 2017-03-09T00:59:26Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+        return LoadFastEnough4Pwa.generateAuditResult({
+          rawValue: false,
+          debugString: `Under mobile conditions, the TTI was at ${ttiResult.displayValue}. ` +
+          'More details in "Performance" section.',


fancy link? More details in ["Performance" section](#performance).

can't do markdown in debugText. can only do it in helpText. :/

and displayValue already has a unit in it. :)

brendankenny · 2017-03-09T02:32:48Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

@@ -0,0 +1,62 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


this is hideous

but for reals, I think we should have one style or the other, and not mix them based on who wrote the file :)

i am very happy to change the codebase in one full sweep.

i am very happy to change the codebase in one full sweep.

sounds perfect for another PR :P

brendankenny · 2017-03-09T02:34:36Z

lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js

+
+    return FastPWAAudit.audit(generateArtifactsWithTrace(traceEvents)).then(result => {
+      assert.equal(result.rawValue, false, 'not failing a long TTI value');
+      TTIAudit.audit = origTTI;


another reason why we need randomized test order :)

brendankenny · 2017-03-09T02:35:40Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+//   https://developers.google.com/web/progressive-web-apps/checklist
+const MAXIMUM_TTI = 10 * 1000;
+
+class LoadFastEnough4Pwa extends Audit {


also hideous :P

brendankenny · 2017-03-09T02:37:00Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+'use strict';
+
+const Audit = require('./audit');
+const TTIMetric = require('./time-to-interactive');


we should really just make a "page timing" computed artifact and move both FMP and TTI in there

Yeah I have need for it now too in OffscreenImages thanks to @brendankenny obnoxiously useful suggestion :P

TBH i think audits depending on audits is almost the same as computed artifacts depending on eachother.

and i'm kinda less interested in making one big "page timing" one, as it'd have to run TTI tracing model stuff even if you only want FMP.

but now don't we have to run the TTI tracing model stuff 3x instead of just once?

gotta rerun parts of it, but the tracing model is a comptued artifact.

i profiled a run and reruning the TTI.audit() probably is like 3ms max, but we have like 6000ms of main thread time total.

brendankenny · 2017-03-09T02:43:40Z

lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js

+    const mockTTIResult = {extendedInfo: {value: {timings: {timeToInteractive: 15000}}}};
+    TTIAudit.audit = _ => Promise.resolve(mockTTIResult);
+
+    return FastPWAAudit.audit(generateArtifactsWithTrace(traceEvents)).then(result => {


why not just pass a trace in? If there was a trace with one really really long top level event it should fail the test :)

paulirish · 2017-03-10T01:05:05Z

Okay! Added checks to make sure network traffic was sufficiently slow.

Screenshots of the two failure cases below:

Test changed to monkeypatch TTI audit, as creating fake traces involves screenshots, etc.

ptal

patrickhulce · 2017-03-10T01:15:59Z

lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js

+    });
+  });
+
+  it('fails a bad TTI value', () => {


add a quick test for the network case?

quick you say..

patrickhulce · 2017-03-10T01:16:49Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+      category: 'PWA',
+      name: 'load-fast-enough-for-pwa',
+      description: 'Page load is fast enough on 3G',
+      helpText: 'Satisfied if the _Time To Interactive_ duration is shorter than _10 seconds_, as defined by the [PWA Baseline Checklist](https://developers.google.com/web/progressive-web-apps/checklist).',


maybe add a note here too that emulation must be enabled?

brendankenny · 2017-03-13T01:47:59Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

@@ -0,0 +1,62 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


but for reals, I think we should have one style or the other, and not mix them based on who wrote the file :)

brendankenny · 2017-03-13T05:29:53Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+  static audit(artifacts) {
+    const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS];
+    const allRequestLatencies = networkRecords.map(record => {
+      if (!record._timing) return Infinity;


why default with Infinity and not e.g. 0?

because if there is no timing data on a request then we don't care about it. we don't want to consider it for this analysis.

i'm also happy to filter netRecords to only the ones with timing first.

or alternatively the allRequestLatencies.every(val => val > latency3gMin) thing will return true if val is undefined.

definitely don't want to depend on implicit NaN behavior. I was just wondering, but filtering does sound like it might be clearer (plus Infinity doesn't make it through JSON.stringify(), so it makes sense if including the latencies in the extendedInfo)

brendankenny · 2017-03-13T05:32:56Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+
+      const extendedInfo = {
+        formatter: Formatter.SUPPORTED_FORMATS.NULL,
+        value: {allRequestLatencies, timeToInteractive}


do we care about having either one of these in the extended info? timeToInteractive may make sense, I guess (e.g. if you run the PWA checks but not the perf ones), but probably not the latencies

shrugs.
i am just thinking that people will probably want to debug failures here ( in case throttling messed up or they are trying to use diff throttling or real cellular )...

and if we know that A BUNCH of their reqs were slow enough but 1 had a latency that barely missed things, then we can change our policy here.

i am just thinking that people will probably want to debug failures here ( in case throttling messed up or they are trying to use diff throttling or real cellular )...

and if we know that A BUNCH of their reqs were slow enough but 1 had a latency that barely missed things, then we can change our policy here.

OTOH things only get added to the results object, no one really gets around to trimming unused data out of extendedInfo, but fair enough :P

brendankenny · 2017-03-13T05:33:54Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+    });
+
+    const latency3gMin = Emulation.settings.TYPICAL_MOBILE_THROTTLING_METRICS.latency - 10;
+    const areLatenciesAll3G = allRequestLatencies.every(val => val > latency3gMin);


if you don't include areLatenciesAll3G in the extendedInfo, maybe pull this all into a areLatenciesAll3G(networkRecords) function?

incidentally, this is a really cool way of checking that it's a sufficiently slow connection :)

paulirish · 2017-03-17T06:03:14Z

PTAL. updated.

paulirish · 2017-03-22T02:45:05Z

@ebidel @brendankenny can i get an amen?

brendankenny · 2017-03-22T02:54:22Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

@@ -0,0 +1,62 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


i am very happy to change the codebase in one full sweep.

sounds perfect for another PR :P

brendankenny · 2017-03-22T02:56:01Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+ */
+
+const Audit = require('./audit');
+const TTIMetric = require('./time-to-interactive');


calling into another audit really is an architecture smell, but we can fix that in a follow up

brendankenny · 2017-03-22T02:58:04Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+  static audit(artifacts) {
+    const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS];
+    const allRequestLatencies = networkRecords.map(record => {
+      if (!record._timing) return Infinity;


definitely don't want to depend on implicit NaN behavior. I was just wondering, but filtering does sound like it might be clearer (plus Infinity doesn't make it through JSON.stringify(), so it makes sense if including the latencies in the extendedInfo)

brendankenny · 2017-03-22T03:05:31Z

lighthouse-core/test/audits/load-fast-enough-for-pwa-test.js

+    TTIAudit.audit = generateTTIResults(5000);
+    return FastPWAAudit.audit(generateArtifacts()).then(result => {
+      assert.equal(result.rawValue, true, 'fixture trace is not passing audit');
+    }).catch(err => {


don't need catch block (here and below), test will fail if a rejected promise is returned

brendankenny · 2017-03-22T03:12:00Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+
+      const extendedInfo = {
+        formatter: Formatter.SUPPORTED_FORMATS.NULL,
+        value: {allRequestLatencies, timeToInteractive}


i am just thinking that people will probably want to debug failures here ( in case throttling messed up or they are trying to use diff throttling or real cellular )...

and if we know that A BUNCH of their reqs were slow enough but 1 had a latency that barely missed things, then we can change our policy here.

OTOH things only get added to the results object, no one really gets around to trimming unused data out of extendedInfo, but fair enough :P

ebidel

Amen from me.

brendankenny

🏗️🏁⏱ LGTM

patrickhulce reviewed Mar 9, 2017

View reviewed changes

ebidel suggested changes Mar 9, 2017

View reviewed changes

brendankenny requested changes Mar 9, 2017

View reviewed changes

paulirish force-pushed the ttiunder10 branch from 221358d to 200e775 Compare March 10, 2017 01:06

patrickhulce reviewed Mar 10, 2017

View reviewed changes

brendankenny mentioned this pull request Mar 10, 2017

Simplify report generation: no pretty print, no formatter classes #1838

Merged

paulirish force-pushed the ttiunder10 branch from 200e775 to e24f8b7 Compare March 11, 2017 00:20

brendankenny requested changes Mar 13, 2017

View reviewed changes

brendankenny mentioned this pull request Mar 13, 2017

remove explicit required generateAuditResult() call #1857

Merged

paulirish added this to the Report v2.0alpha milestone Mar 17, 2017

paulirish added the blockingReportRefactor label Mar 17, 2017

paulirish modified the milestone: nevermind Mar 17, 2017

paulirish mentioned this pull request Mar 17, 2017

Move perf metrics into performance section #1876

Closed

paulirish added 4 commits March 17, 2017 16:57

Add TTI < 10s audit for PWA

17f51ad

feedback.

08a9a89

adjust formatter path.

c333c8a

test with network values.

544095f

paulirish force-pushed the ttiunder10 branch from 7ac35a2 to 544095f Compare March 17, 2017 05:57

paulirish added 2 commits March 17, 2017 17:00

rebase for generateAuditResult change.

0311c22

add more data to extendedInfo

2e0004a

brendankenny reviewed Mar 22, 2017

View reviewed changes

ebidel approved these changes Mar 22, 2017

View reviewed changes

feedback.

b35ac3c

brendankenny approved these changes Mar 22, 2017

View reviewed changes

brendankenny merged commit bad5bda into master Mar 22, 2017

brendankenny deleted the ttiunder10 branch March 22, 2017 19:26

brendankenny mentioned this pull request Jun 10, 2020

Align load-fast-enough-for-pwa with Core Web Vitals? #10943

Closed

		@@ -0,0 +1,62 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

Add TTI < 10s audit for PWA #1840

Add TTI < 10s audit for PWA #1840

Conversation

paulirish commented Mar 9, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Mar 10, 2017

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

paulirish commented Mar 17, 2017

paulirish commented Mar 22, 2017

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

ebidel left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment