search

sitespeedio / chrome-har

Create HAR files from Chrome Debugging Protocol data
MIT License
149 stars 50 forks source link

Add an option to include requestId as _request_id in the HAR #25

Closed mnmkng closed 4 years ago

mnmkng commented 6 years ago

Hi @soulgalore

I've prepared the PR @jancurn suggested in https://github.com/sitespeedio/chrome-har/issues/24

Its a simple change, but I've added a test anyway. Feel free to comment.

Cheers!

soulgalore commented 6 years ago

Hi @mnmkng great! I'll have a look and merge tonight. There were two eslint errors on Travis can you fix them? You can run npm lint:fix and push the change + add a ignore for the console log.

One last thing, do you use Chrome-HAR standalone or through Browsertime (is a new release of Chrome-HAR enough or do you need BT too?).

Best Peter

tobli commented 6 years ago

Hi @mnmkng, thanks for contributing. Please see my comment here: https://github.com/sitespeedio/chrome-har/issues/24#issuecomment-405541814. I'd prefer removing the preference, and just naming the property _request_id in the first place. Note however the added 'r'.

mnmkng commented 6 years ago

Thank you for the comments guys! Could @soulgalore please confirm that @tobli 's way is the way to go? I would update the PR.

soulgalore commented 6 years ago

Yes please do :)

mnmkng commented 6 years ago

It's done :) And to answer your question above. We are not using Browsertime so a release of chrome-har is fine. Thank you very much. We're using it with Puppeteer.

Which reminds me:

We're dealing with the same issue as https://github.com/sitespeedio/chrome-har/issues/15. Could you please point me to how you're extracting the perflog dumps using Browsertime? We're currently listening to various Page and Network DevTool events via Puppeteer.CDPSession, but their order is not guaranteed and therefore always wrong. Are you using the Tracing API ?

I'd gladly prepare a Puppeteer compatibility PR, if we manage to resolve the issue.

soulgalore commented 6 years ago

Ah cool. @tobli do you have time to look before merge?

Hmm yes, I wonder though if it isn't Puppeteer that handles it wrong, so there's a way to configure it differently? We use the trace log enabled with page/network Selenium/Chromedriver style, you can check it here.

tobli commented 6 years ago

Looking closer into what WPT does I'm not sure this is correct. For redirects WPT adds a "-1", "-2" etc. suffix in the _request_id field, rather than just a "r". Need to check in more detail, but I suspect that long redirect chains doesn't work correctly with our current approach. WPT uses the "_raw_id" extension field for the original request id in a long chain. For an example, check this HAR from this test.

I'd like us to add more tests for long redirect chains before locking down this in public api, as it might need to change.

mnmkng commented 6 years ago

@tobli Is there anything I can do to help? Perhaps add the tests? Could you elaborate more on the neccessary steps or is it too complex without knowing the very details of chrome-har?

_raw_id works for us too. We just need the requestId to call Network.getResponseBody.

jancurn commented 6 years ago

Hey guys, it would be great if we could get the Puppeteer request ID to the HAR in any shape or form. Currently this is a showstopper for us as there is no correct way to attach the raw content of the HTTP requests to the resulting HAR file. The only other way for us to move forward is to fork this repo and republish it on NPM...

soulgalore commented 6 years ago

@jancurn yeah sorry. Both me and @tobli is on vacation see https://github.com/sitespeedio/sitespeed.io#vacation-july---august and @beenanner is at Next '18.

@tobli is the creator of the project so I wanna make sure things get the way he wants.

jancurn commented 6 years ago

I understand. Many thanks for your help guys!