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.

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

report: add cpu info to report output #28188

Closed
wants to merge 1 commit into from

Conversation

@boneskull
Copy link
Contributor

commented Jun 12, 2019

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 (EDIT: the max theoretical usage is 400% in this case)

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@boneskull

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@boneskull boneskull force-pushed the boneskull:process-report-cpus branch from e703fd1 to 3f87627 Jun 12, 2019

Show resolved Hide resolved doc/api/report.md Outdated

@boneskull boneskull force-pushed the boneskull:process-report-cpus branch from 3f87627 to 6f3ddc2 Jun 12, 2019

@boneskull boneskull removed the request for review from cjihrig Jun 12, 2019

@boneskull boneskull force-pushed the boneskull:process-report-cpus branch from 6f3ddc2 to 6ad680e Jun 12, 2019

Show resolved Hide resolved doc/api/report.md
@Trott

Trott approved these changes Jun 12, 2019

@boneskull boneskull self-assigned this Jun 14, 2019

@boneskull boneskull force-pushed the boneskull:process-report-cpus branch from 6ad680e to b5aa334 Jun 14, 2019

boneskull added a commit to boneskull/node that referenced this pull request Jun 14, 2019

report: add cpu info to report output
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.osCpus` would be breaking.

PR-URL: nodejs#28188
Refs: nodejs/diagnostics#307

@boneskull boneskull force-pushed the boneskull:process-report-cpus branch from b5aa334 to 3c225ee Jun 14, 2019

boneskull added a commit to boneskull/node that referenced this pull request Jun 14, 2019

report: add cpu info to report output
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.osCpus` would be breaking.

PR-URL: nodejs#28188
Refs: nodejs/diagnostics#307
report: add cpu info to report output
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

@boneskull boneskull force-pushed the boneskull:process-report-cpus branch from 3c225ee to 7c2fa6b Jun 14, 2019

@boneskull boneskull removed the author ready label Jun 14, 2019

@boneskull

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Not sure why that check is failing...

@boneskull

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

I'd like to land this, but:

  1. build failure, may be unrelated
  2. unsure if I need to manually add all Reviewed-By lines to the commit message
@addaleax

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@richardlau

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

2. unsure if I need to manually add all `Reviewed-By` lines to the commit message

node-core-utils will do that for you.

@richardlau

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

I have reported one of the OS X Jenkins workers as acting up: nodejs/build#1837

Hopefully someone from @nodejs/build-infra can either fix it or temporarily disconnect it from Jenkins until it can be fixed.

@boneskull

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Do I need to wait on CI fix to land this?

@Trott

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Do I need to wait on CI fix to land this?

Yes. The tests need to run and pass on all the platforms.

@nodejs-github-bot

This comment has been minimized.

Trott added a commit to Trott/io.js that referenced this pull request Jun 16, 2019

report: add cpu info to report output
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>
@Trott

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Landed in 7561a38

@Trott Trott closed this Jun 16, 2019

BridgeAR added a commit that referenced this pull request Jun 17, 2019

report: add cpu info to report output
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>

@BridgeAR BridgeAR referenced this pull request Jun 17, 2019

Merged

v12.5.0 proposal #28268

targos added a commit that referenced this pull request Jun 18, 2019

report: add cpu info to report output
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>
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

This should probably be semver-minor?

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

The report feature is still experimental, and does not follow semver.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@cjihrig thanks!

@BridgeAR BridgeAR removed the semver-minor label Jun 20, 2019

BridgeAR added a commit that referenced this pull request Jun 26, 2019

2019-06-18, Version 12.5.0 (Current)
Notable changes:

This release contains `semver-major` commits. These are in fact not
`semver-major` due to follow-up commits that remove all breaking changes.

* build:
  * The startup time is reduced by enabling V8 snapshots by default
    #28181
* deps:
  * Updated `V8` to 7.5.288.22 #27375
    * The numeric separator (v8.dev/features/numeric-separators) feature is now
      enabled by default
  * Updated `OpenSSL` to 1.1.1c #28211
* inspector:
  * The `--inspect-publish-uid` flag was added to specify ways of the inspector
    web socket url exposure #27741
* n-api:
  * Accessors on napi_define_* are now ECMAScript-compliant
    #27851
* report:
  * The cpu info got added to the report output
    #28188
* src:
  * Restore the original state of the stdio file descriptors on exit to prevent
    leaving stdio in raw or non-blocking mode
    #24260
* tools,gyp:
  * Introduce MSVS 2019 #27375
* util:
  * inspect:
    * Array grouping became more compact and uses more columns than before
      #28059
      #28070
    * Long strings will not be split at 80 characters anymore. Instead they will
      be split on new lines #28055
* worker:
  * `worker.terminate()` now returns a promise and using the callback is
    deprecated #28021

PR-URL: #28268

BridgeAR added a commit that referenced this pull request Jun 26, 2019

2019-06-18, Version 12.5.0 (Current)
Notable changes:

* build:
  * The startup time is reduced by enabling V8 snapshots by default
    #28181
* deps:
  * Updated `V8` to 7.5.288.22 #27375
    * The numeric separator (v8.dev/features/numeric-separators) feature is now
      enabled by default
  * Updated `OpenSSL` to 1.1.1c #28211
* inspector:
  * The `--inspect-publish-uid` flag was added to specify ways of the inspector
    web socket url exposure #27741
* n-api:
  * Accessors on napi_define_* are now ECMAScript-compliant
    #27851
* report:
  * The cpu info got added to the report output
    #28188
* src:
  * Restore the original state of the stdio file descriptors on exit to prevent
    leaving stdio in raw or non-blocking mode
    #24260
* tools,gyp:
  * Introduce MSVS 2019 #27375
* util:
  * inspect:
    * Array grouping became more compact and uses more columns than before
      #28059
      #28070
    * Long strings will not be split at 80 characters anymore. Instead they will
      be split on new lines #28055
* worker:
  * `worker.terminate()` now returns a promise and using the callback is
    deprecated #28021

PR-URL: #28268

BridgeAR added a commit that referenced this pull request Jun 26, 2019

2019-06-26, Version 12.5.0 (Current)
Notable changes:

* build:
  * The startup time is reduced by enabling V8 snapshots by default
    #28181
* deps:
  * Updated `V8` to 7.5.288.22 #27375
    * The numeric separator (v8.dev/features/numeric-separators) feature is now
      enabled by default
  * Updated `OpenSSL` to 1.1.1c #28211
* inspector:
  * The `--inspect-publish-uid` flag was added to specify ways of the inspector
    web socket url exposure #27741
* n-api:
  * Accessors on napi_define_* are now ECMAScript-compliant
    #27851
* report:
  * The cpu info got added to the report output
    #28188
* src:
  * Restore the original state of the stdio file descriptors on exit to prevent
    leaving stdio in raw or non-blocking mode
    #24260
* tools,gyp:
  * Introduce MSVS 2019 #27375
* util:
  * inspect:
    * Array grouping became more compact and uses more columns than before
      #28059
      #28070
    * Long strings will not be split at 80 characters anymore. Instead they will
      be split on new lines #28055
* worker:
  * `worker.terminate()` now returns a promise and using the callback is
    deprecated #28021

PR-URL: #28268

BridgeAR added a commit that referenced this pull request Jun 27, 2019

2019-06-27, Version 12.5.0 (Current)
Notable changes:

* build:
  * The startup time is reduced by enabling V8 snapshots by default
    #28181
* deps:
  * Updated `V8` to 7.5.288.22 #27375
    * The numeric separator (v8.dev/features/numeric-separators) feature is now
      enabled by default
  * Updated `OpenSSL` to 1.1.1c #28211
* inspector:
  * The `--inspect-publish-uid` flag was added to specify ways of the inspector
    web socket url exposure #27741
* n-api:
  * Accessors on napi_define_* are now ECMAScript-compliant
    #27851
* report:
  * The cpu info got added to the report output
    #28188
* src:
  * Restore the original state of the stdio file descriptors on exit to prevent
    leaving stdio in raw or non-blocking mode
    #24260
* tools,gyp:
  * Introduce MSVS 2019 #27375
* util:
  * inspect:
    * Array grouping became more compact and uses more columns than before
      #28059
      #28070
    * Long strings will not be split at 80 characters anymore. Instead they will
      be split on new lines #28055
* worker:
  * `worker.terminate()` now returns a promise and using the callback is
    deprecated #28021

PR-URL: #28268

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 27, 2019

2019-06-27, Version 12.5.0 (Current)
Notable changes:

* build:
  * The startup time is reduced by enabling V8 snapshots by default
    nodejs#28181
* deps:
  * Updated `V8` to 7.5.288.22 nodejs#27375
    * The numeric separator (v8.dev/features/numeric-separators) feature is now
      enabled by default
  * Updated `OpenSSL` to 1.1.1c nodejs#28211
* inspector:
  * The `--inspect-publish-uid` flag was added to specify ways of the inspector
    web socket url exposure nodejs#27741
* n-api:
  * Accessors on napi_define_* are now ECMAScript-compliant
    nodejs#27851
* report:
  * The cpu info got added to the report output
    nodejs#28188
* src:
  * Restore the original state of the stdio file descriptors on exit to prevent
    leaving stdio in raw or non-blocking mode
    nodejs#24260
* tools,gyp:
  * Introduce MSVS 2019 nodejs#27375
* util:
  * inspect:
    * Array grouping became more compact and uses more columns than before
      nodejs#28059
      nodejs#28070
    * Long strings will not be split at 80 characters anymore. Instead they will
      be split on new lines nodejs#28055
* worker:
  * `worker.terminate()` now returns a promise and using the callback is
    deprecated nodejs#28021

PR-URL: nodejs#28268
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.