-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
inspector: add initial support for network inspection #53593
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
499ab30
to
6a9a7cf
Compare
CC @nodejs/undici regarding support for |
Hey, this is incredibly cool. |
Yes, at the very least we should probably integrate with fetch/undici, it's hookable already through the dispatcher so it shouldn't be too hard (before this is stable, not to block this PR) |
I think the fetch api is also very important and I hope it can be supported |
@benjamingr I wasn't aware of undici's dispatcher feature, and I've now seen that undici includes diagnostics channels. I'm thinking using |
I received this update before taking a shower, and this piece can finally be pushed forward, which is great. About two weeks ago, I developed a library for this issue, and some key ideas should be useful: The 'v8 inspector' lacks a network domainIt is used How to listen for HTTP/HTTPS requests?Basic ideas
Key points
How to make full use of the
|
One thing I was prototyping years ago was implementing Inspector domains in the user land, in JS. I.e. instead of piping Another thing we discussed a lot in the past is that Network domain in Node should be reversed as most users will be interested in debugging server application. There should be Chrome DevTools at the time was very adamant not to reuse Chrome domains for the domains not in V8. Please rename this domain to |
Could we implement it in terms of diagnostic channnels that external libs (undici) can just hook into? |
I completely agree, and I also believe that the network domain in Node should be reversed. Now we can actually extend the v8 inspector launched by the node, but it is more related to remote debugging, and due to domain limitations, network and other domains cannot be implemented. |
Note needs both HTTP server and HTTP client support... It would be invaluable to be able to see request that was received and what was sent to other microservices. |
fc0ed45
to
5656be6
Compare
@nodejs/undici For network inspection on fetch API, we need undici's diagnostics_channel to support a hook when body is received. Has there been any progress on nodejs/undici#1342? |
I would like to confirm if listening to network requests through the undici library diagnostics.channel is compatible with previous libraries? |
@GrinZero I'm going to add as many features of the @eugeneo I need your advice on how to properly define and implement the custom |
5656be6
to
1c754b4
Compare
I am wondering if it is possible to be more open and allow the node v8 inspector to directly expose the websocket server. This way, developers can not only operate devtool as a CDP client, but also extend devtool as a CDP server in the future. |
1c754b4
to
6e727ee
Compare
dataReceived(request._inspectorRequestId, DateNow() / 1000, chunk.length); | ||
responseString += chunk.toString(); | ||
}; | ||
response.on('data', onData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the test results, consuming response data and entering flowing mode in the diagnostics_channel hook is causing some issues in the core. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show the issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/nodejs/node/actions/runs/9696727280/job/26759268382?pr=53593
I'll investigate and summarize the issues later.
We could add fetch support via nodejs/undici#2701. Generically I think we should add some APIs to let devs integrate 3rd party clients. |
+1 to this being diagnostics_channel based and +1 for letting userland tools integrate with it. |
6e727ee
to
b874c90
Compare
The idea of supporting network inspection in Node.js was first proposed 7 years age in the nodejs/diagnostics#75. Despite numerous discussions, we have yet to settle on an implementation approach. This PR aims to serve as a starting point to explore and refine how we can achieve this feature.
Overview
This PR introduces basic support for the Network domain of the Chrome DevTools Protocol (CDP) and its corresponding agent implementation in Node.js. Although this is an initial implementation with several pending tasks, it sets a foundation to verify if we are heading in the right direction.
Demo
Currenlty, the Node-specific DevTools Frontend lacks a network tab. Therefore, you'll need to use the Chrome DevTools Frontend, accessible via
devtools://devtools/bundled/inspector.html
. Below is a simple demonstration:Implementation Strategy
Network activity tracking
We considered several ways to track network activities, as discussed in nodejs/diagnostics#75. This PR captures request and response data with the
diagnostic_channel
module. This approach enables us to monitor activities in both core modules (http
,https
) and external libraries (undici
) without changing the core implementation.Custom CDP domain (
NodeNetwork
)TBD
Future work
To fully support the Network domain of the CDP, several tasks remain:
Network.loadingFailed
Limitations and Challenges
These APIs can be supported once each
diagnostics_channel
provides sufficient hook timing and resources.http2
moduleThe following features may be challenging to fully implement in the initial phase, and we need to evaluate the demand for them.
cc @nodejs/inspector @eugeneo