Skip to content
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

fix(har timing): record connect timing for proxied connections #32855

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Sep 27, 2024

Fixes a bug discovered in #32647. When using http proxy, the connect event isn't emitted so we don't populate tcpConnectionAt. The updated version of https-proxy-agent emits a proxyConnect as a replacement, so this PR updates and listens to that event.
For socks proxies, the on("socket") event is emitted once the SOCKS connection is established, which is the equivalent of having a TCP connection available.

@Skn0tt Skn0tt self-assigned this Sep 27, 2024
@Skn0tt Skn0tt requested a review from dgozman September 27, 2024 12:27
@Skn0tt Skn0tt changed the title fix(har timing): record 'connect' timing for http proxy fix(har timing): record 'connect' timing for proxied connections Sep 27, 2024
@Skn0tt Skn0tt changed the title fix(har timing): record 'connect' timing for proxied connections fix(har timing): record connect timing for proxied connections Sep 27, 2024

This comment has been minimized.

Skn0tt and others added 2 commits September 27, 2024 14:47
Co-authored-by: Max Schmitt <max@schmitt.mx>
Signed-off-by: Simon Knott <info@simonknott.de>

This comment has been minimized.

Copy link
Contributor

Test results for "tests 1"

1 flaky ⚠️ [chromium] › components/splitView.spec.tsx:35:5 › should render sidebar first

36416 passed, 757 skipped
✔️✔️✔️

Merge workflow run.

@dgozman
Copy link
Contributor

dgozman commented Sep 27, 2024

Do we have to upgrade the deps? I am always wary of introducing regressions.

@Skn0tt
Copy link
Member Author

Skn0tt commented Sep 27, 2024

For SOCKS proxy we don't, but for HTTP proxy yes. The proxyConnect event was only added in 6.1.0 and without it we have no way of knowing when the proxy started the connection.

If we don't want to update, another alternative would be leave things as for the HTTP proxy and set connect: -1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants