core(lantern): add network timings to debug traces #14571
Conversation
| // TODO: timing information currently difficult to model for warm h2 connections. | ||
| connectionTiming = { | ||
| timeToFirstByte, | ||
| }; |
There was a problem hiding this comment.
the way warm h2 connections are simulated means there's not really anything meaningful going on here (timeToFirstByte will actually always be 0), but that's something I'd like to iterate on
| * @param {CompleteNodeTiming} timing | ||
| * @return {LH.TraceEvent} | ||
| */ | ||
| function createWillSendRequestEvent(requestId, record, timing) { |
There was a problem hiding this comment.
adding a ResourceWillSendRequest trace event for the navigation for completeness, though the perf panel doesn't seem to depend on it
There was a problem hiding this comment.
Heh yeah found the same thing (in rum-trace)
| proxyEnd: -1, | ||
| pushStart: 0, | ||
| pushEnd: 0, | ||
| }, |
There was a problem hiding this comment.
@paulirish you might have lots of thoughts. Only so much we can do with the timings we have, so it's a bit handwavey (especially sendStart/sendEnd/receiveHeadersEnd), but maybe you have ideas
| * @param {CompleteNodeTiming} timing | ||
| * @return {LH.TraceEvent} | ||
| */ | ||
| function createWillSendRequestEvent(requestId, record, timing) { |
There was a problem hiding this comment.
Heh yeah found the same thing (in rum-trace)
core/lib/lantern-trace-saver.js
Outdated
| @@ -31,7 +31,11 @@ function convertNodeTimingsToTrace(nodeTimings) { | |||
| } else { | |||
| // Ignore data URIs as they don't really add much value | |||
| if (/^data/.test(node.record.url)) continue; | |||
| traceEvents.push(...createFakeNetworkEvents(node.record, timing)); | |||
| if (requestId === 1) { | |||
| traceEvents.push(createWillSendRequestEvent(requestId, node.record, timing)); | |||
There was a problem hiding this comment.
you could do this conditional within createFakeNetworkEvents instead.
IMO that'd be a bit cleaner :)
There was a problem hiding this comment.
you could do this conditional within
createFakeNetworkEventsinstead.
yeah, I was trying to keep createFakeNetworkEvents ignorant of the requestId, but it's already pretty hacky, so that's fine. I suspect this needs to be reworked for redirects and timespans anyway.
| @@ -481,8 +490,9 @@ class Simulator { | |||
| } | |||
| } | |||
|
|
|||
| const nodeTimings = this._computeFinalNodeTimings(); | |||
| ALL_SIMULATION_NODE_TIMINGS.set(options.label || 'unlabeled', nodeTimings); | |||
| // `nodeTimings` are used for simulator consumers, `completeNodeTimings` kept for debugging. | |||
| const receiveResponseData = { | ||
| ...requestData, | ||
| statusCode: record.statusCode, | ||
| mimeType: record.mimeType, | ||
| encodedDataLength: record.transferSize, | ||
| fromCache: record.fromDiskCache, | ||
| fromServiceWorker: record.fetchedViaServiceWorker, | ||
| timing: { | ||
| requestTime: toMicroseconds(startTime) / (1000 * 1000), |
There was a problem hiding this comment.
Might wanna leave a comment that requestTime is seconds whereas the rest of the timings are ms.
| proxyEnd: -1, | ||
| pushStart: 0, | ||
| pushEnd: 0, | ||
| }, |
Adds network request timing breakdowns to lantern debug traces. When debugging simulator results (e.g. for debugging #14166), it can be really helpful to see time for DNS, connection, to get the first byte back, etc.
Because the changes are kind of thread through four different files, it might be helpful to look at individual commits (some general trace fixes, getting full node timings to the trace saver, and adding new timings to network nodes).
Changes have no effect on simulator functionality or results. Timings available to audits are unchanged, it's only the trace generator that has access to the full node timings.
Example
Trace from main:
Trace from this branch:
For reference, the "real" (devtools-throttled) trace: