feat(fetch): record timings #32613
feat(fetch): record timings #32613
Conversation
This comment has been minimized.
This comment has been minimized.
Skn0tt
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -132,9 +134,13 @@ export async function createConnectionAsync( | |||
| port: options.port as number, | |||
| host: address }); | |||
|
|
|||
| (socket as any).dnsLookupAt = dnsLookupAt; | |||
There was a problem hiding this comment.
I think we should make this a proper part of the API in some way.
There was a problem hiding this comment.
Agree it's a little hacky, but I can't find anything better. How would you do it?
There was a problem hiding this comment.
Create a function here called timingForSocket(socket) that can read values that were set through symbols to avoid clashing with proper fields. Basically, encapsulate the internal symbols to this file.
| const request = requestConstructor(url, requestOptions as any, async response => { | ||
| const notifyRequestFinished = (body?: Buffer) => { | ||
| const timings: har.Timings = { | ||
| send: requestFinishAt! - startAt, | ||
| wait: firstByteAt! - requestFinishAt!, |
There was a problem hiding this comment.
What if body was empty, and we did not set firstByteAt?
There was a problem hiding this comment.
Good catch. I've replaced firstByteAt with a measure of when the response headers are sent in 5d8ab8a. I think that's more in-line with the HAR definition anyways.
| wait: firstByteAt! - requestFinishAt!, | ||
| receive: endAt! - firstByteAt!, | ||
| dns: dnsLookupAt ? dnsLookupAt - startAt : -1, | ||
| connect: (tlsHandshakeAt ?? tcpConnectionAt!) - startAt, |
There was a problem hiding this comment.
Why not just tcpConnectionAt - startAt? This warrants a comment at least.
There was a problem hiding this comment.
fair! added comments in 9479423
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -132,9 +134,13 @@ export async function createConnectionAsync( | |||
| port: options.port as number, | |||
| host: address }); | |||
|
|
|||
| (socket as any).dnsLookupAt = dnsLookupAt; | |||
There was a problem hiding this comment.
Create a function here called timingForSocket(socket) that can read values that were set through symbols to avoid clashing with proper fields. Basically, encapsulate the internal symbols to this file.
Co-authored-by: Dmitry Gozman <dgozman@gmail.com> Signed-off-by: Simon Knott <info@simonknott.de>
This comment has been minimized.
This comment has been minimized.
|
Should we be respecting |
This comment has been minimized.
This comment has been minimized.
Nice catch! I think we should respect all the options. |
|
I've implemented |
Test results for "tests 1"1 failed 1 flaky35483 passed, 659 skipped Merge workflow run. |
Related to #19621
Adds some instrumentation to collect timings for
APIRequestContextrequests and adds them to the HAR trace. Doesn't yet expose them via an API, but makes ourDurationfield in the trace viewer show a nice duration:I'm gonna add it to our API in a separate PR.