Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 6.4k
feat(fetch): expose timings#32647
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
Uh oh!
There was an error while loading. Please reload this page.
feat(fetch): expose timings #32647
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -302,6 +302,7 @@ export abstract class APIRequestContext extends SdkObject { | ||
| const requestOptions = { ...options, agent }; | ||
| const startAt = monotonicTime(); | ||
| const startAtWallTime = Date.now(); | ||
| let dnsLookupAt: number | undefined; | ||
| let tcpConnectionAt: number | undefined; | ||
| let tlsHandshakeAt: number | undefined; | ||
| @@ -433,14 +434,35 @@ export abstract class APIRequestContext extends SdkObject { | ||
| const chunks: Buffer[] = []; | ||
| const notifyBodyFinished = () => { | ||
| function relativeTime(time: number | undefined): number { | ||
| if (!time) | ||
| return -1; | ||
| return time - startAt; | ||
| } | ||
| const endAt = monotonicTime(); | ||
| // spec: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming | ||
| const timing: channels.ResourceTiming = { | ||
| startTime: startAtWallTime, | ||
| domainLookupStart: dnsLookupAt ? 0 : -1, | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why zero and not ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understand is that the browser has some other steps like cache resolution before the DNS lookup, so there might be time between startTime and the DNS lookup start. On Node.js, I don't think there's anything between that - so it's zero, because we know the DNS lookup happens immediately after the request start. | ||
| domainLookupEnd: relativeTime(dnsLookupAt), | ||
| connectStart: dnsLookupAt ? relativeTime(dnsLookupAt) : 0, | ||
| secureConnectionStart: tlsHandshakeAt ? relativeTime(dnsLookupAt) : 0, | ||
| connectEnd: relativeTime(tlsHandshakeAt ?? tcpConnectionAt), | ||
| requestStart: relativeTime(tlsHandshakeAt ?? tcpConnectionAt), | ||
| responseStart: relativeTime(responseAt), | ||
| }; | ||
| const responseEndTiming = endAt - startAt; | ||
| const body = Buffer.concat(chunks); | ||
| notifyRequestFinished(body); | ||
| fulfill({ | ||
| url: response.url || url.toString(), | ||
| status: response.statusCode || 0, | ||
| statusText: response.statusMessage || '', | ||
| headers: toHeadersArray(response.rawHeaders), | ||
| body | ||
| body, | ||
| timing, | ||
| responseEndTiming, | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have two sets of timings now? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For browser requests, most of the timings are transmitted in the I thought about amending the | ||
| }); | ||
| }; | ||
| @@ -508,6 +530,9 @@ export abstract class APIRequestContext extends SdkObject { | ||
| } | ||
| }); | ||
| // socks / http proxy | ||
| socket.on('proxyConnect', () => { tcpConnectionAt = monotonicTime(); }); | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding timings to the protocol uncovered that | ||
| serverIPAddress = socket.remoteAddress; | ||
| serverPort = socket.remotePort; | ||
| }); | ||
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 drive-by fix - pretty sure it shouldn't say "start the domain name lookup"