Request priority in HAR file for Chromium based browsers#164
Request priority in HAR file for Chromium based browsers#164ozcoder wants to merge 8 commits intocloudflare:mainfrom
Conversation
Store the initial priority of each request. Store if the priority changes. Insert the initial priority value into the HAR file. Insert the final priority value into the HAR file.
Set types correctly on newPriorities and priorities. Check before using variables. Removed console debugging. Added fields to HarEntry.
|
The change itself looks reasonable, but I'm not able to verify it locally. How can I run telescope so that there are |
|
@sufian-cf if you run from the command line |
MildMax
left a comment
There was a problem hiding this comment.
verified working order of the initialPriority , priority, and resourceType in the chrome browser
sergeychernyshev
left a comment
There was a problem hiding this comment.
Looks like a good solution, we can make a couple improvements, see comments below.
Also, if you can make a fixture for testing the case with changing priority, it would be great. And maybe for testing duplicate requests.
| const fullURL = request.url + (request.urlFragment || ''); | ||
| const resourceType = type || 'Other'; | ||
|
|
||
| if (occasion.priorities[fullURL as keyof PriorityInfo]) { |
There was a problem hiding this comment.
Can we use requestId as a key instead of request URL?
There was a problem hiding this comment.
Mapping by URL will break if there are multiple requests to the same URL.
There was a problem hiding this comment.
I was using an array to get around that.
Now, using a slightly more complicated way, I can get the requestId from the CDP universe to the Playwright universe.
Create mapping of hash of url and starting time to requestId. Gather resourceType is a better place. Fixup types.
| const { requestId, response } = params; | ||
|
|
||
| if (response.timing) { | ||
| const hashString = crypto.createHash('sha256').update(response.url + response.timing.sendStart).digest('hex'); |
There was a problem hiding this comment.
Here we hash response.url + response.timing.sendStart
But when we look the hash up (in src/testRunner.ts), we use request.url + request.timing.requestStart
Is this difference intentional?
There was a problem hiding this comment.
They are the same value. It's because we are dealing with the CDP universe in one place and the Playwright universe in the other place. I had to find a stable attribute that was known to the request and the response and would be different when the same URL is requested more than once.
| const client: CDPSession = await page.context().newCDPSession(page); | ||
| await client.send('Network.enable'); | ||
|
|
||
| let occasion = this; |
There was a problem hiding this comment.
Nit: this closure isn't needed -- this will still be bound to the same object in the event handlers below, since they're arrow functions.
There was a problem hiding this comment.
Ah, yes. Old habit.
| async createPage(browser: BrowserContext): Promise<Page> { | ||
| const page = browser.pages()[0]; | ||
| const client: CDPSession = await page.context().newCDPSession(page); | ||
| await client.send('Network.enable'); |
There was a problem hiding this comment.
Is it possible Network.requestWillBeSent / other events below will fire before this client.send('Network.enable')?
There was a problem hiding this comment.
Shouldn't happen since even preparePage hasn't been called until the end of this method.
|
Hey @ozcoder, my apologies for the delay. I've taken a look at this approach and have manually verified that it sometimes works, but not for all requests. I've left a few comments, and overall am a bit concerned with the approach. That being said, I think this has surfaced a real bug in our existing code. If the page load triggers multiple requests with the same URL, we're going to only be updating performance information on the first one, since we look up the corresponding entry via I've just created #243 and will follow up with a fix that introduces a unique id per request we can use to match things. |
Adds
_priorityand_initialPriorityto the HAR file when testing with a Chromium based browser.Closes #16