clients(lr): gather network timing numbers from headers #7888

exterkamp · 2019-04-01T18:57:01Z

Summary
Add network request timing data from lightrider headers like FetchedSize. Add network-request tests.

…lr-timings

lighthouse-core/lib/network-request.js

patrickhulce

exciting stuff! :D

lighthouse-core/lib/network-request.js

patrickhulce · 2019-04-01T21:18:28Z

lighthouse-core/lib/network-request.js

+
+    this.timing.connectStart = 0;
+    this.timing.connectEnd = TCPTime;
+    this.timing.sslStart = TCPTime - SSLTime;


should we check these assumptions too, not just total time?

Hm, would we check that SSLTime !> TCPTime, thus making SSLStart negative? We can.

lighthouse-core/lib/network-request.js

Co-Authored-By: exterkamp <shaneexterkamp5@gmail.com>

googlebot · 2019-04-01T22:26:09Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

brendankenny

one issue is that you won't have any particular debugging/logging signal if any of those bail points are hit, just that the lrStatistics in the audit won't be set, right?

lighthouse-core/lib/network-request.js

brendankenny · 2019-04-02T00:12:44Z

lighthouse-core/lib/network-request.js

@@ -223,6 +242,7 @@ module.exports = class NetworkRequest {

    this._updateResponseReceivedTimeIfNecessary();
    this._updateTransferSizeForLightrider();
+    this._updateTimingsForLightrider();


can we combine these two into one call (even if it then calls the two functions) so the control flow is clear. this._fixLightriderWithHeaders or whatever

what's the benefit of combining them here?

I actually like that it's explicitly calling out what will be touched and for what reason

what's the benefit of combining them here?

they're always called together, so making them rooted in a single call means it's not possible to separate them accidentally, and to keep this isolated, a clear branching point where some of the numbers from earlier are replaced by values from the headers.

I think it's totally fine to keep them as separate functions (called from that single function) with descriptive names.

I was also thinking we could add new method called _finalizeRequestDetails (or _finalizeRequestDetailsOnLoadingComplete) which onLoadingFailed and onLoadingFinished both call.

But i agree with keeping these two LR methods separate.

I was also thinking we could add new method called _finalizeRequestDetails

heaven forfend that we keep platform-specific workarounds grouped by platform, but this would at least be an improvement

lighthouse-core/lib/network-request.js

Lets get more authors on this to confuse Googlebot. Co-Authored-By: exterkamp <shaneexterkamp5@gmail.com>

… out breaking changes. Skipped tests.

…lr-timings

lighthouse-core/audits/network-requests.js

lighthouse-core/lib/network-request.js

brendankenny · 2019-04-04T23:02:50Z

lighthouse-core/lib/network-request.js

@@ -223,6 +242,7 @@ module.exports = class NetworkRequest {

    this._updateResponseReceivedTimeIfNecessary();
    this._updateTransferSizeForLightrider();
+    this._updateTimingsForLightrider();


I was also thinking we could add new method called _finalizeRequestDetails

heaven forfend that we keep platform-specific workarounds grouped by platform, but this would at least be an improvement

lighthouse-core/audits/network-requests.js

lighthouse-core/lib/network-request.js

lighthouse-core/test/lib/network-request-test.js

brendankenny · 2019-04-04T23:20:32Z

lighthouse-core/test/lib/network-request-test.js

+      global.isLightrider = false;
+
+      assert.equal(req.transferSize, 100);
+      req._updateTransferSizeForLightrider();


ideally we shouldn't be testing the internal implementation.

How do you feel about using network-records-to-devtools-log.js? It already supports headers, so should be pretty easy to change this code over to using that. It would create the devtools log from a minimal example network record with X-* response headers, run the log through NetworkRecorder.recordsFromLogs, and then assert on the record that comes out.

the change really should be pretty trivial. I can add it if you want, so we can get this in faster

Yeah can you get that. I'm not 100% on what it entails.

lighthouse-core/lib/network-request.js

lighthouse-core/audits/network-requests.js

lighthouse-core/lib/network-request.js

lighthouse-core/test/lib/network-request-test.js

patrickhulce

I think maybe the meat got removed because of the breaking changes comment? even without it, I think there are a few things to fix before landing, so IMO 4.3.0 should proceed without it

lighthouse-core/lib/network-request.js

lighthouse-core/test/lib/network-request-test.js

brendankenny · 2019-04-08T18:43:50Z

lighthouse-core/test/lib/network-request-test.js

+      assert.equal(req.responseReceivedTime, 1);
+    });
+
+    it('does nothing if timing is not initialized', function() {


seems like this test doesn't do anything since the headers aren't set?

Right, it is confirming that the header timing isn't changed if no header is set. Like if we made a change that always messed this up and set the value no matter what the header said it would break this b/c we shouldn't be messing with the timing w/o a header.

lighthouse-core/lib/network-request.js

lighthouse-core/test/lib/network-request-test.js

brendankenny · 2019-04-08T18:52:03Z

lighthouse-core/test/lib/network-request-test.js

+      global.isLightrider = false;
+
+      assert.equal(req.transferSize, 100);
+      req._updateTransferSizeForLightrider();


the change really should be pretty trivial. I can add it if you want, so we can get this in faster

paulirish

my concerns are resolved

Co-Authored-By: exterkamp <shaneexterkamp5@gmail.com>

patrickhulce

my concerns are resolved too, excited for the followup that gives this teeth :)

* generate devtoolsLogs for tests * feedback

brendankenny

⏱ 📈 📜

exterkamp added 8 commits March 26, 2019 11:15

Add translations.

7a0362b

Add LR timings processing.

6e7ff4e

Paul fixed my picture.

e57809b

Added tests for network-request

3851776

Merge branch 'master' into lr-timings

4d95018

remove unfinished test

ca47e29

Add lrStatistics.

b9b0cad

Merge branch 'lr-timings' of github.com:GoogleChrome/lighthouse into …

9716274

…lr-timings

exterkamp requested review from paulirish, brendankenny, patrickhulce and connorjclark April 1, 2019 18:57

paulirish requested changes Apr 1, 2019

View reviewed changes

patrickhulce reviewed Apr 1, 2019

View reviewed changes

Double float parsing

54043fe

Co-Authored-By: exterkamp <shaneexterkamp5@gmail.com>

Foodback.

e5e7e6d

brendankenny reviewed Apr 2, 2019

View reviewed changes

brendankenny and others added 3 commits April 4, 2019 10:03

Update lighthouse-core/lib/network-request.js

04434ef

Lets get more authors on this to confuse Googlebot. Co-Authored-By: exterkamp <shaneexterkamp5@gmail.com>

Brendan foodback. Added comments. Changed X-Headers to *Ms. Commented…

9cdf48c

… out breaking changes. Skipped tests.

Merge branch 'lr-timings' of github.com:GoogleChrome/lighthouse into …

47f0038

…lr-timings

exterkamp changed the title ~~[WIP] core: add network request advanced timings~~ core: gather advanced LR timing statistics Apr 4, 2019

Remove unused SSLMs & Header.

2456b72

exterkamp commented Apr 4, 2019

View reviewed changes

lighthouse-core/audits/network-requests.js Outdated Show resolved Hide resolved

connorjclark reviewed Apr 4, 2019

View reviewed changes

lighthouse-core/lib/network-request.js Outdated Show resolved Hide resolved

lighthouse-core/lib/network-request.js Outdated Show resolved Hide resolved

lighthouse-core/lib/network-request.js Outdated Show resolved Hide resolved

paulirish mentioned this pull request Apr 4, 2019

☂️ 👣 Breaking changes for v5 #7752

Closed

8 tasks

exterkamp added 2 commits April 4, 2019 15:27

Connor foodback. Better comments throughout.

62c278f

Replaced LrStatistics with top level vars. Ts is happy now.

7087094

exterkamp added the waiting4reviewer label Apr 4, 2019

brendankenny reviewed Apr 4, 2019

View reviewed changes

lighthouse-core/lib/network-request.js Outdated Show resolved Hide resolved

lighthouse-core/audits/network-requests.js Outdated Show resolved Hide resolved

brendankenny added 4.3 waiting4committer and removed waiting4reviewer labels Apr 5, 2019

exterkamp added 2 commits April 5, 2019 16:56

revert 1/2 of flattening.

0ee5681

prefixes for timing data.

66099ce

exterkamp added waiting4reviewer and removed waiting4committer labels Apr 6, 2019

paulirish reviewed Apr 6, 2019

View reviewed changes

lighthouse-core/lib/network-request.js Outdated Show resolved Hide resolved

lighthouse-core/lib/network-request.js Outdated Show resolved Hide resolved

lighthouse-core/test/lib/network-request-test.js Outdated Show resolved Hide resolved

Removed non functioning code, added SSL & TCP bail condition

d4bd2c2

exterkamp added waiting4committer waiting4reviewer and removed waiting4reviewer waiting4committer labels Apr 6, 2019

connorjclark approved these changes Apr 8, 2019

View reviewed changes

patrickhulce suggested changes Apr 8, 2019

View reviewed changes

Some testing changes.

6886c75

brendankenny reviewed Apr 8, 2019

View reviewed changes

paulirish approved these changes Apr 8, 2019

View reviewed changes

Update lighthouse-core/test/lib/network-request-test.js

77f870a

Co-Authored-By: exterkamp <shaneexterkamp5@gmail.com>

patrickhulce approved these changes Apr 8, 2019

View reviewed changes

testing, NaN checking.

3992bb0

brendankenny mentioned this pull request Apr 8, 2019

tests: generate devtoolsLogs for network-request-test.js tests #8074

Merged

tests: generate devtoolsLogs for network-request-test.js tests (#8074)

b8d3ea7

* generate devtoolsLogs for tests * feedback

brendankenny approved these changes Apr 8, 2019

View reviewed changes

brendankenny merged commit aca78af into master Apr 8, 2019

brendankenny deleted the lr-timings branch April 8, 2019 23:08

brendankenny changed the title ~~core: gather advanced LR timing statistics~~ clients(lr): gather network timing numbers from headers Apr 8, 2019

exterkamp mentioned this pull request Apr 9, 2019

clients(lr): apply Lightrider timings to NetworkRequests #8109

Merged

connorjclark mentioned this pull request May 18, 2023

clients(lr): set network timings from headers #15091

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clients(lr): gather network timing numbers from headers #7888

clients(lr): gather network timing numbers from headers #7888

exterkamp commented Apr 1, 2019

patrickhulce left a comment

patrickhulce Apr 1, 2019

exterkamp Apr 1, 2019

googlebot commented Apr 1, 2019

brendankenny left a comment

brendankenny Apr 2, 2019

patrickhulce Apr 2, 2019

brendankenny Apr 2, 2019

paulirish Apr 2, 2019

brendankenny Apr 4, 2019

brendankenny Apr 4, 2019

brendankenny Apr 4, 2019

brendankenny Apr 8, 2019

exterkamp Apr 8, 2019

patrickhulce left a comment

brendankenny Apr 8, 2019

exterkamp Apr 8, 2019

brendankenny Apr 8, 2019

paulirish left a comment

patrickhulce left a comment

brendankenny left a comment

clients(lr): gather network timing numbers from headers #7888

clients(lr): gather network timing numbers from headers #7888

Conversation

exterkamp commented Apr 1, 2019

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

googlebot commented Apr 1, 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

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

paulirish left a comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment