feat(redirects-audit): Adding Redirect audit (PSI Compat) #3308

wardpeet · 2017-09-12T17:36:07Z

Looking at psi it allows 1 redirect (not sure if we should only consider subdomains as allowed redirects).
https://example.com -> https://m.example.com

Report:

Perhaps we should use start & end time to calculate wasted ms?

patrickhulce · 2017-09-12T17:59:59Z

nice! using the time taken for everything but the last request (minus TCP handshake if its the same domain) as wasted ms sounds good to me

wardpeet · 2017-09-12T19:27:19Z

do we need a smoke test?

brendankenny · 2017-09-12T19:32:19Z

Ideally yes, though we'd have to add some kind of redirect support to the test server?

Alternatively, some of the PWAs for the smokehouse run on live sites were chosen because they redirect. We could expand pwa-config.js to include the redirect test to smoke test it

brendankenny · 2017-09-12T19:35:29Z

if it ends up being complicated, we can do in a followup to keep the PR single-purpose

paulirish · 2017-09-13T02:44:05Z

lighthouse-core/audits/redirects.js

+          displayValue: pageRedirects.length,
+          rawValue: passed,
+          extendedInfo: {
+            value: pageRedirects


yah lets calculated the wasted time. we can then show this in opportunities rather than diagnostics.

paulirish

a few more things noted. still thinking this should move to Opportunities with a wastedMs calculation.

paulirish · 2017-09-20T01:05:18Z

lighthouse-core/audits/redirects.js

+const Audit = require('./audit');
+
+// PSI allows one redirect (http://example.com => http://m.example.com)
+const REDIRECT_TRESHOLD = 1;


paulirish · 2017-09-20T01:07:32Z

lighthouse-core/audits/redirects.js

+        const passed = pageRedirects.length <= REDIRECT_TRESHOLD;
+        if (!passed) {
+          debugString = `Your page has ${pageRedirects.length} redirects.` +
+            ' Redirects introduce additional delays before the page can be loaded.';


I guess we dont need this second sentence since its already in helptext

wardpeet · 2017-09-21T14:20:03Z

Local test page: (smokehouse)

Gmail looks like this:

Just a random website with 1 redirect looks like thise:

Zero redirects

kdzwinel · 2017-09-21T15:29:13Z

lighthouse-core/audits/redirects.js

+  }
+
+  static isRedirect(requestKey) {
+    return requestKey.includes('redirected');


it looks like we depend here on 'redirected' being used in the name of request ID. IMO it'd be better to use something more solid likeWebInspector.NetworkRequest.redirects or request status code.

Since I'm dealing with a similar thing here: #3311 . How about creating a MainResource computed artifact that exposes WebInspector.NetworkRequest.redirects that we both could use?

cc @brendankenny

@kdzwinel Should we name it more like getMainResourceChain?

@kdzwinel I think we have a slightly different use case :) I need to get the redirects as well so I cannot use the computed artifact

@wardpeet maybe I'm missing something, but when you request a MainResource artifact you also get all NetworkRequest (that contain all timing info) representing all redirects that led to that resource. Check out this:

Isn't that all you need to compute the time wasted in redirects?

ah sorry I missed that! Great!

Awesome 🙌 I already got one 👍 on my PR, so I hope to merge soon.

MainResource computed artifact sgtm, we'll prob want to move away from more "chain-like" computed artifacts though going forward and try to make use of the page-dependency-graph instead

patrickhulce · 2017-09-27T20:58:57Z

lighthouse-cli/test/smokehouse/redirects/expectations.js

+const cacheBuster = Number(new Date());
+module.exports = [
+  {
+    initialUrl: `http://localhost:10200/online-only.html?cb=${cacheBuster}&delay=500&redirect=%2Foffline-only.html%3Fcb=${cacheBuster}%26delay=500%26redirect%3D%2Fredirects-final.html`,


ha this is quite the URL :)

patrickhulce

sweet I like the look of this one!

patrickhulce · 2017-09-27T20:59:08Z

lighthouse-cli/test/smokehouse/redirects/expectations.js

+    url: 'http://localhost:10200/redirects-final.html',
+    audits: {
+      'redirects': {
+        score: 2,


let's assert on details length instead

patrickhulce · 2017-09-27T20:59:44Z

lighthouse-cli/test/smokehouse/redirects/expectations.js

+    url: 'http://localhost:10200/redirects-final.html',
+    audits: {
+      'redirects': {
+        score: 1,


patrickhulce · 2017-09-27T21:00:36Z

lighthouse-core/audits/redirects.js

+
+        return {
+          debugString,
+          score: pageRedirects.length,


score actually has meaning for the opportunities and is used to color the bar based on how bad the offending opportunity was, they can still get this information from the details length

wardpeet · 2017-09-29T20:43:39Z

@patrickhulce Updated the PR :) what do you think?

patrickhulce

thanks for the quick turnaround @wardpeet! :)

patrickhulce · 2017-09-29T20:55:18Z

chrome-launcher/ask.js

@@ -0,0 +1,32 @@
+/**


revert these? :)

patrickhulce · 2017-09-29T20:56:38Z

lighthouse-core/audits/redirects.js

+
+        return {
+          debugString,
+          score: passed,


oops, maybe I wasn't very clear last time my bad!

score is automatically determined for the performance opportunities, let's reuse that

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

Line 141 in 4b8d8a1

score: UnusedBytes.scoreForWastedMs(wastedMs),

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

Lines 21 to 30 in 4b8d8a1

/**

* @param {number} wastedMs

* @return {number}

*/

static scoreForWastedMs(wastedMs) {

if (wastedMs === 0) return 100;

else if (wastedMs < WASTED_MS_FOR_AVERAGE) return 90;

else if (wastedMs < WASTED_MS_FOR_POOR) return 65;

else return 0;

}

patrickhulce · 2017-09-29T20:57:37Z

lighthouse-cli/test/smokehouse/redirects/expectations.js

+    url: 'http://localhost:10200/redirects-final.html',
+    audits: {
+      'redirects': {
+        score: false,


once the numeric score is brought back, let's assert that this score is in the lowest bracket

patrickhulce · 2017-09-30T00:04:25Z

lighthouse-core/audits/redirects.js

+        let debugString = null;
+
+        const pageRedirects = redirectRequests.map(request => {
+          const wastedMs = (request.endTime - request.startTime) * 1000;


I don't think this is what we want here, from the screenshot of gmail it looks like request.endTime is the end time of the final mainResource, I think we want something more like requests[1].startTime - requests[0].startTime?

patrickhulce · 2017-09-30T00:05:19Z

lighthouse-core/audits/redirects.js

+          };
+        });
+
+        const passed = pageRedirects.length <= REDIRECT_THRESHOLD;


let's go ahead and nuke the passed and debugString now that passed isn't needed, hopefully its clear from the table how many redirects they had :)

patrickhulce · 2017-09-30T00:06:06Z

lighthouse-core/audits/redirects.js

+
+        const headings = [
+          {key: 'url', itemType: 'text', text: 'URL'},
+          {key: 'wastedMs', itemType: 'text', text: 'Wasted ms'},


nit: let's set the text to "Time for Redirect"

wardpeet · 2017-10-01T13:03:09Z

should have fixed everything.

One issue I found is that the displayValue is not the same as when you make a sum of all separate wastedMs in the table and this is due the formatMilliseconds which rounds the numbers in the table

patrickhulce

looks great! I'm ready to approve % open question about how to handle totalWastedMs :)

patrickhulce · 2017-10-02T16:30:25Z

lighthouse-core/audits/redirects.js

+      name: 'redirects',
+      description: 'Avoids page redirects.',
+      failureDescription: 'Has page redirects.',
+      helpText: ' Redirects introduce additional delays before the page can be loaded. [Learn more](https://developers.google.com/speed/docs/insights/AvoidRedirects).',


nit: remove space at beginning

patrickhulce · 2017-10-02T16:33:24Z

lighthouse-core/audits/redirects.js

+          const request = redirectRequests[i - 1];
+          const nextRequest = redirectRequests[i];
+          const wastedMs = (nextRequest.startTime - request.startTime) * 1000;
+          totalWastedMs += wastedMs;


I liked what you had about allowing a single redirect before like PSI, wdyt about excluding the first redirect cost from totalWastedMs but still reporting how long it took?

I like as we should report all redirects as a cost is a cost +1

patrickhulce

nicely done! 🎉 💯

to you @paulirish if you still have lingering requested changes

patrickhulce · 2017-10-04T05:30:04Z

lighthouse-core/audits/redirects.js

+      category: 'Performance',
+      name: 'redirects',
+      description: 'Avoids page redirects.',
+      failureDescription: 'Has page redirects.',


nit: I'd prefer adding an 'excessive' or some qualifier here so it's clear you can have one

😱 I'm not that good with words in english so what about
Has more than one page redirects?

heh sure Has more than one page redirect 👍

patrickhulce · 2017-10-04T05:32:45Z

lighthouse-cli/test/smokehouse/redirects/expectations.js

+    audits: {
+      'redirects': {
+        score: 100,
+        rawValue: '>=0',


nit: we can assert 0 exactly here, right?

brendankenny

sorry, drive-by nits on just the smoke testing part :)

brendankenny · 2017-10-04T05:52:00Z

lighthouse-cli/test/fixtures/static-server.js

@@ -51,7 +51,8 @@ function requestHandler(request, response) {
  }

  function sendResponse(statusCode, data) {
-    let headers;
+    let headers = {};


this change not needed anymore?

brendankenny · 2017-10-04T05:56:29Z

lighthouse-cli/test/fixtures/static-server.js

@@ -51,7 +51,8 @@ function requestHandler(request, response) {
  }

  function sendResponse(statusCode, data) {
-    let headers;
+    let headers = {};
+    let delay = 0;


maybe move delay declaration to below the filePath.endsWith set of conditionals since it's not used until inside if (queryString) {}?

brendankenny · 2017-10-04T05:58:17Z

lighthouse-cli/test/fixtures/static-server.js

+
+      // redirect url to new url if present
+      if (typeof queryString.redirect !== 'undefined') {
+        if (delay > 0) {


since delay defaults to 0, for code readability this could just drop the delay check and do return setTimeout(sendRedirect, delay, queryString.redirect); for both

brendankenny · 2017-10-04T06:01:33Z

lighthouse-cli/test/fixtures/static-server.js

    }

    response.writeHead(statusCode, headers);

-    // Delay the response by the specified ms defaulting to 2000ms for non-numeric values
-    if (queryString && typeof queryString.delay !== 'undefined') {
-      response.write('');


was response.write(''); important here? I have no idea :) but it was here since the first version

I don't think so as response.write(''); as response.end() is important to close the request.
maybe @paulirish still knows why he added it.

i barely remember it being important. is it fine if we keep it as is?

keep it as is = add the response.write('')?

okay scratch what i said. response.write('') is a noop.

.write('.') does change things quite considerably, but in a bad direction. (the latency moves from TTFB to content download).

your change here is great. let's keep it. :)

brendankenny · 2017-10-04T06:07:31Z

lighthouse-cli/test/smokehouse/redirects/expectations.js

+
+module.exports = [
+  {
+    initialUrl: `http://localhost:10200/online-only.html?cb=${cacheBuster}&delay=500&redirect=%2Foffline-only.html%3Fcb=${cacheBuster}%26delay=500%26redirect%3D%2Fredirects-final.html`,


smokehouse should be opening a new instance of chrome for each expectations entry, so are the cache busters necessary?

Probably not 👍

paulirish · 2017-10-05T19:02:10Z

I verified the numbers we're reporting:

(I updated the table below via contenteditable, dont freak out)

This list does leave off the last URL destination. (which is accounts.google.com/...), Which is kind of awkward. I'm going to try to tweak the table rendering to also include that.

Numbers are good. 👍

wardpeet · 2017-10-05T20:22:33Z

@paulirish I can add it to the table :) give me a sec 😸

paulirish

I have some proposed superficial changes. I'd like to merge this PR as it is and i'll followup with a PR for ward to review.

paulirish · 2017-10-05T23:39:12Z

lighthouse-cli/test/fixtures/static-server.js

    }

    response.writeHead(statusCode, headers);

-    // Delay the response by the specified ms defaulting to 2000ms for non-numeric values
-    if (queryString && typeof queryString.delay !== 'undefined') {
-      response.write('');


okay scratch what i said. response.write('') is a noop.

.write('.') does change things quite considerably, but in a bad direction. (the latency moves from TTFB to content download).

your change here is great. let's keep it. :)

wardpeet added this to the Loading Performance Audits milestone Sep 12, 2017

wardpeet added the new_audit label Sep 12, 2017

paulirish requested changes Sep 13, 2017

View reviewed changes

paulirish reviewed Sep 20, 2017

View reviewed changes

wardpeet force-pushed the feature/audit-redirects branch from 65e45ac to 3e0f9d9 Compare September 21, 2017 14:16

kdzwinel mentioned this pull request Sep 21, 2017

new SEO audit: HTTP status code #3311

Merged

kdzwinel reviewed Sep 21, 2017

View reviewed changes

Metaway mentioned this pull request Sep 21, 2017

null #3389

Closed

patrickhulce reviewed Sep 27, 2017

View reviewed changes

patrickhulce suggested changes Sep 27, 2017

View reviewed changes

wardpeet added 7 commits September 29, 2017 21:01

Adding redirects landing page

f69c4bc

Add unit tests

f427c46

Add smoketests for redirects audit

3c395e1

Fix unit tests after wastedMS being added

cce0b3b

Review changes

1272668

Fixes new eslint rules

bc334e2

Add delay to requests on smokehouse

60236d3

wardpeet force-pushed the feature/audit-redirects branch from 8da4782 to f9db70a Compare September 29, 2017 20:34

patrickhulce suggested changes Sep 29, 2017

View reviewed changes

wardpeet added 4 commits September 29, 2017 23:13

Move to requestMainResource

4298d7f

Review changes

78ddb26

Remove test-config

a90ace8

Use unusedbytes to calculate score

569e407

wardpeet force-pushed the feature/audit-redirects branch from 46255c4 to 569e407 Compare September 29, 2017 22:29

wardpeet added 2 commits September 30, 2017 01:32

Fix travis

1e933c7

fix travis score

16da79f

patrickhulce reviewed Sep 30, 2017

View reviewed changes

Fix time for redirect

be4bfa7

patrickhulce reviewed Oct 2, 2017

View reviewed changes

paulirish modified the milestones: Loading Performance Audits, Sprint Uno: Oct 2-13 Oct 2, 2017

Review changes: hide first redirect from wasted MS

2256807

patrickhulce approved these changes Oct 4, 2017

View reviewed changes

patrickhulce assigned paulirish Oct 4, 2017

brendankenny reviewed Oct 4, 2017

View reviewed changes

Review changes smokehouse

08fc271

Avoid mutating mainResource artifact

d56803e

GoogleChrome deleted a comment from googlebot Oct 5, 2017

paulirish approved these changes Oct 5, 2017

View reviewed changes

paulirish merged commit b074ac7 into GoogleChrome:master Oct 5, 2017

wardpeet deleted the feature/audit-redirects branch October 6, 2017 05:45

dependencies bot mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

brendankenny mentioned this pull request Mar 15, 2023

redirects audit perhaps too lenient #14895

Closed

	/**
	* @param {number} wastedMs
	* @return {number}
	*/
	static scoreForWastedMs(wastedMs) {
	if (wastedMs === 0) return 100;
	else if (wastedMs < WASTED_MS_FOR_AVERAGE) return 90;
	else if (wastedMs < WASTED_MS_FOR_POOR) return 65;
	else return 0;
	}

feat(redirects-audit): Adding Redirect audit (PSI Compat) #3308

feat(redirects-audit): Adding Redirect audit (PSI Compat) #3308

Conversation

wardpeet commented Sep 12, 2017 • edited by brendankenny Loading

patrickhulce commented Sep 12, 2017

wardpeet commented Sep 12, 2017

brendankenny commented Sep 12, 2017

brendankenny commented Sep 12, 2017

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Sep 21, 2017

kdzwinel Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel Sep 26, 2017 • edited Loading

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 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

wardpeet commented Sep 29, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Sep 29, 2017 • edited Loading

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 Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

wardpeet commented Oct 1, 2017

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

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

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

wardpeet Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

paulirish commented Oct 5, 2017

wardpeet commented Oct 5, 2017

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Sep 12, 2017 •

edited by brendankenny

Loading

kdzwinel Sep 21, 2017 •

edited

Loading

kdzwinel Sep 26, 2017 •

edited

Loading

patrickhulce Sep 29, 2017 •

edited

Loading

patrickhulce Sep 30, 2017 •

edited

Loading

wardpeet Oct 4, 2017 •

edited

Loading