-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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.
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.
Yeah this seems wrong - we can't consume the request this way :D
We'd need to 'tee' the stream (discussions happening elsewhere) though with the inspector cloning it should also be fine.
I think for this PR it would make sense to exclude request bodies and do it in a follow up PR.
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.
Dropped support for response body inspection for now. fbf5a31
@benjamingr Do you have any discussion links for reference?
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
This comment was marked as outdated.
This comment was marked as outdated.
The PR is currently blocked by this discussion: #53593 (comment). |
I've added TSC agenda since I believe there is a decision to be made here regarding whether or not we should emit Our options are:
If people believe there is a better forum than the TSC or that a decision is premature because there are ongoing efforts to resolve this - let me know. cc @nodejs/diagnostics |
@benjamingr I reached out to DevTools team lead directly internally at Google. I will try pinging random team members if the lead is not responsive, it's vacation season... |
That's appreciated thanks |
Hi, Chrome DevTools TL here. Is there something mole a design doc we could discuss? I am very much support to be of making DevTools Network tab work with request sent from the node app. I am less sure about supporting a „reversed“ network domain for the requests received by the node app in the DevTools proper. This is much trickier, generally not in line with out priorities and requires a careful planning. (I imagine DevTools extension as one possible solution for this). Anyway, it would be the best to discuss this all in a design doc. |
@danilsomsikov hey! thanks for chiming in that's very appreciated. The ask is basically around #53593 (comment) |
The ask is based on the assumption that, if Chrome DevTool protocol evolves and changes Additionally, CDP |
Hey, given we have Chrome's blessing (thanks!) to use I think incoming requests would be awesome at a later PR and I agree a dedicated UI could be very cool for that. |
Unblock and we can remove duplication as a follow-up.
Thanks for the work! We can continue iterate on the implementation and I strongly believe this needs Chrome DevTools team's involvement because the default dedicated Node.js inspector doesn't show the "Network" tab, so the public audience would be very limited with this initial work. A dedicated "Network" tab in the default inspector for Node.js could be really helpful! |
const wallTime = DateNow(); | ||
const timestamp = wallTime / 1000; | ||
request._inspectorRequestId = getNextRequestId(); | ||
NodeNetwork.requestWillBeSent({ |
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.
I tried it out today when I had free time. Is it possible to retrieve the original call location from the stack obtained in this way, which indicates diagnostics_channel
?
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.
Does 'the original call location' refer to the location in the user script where the network activity started? I mean, are you looking to retrieve the location indicated below?
const res = await fetch("https://nodejs.org/en"); // <-- here
If so, I think it's difficult with the current diagnostics_channel
API. cc @nodejs/diagnostics
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.
Yes, I mean it's not easy to do with dc.
Thank you for all discussing the direction. I agree with using the After merging this change, I'll sort out the remaining tasks and propose the design doc for server-side inspection and the dedicated UI to collaborate with the Chrome DevTools team. Again, I truly appreciate all your support! |
This comment was marked as outdated.
This comment was marked as outdated.
@danilsomsikov BTW, would it be reasonable to display the default network tab on the devtools frontend for JS apps ( |
Sure |
@danilsomsikov So, I would like to request to enable the display of the network tab on the frontend. What should I do to proceed? |
Please file a feature request at https://goo.gle/devtools-bug |
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. 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.
Summary
This description outlines the strategy to support network inspection in Node.js and the design of the APIs that allow third-party libraries to integrate with the network inspection. Specifically, it introduces the
NodeNetwork
domain, a Node.js-specific extension of the standardNetwork
domain, which supports both client and server application network activities.User stories
As a client app developer
I want to be able to check the network activities triggered by the client APIs (the
http
module,fetch
API, andWebSocket
API) on devtools such as Chrome DevTools when I run the app in debugging mode vianode --inspect index.js
.As a serer app developer
I want to be able to check the network traffics happened in my server on devtools such as Chrome DevTools when I run the app in debugging mode via
node --inspect index.js
.As a HTTP client library developer
I want to enable my library to integrate with network inspection. For example, when my library sends a HTTP request and receives a HTTP response, it sends protocol events with debugging data to allow a library user to inspect them on devtools.
Design
Tracking network activities
Network activities can be captured within
diagnostics_channel
hooks. This approach enables us to monitor activities in both core modules (http
,https
) and external libraries (undici
) without changing the core implementation.Emit protocol events to DevTools
Network activities captured in
diagnostics_channel
are passed to the inspector agent using theinspector.NodeNetwork
API as customNodeNetwork
domain objects. The customNodeNetwork
domain extends the standardNetwork
domain by including Node.js-specific events and commands, enabling more granular and relevant tracking of network activities specific to the Node.js environment. It also allows third-party libraries to integrate with Node.js's inspector mechanism.The
NodeNetwork
domain will support both client activities (such as a request sent from the client) and server activities (such as a request received by the server).When
NodeNetwork
event is sent, Node.js internally sends some of theNetwork
domain events to the devtools frontend so that Chrome DevTools can capture them and show network activities in the network panel.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:Network activity sources
These APIs can be supported once each
diagnostics_channel
provides sufficient hook timing and resources.http
modulehttps
modulehttp2
moduleScope of this PR
This PR aims to provide a minimal implementation for network inspection, focusing on delivering the fundamental functionalities. The tasks accomplished in this PR include:
diagnositcs_channel
, targetinghttp
andhttps
GET requests.NodeNetwork
domain.Network
domain.Future work
To fully support the Network domain of the CDP, several tasks remain:
Network.loadingFailed
Limitations and Challenges
cc @nodejs/inspector @eugeneo