Add details to "is fast on 3g" audit #2159

ebidel · 2017-05-05T17:25:47Z

R: all

This is really handy for debugging...knowing what requests failed the check.

paulirish · 2017-05-05T17:44:31Z

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

@@ -41,7 +40,7 @@ class LoadFastEnough4Pwa extends Audit {
      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). Network throttling is required (specifically: RTT latencies >= 150 RTT are expected).',


we'll need to decide if we want to essentially rename "FI" to "TTI" and bury the old one as version 0.1.

if we dont then we'll have to change this text.

if we dont then we'll have to change this text.

I'd like to keep it a little while longer, at least. I want to get a few plots/ runs in with it

probably makes sense to change the test regardless to reduce confusion with Consistently Interactive

paulirish · 2017-05-05T17:51:31Z

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

@@ -57,16 +56,21 @@ class LoadFastEnough4Pwa extends Audit {
        // Ignore requests that don't have timing data or resources that have
        // previously been requested and are coming from the cache.
        if (!record._timing || record._fromDiskCache || record._fromMemoryCache) {
-          return undefined;
+          return {url: record._url};


since we're now using an object we can be more explicit

const ret = {url: record._url}; if (!record._timing)) ret.isMissingTiming = true; if (record._fromDiskCache || record._fromMemoryCache) ret.isFromCache = true; if (ret.isFromCache || ret.isMissingTiming) return ret; ret.latency = record._timing.receiveHeadersEnd - record._timing.sendEnd; ret.latencyStr = ret.latency.toLocaleString(undefined, {maximumFractionDigits: 2}); return ret;

I can use more vars in the code if it'll make that part more clear, but I'm thinking we should start being diligent about sliming down audit results. Extra bits are nice, but if we're not going to use them in a renderer, they're extra weight.

paulirish · 2017-05-05T17:54:45Z

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 === undefined || val > latency3gMin);
+          val.latency === undefined || val.latency > latency3gMin);


const excludedLatencies = allRequestLatencies.filter(val => val.isFromCache || val.isMissingTiming); const includedLatencies = allRequestLatencies.filter(val => !excludedLatencies.includes(val)); // assert includedLatencies.length > 1 const areLatenciesAll3G = includedLatencies.every(val => val.latency > latency3gMin);

paulirish · 2017-05-05T17:57:07Z

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

@@ -78,12 +82,18 @@ class LoadFastEnough4Pwa extends Audit {
          value: {areLatenciesAll3G, allRequestLatencies, isFast, timeToFirstInteractive}
        };

+        const details = Audit.makeV2TableDetails([
+          {key: 'url', itemType: 'url', text: 'URL'},
+          {key: 'latency', itemType: 'text', text: 'Latency (ms)'},


key: 'latencyStr'

and you could add another column for "Cached?" in here..

Is a cached column worth showing? In theory, this table doesn't contain any requests that come from the cache.

paulirish · 2017-05-05T17:57:59Z

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

+        const details = Audit.makeV2TableDetails([
+          {key: 'url', itemType: 'url', text: 'URL'},
+          {key: 'latency', itemType: 'text', text: 'Latency (ms)'},
+        ], allRequestLatencies.filter(record => record.latency !== undefined));


and replace this guy with excludedLatencies

brendankenny · 2017-05-05T18:12:28Z

This is helpful for debugging, but isn't it irrelevant for the audit result? The audit is for TTFI, the 3g check is just to make sure we can do the check. If a user wants to know why their TTIFI was too slow this table doesn't help.

ebidel · 2017-05-05T19:21:39Z

This is helpful for debugging, but isn't it irrelevant for the audit result?

We say "Network throttling is required (specifically: RTT latencies >= 150 RTT are expected)." Without seeing which requests didn't meet that requirement, it's hard to know why you failed the audit.

brendankenny · 2017-05-05T19:38:56Z

But it's non-actionable information that looks like it's something they're doing, but unless they disabled throttling it's a lighthouse bug -- we didn't throttle everything correctly (or it's something like the caching bug). They can't slow down server responses in order to pass the test :)

We could only include the details when areAllLatencies3G fails, but this feels more like a debugString situation

ebidel · 2017-05-05T19:52:48Z

We could only include the details when areAllLatencies3G fails, but this feels more like a debugString situation

Yea, we should do that. debugString might be good, but we're not setup for that.

ebidel · 2017-05-05T20:20:12Z

Changed so we only show the details when areLatenciesAll3G !== true.

brendankenny

This still seems like this will be a bit confusing (nothing for the user to fix; maybe we should add a call to report a bug in a header or something?), but I don't feel super strongly :)

brendankenny · 2017-05-06T00:43:24Z

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

@@ -41,7 +40,7 @@ class LoadFastEnough4Pwa extends Audit {
      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). Network throttling is required (specifically: RTT latencies >= 150 RTT are expected).',


if we dont then we'll have to change this text.

I'd like to keep it a little while longer, at least. I want to get a few plots/ runs in with it

probably makes sense to change the test regardless to reduce confusion with Consistently Interactive

ebidel changed the title ~~Add details to "Is fast on 3g" audit~~ Add details to "is fast on 3g" audit May 5, 2017

ebidel force-pushed the latencies branch from edb7467 to e05c440 Compare May 5, 2017 17:38

Add details to "Is fast on 3g" audit

48a5ca7

ebidel force-pushed the latencies branch from e05c440 to 48a5ca7 Compare May 5, 2017 17:39

paulirish reviewed May 5, 2017

View reviewed changes

paulirish requested changes May 5, 2017

View reviewed changes

Only show details when all request are not realistic for 3g

8456f71

brendankenny reviewed May 6, 2017

View reviewed changes

paulirish approved these changes May 9, 2017

View reviewed changes

paulirish merged commit b50f780 into master May 9, 2017

paulirish deleted the latencies branch May 9, 2017 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add details to "is fast on 3g" audit #2159

Add details to "is fast on 3g" audit #2159

ebidel commented May 5, 2017

paulirish May 5, 2017

brendankenny May 6, 2017

paulirish May 5, 2017

ebidel May 5, 2017

paulirish May 5, 2017

paulirish May 5, 2017

ebidel May 5, 2017

paulirish May 5, 2017

brendankenny commented May 5, 2017

ebidel commented May 5, 2017

brendankenny commented May 5, 2017

ebidel commented May 5, 2017

ebidel commented May 5, 2017

brendankenny left a comment

brendankenny May 6, 2017

Add details to "is fast on 3g" audit #2159

Add details to "is fast on 3g" audit #2159

Conversation

ebidel commented May 5, 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

brendankenny commented May 5, 2017

ebidel commented May 5, 2017

brendankenny commented May 5, 2017

ebidel commented May 5, 2017

ebidel commented May 5, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment