Conversation
|
e2e same like: #62915 (comment) |
|
Review requested:
|
7865a8d to
4bd2033
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64369 +/- ##
==========================================
- Coverage 90.37% 90.37% -0.01%
==========================================
Files 792 792
Lines 275426 275724 +298
Branches 52800 52855 +55
==========================================
+ Hits 248926 249176 +250
- Misses 16931 16975 +44
- Partials 9569 9573 +4
🚀 New features to boost your workflow:
|
929ddfe to
eaa29a9
Compare
d499741 to
8fa14bc
Compare
|
I think it's time to conduct a re-review. Thank you. @metcoder95 @ShogunPanda @mcollina |
|
push |
|
push |
|
Friendly ping on the next step. The existing review feedback has been addressed, @metcoder95 has approved, and the PR is currently mergeable. It is still labeled "needs-ci" — could a collaborator please trigger the full CI? If CI is green, could @nodejs/http and @nodejs/inspector give it a final look or let me know if anything else is needed before landing? Happy to address any follow-up feedback promptly. Thanks! |
|
push |
|
push... @jasnell Can you help me add the label to run the CI |
Signed-off-by: GrinZero <774933704@qq.com>
Signed-off-by: GrinZero <774933704@qq.com>
0cb2dec to
71b18a4
Compare
|
Hi @mcollina , I’ve rebased #64369 onto the latest main and resolved the HTTP outgoing conflict. The PR is mergeable again, and CI is running. The targeted HTTP/Inspector tests pass locally. When you have a chance, could you review the HTTP integration, particularly the request-body tracking and raw response-body hook? I’d appreciate your feedback and will address any comments promptly. Thanks! |
|
Can you do a run of the benchmarks? |
I tried comparing the main branch and found that the newly added features resulted in a performance drop of around 18%. Due to the collection of body data, this decrease is actually within expectations. But did CI just fail? I think while investigating, I can also see if there are any ways to optimize.
I can’t access the Jenkins results due to the CI read permissions. Both node-test-commit/92841 and node-test-pull-request/78032 failed, although the reported platform checks appear green. Could someone with CI access share the failing job/platform, test name, and relevant console output? If the failure is unrelated to this change, could you please rerun the affected job? Thanks! @mcollina |

Summary
This PR supersedes #62915 with the same feature work rebased onto current
main.It adds builtin
http/httpsrequest-body support to network inspection, soNetwork.getRequestPostDatacan return text request bodies while preserving the existing rejection behavior for binary request bodies.It also moves builtin
httpresponse-body tracking to a raw-byte hook beforeIncomingMessagedecoding, so response inspection remains correct even when user code callsresponse.setEncoding(...).In addition, this PR lets
Network.requestWillBeSentcarry JS-provided initiator data captured at request construction time. The C++ inspector side validates structuredinitiator.stackobjects against the inspector protocol schema before forwarding them to DevTools.Problem
Builtin
http/httpsnetwork inspection currently emits request and response lifecycle events, but the builtin client path does not expose request-body bytes to the inspector. As a result,Network.getRequestPostDatacannot return POST data for builtinhttp/httpsrequests.There are two related gaps:
IncomingMessage'data'events are not a stable raw-byte source. If user code callsresponse.setEncoding('utf8'), chunks observed through userland become strings, while the inspector protocol expects byte-oriented payloads.requestWillBeSentuntil request-body state is known means a C++ stack captured at emission time would point into Node.js internals rather than the user call site. The initiator needs to be captured when the request is created and validated before being emitted.Approach
Network.dataSent(...) -> NetworkAgent::getRequestPostData(...)buffering pipeline instead of changing the CDP schema.http.client.request.bodyChunkSenthttp.client.request.bodySenthttp.client.response.bodyChunkReceivedrequestWillBeSentuntil body state is known, sohasPostDatais accurate for both body and body-less requests.initiator.stackwith generated inspector protocol schema support in C++.Behavior
After this change:
httpandhttpsPOST requests with UTF-8 text bodies are available throughNetwork.getRequestPostDatahttpresponse inspection continues to work even if user code callsresponse.setEncoding('utf8')Network.requestWillBeSentreports accuratehasPostDataNetwork.requestWillBeSentcan carry JS-captured initiator stack data from the request construction siteTests
This PR adds and extends coverage in:
test/parallel/test-diagnostics-channel-http.jstest/parallel/test-inspector-network-http.jstest/parallel/test-inspector-emit-protocol-event.jstest/parallel/test-inspector-emit-protocol-event-errors.jsThe updated tests cover:
write()andend()httpandhttpsNetwork.getRequestPostDataresponse.setEncoding('utf8')hasPostDatafor body-less requestsVerification
Validated with the targeted test suite:
Also validated manually with an end-to-end inspector run:
Refs