fix(har timing): record `connect` timing for proxied connections by Skn0tt · Pull Request #32855 · microsoft/playwright

Simon Knott (Skn0tt) · 2024-09-27T12:27:20Z

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.

Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

github-actions · 2024-09-27T13:29:37Z

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.

Dmitry Gozman (dgozman) · 2024-09-27T14:39:59Z

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

Simon Knott (Skn0tt) · 2024-09-27T14:52:54Z

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 is for the HTTP proxy and set connect: -1.

Simon Knott (Skn0tt) · 2024-10-07T07:29:23Z

Could you give this another look Dmitry Gozman (@dgozman)?

…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

…ns" (#32855) (#33003) This reapplies what we reverted in #32989. Max and me debugged this, and found that the test failures come from SOCKS proxy now preferring IPv6 over IPv4. We've updated the tests and made sure that this doesn't mask any breaking change. I'm enabling CQ1 to make sure we don't oversee any other CI failures.

…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

…onnections" (#32855) (#33003)" (#34535)

…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

… timing for proxied connections" (#32855) (#33003)" (#34538)

Simon Knott (Skn0tt) · 2024-09-27T12:27:20Z

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.

Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

github-actions · 2024-09-27T13:29:37Z

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.

Dmitry Gozman (dgozman) · 2024-09-27T14:39:59Z

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

Simon Knott (Skn0tt) · 2024-09-27T14:52:54Z

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 is for the HTTP proxy and set connect: -1.

Simon Knott (Skn0tt) · 2024-10-07T07:29:23Z

Could you give this another look Dmitry Gozman (@dgozman)?

…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

…ns" (#32855) (#33003) This reapplies what we reverted in #32989. Max and me debugged this, and found that the test failures come from SOCKS proxy now preferring IPv6 over IPv4. We've updated the tests and made sure that this doesn't mask any breaking change. I'm enabling CQ1 to make sure we don't oversee any other CI failures.

…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

…onnections" (#32855) (#33003)" (#34535)

…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

… timing for proxied connections" (#32855) (#33003)" (#34538)

Simon Knott (Skn0tt) · 2024-09-27T12:27:20Z

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.

Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

github-actions · 2024-09-27T13:29:37Z

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.

Dmitry Gozman (dgozman) · 2024-09-27T14:39:59Z

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

Simon Knott (Skn0tt) · 2024-09-27T14:52:54Z

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 is for the HTTP proxy and set connect: -1.

Simon Knott (Skn0tt) · 2024-10-07T07:29:23Z

Could you give this another look Dmitry Gozman (@dgozman)?

…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

…ns" (#32855) (#33003) This reapplies what we reverted in #32989. Max and me debugged this, and found that the test failures come from SOCKS proxy now preferring IPv6 over IPv4. We've updated the tests and made sure that this doesn't mask any breaking change. I'm enabling CQ1 to make sure we don't oversee any other CI failures.

…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

…onnections" (#32855) (#33003)" (#34535)

…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

… timing for proxied connections" (#32855) (#33003)" (#34538)

Simon Knott (Skn0tt) · 2024-09-27T12:27:20Z

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.

Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

github-actions · 2024-09-27T13:29:37Z

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.

Dmitry Gozman (dgozman) · 2024-09-27T14:39:59Z

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

Simon Knott (Skn0tt) · 2024-09-27T14:52:54Z

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 is for the HTTP proxy and set connect: -1.

Simon Knott (Skn0tt) · 2024-10-07T07:29:23Z

Could you give this another look Dmitry Gozman (@dgozman)?

…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

…ns" (#32855) (#33003) This reapplies what we reverted in #32989. Max and me debugged this, and found that the test failures come from SOCKS proxy now preferring IPv6 over IPv4. We've updated the tests and made sure that this doesn't mask any breaking change. I'm enabling CQ1 to make sure we don't oversee any other CI failures.

…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

…onnections" (#32855) (#33003)" (#34535)

…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

… timing for proxied connections" (#32855) (#33003)" (#34538)

Simon Knott (Skn0tt) · 2024-09-27T12:27:20Z

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.

Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

github-actions · 2024-09-27T13:29:37Z

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.

Dmitry Gozman (dgozman) · 2024-09-27T14:39:59Z

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

Simon Knott (Skn0tt) · 2024-09-27T14:52:54Z

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 is for the HTTP proxy and set connect: -1.

Simon Knott (Skn0tt) · 2024-10-07T07:29:23Z

Could you give this another look Dmitry Gozman (@dgozman)?

…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

…ns" (#32855) (#33003) This reapplies what we reverted in #32989. Max and me debugged this, and found that the test failures come from SOCKS proxy now preferring IPv6 over IPv4. We've updated the tests and made sure that this doesn't mask any breaking change. I'm enabling CQ1 to make sure we don't oversee any other CI failures.

…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

…onnections" (#32855) (#33003)" (#34535)

…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

… timing for proxied connections" (#32855) (#33003)" (#34538)

Simon Knott (Skn0tt) · 2024-09-27T12:27:20Z

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.

Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

github-actions · 2024-09-27T13:29:37Z

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.

Dmitry Gozman (dgozman) · 2024-09-27T14:39:59Z

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

Simon Knott (Skn0tt) · 2024-09-27T14:52:54Z

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 is for the HTTP proxy and set connect: -1.

Simon Knott (Skn0tt) · 2024-10-07T07:29:23Z

Could you give this another look Dmitry Gozman (@dgozman)?

…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

…ns" (#32855) (#33003) This reapplies what we reverted in #32989. Max and me debugged this, and found that the test failures come from SOCKS proxy now preferring IPv6 over IPv4. We've updated the tests and made sure that this doesn't mask any breaking change. I'm enabling CQ1 to make sure we don't oversee any other CI failures.

…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

…onnections" (#32855) (#33003)" (#34535)

…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

… timing for proxied connections" (#32855) (#33003)" (#34538)

Simon Knott (Skn0tt) · 2024-09-27T12:27:20Z

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.

Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

github-actions · 2024-09-27T13:29:37Z

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.

Dmitry Gozman (dgozman) · 2024-09-27T14:39:59Z

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

Simon Knott (Skn0tt) · 2024-09-27T14:52:54Z

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 is for the HTTP proxy and set connect: -1.

Simon Knott (Skn0tt) · 2024-10-07T07:29:23Z

Could you give this another look Dmitry Gozman (@dgozman)?

…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

…ns" (#32855) (#33003) This reapplies what we reverted in #32989. Max and me debugged this, and found that the test failures come from SOCKS proxy now preferring IPv6 over IPv4. We've updated the tests and made sure that this doesn't mask any breaking change. I'm enabling CQ1 to make sure we don't oversee any other CI failures.

…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

…onnections" (#32855) (#33003)" (#34535)

…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

… timing for proxied connections" (#32855) (#33003)" (#34538)

Simon Knott (Skn0tt) · 2024-09-27T12:27:20Z

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.

Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

github-actions · 2024-09-27T13:29:37Z

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.

Dmitry Gozman (dgozman) · 2024-09-27T14:39:59Z

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

Simon Knott (Skn0tt) · 2024-09-27T14:52:54Z

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 is for the HTTP proxy and set connect: -1.

Simon Knott (Skn0tt) · 2024-10-07T07:29:23Z

Could you give this another look Dmitry Gozman (@dgozman)?

…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

…ns" (#32855) (#33003) This reapplies what we reverted in #32989. Max and me debugged this, and found that the test failures come from SOCKS proxy now preferring IPv6 over IPv4. We've updated the tests and made sure that this doesn't mask any breaking change. I'm enabling CQ1 to make sure we don't oversee any other CI failures.

…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

…onnections" (#32855) (#33003)" (#34535)

…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

… timing for proxied connections" (#32855) (#33003)" (#34538)

fix(har timing): record 'connect' timing for http proxy
988da93

Simon Knott (Skn0tt) self-assigned this Sep 27, 2024

Simon Knott (Skn0tt) requested a review from Dmitry Gozman (dgozman)September 27, 2024 12:27

also add socks proxy
6492999

Simon Knott (Skn0tt) changed the title ~~fix(har timing): record 'connect' timing for http proxy~~fix(har timing): record 'connect' timing for proxied connectionsSep 27, 2024

Simon Knott (Skn0tt) changed the title ~~fix(har timing): record 'connect' timing for proxied connections~~fix(har timing): record connect timing for proxied connectionsSep 27, 2024

This comment has been minimized.
Sign in to view

Max Schmitt (mxschmitt) reviewed Sep 27, 2024
View reviewed changes

Comment threadpackages/playwright-core/src/server/fetch.ts Outdated

Simon Knott (Skn0tt)and others added 2 commits September 27, 2024 14:47

Update packages/playwright-core/src/server/fetch.ts
11014ea
Co-authored-by: Max Schmitt <max@schmitt.mx> Signed-off-by: Simon Knott <info@simonknott.de>

update comment
1ae95ba

This comment has been minimized.
Sign in to view

Dmitry Gozman (dgozman) approved these changes Oct 7, 2024
View reviewed changes

Simon Knott (Skn0tt) merged commit de4a4d1 into microsoft:mainOct 7, 2024

Simon Knott (Skn0tt) mentioned this pull request Oct 7, 2024
Revert "fix(har timing): record connect timing for proxied connections" #32989
Merged

Simon Knott (Skn0tt) added a commit that referenced this pull request Oct 8, 2024
Revert "fix(har timing): record connect timing for proxied connecti…
1b589c4
…ons" (#32989) Reverts #32855. This broke two tests on main, and we don't yet know how to fix it other than downgrading.

Dmitry Gozman (dgozman) added a commit to dgozman/playwright that referenced this pull request Jan 29, 2025
Revert "Reapply "fix(har timing): record connect timing for proxied c…
216c9a2
…onnections" (microsoft#32855) (microsoft#33003)" This reverts commit 042161e.

Dmitry Gozman (dgozman) added a commit that referenced this pull request Jan 29, 2025
Revert "Reapply "fix(har timing): record connect timing for proxied c…
7d8265e
…onnections" (#32855) (#33003)" (#34535)

Dmitry Gozman (dgozman) added a commit to dgozman/playwright that referenced this pull request Jan 29, 2025
cherry-pick(microsoft#34535): Revert "Reapply "fix(har timing): recor…
0c281a3
…d connect timing for proxied connections" (microsoft#32855) (microsoft#33003)"

Dmitry Gozman (dgozman) added a commit that referenced this pull request Jan 29, 2025
cherry-pick(#34535): Revert "Reapply "fix(har timing): record connect…
1efbedd
… timing for proxied connections" (#32855) (#33003)" (#34538)

Shahzad (shahzad31) mentioned this pull request Jul 12, 2026
[Feature]: Expose recorded ResourceTiming on APIResponse (parity with Request.timing()) #41747
Closed