-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: refactor NetworkRequest time properties #14311
Conversation
const artifacts = mockArtifacts(PASSING_CHAIN_RECORDS_2); | ||
const artifacts = mockArtifacts(PASSING_CHAIN_RECORDS_2, { | ||
// Precision issues from converting between ms and s create slight differences during roundtrip. | ||
skipVerification: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably put an epsilon on those comparisons, but for a quick fix, could also just pick a different timestamp :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to changing the timestamp. We don't want to skip verification if possible, especially on an already neglected audit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wait for any changes the tests need to settle before digging in too closely, but overall looks great and I love the additional precision we'll have to describe network requests.
- Can we pick a different name than
internalNetworkRequestTime
? It's a key timestamp for request timing, so it's weird to putinternal
on the front of that. Makes it seem like you shouldn't be looking at it. - Relatedly: for the
network-requests
hidden audit, that time is really important for post-analysis. e.g. a low-priority image could have a very quickmainThreadStartTime
but a much laterinternalNetworkRequestTime
due to being queued after a bunch of other requests. I guess we just need to include both in the output?
Also, doesn't it seem suspicious that yarn test-lantern
is all 0% changes? If the network timings have all changed I would expect at least some movement.
Tests should be good now.
I forgot to mention - this PR should have zero impact on lantern. I figured it'd be good to split that out entirely. The behavior remains exactly the same because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really excited about the improved precision these extra timestamps bring. I disagree with my earlier comment that we should try to preserve the network-requests
hidden audit properties; I think the new properties and names make it so much clearer it outweighs any breakages for the few of us looking at the audit.
In retrospect it would have been easier to review if we'd broken this up into
- rename
startTime
/endTime
and addmainThreadStartTime
/mainThreadEndTime
with no other changes
and then some order of:
- switch to milliseconds in
network-request
- switch audits to using mainThreadStart/End where appropriate
- CRC changes (though those are mostly self contained)
it's hard to track all of those simultaneously and feel confident we're getting it right in every file, and our network-timing-related test coverage isn't always the best since we also need to avoid smoke flakes, but hopefully the power of combined reviewership will win out here.
timestamp: | ||
networkRecord.redirectResponseTimestamp || networkRecord.mainThreadStartTime / 1000 || 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the different timestamps seem like they should default to each other if one is set, otherwise you can get subtly broken things like networkRequestTime
of 0, before a set mainThreadStartTime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not following this / sure how to address this. Where would the values be set in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically if either rendererStartTime
or networkRequestTime
is set (but not both), the one not set should probably default to value of the other, not 0.
@@ -218,7 +218,7 @@ describe('network recorder', function() { | |||
|
|||
const [mainDocument, loaderPrefetch, _ /* favicon */, loaderScript, implScript] = records; | |||
expect(mainDocument.initiatorRequest).toBe(undefined); | |||
expect(loaderPrefetch.startTime < loaderScript.startTime).toBe(true); | |||
expect(loaderPrefetch.mainThreadStartTime < loaderScript.mainThreadStartTime).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a different assertion than before, though. Maybe assert both since the fetch time of prefetch is important?
core/lib/network-request.js
Outdated
*/ | ||
this.networkRequestTime = -1; | ||
/** ms. When the first byte of the response headers is received. */ | ||
this.responseHeadersReceivedTime = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshed: I think I prefer the old responseReceivedTime
name (with clarification of how it's defined in a comment).
I can see the benefits of being clearer internally, but when using an instance externally, this property name makes me have to think a lot to parse it, and it kind of implies there are other response-received times, even though there's just the one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i'd prefer headersReceivedTime
to be more terse)
do you think we'd ever want to introduce more granular network timings? if so, we should be specific here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can drop "headers" if we sneak "start" into the name somewhere.
responseStartTime
i like. i guess we could just call out the 'headers received' technicality in a comment as the distinction isn't really important.responseHeadersReceivedTime
does feel a bit wordy and overkill at this point.headersReceivedTime
well... less wordy but it gets me thinking about "but what about request headers??" so the thinking overhead is still there.responseReceivedTime
(the old one) i dont like cuz it can easily be interpreted as 'end of response received'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responseHeadersEndTime
is another option if we want to be both specific, consistent with start/end, and precise, though it's unfortunate that usually what we want is responseBodyStartTime
.
responseStartTime
does seem like it would trip up a lot on the fact that the server has already responded :/
if (!this.responseReceivedTime || this.responseReceivedTime < 0) { | ||
this.responseReceivedTime = headersReceivedTime; | ||
this.networkRequestTime = timing.requestTime * 1000; | ||
const headersReceivedTime = this.networkRequestTime + timing.receiveHeadersEnd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, is it actually when the last header byte is received?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no "start" equivalent on the timing object... It seems in practice it wouldn't make a difference, surely all sites flush out all the headers upfront
does the lack of receiveHeadersStart
imply that blink doesn't "stream" headers? if a preload header comes on the main document...then the server blocks before doing more headers then the body....would Chrome delay that preload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, is it actually when the last header byte is received?
yes
I've poked at this with the netlog, but i've never had a testcase where the response headers get split over two packets.
But.. if i remember right even the //net stack doesnt really call out headersStart as a thing.
@@ -111,10 +111,10 @@ describe('NetworkRequest', () => { | |||
describe('update fetch stats for Lightrider', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests for the new timestamps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test for _updateTimingsForLightrider
or something else? There aren't really any tests here for (old) startTime, endTime, etc. and I'm not sure what it'd be other than check _onX() multiplies timestamp by 1000
or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, doh, I guess I should just add the other fields here in getRequest()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added - but I had to make a change to getResponseReceivedEvent
to fix roundtrip record->log errors. Don't understand why creating a timings was behind a flag, but now it isn't and it fixed the roundtripping problem... seem ok?
@@ -248,7 +248,8 @@ class NetworkRecorder extends RequestEventEmitter { | |||
|
|||
let candidates = recordsByURL.get(initiatorURL) || []; | |||
// The initiator must come before the initiated request. | |||
candidates = candidates.filter(cand => cand.responseReceivedTime <= record.startTime); | |||
candidates = candidates | |||
.filter(cand => cand.responseHeadersEndTime <= record.networkRequestTime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this could be rendererStartTime instead of networkRequestTime.
as we have to go back to renderer to make new requests.
redirect would be the only case where that wouldnt happen but 1) thats cuz we artificially separate them into reqs and 2) i don't think we'll hit this situation with redirects.
const nodeB = new NetworkNode(request({startTime: 2, endTime: 3})); | ||
const nodeC = new NetworkNode(request({startTime: 2, endTime: 5})); | ||
const nodeD = new NetworkNode(request({startTime: 2, endTime: 7})); | ||
const nodeA = new NetworkNode(request({networkRequestTime: 0, networkEndTime: 1})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i noticed there's a bunch of other startTime
and endTime
's in this file.. and while some belong to simulator nodes.. i think many should get renamed too..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hold up how the hell are these other tests passing?!
like should simulate cached network graphs
, the start/end time on nodeA/B just don't seem to matter (delete them, still passes), even the timing assert still passes 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't like how simulator naming got more complicated because of network timing internals, so I kept the start/end time naming in these test files and just modified the request()
helper function to translate back to the network timings domain
We can return to this in #14500 if needed
I was thinking about the rename aspect of this PR separate from the behavior changes.. So I went through the changes and applied just the renames.... That diff: https://github.com/GoogleChrome/lighthouse/compare/renamenetwork And then diffing this PR here against my rename-only branch: https://github.com/GoogleChrome/lighthouse/compare/renamenetwork..network-request-timing-overhaul Maybe this diff is informative... I gotta run but i was also thinking.. maybe it'd be good for this branch to call things |
Broken up into multiple PRs:
Implementing refactor as described here: #14227 (comment)
new time properties / renames
rendererEndTime
is not actually something we have yet (only exists in trace), so at the moment it's always equal tonetworkEndTime
. It is included to fill out our model of the network, and in some places the former is used instead of the latter even though it doesn't make a difference yet. At the very least, it better documents intent.excerpt from network request constructor:
Tons of subtle changes to determine which time property should be used, thanks to @paulirish for going through all of them one-by-one with me.
I had to do an annoying refactor of critical request chain, in an attempt to avoid making a breaking change to its audit values. It's a weird gatherer that saves the network request on it, then walks it in the audit...but needs to walk both the "gatherer chain" and the "audit detail chain"... which now has drifted in shape. The simple fix was to copy the chain traversal code and treat the two separately.
s -> ms
This PR also changes our network record time values to be in ms instead of seconds. This should not impact any audit values -
network-requests
was already converted to ms.However
critical-request-chain
timings and startTime/endTime in lanternnode
s remain as seconds. They'll be adjusted in a followup.(Same deal in #14567)