core(diagnostics): add diagnostic audits #7052

patrickhulce · 2019-01-17T21:24:34Z

Summary
Collect page vital stats into a single audit. Also adds two new user-facing audits for server latency by origin and RTT by origin.

Related Issues/PRs
#6775

benschwarz · 2019-01-17T21:41:17Z

These are going to be hugely valuable! Great work so far @patrickhulce

exterkamp

Audits LGTM, not sure about the scoring of the audits, but since they are informational I guess it doesn't matter?

Mostly nits about the UIStrings and diagnostic suggestions.

lighthouse-core/audits/diagnostics.js

exterkamp · 2019-01-17T23:26:26Z

lighthouse-core/audits/diagnostics.js

+    return {
+      score: 1,
+      rawValue: 1,
+      details: {items: [diagnostics]},


Just a note probably for another PR, but this format is weird for informational audits. Can we make a new details format for just info? Like, just put the fields directly in details? Or if ts won't like that:

details: { ... information: { key: value, ... }, ... }

The array makes it weird 😦

yeah agreed, it's the same as the metrics audit for now

lighthouse-core/audits/network-rtt.js

lighthouse-core/audits/network-server-latency.js

lighthouse-core/audits/network-rtt.js

paulirish · 2019-01-18T02:34:33Z

lighthouse-core/audits/diagnostics.js

+      numFonts: records.filter(r => r.resourceType === 'Font').length,
+      numTasks: toplevelTasks.length,
+      numTasksOver10ms: toplevelTasks.filter(t => t.duration > 10).length,
+      numTasksOver25ms: toplevelTasks.filter(t => t.duration > 25).length,


no numTasksOver50ms ? 😮

heh, I'll add it, for lantern I was treating numTasksOver10ms as the rough equivalent :)

paulirish · 2019-01-18T02:36:42Z

lighthouse-core/audits/main-thread-tasks.js

+    const tasks = await MainThreadTasksComputed.request(trace, context);
+
+    const results = tasks
+      .filter(task => task.duration > 5)


this 5ms threshold should get mentioned in the description, probably?

sounds good

paulirish · 2019-01-18T02:37:19Z

lighthouse-core/audits/network-rtt.js

+    for (const [origin, additionalRtt] of analysis.additionalRttByOrigin.entries()) {
+      // TODO: https://github.com/GoogleChrome/lighthouse/issues/7041
+      if (!Number.isFinite(additionalRtt)) continue;
+      if (origin.startsWith('__')) continue;


there's a __SUMMARY__ entry in there that has the overall information, I'll flip it to only include http entries

paulirish · 2019-01-18T02:41:59Z

lighthouse-core/audits/network-server-latency.js

+
+const UIStrings = {
+  /** Descriptive title of a Lighthouse audit that tells the user the server latencies observed from each origin the page connected to. This is displayed in a list of audit titles that Lighthouse generates. */
+  title: 'Server Latencies',


Server Backend Latencies?

paulirish · 2019-01-18T02:42:33Z

lighthouse-core/audits/network-server-latency.js

+  title: 'Server Latencies',
+  /** Description of a Lighthouse audit that tells the user that server latency can effect their website's performance negatively. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
+  description: 'Server latencies can impact web performance. ' +
+    'If the server latency of an origin is high, it\'s an indication the server is overloaded ' +


I think this description should also indicate how the RTT is factored in. Are these values JUST the backend latency or the BL + RTT?

it's our best guess for "just the backend latency"

I'm not sure how to put this without unnecessarily confusing folks. I'm not confident that the number of people who will understand this subtlety is very high (or the people that will know what to do with this audit, honestly)

k. well given the updated title change i think it's good.

or the people that will know what to do with this audit, honestly

seems like the audit should always be hidden, then?

brendankenny

Also adds two new user-facing audits for server latency by origin and RTT by origin.

can we split this out? I'm not sure why these are in this PR instead of their own :)

patrickhulce · 2019-01-20T15:11:34Z

can we split this out?

sure

I'm not sure why these are in this PR instead of their own :)

because those are also critical page vitals that I've been using to debug variance

patrickhulce · 2019-01-28T16:57:28Z

Can this get some priority when reviewing as folks come back from NY? :)

This is blocking my further efforts to expose variance, so I'd love to get it merged ASAP

paulirish · 2019-01-28T18:29:14Z

Also adds two new user-facing audits for server latency by origin and RTT by origin.

Proposed compromise: keep these audits in the PR but don't expose as user-facing. That can be done in a followup.

patrickhulce · 2019-01-28T18:50:26Z

Proposed compromise: keep these audits in the PR but don't expose as user-facing. That can be done in a followup.

Deal!

brendankenny · 2019-01-30T19:09:17Z

Proposed compromise: keep these audits in the PR but don't expose as user-facing.

lol, I was less concerned about the audits than the length of the PR (before tests). Do I need to forward "Small CLs" to everyone? :P

Smaller PRs get reviewed faster and take fewer rounds of review!

old news

patrickhulce · 2019-01-30T19:48:21Z

So is the conclusion that we're going back to the version without those two audits?

I'm fine either way, but most recent attempt suggests the divide between L and XL does not help much in terms of eyeballs and I don't see a way of making this an XS/S with all our fixtures that get updated 😛

There's ~140 lines of substance, ~800 lines of fixtures, ~170 lines of strings/new audit boilerplate. Let me know how it'd be preferred to be divided for review.

brendankenny

It does feel like we're coming to a decision point with putting diagnostics in the LHR. LHRs are approaching ~1MB in size again. main-thread-tasks doesn't end up too bad (2-6% on bad sites), but network-requests tends to be big (10-15% on bad sites, basically the same size as the thumbnails), all for data that is looked at a tiny fraction of the times LH is run.

At some point we should ask folks to just save artifacts, cause that's what they really want. Or maybe we make a --preset=diagnostic and add these audits or something.

Aside from that, everything here LGTM, but it all needs tests :S

brendankenny · 2019-01-30T23:54:22Z

lighthouse-core/audits/network-server-latency.js

+  title: 'Server Latencies',
+  /** Description of a Lighthouse audit that tells the user that server latency can effect their website's performance negatively. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
+  description: 'Server latencies can impact web performance. ' +
+    'If the server latency of an origin is high, it\'s an indication the server is overloaded ' +


or the people that will know what to do with this audit, honestly

seems like the audit should always be hidden, then?

brendankenny · 2019-01-30T23:56:35Z

lighthouse-core/config/default-config.js

+        {id: 'network-rtt', weight: 0},
+        {id: 'network-server-latency', weight: 0},
+        {id: 'main-thread-tasks', weight: 0},
+        {id: 'diagnostics', weight: 0},


can we move these audits down to the bottom and add a comment? Kind of subtle that no group in the performance category means they're not visible :)

brendankenny · 2019-01-30T23:57:22Z

lighthouse-core/audits/network-rtt.js

+ */
+'use strict';
+
+const Audit = require('./audit');


brendankenny · 2019-01-30T23:59:03Z

lighthouse-core/audits/network-rtt.js

+    const results = [];
+    for (const [origin, additionalRtt] of analysis.additionalRttByOrigin.entries()) {
+      // TODO: https://github.com/GoogleChrome/lighthouse/issues/7041
+      if (!Number.isFinite(additionalRtt)) continue;


seems important to include these if using them for debugging/anomaly spotting?

good point, done

brendankenny · 2019-01-31T00:00:01Z

lighthouse-core/audits/network-server-latency.js

+    const tableDetails = Audit.makeTableDetails(headings, results);
+
+    return {
+      score: Math.max(1 - (maxLatency / 500), 0),


fwiw, if it's informative this gets nulled out

removed

I played around with it being scored, but that felt like it put too much emphasis on it

brendankenny · 2019-01-31T00:03:16Z

lighthouse-cli/test/cli/__snapshots__/index-test.js.snap

@@ -96,9 +96,21 @@ Object {
    Object {
      "path": "font-display",
    },
+    Object {


needs to be updated

patrickhulce · 2019-01-31T01:35:24Z

It does feel like we're coming to a decision point with putting diagnostics in the LHR. LHRs are approaching ~1MB in size again.

Yeah, I agree. I also think we need to find a balance with being able to provide a bit more information when users are confused though. There's the extreme like #5844, but basic stats on the page seem like fair game for intermediate devs to understand.

I think there's value in having diagnostic information available in the report (even if it should be hidden a bit), so that a junior dev can call over to someone who knows a bit more and help them interpret something when things get trickier.

patrickhulce · 2019-02-01T20:02:52Z

bump

patrickhulce · 2019-02-04T15:43:35Z

🏏 🏏 🏏

brendankenny

I somehow completely missed your push/bump last week and was still waiting for your update. Really sorry to make you wait on this.

This reverts commit 4513546.

patrickhulce requested review from brendankenny, exterkamp and paulirish as code owners January 17, 2019 21:24

exterkamp previously requested changes Jan 18, 2019

View reviewed changes

paulirish mentioned this pull request Jan 18, 2019

core(lhr): include resourceSize in network-requests audit #7056

Merged

paulirish reviewed Jan 18, 2019

View reviewed changes

brendankenny reviewed Jan 19, 2019

View reviewed changes

core(diagnostics): add diagnostic audits

73c49ec

patrickhulce force-pushed the page_vitals branch from 0c164a2 to 73c49ec Compare January 22, 2019 04:54

just expose toplevel tasks

6e911fa

paulirish assigned brendankenny and exterkamp Jan 28, 2019

patrickhulce added 3 commits January 28, 2019 14:03

add back rtt and server latency

557d281

not user facing

dd797c8

update sample json

512ae0b

brendankenny reviewed Jan 31, 2019

View reviewed changes

patrickhulce added 4 commits January 30, 2019 19:36

Merge branch 'master' into page_vitals

6fb1fd9

feedback

b488923

tests

0c87b0b

tests

501f5b5

brendankenny approved these changes Feb 4, 2019

View reviewed changes

brendankenny merged commit 4513546 into master Feb 4, 2019

brendankenny deleted the page_vitals branch February 4, 2019 19:34

patrickhulce added a commit that referenced this pull request Feb 4, 2019

Revert "core(diagnostics): add diagnostic audits (#7052)"

2d4c2f2

This reverts commit 4513546.

patrickhulce mentioned this pull request Feb 5, 2019

core(networkanalysis): set additional RTT to 0 not NaN #7156

Merged

core(diagnostics): add diagnostic audits #7052

core(diagnostics): add diagnostic audits #7052

Conversation

patrickhulce commented Jan 17, 2019 • edited by brendankenny Loading

benschwarz commented Jan 17, 2019

exterkamp 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 Jan 18, 2019 • 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

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

patrickhulce commented Jan 20, 2019

patrickhulce commented Jan 28, 2019 • edited Loading

paulirish commented Jan 28, 2019

patrickhulce commented Jan 28, 2019

brendankenny commented Jan 30, 2019

patrickhulce commented Jan 30, 2019

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

patrickhulce Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jan 31, 2019

patrickhulce commented Feb 1, 2019

patrickhulce commented Feb 4, 2019

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce commented Jan 17, 2019 •

edited by brendankenny

Loading

patrickhulce Jan 18, 2019 •

edited

Loading

patrickhulce commented Jan 28, 2019 •

edited

Loading

patrickhulce Jan 31, 2019 •

edited

Loading