always construct networkRecords from devtoolsLog #2133

brendankenny · 2017-05-03T05:25:09Z

PR building on (and to merge into) #2062

Hopefully simplifies network and devtoolsLog recording. In total (with existing #2062 changes):

driver.networkRecorder was pulling triple duty, serving as network monitor, redirect monitor, and recorder of network activity for gatherers. Its replacement driver._networkStatus is only used for network events and is entirely internal to driver
devtoolsLog is taken for every pass
passData.networkRecords (passed into Gatherer.afterPass()) is constructed from the devtoolsLog for that pass. This eliminates questions of if network or devtoolsLog should start recording first and in what order, and it means that the networkRecords in afterPass() is the exact same as what audits get from the computed artifact. Downside is that networkRecords is reconstructed for every pass + possibly for computed artifacts, but this adds maybe 30-40ms total to a theverge.com run, so not a big deal.
enableUrlUpdateIfRedirected becomes a purely internal driver affair, as it no longer updates options.url directly, which lets driver worry about redirects and GatherRunner worry about options. Instead, driver.goToUrl() resolves on the final loaded (possibly redirected) URL. options.url is then set in GatherRunner.loadPage so that everything outside of that doesn't need to know anything changed.
beginTrace and beginDevtoolsLog are moved out of GatherRunner.loadPage to GatherRunner.pass as that makes a lot more sense and it matches the afterPass() location of endTrace and endDevtoolsLog (previously endNetworkCollect)

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

patrickhulce

I really like these changes, moving towards a cleaner 🚗 + 🏃

patrickhulce · 2017-05-03T16:37:27Z

lighthouse-core/gather/driver.js

+     * Used for monitoring network status events.
+     * @private {?NetworkRecorder}
+     */
+    this._networkStatus = null;


I'm not in love with this name change, network status makes me thing it's only watching for offline/online or something yet we listen for individual requests, dispatch all network messages to it, etc. Thoughts?

yeah, I just wanted to move away from the "records" part since we aren't using it for records. We're using it for busy, idle, and requestloaded events. Not sure what to call that :)

how about _networkStatusRecorder or _networkStatusMonitor?, just networkStatus makes me think it's literally a string 'offline' or something

ah, I see what you're saying. _networkStatusMonitor sounds great. @paulirish? (I can do the same thing for the begin/end methods)

patrickhulce · 2017-05-03T16:39:31Z

lighthouse-core/gather/driver.js

+      this._networkStatus = new NetworkRecorder([], this);
+      this.enableUrlUpdateIfRedirected(options);
+
+      this.sendCommand('Network.enable').then(resolve, reject);


can't we just return the result of this.sendCommand instead of wrapping in another promise?

oh it was pre-existing, might as well fix though :)

can't we just return the result of this.sendCommand instead of wrapping in another promise?

done

patrickhulce · 2017-05-03T16:45:17Z

lighthouse-core/gather/driver.js

@@ -506,10 +543,17 @@ class Driver {
    const maxWaitMs = (options.flags && options.flags.maxWaitForLoad) ||
        Driver.MAX_WAIT_FOR_FULLY_LOADED;

-    return this.sendCommand('Page.enable')
+    const redirectTracking = {url};


it's hard to tell what this is for and why it's passed into _beginNetworkMontoring's options argument, maybe redirectTrackingOptions/Container/Something at least?

patrickhulce · 2017-05-03T16:55:18Z

lighthouse-core/gather/driver.js

   */
-  enableUrlUpdateIfRedirected(opts) {
-    this._networkRecorder.on('requestloaded', redirectRequest => {
+  enableUrlUpdateIfRedirected(options) {


let's rename away from options now that it's not mutating the actual options object

patrickhulce · 2017-05-03T16:55:24Z

lighthouse-core/gather/driver.js

+   * @return {!Promise}
+   * @private
+   */
+  _beginNetworkMonitoring(options) {


patrickhulce · 2017-05-03T16:57:00Z

lighthouse-core/gather/driver.js

+      .then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs, maxWaitMs))
+      .then(_ => {
+        this._endNetworkMonitoring();
+        return redirectTracking.url;


since we're cleaning this up to not touch options it feels weird to still do this object mutation, what if it were restructured slightly so that we called _monitorUrlChanges() and could call getLatestUrl() at any point

went more or less with this, but only internal to driver. Didn't do a getLatestUrl() as driver is only semi-stateful, so it's not always clear that's what "latest" would be referring to, but got rid of all the options.url business. gotoURL still resolves on the final URL since during that method is the only time redirects are being tracked.

paulirish

on the whole i like it. :)

paulirish · 2017-05-03T20:20:45Z

lighthouse-core/gather/driver.js

+    if (!this._networkStatus) return;
+
+    const regexFilter = /^Network\./;
+    if (!regexFilter.test(method)) return;


while you're here can you change this to be free of regex:

if (!method.startsWith('Network.')) return;

my fault. thanks.

I just moved it into NetworkRecorder.dispatch (to mirror devtools-log.record), though we could arguably drop it altogether because the switch statement rejects anything not recognized

paulirish · 2017-05-03T21:52:14Z

lighthouse-core/gather/driver.js

+   * @return {!Promise}
+   * @private
+   */
+  _beginNetworkMonitoring(options) {


rename to beginNetworkIdleMonitoring since its for busy idle detection. not to be confused with devtoolsLog like stuff?

rename to beginNetworkIdleMonitoring since its for busy idle detection. not to be confused with devtoolsLog like stuff?

we also watch for requestloaded to watch for url redirect. Not sure if that matters :)

brendankenny · 2017-05-04T03:11:10Z

fixed up and added a test that replays a wikipedia load to test redirect tracking. PTAL

patrickhulce

lgtm % nit

patrickhulce · 2017-05-04T03:32:35Z

lighthouse-core/gather/driver.js

+
+    // Update startingUrl if it's ever redirected.
+    this._monitoredUrl = startingUrl;
+    this._networkStatusMonitor.on('requestloaded', redirectRequest => {


nice this is so much more understandable 👍

patrickhulce · 2017-05-04T03:34:37Z

lighthouse-core/lib/network-recorder.js

@@ -155,6 +148,10 @@ class NetworkRecorder extends EventEmitter {
   * @param {!Object<string, *>=} params
   */
  dispatch(method, params) {
+    if (!/^Network\./.test(method)) {


nit: I believe paul wanted startsWith instead

startsWith

done

patrickhulce · 2017-05-04T03:36:00Z

lighthouse-core/lib/network-recorder.js

+   * Construct network records from a log of devtools protocol messages. If a
+   * driver is provided, it will be used for request content lookups.
+   * @param {!DevtoolsLog} devtoolsLog
+   * @param {!Driver=} driver


unrelated, but I still vote for a driver helper function instead, this is a really annoying artifact of having it be the method on network records :(

unrelated, but I still vote for a driver helper function instead, this is a really annoying artifact of having it be the method on network records :(

the problem is we need a live driver to get resource content, which will only be accessible in gatherers, but we need to create records from a devtools log during audits too (via a computed artifact) when there is no live driver. The first one argues for an instance method on driver, the second argues for a static method. So...it could be a static driver method, but still take a driver instance? But that seems like it has the same issue.

Not sure if there's a good solution :S

IMO, audits shouldn't need to live request content if gatherers didn't save them into a separate artifact meaning it can just be a method on a driver to getResourceContent and pass in a network record id, the construction of network records is not capable of reconstructing the content anyway

IMO, audits shouldn't need to live request content if gatherers didn't save them into a separate artifact

right, it's not possible for audits to do that on current master already, as we throw away the driver by that point

meaning it can just be a method on a driver to getResourceContent and pass in a network record id, the construction of network records is not capable of reconstructing the content anyway

well I wish I had thought of that before I spent all that time reverse engineering that 'web-inspector' madness and then making it work again with this PR :)

I think Paul was going for the current approach because it makes accessing the content pretty seamless. We already use devtools' WebInspector.NetworkRequest objects, and they have requestContent on them, so it makes some sense to hook that method up similarly to how devtools does and have it send the command on your behalf.

It seems like we have a "this thing works here but doesn't work there" in both cases, so the choice would be:

(current behavior) Support record.requestContent(). Meanwhile in audits throw on record.requestContent() (called when a driver wasn't used to initialize the network records) and document that it only works in audits, not gatherers

If an audit wants to access content, they have to do driver.getRequestContent(record) or whatever. Meanwhile always throw on record.requestContent() and document driver method if you want content.

I do kind of like the explicitness of the second approach, making it crystal clear that you'll only be able to access content through the driver (so if you don't have a driver anymore, you won't get content). OTOH we're breaking the API of NetworkRequest and breaking away from the way devtools code does things. OTOOH probably no one would have known that record.requestContent existed without paul anyways and would have happily used a driver method (analogous to getObjectProperty).

@paulirish wdyt?

also, it appears that all the other request*() methods on WebInspector.NetworkRequest don't send additional protocol commands, they already have what they need locally

oh let's pick 2, pick 2, pick 2, 😄 , can we @paulirish please? it's a bit silly to try to conform to the interface only here, when it makes the rest of the code more complicated especially when the assumptions of devtools are just very different than the assumptions in LH

ya cool let's pick 2.

make record.requestContent() throw and go back through protocol manually if you need responseBodies.

works for me.
do we need to change all that in this PR? would be nice not to..

brendankenny · 2017-05-05T04:12:08Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

@@ -47,7 +47,7 @@ class ResponseCompression extends Gatherer {

      if (!isContentEncoded) {
        unoptimizedResponses.push({
-          record: record,
+          requestId: record.requestId,


removed the full request record from the extendedInfo as it wasn't used and I assumed it would have problems with serialization of the report JSON? Or was that just the full set of network records?

Regardless, it wasn't used except to get requestId, so seemed fine to remove :)

ya good call. we shouldn't put records in extendedInfo

paulirish

lets do it

paulirish · 2017-05-05T04:28:28Z

lighthouse-core/gather/driver.js

+  getRequestContent(requestId) {
+    return this.sendCommand('Network.getResponseBody', {
+      requestId,
+    }).then(result => result.body);


add a comment in here that we're ignoring base64Encoded ?

// ignoring the result.base64Encoded boolean, which indicates if body is already encoded

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

always construct networkRecords from devtoolsLog

43d4da6

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

brendankenny force-pushed the alldevtoolslogallthetime branch from 8913cb4 to 43d4da6 Compare May 3, 2017 05:30

brendankenny requested a review from paulirish May 3, 2017 05:39

patrickhulce suggested changes May 3, 2017

View reviewed changes

paulirish reviewed May 3, 2017

View reviewed changes

feedback

62949d4

brendankenny force-pushed the alldevtoolslogallthetime branch from 7ed58df to 62949d4 Compare May 4, 2017 03:07

patrickhulce approved these changes May 4, 2017

View reviewed changes

startsWith

253e41d

brendankenny assigned paulirish May 4, 2017

get network record content directly from driver

b9a62b7

brendankenny commented May 5, 2017

View reviewed changes

paulirish approved these changes May 5, 2017

View reviewed changes

clarification

faafddf

brendankenny merged commit fc03fc1 into netrecordsComputed May 5, 2017

brendankenny deleted the alldevtoolslogallthetime branch May 5, 2017 04:35

paulirish mentioned this pull request May 5, 2017

Move networkRecords to a computed artifact #2062

Closed

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

always construct networkRecords from devtoolsLog (GoogleChrome#2133)

fa8eb00

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

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

always construct networkRecords from devtoolsLog (GoogleChrome#2133)

d7e4d1b

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

brendankenny mentioned this pull request Jun 8, 2017

move computed artifacts dependent on networkRecords to devtoolsLog #2467

Merged

brendankenny mentioned this pull request Sep 22, 2018

core: remove some trivial uses of WebInspector #6090

Merged

always construct networkRecords from devtoolsLog #2133

always construct networkRecords from devtoolsLog #2133

Conversation

brendankenny commented May 3, 2017 • edited Loading

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

patrickhulce May 4, 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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented May 4, 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

brendankenny May 4, 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

brendankenny May 5, 2017 • edited Loading

Choose a reason for hiding this comment

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

brendankenny commented May 3, 2017 •

edited

Loading

patrickhulce May 4, 2017 •

edited

Loading

brendankenny May 4, 2017 •

edited

Loading

brendankenny May 5, 2017 •

edited

Loading