feat(har): record sizes, compression and httpVersion#6695
feat(har): record sizes, compression and httpVersion#6695tnolet wants to merge 15 commits intomicrosoft:masterfrom checkly:har-tracer-update
Conversation
The http version was hard coded to http/1.1 which was wrong. Als h2 and h3 have different ways of reporting headers and lenghts
HTTP version is now resolved in one place or substituted by a fallback. Works with reporting h2 and later.
|
|
tnolet
changed the title
tnolet
changed the title
…pe from response header
dgozman
left a comment
dgozman
left a comment
There was a problem hiding this comment.
Looks good overall, left a few comments.
| this._log.pages.push(pageEntry); | ||
| page.on(Page.Events.Request, (request: network.Request) => this._onRequest(page, request)); | ||
| page.on(Page.Events.Response, (response: network.Response) => this._onResponse(page, response)); | ||
| page.on(Page.Events.RequestFinished, (request: network.Request) => this._onRequestFinished(page, request)); |
There was a problem hiding this comment.
Not all requests finish, some of them stall, and others fail. I think we should update half of the fields on Response event, and another half on RequestFinished/RequestFailed. WDYT?
There was a problem hiding this comment.
yes, this is a correct observation. Somehow it is feels messy though. Reading the code it's not super clear what gets done when. I do think we have no other way, this is how HTTP / Browser networking is.
Testing this is tricky. We would need a local server for tests that does not respond or stalls some requests.
There was a problem hiding this comment.
I think you can just add a test with a failed request and see that some information is there. I do not think that extra testing complexity will be worth it in this particular case.
| postData: undefined, | ||
| postData: postDataForHar(request) || undefined, | ||
| headersSize: -1, | ||
| bodySize: -1, |
There was a problem hiding this comment.
Do you want to set bodySize here as well? See how DevTools does it here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/models/har/Log.ts;l=378-388?q=f:front_end%20-f:out%2F%20responseHeadersText.
…ructing Response webkit reports protocol in the .metrics object later than chromium, so protocol is not available during constructing the response. This method allows setting it later in the cycle.
|
Hey @tnolet! What's the status of this? For the firefox, it'll be very easy to export transfer size: https://ffsearch.azurewebsites.net/#path=%2Fhome%2Fjoe%2Fplaywright%2Fbrowser_patches%2Ffirefox%2Fcheckout%2Fnetwerk%2Fprotocol%2Fhttp%2FnsIHttpChannel.idl&line=90 I can help with this if needed! |
@aslushnikov I think we are pretty damn far with this. It would be great if you can help patch FF so we can keep this PR simple and not make any exemptions for FF. How should we coordinate this? I see the following steps:
|
|
Closing since #7470 is merged. |
4 participants
This PR attempts to update a set of missing properties in the HAR file, where earlier a default -1 was reported and also report the correct HTTP Version, which was hard coded to
HTTP/1.1The test in
tests/har.spec.ts:244should show what is being done.bodySizeheadersSizehttpVersion_transferSize: note this is not an official property, but used by Chrome.content.sizecontent.compressionThe biggest change is that the fields at the end of the request/response cycle are now done on
Page.Events.onRequestFinishedinstead ofPage.Events.onResponse.Note 1
This does not work yet for Firefox as no
transferSizelike property is available. However, we can probably make that happen with some work on the NetworkObserver.js patches.Note 2
I went down the rabbit hole of solving this problem by populating the various
sizeusing theperformance.getEntriesByName()API from the browser. However, CORS settings makes this unreliable.