http: fix perf_hooks detail.req.url port and proxied path - #64311
Merged
nodejs-github-bot merged 1 commit intoJul 12, 2026
Merged
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
|
Review requested:
|
Sorry, something went wrong.
mcollina
approved these changes
Jul 5, 2026
mcollina
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
joyeecheung
approved these changes
Jul 6, 2026
The perf_hooks HTTP client entry built the reported URL from the bare hostname, dropping non-default ports and IPv6 brackets, and appended the request path even after it had been rewritten to absolute-form for proxying, duplicating the protocol and authority. Report the connection authority captured at request creation and, for proxied requests, use the rewritten absolute-form target as-is. Fixes: nodejs#59625 Signed-off-by: Stefano Baghino <stefano@baghino.me>
stefanobaghino
force-pushed
the
perf_hooks/fix-http-detail-url
branch
from
f627005 to
9e4d4b0
Compare
July 6, 2026 16:20
Copy link
Copy Markdown
Contributor
Author
|
Amended the commit to comply with the Signed-off-by requirement (and re-wrapped the message body at 72 columns), which is what made the commit message linting fail. |
Sorry, something went wrong.
jasnell
approved these changes
Jul 7, 2026
theanarkh
approved these changes
Jul 7, 2026
Copy link
Copy Markdown
Contributor
Author
|
🤔 The failure looks like an unrelated flaky test. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
Is there some way in which I can trigger CI myself? 🙂 |
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
Thanks for the reviews, everyone! This looks ready to land whenever a collaborator has a chance. Very happy to help move it along in any way I can — rebasing, amending the commit, or anything else — just let me know what would be useful. |
Sorry, something went wrong.
aduh95
pushed a commit
that referenced
this pull request
Jul 21, 2026
The perf_hooks HTTP client entry built the reported URL from the bare hostname, dropping non-default ports and IPv6 brackets, and appended the request path even after it had been rewritten to absolute-form for proxying, duplicating the protocol and authority. Report the connection authority captured at request creation and, for proxied requests, use the rewritten absolute-form target as-is. Fixes: #59625 Signed-off-by: Stefano Baghino <stefano@baghino.me> PR-URL: #64311 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: theanarkh <theratliter@gmail.com>
aduh95
pushed a commit
that referenced
this pull request
Jul 21, 2026
The perf_hooks HTTP client entry built the reported URL from the bare hostname, dropping non-default ports and IPv6 brackets, and appended the request path even after it had been rewritten to absolute-form for proxying, duplicating the protocol and authority. Report the connection authority captured at request creation and, for proxied requests, use the rewritten absolute-form target as-is. Fixes: #59625 Signed-off-by: Stefano Baghino <stefano@baghino.me> PR-URL: #64311 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: theanarkh <theratliter@gmail.com>
RafaelGSS
pushed a commit
that referenced
this pull request
Jul 29, 2026
The perf_hooks HTTP client entry built the reported URL from the bare hostname, dropping non-default ports and IPv6 brackets, and appended the request path even after it had been rewritten to absolute-form for proxying, duplicating the protocol and authority. Report the connection authority captured at request creation and, for proxied requests, use the rewritten absolute-form target as-is. Fixes: #59625 Signed-off-by: Stefano Baghino <stefano@baghino.me> PR-URL: #64311 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: theanarkh <theratliter@gmail.com>
bartlomieju
added a commit
to badgerbees/deno
that referenced
this pull request
Aug 5, 2026
Follow-up cleanups to the kAuthority/kProxyRewrittenToAbsolute port so it matches nodejs/node#64311 more closely. Initialize kProxyRewrittenToAbsolute next to kPath rather than after the proxy rewrite block. Node can initialize it late because its rewrite happens in rewriteForProxiedHttp() after construction, but here the rewrite is inline in the constructor, so `x = x || false` was load-bearing on statement order. Setting it before the rewrite drops that coupling. Hoist posColon out of the bare block it was wrapped in and restore the RFC 3986 comment from upstream. Drop the `|| host` fallback in the perf entry. `host` always resolves to at least "localhost", so kAuthority is never empty and the fallback was dead - it only obscured that a caller-supplied Host header no longer determines the reported URL, which is what Node does. Apply the same fix to the OpenTelemetry client span, which built url.full the same way and so carried the same proxy path duplication. This part has no upstream counterpart; Node has no OTel instrumentation here.
aduh95
pushed a commit
that referenced
this pull request
Aug 6, 2026
The perf_hooks HTTP client entry built the reported URL from the bare hostname, dropping non-default ports and IPv6 brackets, and appended the request path even after it had been rewritten to absolute-form for proxying, duplicating the protocol and authority. Report the connection authority captured at request creation and, for proxied requests, use the rewritten absolute-form target as-is. Fixes: #59625 Signed-off-by: Stefano Baghino <stefano@baghino.me> PR-URL: #64311 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: theanarkh <theratliter@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The detail.req.url reported on http performance entries for HttpClient misrepresents the request URL in two ways: the port is dropped when it is non-default (http://localhost:3000/foo is reported as http://localhost/foo), and when the request path has been rewritten to absolute-form for HTTP proxying it is appended to the protocol and hostname again (http://localhosthttp://localhost:3000/foo).
The URL is now built from the connection authority captured at request creation (which carries the non-default port and IPv6 brackets, same as the auto-generated Host header), and when the path has been rewritten to absolute-form for proxying it is reported as-is, since it is already the full normalized URL.
Fixes: #59625