Move networkRecords to a computed artifact #2062

paulirish · 2017-04-25T06:12:43Z

This PR moves networkRecords into a computed artifact. It's a little different than other ones though:

A number of gatherers rely on tracingData.networkRecords. As such, we continue to generate these at runtime, so they're available in afterPass. However after the gathering phase is done... we trash this tracingData.networkRecords data. Afterwards when auditing, the networkRecords are recomputed from the artifacts.devtoolsLogs which these days is collected and stored.

The primary motivation here is we want enable splitting up Gathering from Auditing. (So you could Gather on one machine and, later, Audit on another.) However, networkRecords contains circular references, so currently artifacts is not serializable. After this PR is merged, all our artifacts are serializable. \o/

This PR also has a breaking change regarding our ingesting of artifacts.performanceLog. It's now artifacts.devtoolsLogs and that must be an object (typically with a first key of defaultPass).

Reviewers: The w=1 url will help, as there's some decent whitespace changes in here.

There's also more changes we could consider in scope for this PR. (Reducing the double request... calls we have in a few audits now.) However, based on the # of lines this'll touch, I'd recommend I finish that up in a followup.

patrickhulce

sweet change, maybe needs a few tweaks for smokehouse though?

patrickhulce · 2017-04-25T22:12:21Z

lighthouse-core/audits/byte-efficiency/total-byte-weight.js

+    return artifacts.requestNetworkRecords(devtoolsLogs).then(networkRecords => {
+      return artifacts.requestNetworkThroughput(networkRecords).then(networkThroughput => {
+        let totalBytes = 0;
+        const results = networkRecords.reduce((prev, record) => {


forEach ;)?

patrickhulce · 2017-04-25T22:16:32Z

lighthouse-core/gather/gather-runner.js

-                  (tracingData.networkRecords[passName] = passData.networkRecords);
+              tracingData.devtoolsLogs[passName] = passData.devtoolsLog;
+
+              // DD SOMETHING WITH


care to clarify?

patrickhulce · 2017-04-25T22:18:48Z

readme.md

@@ -337,7 +337,8 @@ Lighthouse can be used to analyze trace and performance data collected from othe
    "traces": {
      "defaultPass": "/User/me/lighthouse/lighthouse-core/test/fixtures/traces/trace-user-timings.json"
    },
-    "performanceLog": "/User/me/lighthouse/lighthouse-core/test/fixtures/traces/perflog.json"
+    "devtoolsLogs":


this seems like invalid json, missing some {}?

paulirish · 2017-04-26T21:53:29Z

@patrickhulce thanks for taking a look! updated now.

patrickhulce

a-ok by me

patrickhulce · 2017-04-26T21:55:41Z

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -83,6 +83,7 @@ function runLighthouse(url, configPath, saveAssetsPath) {
    }

    runCount++;
+    console.log(`${log.dim}$ ${command} ${args.join(' ')} ${log.reset}`);


is this in your separate PR now?

rebased to master so we're good here.

patrickhulce · 2017-04-26T21:58:00Z

lighthouse-core/audits/critical-request-chains.js

-      function walk(node, depth) {
-        const children = Object.keys(node);
+    const devtoolsLogs = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
+    return Promise.resolve().then(_ =>


is there a reason to wrap this in a Promise.resolve? what can fail before we get into the .then of requestNetworkRecords?

no reason. nuked it.

brendankenny · 2017-04-27T21:21:12Z

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

-      });
+    const devtoolsLogs = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
+    return artifacts.requestNetworkRecords(devtoolsLogs).then(networkRecords => {
+      return artifacts.requestNetworkThroughput(networkRecords).then(networkThroughput =>


since computed artifacts can call each other now, do you want to make requestNetworkThroughput take devtoolsLogs directly now and have it create the networkRecords internally?

i do, yes! totally makes sense. in fact, i mentioned this at the end of the issue description :) ..

i'd do it in this PR but it'd add a lot more to the diff for no immediate benefit. I am suggesting doing that cleanup in a followup.

i do, yes! totally makes sense. in fact, i mentioned this at the end of the issue description :) ..

got it, didn't realize that's what that was referring to

but it'd add a lot more to the diff

although, would it? It would remove a lot of the lines just due to nesting added to this PR. Is the hard part network-throughput-test.js?

benschwarz · 2017-04-27T22:46:45Z

Great! I think this will make custom audits that monitor the network easier too, right?

paulirish · 2017-04-27T23:18:11Z

Great! I think this will make custom audits that monitor the network easier too, right?

Probably! However in an audit you can't really "monitor" as the requests have all finished by then. :)

benschwarz · 2017-04-27T23:32:25Z

Probably! However in an audit you can't really "monitor" as the requests have all finished by then. :)

Indeed. Do you think it'd be possible to keep the request log in the same format that Network.responseReceived returns it in? Or does it do this already?

brendankenny

I think there's an issue here: devtoolsLog only gets recorded when a trace is happening, so will be empty in all other passes. I think this is working with current default.js because we don't use non-DEFAULT_PASS networkRecords in any audits, only in gatherers, which get the network record whether or not devtoolsLog was created. If an audit comes along that does need the network records from another pass, I'm pretty sure artifacts.devtoolsLogs['other-pass'] will be an empty array.

Rambling ideas:

We could always do a devtoolsLog, but as I note inline, it is a little weird to tie that to begin/endTrace when we rely on it for network recording. I'm not sure how best to rationalize timing between trace and network begin and end. Size overhead of saved artifacts is going to get worse, too, though not by much (just additional saved Page domain stuff).

Maybe NetworkRecorder could keep a log of both devtools messages and the constructed network log, and then if there is no trace that pass, passData.devtoolsLog could be set to NetworkRecorder's version?

That also seems too roundabout, so maybe we should just always take a devtoolsLog, and then only keep it if config.recordTrace or config.recordNetwork was true? Would still need to figure out what starts and ends the logging. Maybe devtoolsLog creation and control should be moved out of driver and into gather-runner, responding to driver events? Makes a certain amount of sense, though we'd need a way to listen to all events based on domain, which node event emitters don't handle well...

brendankenny · 2017-04-27T22:47:49Z

lighthouse-core/audits/byte-efficiency/offscreen-images.js

@@ -42,7 +42,7 @@ class OffscreenImages extends Audit {
      informative: true,
      helpText: 'Images that are not above the fold should be lazily loaded after the page is ' +
        'interactive. Consider using the [IntersectionObserver](https://developers.google.com/web/updates/2016/04/intersectionobserver) API.',
-      requiredArtifacts: ['ImageUsage', 'ViewportDimensions', 'traces', 'networkRecords']
+      requiredArtifacts: ['ImageUsage', 'ViewportDimensions', 'traces', 'devtoolsLogs']


this (and the old networkRecords) doesn't appear to be used?

used by the shared audit they extend

oh, I see. const Audit = require('./byte-efficiency-audit'); makes that difficult to spot. We should probably rename at some point :)

brendankenny · 2017-04-27T22:52:04Z

lighthouse-core/audits/byte-efficiency/uses-optimized-images.js

@@ -46,7 +46,7 @@ class UsesOptimizedImages extends Audit {
        'The following images could have smaller file sizes when compressed with ' +
        '[WebP](https://developers.google.com/speed/webp/) or JPEG at 80 quality. ' +
        '[Learn more about image optimization](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/image-optimization).',
-      requiredArtifacts: ['OptimizedImages', 'networkRecords']
+      requiredArtifacts: ['OptimizedImages', 'devtoolsLogs']


also unused?

used by the shared audit they extend

brendankenny · 2017-04-27T22:52:29Z

lighthouse-core/audits/byte-efficiency/uses-request-compression.js

@@ -39,7 +39,7 @@ class ResponsesAreCompressed extends Audit {
      helpText: 'Text-based responses should be served with compression (gzip, deflate or brotli)' +
        ' to minimize total network bytes.' +
        ' [Learn more](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/optimize-encoding-and-transfer).',
-      requiredArtifacts: ['ResponseCompression', 'networkRecords']
+      requiredArtifacts: ['ResponseCompression', 'devtoolsLogs']


used by the shared audit they extend

brendankenny · 2017-04-27T22:53:05Z

lighthouse-core/audits/byte-efficiency/uses-responsive-images.js

@@ -44,7 +44,7 @@ class UsesResponsiveImages extends Audit {
        'Image sizes served should be based on the device display size to save network bytes. ' +
        'Learn more about [responsive images](https://developers.google.com/web/fundamentals/design-and-ui/media/images) ' +
        'and [client hints](https://developers.google.com/web/updates/2015/09/automating-resource-selection-with-client-hints).',
-      requiredArtifacts: ['ImageUsage', 'ViewportDimensions', 'networkRecords']
+      requiredArtifacts: ['ImageUsage', 'ViewportDimensions', 'devtoolsLogs']


used by the shared audit they extend

brendankenny · 2017-04-27T22:54:29Z

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

-      });
+    const devtoolsLogs = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
+    return artifacts.requestNetworkRecords(devtoolsLogs).then(networkRecords => {
+      return artifacts.requestNetworkThroughput(networkRecords).then(networkThroughput =>


but it'd add a lot more to the diff

although, would it? It would remove a lot of the lines just due to nesting added to this PR. Is the hard part network-throughput-test.js?

brendankenny · 2017-04-28T00:03:22Z

package.json

@@ -30,6 +30,7 @@
    "cli-unit": "bash lighthouse-core/scripts/run-mocha.sh --cli",
    "viewer-unit": "bash lighthouse-core/scripts/run-mocha.sh --viewer",
    "unit": "bash lighthouse-core/scripts/run-mocha.sh --default",
+    "unit-core": "yarn core-unit",


lol, this is the worst. Just get rid of core-unit. I always forget the order on this as well

brendankenny · 2017-04-28T00:06:00Z

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

@@ -461,7 +474,7 @@ describe('Config', () => {
    it('should merge other values', () => {
      const artifacts = {
        traces: {defaultPass: '../some/long/path'},
-        performanceLog: 'path/to/performance/log',
+        devtoolsLogs: {defaultPass: 'path/to/performance/log'},


'path/to/devtools/log'?

brendankenny · 2017-04-28T00:06:30Z

lighthouse-core/test/gather/fake-driver.js

@@ -68,6 +68,9 @@ module.exports = {
      recordsFromLogs(require('../fixtures/perflog.json'))
    );
  },
+  get devtoolsLog() {
+    return require('../fixtures/perflog.json');


can we move the file to be devtools-log.json or whatever? The two different terms are confusing :)

brendankenny · 2017-04-28T00:09:10Z

lighthouse-core/test/runner-test.js

@@ -462,7 +464,7 @@ describe('Runner', () => {

      // Verify a computed artifact. driverMock will include networkRecords
      // built from fixtures/perflog.json.
-      const networkRecords = results.artifacts.networkRecords.firstPass;
+      const networkRecords = results.artifacts.networkRecords.firstPassDEPRECATED;


at least do this with requestNetworkRecords?

brendankenny · 2017-04-28T00:10:01Z

lighthouse-core/lib/network-recorder.js


+  static recordsFromLogs(logs) {


looks like we don't have unit tests for this anywhere?

patrickhulce · 2017-04-29T00:42:33Z

this would also be great to land before #2023 so that I can add real trace/devtools log tests instead of hacky network records of today

patrickhulce · 2017-04-29T00:43:58Z

I'm pretty sure artifacts.devtoolsLogs['other-pass'] will be an empty array.

It totally will be, but I'll channel my inner Brendan here and say that we should not worry about that until we actually encounter that situation ;)

patrickhulce · 2017-04-29T00:44:45Z

I guess it would be nice to just nuke the recordNetwork flag in the pass config though so we don't get confused later 👍

brendankenny · 2017-04-29T00:52:24Z

It totally will be, but I'll channel my inner Brendan here and say that we should not worry about that until we actually encounter that situation ;)

I see where you're going with that :)

I think in this case we are encountering it, though, since we already support custom configs, we just don't have any that record network but not trace in our tests (and to be fair, it's not likely anyone has written a config that hits this).

I'm hoping with a section on custom audits at I/O we can get more people writing them, though.

…is good.)

paulirish · 2017-04-29T02:28:06Z

ptal

@brendankenny Regarding your large comment: Now we always record devtools log and place it on artifacts. There's no more relationship to recordTrace.

simplify devtoolsLog recording, url redirect tracking, and eliminate explicit network recording

paulirish · 2017-05-05T04:38:39Z

with #2133 merged in here now... 🎉
brb while i resolve the conflicts.

paulirish · 2017-05-05T06:13:16Z

manually merged to provide best control of the commits

commit 752b2b0
Author: Paul Irish [email protected]

merge fixup.

commit e5bd630
Merge: d7e4d1b af479e9
Author: Paul Irish [email protected]

Merge branch 'master' into netrecordspiecemeal

commit d7e4d1b
Author: Brendan Kenny [email protected]

always construct networkRecords from devtoolsLog (#2133)
simplify devtoolsLog recording, url redirect tracking, and eliminate explicit network recording

commit 75d8f4d
Author: Brendan Kenny [email protected]

fixup types

commit 11a1db3
Author: Paul Irish [email protected]

networkRecords => computed artifact.
generate networkRecords during gather via the networkRecorder dispatcher
breaking change: performanceLog => devtoolsLogs

we done with #2062, folks! holla.

paulirish requested a review from brendankenny April 25, 2017 06:12

patrickhulce reviewed Apr 25, 2017

View reviewed changes

paulirish mentioned this pull request Apr 26, 2017

Split Gather phase from Audit phase #1806

Closed

paulirish force-pushed the netrecordsComputed branch from 409ca9f to 317935c Compare April 26, 2017 21:49

patrickhulce approved these changes Apr 26, 2017

View reviewed changes

paulirish force-pushed the netrecordsComputed branch from cf64669 to 94db7a7 Compare April 26, 2017 23:48

paulirish assigned brendankenny Apr 27, 2017

brendankenny reviewed Apr 27, 2017

View reviewed changes

brendankenny requested changes Apr 28, 2017

View reviewed changes

This was referenced Apr 29, 2017

rename references to Audit subclasses for clarity #2103

Merged

Remove recordNetwork #2102

Merged

paulirish force-pushed the netrecordsComputed branch from 4fad27a to 70a12bd Compare April 29, 2017 01:55

paulirish mentioned this pull request Apr 29, 2017

artficact renames #2104

Closed

paulirish added 9 commits April 28, 2017 19:16

WIP kill recordNetwork option

7b034ef

config double passname validation

bd58729

record network events via networkRecorder dispatcher

a206d3f

networkRecords computed artifact. (and some assertions that the data …

d26b466

…is good.)

basic tests.

bb36757

tests for networkrecords. in good shape.

2ab1285

remove runtime assertions, as we have found networkRecords to be equal

7d6e5c4

fix perflog importing.

0e66f17

perflog. must be an object.

06410ab

paulirish added 8 commits April 28, 2017 19:18

breaking change. performanceLog => devtoolsLogs

f853e50

move devtoolsLog assignment outside of if(recordtrace) conditional

e7d39a1

feedback.

fa43ff9

reduce -> forEach

a85c4ce

remove extra promise.resolve()

9506235

update *runner tests with removed netRecords.

60e6223

feedback

fac3c3d

rebase cleanup

aaabf3f

paulirish force-pushed the netrecordsComputed branch from 70a12bd to aaabf3f Compare April 29, 2017 02:23

This was referenced May 1, 2017

feat: add consistently interactive audit #2023

Merged

fix: only record a trace if needed by an audit #2117

Merged

brendankenny and others added 3 commits May 1, 2017 18:30

Merge branch 'master' into netrecordsComputed

8712532

fixup types

9802ad0

Merge branch 'master' into netrecordsComputed

633c84f

brendankenny mentioned this pull request May 3, 2017

always construct networkRecords from devtoolsLog #2133

Merged

always construct networkRecords from devtoolsLog (#2133)

fc03fc1

simplify devtoolsLog recording, url redirect tracking, and eliminate explicit network recording

Merge branch 'master' into netrecordsComputed

fc2b9fd

paulirish closed this May 5, 2017

paulirish deleted the netrecordsComputed branch May 23, 2017 20:21

brendankenny mentioned this pull request Jun 8, 2017

move computed artifacts dependent on networkRecords to devtoolsLog #2467

Merged

Move networkRecords to a computed artifact #2062

Move networkRecords to a computed artifact #2062

Conversation

paulirish commented Apr 25, 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

paulirish commented Apr 26, 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

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

benschwarz commented Apr 27, 2017

paulirish commented Apr 27, 2017

benschwarz commented Apr 27, 2017

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

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 Apr 29, 2017

patrickhulce commented Apr 29, 2017

patrickhulce commented Apr 29, 2017

brendankenny commented Apr 29, 2017

paulirish commented Apr 29, 2017

paulirish commented May 5, 2017

paulirish commented May 5, 2017