Skip to content

report: add --report-exclude-network option#51645

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
Ethan-Arrowood:fix/process-getreport
Mar 1, 2024
Merged

report: add --report-exclude-network option#51645
nodejs-github-bot merged 1 commit intonodejs:mainfrom
Ethan-Arrowood:fix/process-getreport

Conversation

@Ethan-Arrowood
Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood commented Feb 2, 2024

New option --report-exclude-network, also available as report.excludeNetwork, enables the user to exclude networking interfaces in their diagnostic report. On some systems, this can cause the report to take minutes to generate so this option can be used to optimize that.

Fixes: #46060

@nodejs-github-bot 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 Feb 2, 2024
@Ethan-Arrowood Ethan-Arrowood marked this pull request as ready for review February 2, 2024 03:21
@Ethan-Arrowood
Copy link
Contributor Author

Ethan-Arrowood commented Feb 2, 2024

I'm not used to the tests not being in either sequential or parallel. I add some more tests that verify the header.networkInterfaces is missing when this option is set (either by CLI or configuration). I used new test syntax and what not so hopefully that still works in the test/report/ directory.

@anonrig anonrig self-requested a review February 2, 2024 03:39
@Ethan-Arrowood Ethan-Arrowood force-pushed the fix/process-getreport branch 2 times, most recently from 8341898 to 1f12672 Compare February 2, 2024 04:18
@Ethan-Arrowood Ethan-Arrowood added the report Issues and PRs related to process.report. label Feb 2, 2024
@Ethan-Arrowood Ethan-Arrowood changed the title report: add --report-disable-network option report: add --report-network-disabled option Feb 5, 2024
Ethan-Arrowood added a commit to Ethan-Arrowood/node that referenced this pull request Feb 5, 2024
Adds a new option `process.report.networkDisabled` and
cli option `--report-network-disabled` which will disable
any netowkring operations for the `report` generation.

Fixes: nodejs#46060

PR-URL: nodejs#51645
Ethan-Arrowood added a commit to Ethan-Arrowood/node that referenced this pull request Feb 5, 2024
Adds a new option `process.report.networkDisabled` and
cli option `--report-network-disabled` which will disable
any netowkring operations for the `report` generation.

Fixes: nodejs#46060

PR-URL: nodejs#51645
@anonrig anonrig added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Feb 5, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2024

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @anonrig.

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.

@Ethan-Arrowood
Copy link
Contributor Author

@anonrig I'm getting a segmentation fault now (after I made the change Joyee recommended). Trying to debug but this is hard

@Ethan-Arrowood Ethan-Arrowood added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 15, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51645
✔  Done loading data for nodejs/node/pull/51645
----------------------------------- PR info ------------------------------------
Title      report: add `--report-exclude-network` option (#51645)
Author     Ethan Arrowood  (@Ethan-Arrowood)
Branch     Ethan-Arrowood:fix/process-getreport -> nodejs:main
Labels     c++, semver-minor, lib / src, notable-change, needs-ci, report
Commits    1
 - report: add `--report-exclude-network` option
Committers 1
 - Ethan Arrowood 
PR-URL: https://github.com/nodejs/node/pull/51645
Fixes: https://github.com/nodejs/node/issues/46060
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51645
Fixes: https://github.com/nodejs/node/issues/46060
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - report: add `--report-exclude-network` option
   ℹ  This PR was created on Fri, 02 Feb 2024 03:17:12 GMT
   ✔  Approvals: 2
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/51645#pullrequestreview-1864004600
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/51645#pullrequestreview-1881176781
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-02-15T15:18:08Z: https://ci.nodejs.org/job/node-test-pull-request/57100/
- Querying data for job/node-test-pull-request/57100/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/7923747889

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 15, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

New option `--report-exclude-network`, also available as
`report.excludeNetwork`, enables the user to exclude
networking interfaces in their diagnostic report.
On some systems, this can cause the report to take minutes
to generate so this option can be used to optimize that.

Fixes: nodejs#46060

PR-URL: nodejs#51645

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2024
@nodejs-github-bot
Copy link
Collaborator

@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 27, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Ethan-Arrowood
Copy link
Contributor Author

If you like at all of the CI runs for this PR it has only failed flaky tests. There is nothing failing consistently, so I believe this is safe to land

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51645
✔  Done loading data for nodejs/node/pull/51645
----------------------------------- PR info ------------------------------------
Title      report: add `--report-exclude-network` option (#51645)
Author     Ethan Arrowood  (@Ethan-Arrowood)
Branch     Ethan-Arrowood:fix/process-getreport -> nodejs:main
Labels     c++, semver-minor, lib / src, notable-change, needs-ci, report
Commits    1
 - report: add `--report-exclude-network` option
Committers 1
 - Ethan Arrowood 
PR-URL: https://github.com/nodejs/node/pull/51645
Fixes: https://github.com/nodejs/node/issues/46060
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51645
Fixes: https://github.com/nodejs/node/issues/46060
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - report: add `--report-exclude-network` option
   ℹ  This PR was created on Fri, 02 Feb 2024 03:17:12 GMT
   ✔  Approvals: 2
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/51645#pullrequestreview-1864004600
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/51645#pullrequestreview-1881176781
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-03-01T16:27:31Z: https://ci.nodejs.org/job/node-test-pull-request/57536/
- Querying data for job/node-test-pull-request/57536/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8114994573

@Ethan-Arrowood
Copy link
Contributor Author

Commit-queue failed because I pushed a rebase after latest reviews. Can someone reapprove this please? @anonrig @joyeecheung

@nodejs-github-bot
Copy link
Collaborator

Landed in 009665f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. notable-change PRs with changes that should be highlighted in changelogs. report Issues and PRs related to process.report. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

process.report.getReport(); is potentially very slow

6 participants