Feat/network #282

khanghoang · 2019-07-06T08:56:27Z

Add the network tab to ndb

Preview JSON response

Preview image

Log request body

googlebot · 2019-07-06T08:56:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

alexkozy · 2019-07-08T17:55:02Z

It looks exciting. Thanks for doing this! Do you think that it is ready for review or are you going to polish it first?

khanghoang · 2019-07-08T21:38:50Z

@ak239 thanks for checking. It's definitely not ready (yet) for being reviewed. I'm going to refactor the code.

But it's gonna be great if you can check to see whether you like the approach/feature in the meantime.

googlebot · 2019-07-08T21:39:02Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-13T19:50:37Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

khanghoang · 2019-07-13T19:56:09Z

@ak239 I think this PR is ready for review 🎉

josepharhar

Thanks for your work! I tried it out with this simple script to make a request to debug on Ubuntu 18.04 with node v10.16.0:

const http = require('http');

(async () => {
  const options = new URL('http://google.com');
  console.log('making http request to: ' + options);

  const req = http.request(options, res => {
    console.log('response: ' + res.statusCode + ' ' + JSON.stringify(res.headers, null, 2));

    let data = '';
    res.on('data', chunk => data += chunk);
    res.on('end', () => {
      console.log('response body:\n' + data);
    });
    res.on('error', error => {
      console.log('error reading response body: ' + error);
    });
  });

  req.on('error', error => {
    console.log('error sending request: ' + error);
  });
  req.end();
})();

But I got this error when I ran it with ndb:

making http request to: http://google.com/
/home/jarhar/ndb/lib/preload/ndb/httpMonkeyPatching.js:15
  Object.keys(req.headers).reduce((acc, k) => {
         ^

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at formatRequestHeaders (/home/jarhar/ndb/lib/preload/ndb/httpMonkeyPatching.js:15:10)
    at ClientRequest.res (/home/jarhar/ndb/lib/preload/ndb/httpMonkeyPatching.js:51:18)
    at Object.onceWrapper (events.js:286:20)
    at ClientRequest.emit (events.js:198:13)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:556:21)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:109:17)
    at Socket.socketOnData (_http_client.js:442:20)
    at Socket.emit (events.js:198:13)
    at addChunk (_stream_readable.js:288:12)

khanghoang · 2019-07-13T23:58:08Z

@josepharhar good catch. I will look into it.

khanghoang · 2019-07-14T00:28:37Z

@josepharhar I fixed the issue. But the preview tabs doesn't seem to work with html response. Will double check it this evening.

alexkozy

Thank you for your effort! We need one more big refactoring before we can merge it. Feel free to ask any questions.

alexkozy · 2019-07-14T04:17:25Z

front_end/ndb/NdbMain.js

@@ -5,14 +5,20 @@
 */

 Ndb.nodeExecPath = function() {
-  if (!Ndb._nodeExecPathPromise)
-    Ndb._nodeExecPathPromise = Ndb.backend.which('node').then(result => result.resolvedPath);
+  if (!Ndb._nodeExecPathPromise) {


please undo these formatting changes, it will simplify review.

alexkozy · 2019-07-14T04:19:00Z

front_end/ndb/NdbMain.js

@@ -172,9 +198,21 @@ Ndb.NodeProcessManager = class extends Common.Object {
  static async create(targetManager) {
    const manager = new Ndb.NodeProcessManager(targetManager);
    manager._service = await Ndb.backend.createService('ndd_service.js', rpc.handle(manager));
+    InspectorFrontendHost.sendMessageToBackend = manager.sendMessageToBackend.bind(manager);


This line was removed in master and I believe that you can remove it..

alexkozy · 2019-07-14T04:19:20Z

front_end/ndb/NdbMain.js

+   *
+   * @return {Promise} void
+   */
+  async sendMessageToBackend(message) {


.. and this function.

alexkozy · 2019-07-14T04:35:52Z

lib/preload/ndb/httpMonkeyPatching.js

@@ -0,0 +1,153 @@
+const zlib = require('zlib');


We need to use another way to inject this script, and another channel for inspector node process to speak with DevTools frontend. We already have channel between node process and frontend - devtools protocol.

To inject this script please do following:

add httpMonkeyPatchingSource method to backend.js, this method returns source of this file.

Ndb.NodeProcessManager.detected method gets source of this script and inject it to the page using following snippet:

target.runtimeAgent().invoke_evaluate({ expression: await Ndb.backend.httpMonkeyPatchingSource() });

After these steps we can inject monkey patching script to any inspected process. Second step is how we can build a channel. We can use Runtime.evaluate with awaitPromise: true flag to build a channel. Monkey patching script gets following code:

let messages = []; let messageAdded = null; // this function is our instrumentation, to report anything to frontend - call it instead of process.send function reportMessage(message) { messages.push(message); if (messageAdded) { setTimeout(messageAdded, 0); messageAdded = null; } } // this function should be called from frontend in the loop using `target.runtimeAgent().invoke_evaluate` process._getNetworkMessages = async function() { if (!messages.length) await new Promise(resolve => messageAdded = resolve); return messages.splice(0); } }

Frontend calls in the loop following code:

while (true) { const messages = await target.runtimeAgent().invoke_evaluate({ expression: 'process._getNetworkMessages()', awaitPromise: true }); // ... process these messages ... }

Feel free to ask any questions! At the same time we can merge your pull request and I will refactor it.

alexkozy · 2019-07-14T04:36:53Z

services/ndd_service.js

      NODE_PATH: `${process.env.NODE_PATH || ''}${path.delimiter}${path.join(__dirname, '..', 'lib', 'preload')}`,
      NDD_IPC: this._pipe
    };
  }

+  sendMessage(rawMessage) {


please take a look on my previous comment to avoid changes in ndd_service.js.

khanghoang · 2019-07-14T06:48:56Z

@ak239 thanks for such detailed comments. I will address all of them.

khanghoang · 2019-07-14T09:29:26Z

front_end/ndb/NdbMain.js

+      // this doesnt work
+      // target.runtimeAgent().invoke_evaluate({
+      //   expression: `
+      //     const zlib = require('http');


@ak239 Is the script below will be evaluated in the inspected script's context? I followed your suggestion but it didn't work (I think it's because of the import)...

The easiest way to get proper require available is calling Runtime.evaluate with includeCommandLineAPI flag:

target.runtimeAgent().invoke_evaluate({ expression: `require('zlib')`, includeCommandLineAPI: true });

I think this was what I missed. I will try it.

It did work 🎉

khanghoang · 2019-07-14T09:30:56Z

front_end/ndb/NdbMain.js

+      // but this does
+      // target.runtimeAgent().invoke_evaluate({
+      //   expression: `
+      //     console.log('foo');


.. this doesn't have import and it worked. I could see the console in the ndb log, but the script's context was VMxx. Therefore, I'm not sure we could monkey patch the http/https request this way. Please correct me if I'm wrong.

khanghoang · 2019-07-15T03:40:56Z

front_end/ndb/InspectorFrontendHostOverrides.js

+    if (parsedMes.method !== 'Network.getResponseBody')
+      return;
+
+    const mes = await target.runtimeAgent().invoke_evaluate({


@ak239 how can I have target.runtimeAgent() here?...

I believe that we need some help from @josepharhar. We need to find some nicer way to avoid hacking network model.

In theory we can register our own network model, that will implement most stuff on top of runtime agent. Please take a look on NodeWorker or NodeRuntime models. There are examples how we can add custom models, in this case we will add our own NetworkModel implementation and I hope that DevTools frontend will use it as is.

@josepharhar do you have any suggestion how we can do it better and will it work if node target will just have own NetworkModel?

@khanghoang could you rebase your change on top of latest master? It has separated Ndb.Connection class. Alternative idea that will work to me, add Ndb.Connection.addInterceptor({sendRawMessage: function(string):boolean}). If interceptor is added than Ndb.Connection.sendRawMessage first tries to process message using one of interceptor and iff all interceptors return false will send this message to this._channel.

For Network you can add network interceptor in Ndb.NodeProcessManager.detected, something like:

Ndb.NetworkInterceptor = class { constructor() { this._buffer = []; this._target = null; } sendRawMessage(message) { if (!JSON.parse(message).method.startsWith('Network.')) return false; if (!this._target) this._buffer.push(message); else // do logic } setTarget(target) { this._target = target; // do logic with buffer here } } // inside detected const connection = await Ndb.Connection.create(channel); const interceptor = new Ndb.NetworkInterceptor(); connection.addInterceptor(interceptor); // createTarget can send messages - it is why we need to cache them in buffer. const target = this._targetManager.createTarget( info.id, userFriendlyName(info), SDK.Target.Type.Node, this._targetManager.targetById(info.ppid) || this._targetManager.mainTarget(), undefined, false, connection); interceptor.setTarget(target);

I think having our own NetworkManager/NetworkModel for node sounds like a great idea, and seems like a step in the right direction of eventually forking the entire network panel. If I understand correctly, this means that instead of implementing the CDP interface of Network, we will instead be implementing the public methods of SDK.NetworkManager and emitting the events it emits in order to talk to the network panel. This sounds good to me because its closer to the network panel itself and is probably simpler. I'm not sure exactly how to do this within ndb, but I'm sure you know how @ak239

@khanghoang I created proof of concept of network interceptor - feel free to use it - I hope that it covers your use case - 5aef915

@ak239 @josepharhar thanks a lot. I will check it.

khanghoang · 2019-07-15T03:41:10Z

front_end/ndb/InspectorFrontendHostOverrides.js

+  };
+
+  while (true) {
+    const message = await target.runtimeAgent().invoke_evaluate({


... and here as well?

googlebot · 2019-07-23T03:59:58Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-29T03:15:08Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

khanghoang · 2019-07-29T03:34:17Z

@ak239 I have been trying to debug it but couldn't figure it out myself. Sometimes, it works

The other times, it doesn't

It looks like the for some reason, my this._cacheRequests is an empty array even though I add the response to it each time.
The reason I have to store the responses in the frontend code is that this callback function doesn't work if I have async/await (and I need await the responses from the httpMoneyPatching.js). Therefore, storing the responses in the frontend makes sense for me.

Besides that, I didn't fully understand some parts of the code. I think I suppose to use this method to send the response body for the preview tab. I tried but it didn't work, I debugged and the "sessions/callbacks" array is empty. But calling this works.

Also, I need to register a callback when the user clicks on the item in the requests list. I use this but I don't think it's the proper way.

Any suggestions?

alexkozy

I debugged DevTools code and found that NetworkManager by default uses the main target Network domain. In the case of ndb main target is not a real target, it means that I need to add interceptor for this target and interceptor code should manually fetch all available targets and send a command to all of them and get responses from all of them. I will prepare an interceptor this week.

It is the main reasons why you are observing strange issues with implementation.

khanghoang · 2019-08-11T06:53:13Z

@ak239 did you have a chance to take a look at it?

khanghoang · 2019-09-16T06:12:57Z

@ak239 I want to follow up on this PR. Let me know what else I can do.

khanghoang · 2019-10-01T21:01:09Z

@ak239 ping you one last time.

khanghoang · 2019-10-31T21:41:34Z

@ak239 I know you're super busy. Please let me know if you want me to make any code changes.
FYI I use ndb almost every day and really appreciate your work on it.

quantuminformation · 2019-11-01T11:38:59Z

Good work guys, this will make all node devs life easier! Keep it up!

cfrank · 2019-11-15T00:26:32Z

What is the status on reviewing this and potentially getting it merged? I would really appreciate this feature being added to ndp. Thank!

jacob-israel-turner · 2019-12-23T17:00:10Z

Love this PR - would be thrilled to see it merged!

0x7An · 2020-02-24T22:01:32Z

This Network tab is what i`ve been looking for years, thanks @khanghoang for this, hope this get merged soon.

khanghoang · 2020-03-06T19:15:21Z

hey folks, you can try this feature today via a custom build by running npx ndb-plus <node_script>

kumavis · 2020-08-11T03:50:30Z

In the case of ndb main target is not a real target, it means that I need to add interceptor for this target and interceptor code should manually fetch all available targets and send a command to all of them and get responses from all of them.

@ak239 do you need assistance with the interceptor?

superhit0 · 2020-09-08T09:33:18Z

Hey Guys, any update on this PR?

@khanghoang I am getting this error on ndb-plus, works fine on ndb (using yarn)

khanghoang · 2020-09-21T05:54:08Z

@superhit0 it's because macOS has changed the permission type names :(

superhit0 · 2020-09-21T08:07:20Z

@khanghoang probably yeah

alnorris · 2021-03-25T10:52:37Z

Any update on this?

GrinZero · 2022-03-31T17:00:54Z

我觉得这蛮重要的...为什么不更新呢

khanghoang mentioned this pull request Jul 6, 2019

Network tab #12

Open

khanghoang force-pushed the feat/network branch from 3a1376d to 8b045f6 Compare July 13, 2019 19:50

khanghoang marked this pull request as ready for review July 13, 2019 19:52

khanghoang force-pushed the feat/network branch from 8b045f6 to fcae7c9 Compare July 13, 2019 19:59

josepharhar reviewed Jul 13, 2019

View reviewed changes

Add network tab

3ac3d9c

khanghoang force-pushed the feat/network branch from fcae7c9 to 3ac3d9c Compare July 14, 2019 00:27

alexkozy suggested changes Jul 14, 2019

View reviewed changes

khanghoang added 2 commits July 14, 2019 01:52

un-format NdbMain.js

04f071a

could not monkey patch http/https

d357732

khanghoang commented Jul 14, 2019

View reviewed changes

khanghoang added 4 commits July 14, 2019 14:38

use another approach to money patch http/https request

33e6242

Refactor the logic to send data from backend to frontend

1ed024a

Refactor code

be3c73c

Refactor code

9b51ded

khanghoang commented Jul 15, 2019

View reviewed changes

Remove unused code

77b7cfe

khanghoang force-pushed the feat/network branch from 3510d99 to 77b7cfe Compare July 15, 2019 03:41

Remove this and the network tab no longer shows the requests

f99b51a

khanghoang force-pushed the feat/network branch 4 times, most recently from f53a5c6 to 918828b Compare July 29, 2019 03:12

khanghoang added 3 commits July 28, 2019 20:14

feat(interceptor): proof-of-concept for network interception

db2de6e

it works now

74ae0e5

polish the code

971761b

khanghoang force-pushed the feat/network branch from 918828b to 971761b Compare July 29, 2019 03:14

alexkozy reviewed Jul 31, 2019

View reviewed changes

kudryavchik approved these changes Sep 29, 2019

View reviewed changes

jkrems mentioned this pull request Oct 25, 2019

Support Network Inspection nodejs/diagnostics#75

Open

jkrems mentioned this pull request Jan 31, 2022

Network request inspection nodejs/node-inspect#52

Closed

Feat/network #282

Are you sure you want to change the base?

Feat/network #282

Conversation

khanghoang commented Jul 6, 2019 • edited Loading

googlebot commented Jul 6, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

alexkozy commented Jul 8, 2019

khanghoang commented Jul 8, 2019

googlebot commented Jul 8, 2019

googlebot commented Jul 13, 2019

khanghoang commented Jul 13, 2019

josepharhar left a comment

Choose a reason for hiding this comment

khanghoang commented Jul 13, 2019

khanghoang commented Jul 14, 2019

alexkozy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanghoang commented Jul 14, 2019

khanghoang Jul 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanghoang Jul 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkozy Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jul 23, 2019

googlebot commented Jul 29, 2019

khanghoang commented Jul 29, 2019 • edited Loading

alexkozy left a comment

Choose a reason for hiding this comment

khanghoang commented Aug 11, 2019

khanghoang commented Sep 16, 2019

khanghoang commented Oct 1, 2019

khanghoang commented Oct 31, 2019

quantuminformation commented Nov 1, 2019

cfrank commented Nov 15, 2019

jacob-israel-turner commented Dec 23, 2019

0x7An commented Feb 24, 2020

khanghoang commented Mar 6, 2020

kumavis commented Aug 11, 2020

superhit0 commented Sep 8, 2020 • edited Loading

khanghoang commented Sep 21, 2020

superhit0 commented Sep 21, 2020

alnorris commented Mar 25, 2021

GrinZero commented Mar 31, 2022

khanghoang commented Jul 6, 2019 •

edited

Loading

khanghoang Jul 14, 2019 •

edited

Loading

khanghoang Jul 14, 2019 •

edited

Loading

alexkozy Jul 16, 2019 •

edited

Loading

khanghoang commented Jul 29, 2019 •

edited

Loading

superhit0 commented Sep 8, 2020 •

edited

Loading