Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proposal: add num cpu cores to process.report output #307

Closed
boneskull opened this issue Jun 10, 2019 · 7 comments
Closed

proposal: add num cpu cores to process.report output #307

boneskull opened this issue Jun 10, 2019 · 7 comments

Comments

@boneskull
Copy link

In the report generated by process.report, we have resourceUsage.cpuConsumptionPercent.

As per the original PR, this is the percentage of CPU in use across all cpu cores. The issue I'm raising here didn't seem to be considered--rather the maintainer decided that granular information about each core was not useful (and they are right).

Now, it's been awhile since my computer architecture classes, but what I think this means is that you cannot just look at the number reported and know what it means. A usage of 100% on system A may be a problem, but a usage of 120% on system B might not be.

(We are not guaranteed to be consuming the report on the machine it was generated on, either.)

So, I propose adding header.osCpus, which would be a number corresponding to the output of os.cpus().length. A consumer can then use both of these to determine the theoretical ceiling of the CPU consumption percentage value (multiply header.osCpus by 100).

If it's a good idea, I could implement. If I can do it in lib/internal/process/report.js that'd be even better. 😝
@sam-github
Copy link

Seems pretty reasonable to me, @richardlau, thoughts?

@richardlau
Copy link
Member

Yes, sounds reasonable. One question is whether it would be useful to include more information about the CPU's in the report, e.g. the information returned by uv_cpu_info() (i.e. the contents of the returned uv_cpu_info_t's)?

The report is written out in C++ code -- for the header section see: https://github.com/nodejs/node/blob/04633eeeb93c5a86a6635940eb4f34a7839ada11/src/node_report.cc#L150-L236

@boneskull
Copy link
Author

@richardlau Isn't the cpu time information in the uv_cpu_info_t put into resourceUsage.userCpuSeconds & resourceUsage.kernelCpuSeconds? So it'd be essentially duplicate information?

At any rate, I don't have a use for anything past the number of cores, and could only speculate on what someone would want to do with the extra info here.

@richardlau
Copy link
Member

resourceUsage is obtained from uv_getrusage(): https://github.com/nodejs/node/blob/155caf72bd7972aa46a7b4e8210d13150f8feb43/src/node_report.cc#L423-L452
It may well be that the information is duplicated in the two calls.

The only reason I'm speculating on whether it's worth including the extra information is in terms of the report format (i.e. we could go the simple route and have header.osCpus as the count but then if we wanted to expand the information that would potentially be a format change).

@mhdawson
Copy link
Member

+1 on adding the number of cores to the report.

@boneskull boneskull mentioned this issue Jun 12, 2019
Closed
4 tasks
boneskull added a commit to boneskull/node that referenced this issue Jun 14, 2019
@boneskull
report: add cpu info to report output
7c2fa6b 
The report shows CPU consumption %, but without the number of CPU cores,
a consumer cannot tell if the percent (given across all cores) is
actually problematic. E.g., 100% on one CPU is a problem, but 100% on
four CPUs is not necessarily.

This change adds CPU information (similar to `os.cpus()`) to the report
output. Extra info besides the count is also provided as to avoid future
breaking changes in the eventuality that someone needs it; changing the
datatype of `header.cpus` would be breaking.

PR-URL: nodejs#28188
Refs: nodejs/diagnostics#307
Trott pushed a commit to Trott/io.js that referenced this issue Jun 16, 2019
@boneskull @Trott
report: add cpu info to report output
7561a38 
The report shows CPU consumption %, but without the number of CPU cores,
a consumer cannot tell if the percent (given across all cores) is
actually problematic. E.g., 100% on one CPU is a problem, but 100% on
four CPUs is not necessarily.

This change adds CPU information (similar to `os.cpus()`) to the report
output. Extra info besides the count is also provided as to avoid future
breaking changes in the eventuality that someone needs it; changing the
datatype of `header.cpus` would be breaking.

PR-URL: nodejs#28188
Refs: nodejs/diagnostics#307
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to nodejs/node that referenced this issue Jun 17, 2019
@boneskull @BridgeAR
report: add cpu info to report output
6821055 
The report shows CPU consumption %, but without the number of CPU cores,
a consumer cannot tell if the percent (given across all cores) is
actually problematic. E.g., 100% on one CPU is a problem, but 100% on
four CPUs is not necessarily.

This change adds CPU information (similar to `os.cpus()`) to the report
output. Extra info besides the count is also provided as to avoid future
breaking changes in the eventuality that someone needs it; changing the
datatype of `header.cpus` would be breaking.

PR-URL: #28188
Refs: nodejs/diagnostics#307
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Jun 18, 2019
@boneskull @targos
report: add cpu info to report output
4eaac83 
The report shows CPU consumption %, but without the number of CPU cores,
a consumer cannot tell if the percent (given across all cores) is
actually problematic. E.g., 100% on one CPU is a problem, but 100% on
four CPUs is not necessarily.

This change adds CPU information (similar to `os.cpus()`) to the report
output. Extra info besides the count is also provided as to avoid future
breaking changes in the eventuality that someone needs it; changing the
datatype of `header.cpus` would be breaking.

PR-URL: #28188
Refs: nodejs/diagnostics#307
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mmarchini
Copy link
Contributor

@boneskull nodejs/node#28188 landed, so the number of CPUs can be determined by the number of entries in osCpus. Can we close this issue?

@boneskull
Copy link
Author

yep!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@sam-github @boneskull @mmarchini @richardlau @mhdawson