Skip to content

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

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

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

Conversation

@Ethan-Arrowood

@Ethan-ArrowoodEthan-Arrowood commented Feb 2, 2024

Copy link
Copy Markdown
Contributor

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-botnodejs-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

Ethan-Arrowood commented Feb 2, 2024

Copy link
Copy Markdown
ContributorAuthor

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-Arrowoodforce-pushed the fix/process-getreport branch 2 times, most recently from 8341898 to 1f12672CompareFebruary 2, 2024 04:18
@Ethan-ArrowoodEthan-Arrowood added the report Issues and PRs related to process.report. label Feb 2, 2024
Comment threadsrc/node_options.h Outdated
Comment threadsrc/node_report_module.cc Outdated
Comment threadtest/report/test-report-disable-network.js Outdated
Comment threaddoc/api/report.md Outdated
Comment threadlib/internal/process/report.js Outdated
Comment threadsrc/node_report.cc Outdated
Comment threadsrc/node_report.cc Outdated
Comment threadsrc/node_report.cc Outdated
Comment threadtest/report/test-report-config.js Outdated
@Ethan-ArrowoodEthan-Arrowood changed the title report: add --report-disable-network optionreport: add --report-network-disabled optionFeb 5, 2024
@anonriganonrig 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
Copy Markdown
Contributor

The notable-changePRs 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
Copy Markdown
ContributorAuthor

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

Comment threadsrc/node_report_module.cc Outdated
Comment threaddoc/api/report.md Outdated
@nodejs-github-botnodejs-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
Copy Markdown
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-botnodejs-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
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
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-ArrowoodEthan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Ethan-ArrowoodEthan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@joyeecheungjoyeecheung 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
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Ethan-Arrowood

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 009665f

Sign up for freeto 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 / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.notable-changePRs with changes that should be highlighted in changelogs.reportIssues and PRs related to process.report.semver-minorPRs 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

@Ethan-Arrowood@richardlau@nodejs-github-bot@styfle@anonrig@joyeecheung