Remove recordNetwork #2102

paulirish · 2017-04-29T01:06:29Z

Now, we always actually record it. (And it's separated from recordTrace)

This flag was differentiating on if we expose it to gatherers and artifacts objects.

brendankenny

https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/config/plots.json also has a stray recordNetwork

brendankenny · 2017-04-29T01:14:10Z

lighthouse-core/config/config.js

    const passName = pass.passName || Audit.DEFAULT_PASS;
    if (usedNames.has(passName)) {
-      log.warn('config', `passes[${index}] may overwrite trace or network ` +
-          `data of earlier pass without a unique passName (repeated name: ${passName}.`);
+      log.warn('config', `passes[${index}] may overwrite trace ` +


may overwrite data since it still could overwrite network records?

We should probably throw here instead of warning as there's no "may" about it anymore for network records. Can just be Passes must have unique names (repeated name: ${passName}). or something?

brendankenny · 2017-04-29T01:14:21Z

lighthouse-core/config/config.js

@@ -129,17 +129,13 @@ function validatePasses(passes, audits, rootPath) {
    });
  });

-  // Log if multiple passes require trace or network recording and could overwrite one another.
+  // Passes should have unique passName's. Warn otherwise.


drop apostrophe

I guess that's hard to differentiate in a comment :)
// Passes should have unique `passName`s. Warn otherwise.?
// Passes should all have a unique passName. Warn otherwise.?

brendankenny · 2017-04-29T01:18:30Z

lighthouse-core/gather/gather-runner.js

-
-      // Network records only given to gatherers if requested by config.
-      config.recordNetwork && (passData.networkRecords = networkRecords);
+      // expose devtoolsLog & networkRecords to gatherers


there's also a // Network is always recorded for internal use, even if not saved as artifact. comment that needs updating

brendankenny · 2017-04-29T01:20:22Z

lighthouse-core/gather/gather-runner.js

-      // Network records only given to gatherers if requested by config.
-      config.recordNetwork && (passData.networkRecords = networkRecords);
+      // expose devtoolsLog & networkRecords to gatherers
+      passData.devtoolsLog = driver.devtoolsLog;


this is already handled above in config.recordTrace block

good catch. devtoolsLog should never be within a recordTrace conditional.

brendankenny · 2017-04-29T01:22:13Z

lighthouse-core/gather/gather-runner.js

-              config.recordNetwork &&
-                  (tracingData.networkRecords[passName] = passData.networkRecords);
+
+              tracingData.devtoolsLogs[passName] = passData.devtoolsLog;


this has the same problem as #2062 in that there's no passData.devtoolLog if there was no trace. Leave inside the conditional until that's fixed?

fixing that in this PR now. :)

brendankenny · 2017-04-29T01:25:56Z

lighthouse-core/gather/gather-runner.js

-                  (tracingData.networkRecords[passName] = passData.networkRecords);
+
+              tracingData.devtoolsLogs[passName] = passData.devtoolsLog;
+              tracingData.networkRecords[passName] = passData.networkRecords;


// If requested by config comment above needs update

brendankenny · 2017-04-29T01:32:53Z

test errors are from not all passes having names in smokehouse's pwa-config.js so network records are overwriting each other. So, probably 👍 to throw in config.js to catch that early

paulirish · 2017-04-29T03:17:07Z

PTAL

patrickhulce · 2017-04-29T05:54:26Z

lighthouse-core/gather/gather-runner.js

      .then(_ => driver.beginNetworkCollect(options))
+      .then(_ => driver.beginDevtoolsLog())


nit: should we do this the other way around? feasibly there could be some network request recorded before the devtools log start I guess, idk

patrickhulce · 2017-04-29T05:54:37Z

love it

paulirish · 2017-05-01T20:33:04Z

@brendankenny this work for you?

brendankenny

I have a proposal for simplifying things a bit more. I'll make a PR on top of this :)

brendankenny · 2017-05-01T20:37:23Z

lighthouse-core/config/config.js

    const passName = pass.passName || Audit.DEFAULT_PASS;
    if (usedNames.has(passName)) {
-      log.warn('config', `passes[${index}] may overwrite trace or network ` +
-          `data of earlier pass without a unique passName (repeated name: ${passName}.`);
+      throw new Error(`Passes must have unique names (repeated passName: ${passName}.`);


hmm, this will still be confusing when a config doesn't include any pass names ("(repeated passName: defaultPass."). I don't know if there's a super clean solution (like the use of a Set here) if we want to still support one unnamed pass defaulting to defaultPass.

We could drop that, though, since it's a little confusing with the new world of always recording devtoolsLog, and this check could have two checked errors: no pass name and repeated pass names.

brendankenny · 2017-05-01T20:42:10Z

lighthouse-core/gather/driver.js

+  }
+
+  endDevtoolsLog() {
+    this._devtoolsLog.endRecording();


should this return the log like endTrace and endNetworkCollect instead of having GatherRunner call the getter directly?

And actually, should it return a promise? Kind of silly to wrap in yet another promise (less silly when we get async/await tho), but I was noticing the other day that endNetworkCollect is also wrapped in a promise despite being sync...presumably to keep the API easy to reason about.

Just bringing up the idea, not sure how I feel about that :)

brendankenny · 2017-05-01T20:48:17Z

lighthouse-core/gather/gather-runner.js

      .then(_ => driver.beginNetworkCollect(options))
+      .then(_ => driver.beginDevtoolsLog())


need to update outline at top of file

brendankenny · 2017-05-01T20:52:29Z

lighthouse-core/gather/gather-runner.js

@@ -238,20 +239,20 @@ class GatherRunner {
        // an object with a traceEvents property. Normalize to object form.
        passData.trace = Array.isArray(traceContents) ?
            {traceEvents: traceContents} : traceContents;
-        passData.devtoolsLog = driver.devtoolsLog;
        log.verbose('statusEnd', 'Retrieving trace');
      });
    }

    const status = 'Retrieving network records';


update status message?

brendankenny · 2017-05-01T21:26:56Z

I'll make a PR on top of this :)

actually, no I won't :) I really need #2062 to do it cleanly.

Speaking of which, what do you think about reverting the last commit? It should really be in #2062 anyways...this PR can get by pretty easily without touching devtoolsLog and #2062 is already touching it, so it makes a lot more sense over there

brendankenny · 2017-05-01T21:28:07Z

(or just merge them together or whatever if this is getting crazy since they're both mostly reviewed)

This reverts commit 77d1b7e.

brendankenny · 2017-05-01T21:51:22Z

OK, removed devtoolsLog stuff and added a special warning for multiple unnamed passes (so you wouldn't get a warning about repeated passName 'defaultPass' even though you never named any of your passes 'defaultPass). I didn't remove defaultPass though because that's a lot more work in a lot more places and not worth it right now :)

paulirish · 2017-05-01T21:55:10Z

yup. this WFM.

brendankenny

waiting for travis...

* always record network and save in artifacts

paulirish added 2 commits April 28, 2017 17:58

WIP kill recordNetwork option

3339e1e

config double passname validation

f0481df

paulirish force-pushed the killrecordnetwork-master branch from 18902c6 to f0481df Compare April 29, 2017 01:07

brendankenny requested changes Apr 29, 2017

View reviewed changes

feedback

616a050

paulirish force-pushed the killrecordnetwork-master branch from bfc1e62 to 616a050 Compare April 29, 2017 03:02

move devtoolsLog recording outside of trace.

77d1b7e

patrickhulce reviewed Apr 29, 2017

View reviewed changes

brendankenny requested changes May 1, 2017

View reviewed changes

brendankenny added 3 commits May 1, 2017 14:43

Revert "move devtoolsLog recording outside of trace."

273b38a

This reverts commit 77d1b7e.

add warning specifically for multiple unnamed passes in config

2e6328c

remove devtoolsLog changes

f5bb576

brendankenny approved these changes May 1, 2017

View reviewed changes

brendankenny merged commit 07e0aab into master May 1, 2017

brendankenny deleted the killrecordnetwork-master branch May 1, 2017 22:29

paulirish added a commit to paulirish/lighthouse that referenced this pull request May 5, 2017

Remove recordNetwork (GoogleChrome#2102)

e90601a

* always record network and save in artifacts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove recordNetwork #2102

Remove recordNetwork #2102

paulirish commented Apr 29, 2017 •

edited

Loading

brendankenny left a comment

brendankenny Apr 29, 2017 •

edited

Loading

brendankenny Apr 29, 2017

brendankenny Apr 29, 2017

brendankenny Apr 29, 2017

brendankenny Apr 29, 2017

brendankenny Apr 29, 2017

paulirish Apr 29, 2017

brendankenny Apr 29, 2017

paulirish Apr 29, 2017

brendankenny Apr 29, 2017

brendankenny commented Apr 29, 2017

paulirish commented Apr 29, 2017

patrickhulce Apr 29, 2017

patrickhulce commented Apr 29, 2017

paulirish commented May 1, 2017

brendankenny left a comment

brendankenny May 1, 2017

brendankenny May 1, 2017

brendankenny May 1, 2017

brendankenny May 1, 2017

brendankenny May 1, 2017

brendankenny commented May 1, 2017

brendankenny commented May 1, 2017

brendankenny commented May 1, 2017

paulirish commented May 1, 2017

brendankenny left a comment

		.then(_ => driver.beginNetworkCollect(options))
		.then(_ => driver.beginDevtoolsLog())

Remove recordNetwork #2102

Remove recordNetwork #2102

Conversation

paulirish commented Apr 29, 2017 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

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

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

paulirish commented Apr 29, 2017

Choose a reason for hiding this comment

patrickhulce commented Apr 29, 2017

paulirish commented May 1, 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

brendankenny commented May 1, 2017

brendankenny commented May 1, 2017

brendankenny commented May 1, 2017

paulirish commented May 1, 2017

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Apr 29, 2017 •

edited

Loading

brendankenny Apr 29, 2017 •

edited

Loading