node icon indicating copy to clipboard operation
node copied to clipboard

inspector: add initial support for network inspection

Open cola119 opened this issue 1 year ago • 17 comments

The idea of supporting network inspection in Node.js was first proposed 7 years age in the https://github.com/nodejs/diagnostics/issues/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:

  1. Start Node.js with the inspector and wait for a connection:
$ ./node --inspect-wait -e "require('https').get('https://nodejs.org/en', (res) => { console.log(res.statusCode); }); fetch('https://nodejs.org/fr');"
Debugger listening on ws://127.0.0.1:9229/<inspector-websocket-id>
For help, see: https://nodejs.org/en/docs/inspector
  1. Open the Chrome DevTools Frontend and connect to the Node.js inspector
devtools://devtools/bundled/inspector.html?ws=127.0.0.1:9229/<inspector-websocket-id>
  1. Navigate to the network tab to observe network activity.

image

Implementation Strategy

Network activity tracking

We considered several ways to track network activities, as discussed in https://github.com/nodejs/diagnostics/issues/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:

  • [ ] Complete implementation for all network domain events as specified in the https://chromedevtools.github.io/devtools-protocol/tot/Network/
    • CDP is primarily designed for browsers, but we aim to support as many relevant features as possible in Node.js
    • [ ] Network.loadingFailed
    • [x] request url
    • [ ] request headers
    • [x] request timing
    • [x] response data
    • [ ] status code
    • [ ] response headers
    • [ ] ...
  • [ ] Add a network tab on the Node-specific DevTools frontend
    • Collaborate with the Chrome DevTools team to achieve this.

Limitations and Challenges

These APIs can be supported once each diagnostics_channel provides sufficient hook timing and resources.

  • [ ] Support WebSocket
  • [ ] Support fetch API
  • [ ] Support http2 module

The following features may be challenging to fully implement in the initial phase, and we need to evaluate the demand for them.

  • [ ] Non-inspection features (e.g., header/request/response modification, network condition emulation, etc.)

cc @nodejs/inspector @eugeneo

cola119 avatar Jun 26 '24 13:06 cola119

Review requested:

  • [ ] @nodejs/gyp
  • [ ] @nodejs/http
  • [ ] @nodejs/net

nodejs-github-bot avatar Jun 26 '24 13:06 nodejs-github-bot

CC @nodejs/undici regarding support for fetch and WebSocket

MoLow avatar Jun 26 '24 13:06 MoLow

Hey, this is incredibly cool.

benjamingr avatar Jun 26 '24 14:06 benjamingr

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)

benjamingr avatar Jun 26 '24 14:06 benjamingr

I think the fetch api is also very important and I hope it can be supported

GrinZero avatar Jun 26 '24 14:06 GrinZero

@benjamingr I wasn't aware of undici's dispatcher feature, and I've now seen that undici includes diagnostics channels. I'm thinking using diagnostics_channel to monitor network activities could be a more straightforward approach than capturing data within the core implementation.

cola119 avatar Jun 26 '24 14:06 cola119

I received this update before taking a shower, and this piece can finally be pushed forward, which is great. I have decided to share my experiences and ideas in this area, hoping to be helpful for development.

About two weeks ago, I developed a library for this issue, and some key ideas should be useful:

The 'v8 inspector' lacks a network domain

It is useddevtools://devtools/bundled/inspector.htmlSubstitute (this is consistent with the above)

How to listen for HTTP/HTTPS requests?

Basic ideas

  • Firstly, I attempted to hijack the request method of the HTTP module, which allowed me to obtain Options when the user called it. By analyzing the parameters, I obtained key data such as URL and request headers.(request.ts#L77)
  • Next, we will continue to hijack the parameter callback of request. Through this step, we can obtain the IncomingMessage, so that we can obtain key data such as responseData and reponseHeader when the request returns.(request.ts#L50)
  • The final step is to hijack the instance returned by request with write method, so that the data at the time of the request can be obtained. (request.ts#L20)

Key points

  • It is worth noting that when truly sending responseData to devtool, data decompression processing is also required!(tryDecompression(...))
  • When sending requestWillBeSend, it is necessary to have more judgment on the content-type in order to better utilize devtool.

How to make full use of the initiator

A single network domain cannot support operations such as traceability and click to jump. This involves the Debugger domain and the specific chain ⛓️ :

  • Debugger.scriptParsed(Server to Devtool) -->
  • Debugger. getScriptSource (Devtool to Server) -->
  • Debugger.getScriptSourceResponse(Server response to Devtool).

The core of it is scriptId. Currently, I distinguish scriptId by file name, but the actual processing should be more complex.

How to support fetch

fetch.ts

Although I attempted to implement fetch hijacking, I found that the information provided by the fetch was relatively limited, and only basic request header \ request data \ response header \ response data could be displayed.

Data regarding data length and other aspects could not be fully collected. (This can be seen in the code specifically, the core is clone() & hijacking)

I hope this will be helpful for future development. My mastery of C++ is average, and my assistance with PR is limited.

GrinZero avatar Jun 26 '24 15:06 GrinZero

One thing I was prototyping years ago was implementing Inspector domains in the user land, in JS. I.e. instead of piping requestWillBeSent/responseReceived through all the layers we could have a generic backend and one method that JS would use to send different messages. This would also allow the ecosystem add more custom domains that would be able to piggiback on existing Inspector infrastructure. E.g. some database vendor may want to add a custom domain for their database and a custom tool that would connect to Inspector server.

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 requestReceived/responseWillBeSent pair.

Chrome DevTools at the time was very adamant not to reuse Chrome domains for the domains not in V8. Please rename this domain to NodeNetwork and let the tool developers opt in in supporting it. Chances are the domains will diverge (say, to support requestReceived) and it will be really difficult for tools to tell what they are working with, a browser or server.

eugeneo avatar Jun 26 '24 15:06 eugeneo

Could we implement it in terms of diagnostic channnels that external libs (undici) can just hook into?

ronag avatar Jun 26 '24 16:06 ronag

One thing I was prototyping years ago was implementing Inspector domains in the user land,...

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.

GrinZero avatar Jun 26 '24 16:06 GrinZero

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.

eugeneo avatar Jun 26 '24 16:06 eugeneo

@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 https://github.com/nodejs/undici/issues/1342?

cola119 avatar Jun 27 '24 07:06 cola119

@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 avatar Jun 27 '24 07:06 GrinZero

@GrinZero I'm going to add as many features of the Network domain as possible once we confirm that this PR is on the right track. (Currently, I'm trying to figure out how to support both the custom NodeNetwork domain and the Network domain). Any guidance or suggestions you could provide would be very helpful, thank you!

@eugeneo I need your advice on how to properly define and implement the custom NodeNetwork domain. My understanding is that the V8 inspector doesn't support the Network domain, so Node.js needs to support it. Additionally, Node.js should have the custom NodeNetwork domain to handle Node-specific events and commands (e.g., requestReceived) and allow ecosystems to utilize the inspector infrastructure. However, I'm still unsure the best way to proceed and would greatly appreciate your input. Below is a draft architecture overview I have in mind. Thank you for your assistance :)

image

cola119 avatar Jun 27 '24 07:06 cola119

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.

GrinZero avatar Jun 27 '24 11:06 GrinZero

We could add fetch support via https://github.com/nodejs/undici/pull/2701.

Generically I think we should add some APIs to let devs integrate 3rd party clients.

mcollina avatar Jun 28 '24 09:06 mcollina

+1 to this being diagnostics_channel based and +1 for letting userland tools integrate with it.

benjamingr avatar Jun 28 '24 11:06 benjamingr

This PR is ready for review. I'm less familiar with C++ :(, so your guidance would be appreciated.

cola119 avatar Jul 02 '24 14:07 cola119

Any idea if incoming (server) requests can actually be represented in the dev tools UI? If not, perhaps we can reach out to the dev tools team and see if it would be possible to add a way to capture and differentiate incoming requests?

Qard avatar Jul 02 '24 20:07 Qard

Chrome DevTools did not have any support internally to tell incoming vs outgoing requests. I would not expect that had changed since.

eugeneo avatar Jul 02 '24 21:07 eugeneo

We could use a --experimental-network-inspection flag to indicate its experimental status, which would give us the flexibility for breaking changes since there are still many missing features, such as fetch, websocket support, server app support, which could lead to potential bugs or breaking changes.

cola119 avatar Jul 03 '24 08:07 cola119

CI: https://ci.nodejs.org/job/node-test-pull-request/60057/

nodejs-github-bot avatar Jul 04 '24 08:07 nodejs-github-bot

I am very excited about this feature and would like to ask if you are ready to merge it

GrinZero avatar Jul 05 '24 10:07 GrinZero

CI: https://ci.nodejs.org/job/node-test-pull-request/60095/

nodejs-github-bot avatar Jul 05 '24 12:07 nodejs-github-bot

Technically, we can merge this with one approval, but I want to ensure everything is in order before proceeding.

  • Additional reviews from inspector/C++ experts would be greatly appreciated. (@nodejs/inspector, @nodejs/cpp-reviewers, @nodejs/v8-inspector)
  • Once this is ready to be merged, we need to coordinate with the ChromeDevTools team to enable the network panel for the Node.js inspector.
  • @Qard has added the diag-agenda label. Are there any concerns about merging this PR that we discuss at the next diagnostics working group meeting? Since these meetings are held monthly, this could delay the merge by up to a month if there are any issues to address.

cola119 avatar Jul 06 '24 07:07 cola119

CI: https://ci.nodejs.org/job/node-test-pull-request/60134/

nodejs-github-bot avatar Jul 07 '24 05:07 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/60167/

nodejs-github-bot avatar Jul 08 '24 17:07 nodejs-github-bot

  • Are there any concerns about merging this PR that we discuss at the next diagnostics working group meeting?

I think it's fine to land as an MVP of sorts. I mainly wanted to bring this to the Diag WG call to discuss if anything can be done about supporting server requests too and if we can get some help from dev tools folks to have a way to differentiate incoming requests from outgoing. Could also think about supporting the WebSocket client as dev tools can capture those too. That can all be follow-up work though.

Qard avatar Jul 08 '24 19:07 Qard

Consider my comments non-blocking if you want to make it happen before the diag WG meeting; though it seems fine to me to just use that meeting as FYI and exploring ideas instead of using it to set timelines, since the meeting only happens monthly.

joyeecheung avatar Jul 08 '24 19:07 joyeecheung

The https://github.com/nodejs/node/labels/notable-change label has been added by @cola119.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

github-actions[bot] avatar Jul 09 '24 13:07 github-actions[bot]