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

process: add process.cpuUsage() - implementation, doc, tests #10796

Closed
wants to merge 1 commit into from
Closed

process: add process.cpuUsage() - implementation, doc, tests #10796

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Jan 14, 2017 •
edited

Add process.cpuUsage() method that returns the user and system
CPU time usage of the current process

This is a backport of #6157 to v4.x-staging

/cc @nodejs/lts @pmuellr

notes: the original commit made changes in internal/bootstrap_node and internal/process... in v4.x these changes are all applied to src/node

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
Affected core subsystem(s)

src

@MylesBorins
process: add process.cpuUsage() - implementation, doc, tests
0782a5b 
Add process.cpuUsage() method that returns the user and system
CPU time usage of the current process

PR-URL: nodejs#6157
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@MylesBorins MylesBorins added lib / src Issues and PRs related to general changes in the lib or src directory. v4.x labels Jan 14, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v4.x labels Jan 14, 2017
@MylesBorins MylesBorins mentioned this pull request Jan 14, 2017
Closed
9 tasks
@MylesBorins
Copy link
Member Author

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

@mscdex
Copy link
Contributor

mscdex commented Jan 14, 2017

I have thrown these eyes on this PR: 👀

LGTM though.

thefourtheye
thefourtheye reviewed Jan 14, 2017
View reviewed changes
src/node.js
// numbers <= Number.MAX_SAFE_INTEGER.
function previousValueIsValid(num) {
return Number.isFinite(num) &&
num <= Number.MAX_SAFE_INTEGER &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Number.isSafeInteger here?

thefourtheye
thefourtheye reviewed Jan 14, 2017
View reviewed changes
test/parallel/test-process-cpuUsage.js

// Ensure that the return value is the expected shape.
function validateResult(result) {
assert.notEqual(result, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notStrictEqual

thefourtheye
thefourtheye reviewed Jan 14, 2017
View reviewed changes
test/pummel/test-process-cpuUsage.js
@@ -0,0 +1,30 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this test is in pummel?

@thefourtheye
Copy link
Contributor

Ah, never mind the comments. Just noticed that its a backport. LGTM if CI is happy.

jasnell pushed a commit that referenced this pull request Jan 16, 2017
@jasnell
process: add process.cpuUsage() - implementation, doc, tests
151cca6 
Backport to v4.x

Original commit message:
  Add process.cpuUsage() method that returns the user and system
  CPU time usage of the current process

  PR-URL: #6157
  Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

PR-URL: #10796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 16, 2017

Landed in v4.x-staging in 151cca6

@jasnell jasnell closed this Jan 16, 2017
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@MylesBorins
process: add process.cpuUsage() - implementation, doc, tests
bf3a67c 
Backport to v4.x

Original commit message:
  Add process.cpuUsage() method that returns the user and system
  CPU time usage of the current process

  PR-URL: #6157
  Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

PR-URL: #10796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@MylesBorins
process: add process.cpuUsage() - implementation, doc, tests
7a9c8d8 
Backport to v4.x

Original commit message:
  Add process.cpuUsage() method that returns the user and system
  CPU time usage of the current process

  PR-URL: #6157
  Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

PR-URL: #10796
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 21, 2017
MylesBorins added a commit that referenced this pull request Feb 21, 2017
@MylesBorins
2017-02-21, Version 4.8.0 'Argon' (LTS)
58ab54f 
Notable Changes:

* child_process: add shell option to spawn() (cjihrig)
  #4598
* crypto:
  * add ALPN Support (Shigeki Ohtsu)
    #2564
  * allow adding extra certs to well-known CAs (Sam Roberts)
    #9139
* deps:
  * v8: expose statistics about heap spaces (Ben Ripkens)
    #4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
  #5333
* process:
  * add `externalMemory` to `process` (Fedor Indutny)
    #9587
  * add process.cpuUsage() (Patrick Mueller)
    #10796
MylesBorins added a commit that referenced this pull request Feb 22, 2017
@MylesBorins
2017-02-21, Version 4.8.0 'Argon' (LTS)
49c5ff4 
Notable Changes:

* child_process: add shell option to spawn() (cjihrig)
  #4598
* crypto:
  * add ALPN Support (Shigeki Ohtsu)
    #2564
  * allow adding extra certs to well-known CAs (Sam Roberts)
    #9139
* deps:
  * v8: expose statistics about heap spaces (Ben Ripkens)
    #4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
  #5333
* process:
  * add `externalMemory` to `process` (Fedor Indutny)
    #9587
  * add process.cpuUsage() (Patrick Mueller)
    #10796

PR-URL: #10973
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-02-21, Version 4.8.0 'Argon' (LTS)
bddd5cc 
    Notable Changes:

    * child_process: add shell option to spawn() (cjihrig)
      nodejs/node#4598
    * crypto:
      * add ALPN Support (Shigeki Ohtsu)
        nodejs/node#2564
      * allow adding extra certs to well-known CAs (Sam Roberts)
        nodejs/node#9139
    * deps:
      * v8: expose statistics about heap spaces (Ben Ripkens)
        nodejs/node#4463
    * fs: add the fs.mkdtemp() function. (Florian MARGAINE)
      nodejs/node#5333
    * process:
      * add `externalMemory` to `process` (Fedor Indutny)
        nodejs/node#9587
      * add process.cpuUsage() (Patrick Mueller)
        nodejs/node#10796

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
@imyller
2017-02-21, Version 4.8.0 'Argon' (LTS)
2ee49f0 
    Notable Changes:

    * child_process: add shell option to spawn() (cjihrig)
      nodejs/node#4598
    * crypto:
      * add ALPN Support (Shigeki Ohtsu)
        nodejs/node#2564
      * allow adding extra certs to well-known CAs (Sam Roberts)
        nodejs/node#9139
    * deps:
      * v8: expose statistics about heap spaces (Ben Ripkens)
        nodejs/node#4463
    * fs: add the fs.mkdtemp() function. (Florian MARGAINE)
      nodejs/node#5333
    * process:
      * add `externalMemory` to `process` (Fedor Indutny)
        nodejs/node#9587
      * add process.cpuUsage() (Patrick Mueller)
        nodejs/node#10796

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@MylesBorins MylesBorins deleted the backport-cpuUsage-v4.x branch November 14, 2017 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thefourtheye thefourtheye thefourtheye left review comments

@ofrobots ofrobots ofrobots approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@MylesBorins @mscdex @thefourtheye @jasnell @ofrobots @addaleax @nodejs-github-bot