report: fix network queries in getReport libuv with exclude-network #55602

Tofandel · 2024-10-30T13:50:52Z

I also added in the endpoint's report info the keys ip4 or ip6 which contain the ip address of the endpoint (as well as information on which stack it was via the key)

Example of a tcp uv with network (default) (note that the reverse hostname resolving is up to the system dns resolver and some implementation may not resolve 127.0.0.1 to localhost for example on one of the ci server it resolves to test-ibm-rhel9-x64-1.nodejs.cloud)

[
  {
    type: 'tcp',
    is_active: true,
    is_referenced: true,
    address: '0x000055e70fcb85d8',
    localEndpoint: { host: 'localhost', ip4: '127.0.0.1', port: 48986 },
    remoteEndpoint: { host: 'localhost', ip4: '127.0.0.1', port: 38573 },
    sendBufferSize: 2626560,
    recvBufferSize: 131072,
    fd: 24,
    writeQueueSize: 0,
    readable: true,
    writable: true
  },
  {
    type: 'tcp',
    is_active: true,
    is_referenced: true,
    address: '0x000055e70fcd68c8',
    localEndpoint: { host: 'ip6-localhost', ip6: '::1', port: 52266 },
    remoteEndpoint: { host: 'ip6-localhost', ip6: '::1', port: 38573 },
    sendBufferSize: 2626560,
    recvBufferSize: 131072,
    fd: 25,
    writeQueueSize: 0,
    readable: true,
    writable: true
  }
]

Example of a tcp uv without network

[
  {
    type: 'tcp',
    is_active: true,
    is_referenced: true,
    address: '0x000055e70fcb85d8',
    localEndpoint: { host: '127.0.0.1', ip4: '127.0.0.1', port: 48986 },
    remoteEndpoint: { host: '127.0.0.1', ip4: '127.0.0.1', port: 38573 },
    sendBufferSize: 2626560,
    recvBufferSize: 131072,
    fd: 24,
    writeQueueSize: 0,
    readable: true,
    writable: true
  },
  {
    type: 'tcp',
    is_active: true,
    is_referenced: true,
    address: '0x000055e70fcd68c8',
    localEndpoint: { host: '::1', ip6: '::1', port: 52266 },
    remoteEndpoint: { host: '::1', ip6: '::1', port: 38573 },
    sendBufferSize: 2626560,
    recvBufferSize: 131072,
    fd: 25,
    writeQueueSize: 0,
    readable: true,
    writable: true
  }
]

The only difference between the two is that a reverse lookup is not performed on the host and will always contain the ip address without network (this is already the fallback if the reverse lookup fails, so that the change is almost invisible)

It would be nice if eventually getReport could be made to accept options instead of the very weird api with assignements and be made async instead and uv_getnameinfo made in async as well, but that's way too big a task for me to take on (I already barely stitched that PR together with my C++ knowledge which is non existant, I only know C)

RedYetiDev · 2024-10-30T13:55:09Z

Hi! Could you amend your commit to have a valid subsystem? In this case, src.

RedYetiDev · 2024-10-30T14:23:21Z

test/report/test-report-exclude-network.js

+  it('should not do DNS queries in libuv if exclude network', async () => {
+    await fetch('http://127.0.0.1');
+
+    process.report.excludeNetwork = false;
+    let report = process.report.getReport();
+    let tcp = report.libuv.find((uv) => uv.type === 'tcp');
+    assert.notEqual(tcp, null);
+    assert.strictEqual(tcp.localEndpoint.host, 'localhost');
+    assert.strictEqual(tcp.remoteEndpoint.host, 'localhost');
+
+    process.report.excludeNetwork = true;
+    report = process.report.getReport();
+    tcp = report.libuv.find((uv) => uv.type === 'tcp');
+    assert.notEqual(tcp, null);
+    assert.strictEqual(tcp.localEndpoint.host, '127.0.0.1');
+    assert.strictEqual(tcp.remoteEndpoint.host, '127.0.0.1');
+  });


Does this have to go in test/internet?

Good question, it's still a test for getReport and the request is self contained to localhost, so does it make sense in internet?

I don't know where it belongs, we'll see if someone else mentions it.

I don't think this feature access the "internet". It just accesses local network information.

Tofandel · 2024-10-30T15:19:17Z

This should be backported to node 20 and node 22 as well

RedYetiDev · 2024-10-30T15:23:22Z

This should be backported to node 20 and node 22 as well

As long as it's not a semver-major change, which it's not at this point in time, it'll be planned to be backported.

codecov · 2024-10-30T20:47:01Z

Codecov Report

Attention: Patch coverage is 90.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (f67e45e) to head (0ab08f0).
Report is 59 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_report_utils.cc	89.28%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55602      +/-   ##
==========================================
- Coverage   88.44%   88.41%   -0.03%     
==========================================
  Files         654      654              
  Lines      187751   187761      +10     
  Branches    36146    36122      -24     
==========================================
- Hits       166050   166015      -35     
- Misses      14942    14994      +52     
+ Partials     6759     6752       -7

Files with missing lines	Coverage Δ
src/node_report.cc	`93.16% <100.00%> (+0.01%)`	⬆️
src/node_report.h	`100.00% <ø> (ø)`
src/node_report_utils.cc	`80.10% <89.28%> (+5.54%)`	⬆️

... and 60 files with indirect coverage changes

nodejs-github-bot · 2024-10-31T16:11:10Z

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

…e it runs and skip ipv6 if not supported

github-actions · 2024-10-31T20:25:48Z

Failed to start CI

   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/11619348784

nodejs-github-bot · 2024-11-01T00:52:31Z

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

src/node_report_utils.cc

nodejs-github-bot · 2024-11-06T11:54:38Z

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

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 30, 2024

RedYetiDev added the report Issues and PRs related to process.report. label Oct 30, 2024

Tofandel force-pushed the main branch from bd8b10c to 7522af8 Compare October 30, 2024 14:04

Tofandel changed the title ~~fix: network queries in getReport libuv with --report-exclude-network~~ src: network queries in getReport libuv with --report-exclude-network Oct 30, 2024

Tofandel force-pushed the main branch 2 times, most recently from 9d44c7f to dd14b0e Compare October 30, 2024 14:21

RedYetiDev reviewed Oct 30, 2024

View reviewed changes

Tofandel force-pushed the main branch from dd14b0e to b52cd10 Compare October 30, 2024 14:50

Tofandel mentioned this pull request Oct 30, 2024

[Bug?]: Performance impact of process.report.getReport() is undesirable yarnpkg/berry#5167

Closed

1 task

Tofandel force-pushed the main branch from b52cd10 to 148fca5 Compare October 30, 2024 15:18

Tofandel changed the title ~~src: network queries in getReport libuv with --report-exclude-network~~ report: fix network queries in getReport libuv with exclude-network Oct 30, 2024

Tofandel force-pushed the main branch 6 times, most recently from a00c190 to 78b4dfc Compare October 31, 2024 03:41

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2024

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2024

report: fix network queries in getReport libuv with exclude-network

3da21d4

Tofandel force-pushed the main branch 2 times, most recently from 4373985 to f596a70 Compare October 31, 2024 20:24

Don't hardcode hostname or that leads to flaky test depending on wher…

83420e0

…e it runs and skip ipv6 if not supported

Tofandel force-pushed the main branch from f596a70 to 83420e0 Compare October 31, 2024 20:25

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Oct 31, 2024

richardlau added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Nov 1, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2024

Tofandel mentioned this pull request Nov 3, 2024

[BUG] <Freezes during npm install> npm/cli#7814

Open

2 tasks

Koenkk mentioned this pull request Nov 3, 2024

fix: Update Alpine to 3.20 Koenkk/zigbee2mqtt#24622

Open

Ethan-Arrowood self-assigned this Nov 4, 2024

Ethan-Arrowood approved these changes Nov 6, 2024

View reviewed changes

legendecas reviewed Nov 6, 2024

View reviewed changes

src/node_report_utils.cc Show resolved Hide resolved

Bump node report version

a7c18c3

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2024

Tofandel added 4 commits November 6, 2024 13:07

Update documentation

f18b125

Fix json

f4da7ae

Fix report version in test

0ab08f0

Lint md

d796635

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: fix network queries in getReport libuv with exclude-network #55602

report: fix network queries in getReport libuv with exclude-network #55602

Tofandel commented Oct 30, 2024 •

edited

Loading

RedYetiDev commented Oct 30, 2024

RedYetiDev Oct 30, 2024

Tofandel Oct 30, 2024

RedYetiDev Oct 30, 2024

legendecas Nov 6, 2024

Tofandel commented Oct 30, 2024

RedYetiDev commented Oct 30, 2024

codecov bot commented Oct 30, 2024 •

edited

Loading

nodejs-github-bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

nodejs-github-bot commented Nov 1, 2024

nodejs-github-bot commented Nov 6, 2024

report: fix network queries in getReport libuv with exclude-network #55602

Are you sure you want to change the base?

report: fix network queries in getReport libuv with exclude-network #55602

Conversation

Tofandel commented Oct 30, 2024 • edited Loading

RedYetiDev commented Oct 30, 2024

RedYetiDev Oct 30, 2024

Choose a reason for hiding this comment

Tofandel Oct 30, 2024

Choose a reason for hiding this comment

RedYetiDev Oct 30, 2024

Choose a reason for hiding this comment

legendecas Nov 6, 2024

Choose a reason for hiding this comment

Tofandel commented Oct 30, 2024

RedYetiDev commented Oct 30, 2024

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

nodejs-github-bot commented Nov 1, 2024

nodejs-github-bot commented Nov 6, 2024

Tofandel commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading