process.cpuUsage() - implementation, doc, tests by pmuellr · Pull Request #6157 · nodejs/node · GitHub

Checklist

tests and code linting passes (make -j4 lint test)
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

current planned commit message

process: add cpuUsage()

The cpuUsage() function will return user and system CPU usage timings for the
current process.

Drive-by fix to process.hrtime() doc to align with preferred non-usage of "you"
in function description.

Affected core subsystem(s)

process module

Description of change

add the function cpuUsage() to process

See discussion from original PR #5565

const please :)

Hm, is there any concrete advantage to splitting up the 64-bit numbers into two lanes and reassembling them in JS? Like, wouldn’t using a Float64Array + doubles be a bit easier and have the same results?

nit: const

Awesome. LGTM with nits.

@addaleax I copied the style of hrtime() for cpuUsage(). I think it may have been possible to combine all the numeric bits into a single large int, because the cpu usage values are to the microsecond precision, instead of nanosecond (like hrtime()). Figured it would be better to copy the style instead of doing something radically different.

@pmuellr Sounds reasonable – was just wondering because it seemed a little odd :)

I'd suggest a CI run so that we confirm uv_getrusage is fully supported across all platforms(should be). Otherwise we might need to add info in that respect to the docs. I can't tell if you can do that so I launched one for you:

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

Should be fine. On Windows only those two fields this PR is using are populated. The unices share the same function, but don't populate same results for some fields. nit: The inline comment could be rephrased, omitting the part that uv_getrusage is exposed.

cc @mscdex who appears to have primarily reviewed the previous PR.

I didn't really review the last PR, I was mostly just seeing if there was interest in having a separate .metrics or similarly-named namespace for memory, cpu, other resource usage.

Just briefly looking over this PR, I'm wondering if it might just be better to drop the extra * 1000 used to keep the same units as hrtime(). I think it would help in the case if a lot of CPU time is used by the process, where it would reach Number.MAX_SAFE_INTEGER much quicker than without the extra multiplication. hrtime() uses nanoseconds because it actually has that kind of resolution, but these CPU usage values do not.

Just a style nit...

if (_cpuUsage(cpuValues) !== 0)
  throw new Error('unable to obtain cpu usage time');

I'd be +1 on not having the additional * 1000 but don't think it's critical.

definitely use const wherever possible :-)

Generally LGTM with a few nits.

@pmuellr I'd probably just make the commit message something like

process: add new `cpuUsage()` method

@mhdawson that build is green, so guess it passes muster?

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

@eljefedelrodeodeljefe

The inline comment could be rephrased, omitting the part that uv_getrusage is exposed.

My usual excuse here - I copied from Hrtime() just above it :-)

https://github.com/pmuellr/node/blob/c9aae2dfad942b60a6d887830319cdd20ec47897/src/node.cc#L2200

@mscdex

I'm wondering if it might just be better to drop the extra * 1000 used to keep the same units as hrtime(). I think it would help in the case if a lot of CPU time is used by the process, where it would reach Number.MAX_SAFE_INTEGER much quicker than without the extra multiplication. hrtime() uses nanoseconds because it actually has that kind of resolution, but these CPU usage values do not.

I think the argument comes down to aligning w/hrtime()'s nano-second resolution, to presumably make life easier for users, or to stay true to the rusage() precision or just optimize things a bit.

I'm siding with making life easier for users.

@pmuellr What does aligning with hrtime() output really gain us though? Are you expecting users to pass values from cpuUsage() to hrtime() or vice-versa?

I've added the proposed commit message in the header of this PR. Currently:

process: add cpuUsage()

The cpuUsage() function will return user and system CPU usage timings for the
current process.

Drive-by fix to process.hrtime() doc to align with preferred non-usage of "you"
in function description.

@mscdex ah, yes, forgot to mention that

One expected uses of cpuUsage() would be to get the % of cpu this process is currently using. You can do that by dividing a cpuUsage() diff from a hrtime() diff. Example:

'use strict'

var startTime  = process.hrtime()
var startUsage = process.cpuUsage()

// spin the CPU for 500 milliseconds
var now = Date.now()
while (Date.now() - now < 500)

var elapTime = process.hrtime(startTime)
var elapUsage = process.cpuUsage(startUsage)

var elapTimeMS = secNSec2ms(elapTime)
var elapUserMS = secNSec2ms(elapUsage.user)
var elapSystMS = secNSec2ms(elapUsage.system)
var cpuPercent = Math.round(100 * (elapUserMS + elapSystMS) / elapTimeMS)

console.log('elapsed time ms:  ', elapTimeMS)
console.log('elapsed user ms:  ', elapUserMS)
console.log('elapsed system ms:', elapSystMS)
console.log('cpu percent:      ', cpuPercent)

function secNSec2ms (secNSec) {
  return secNSec[0] * 1000 + secNSec[1] / 1000000
}

Whenever you're dealing with hrtime(), it seems you will likely be wanting a single number to deal with, which you can determine the precision of yourself. In this case, it converted the tuples into millisecond values. Reused the same conversion function for both hrtime() and cpuUsage() values.

@pmuellr Right, but what I meant was users are not going to be doing process.cpuUsage(startTime) or process.hrtime(startUsage.user).

I think I am still -1 on artificially adjusting the cpu usage values.

@mscdex yes, correct, you'd never mix/match values from cpuUsage() and hrtime() to get diff values.

Still think there's value in having a shared model for these, so you can have a single (tuple) -> value conversion fn that works with both.

But it's not a deal-breaker for me or anything.

Comments should be capitalized and punctuated.

Extra space before =

* fixed whitespace issue on doc/api/process.md * improved test in test/parallel/test-process-cpuUsage.js, as below: changed: assert(result.user != null); assert(result.system != null); to: assert.notEqual(result, null); The next tests below the original asserts called Number.isFinite() on the fields in result, which also return false on null/undefined check, so the removed asserts were dups. But realized we weren't actually checking the result parameter for null/undefined.

Thanks for the comments @santigimeno .

Pushed a new commit.

Using assert.notStrictEqual didn't seem to make sense to me here, as the intent was to check for null || undefined, so ... would have expected maybe assert.notEqual. But looking at the test as a whole, realized is Number.isFinite is already checking null/undefined, and that I wasn't actually checking result in the first place, so removed the two field asserts, replaced with an assert(result) instead.

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

CI looks good except for unrelated build bot failure (passed previously on that bot). Landing...

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>

Landed in 52cb410. Thanks @pmuellr !

Thanks everyone! Great way to end the week.

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>

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>

* assert: `deep{Strict}Equal()` now works correctly with circular references. (Rich Trott) #6432 * debugger: Arrays are now formatted correctly in the debugger repl. (cjihrig) #6448 * deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu) #6550 * net: Introduced a `Socket#connecting` property. (Fedor Indutny) #6404 - Previously this information was only available as the undocumented, internal `_connecting` property. * process: Introduced `process.cpuUsage()`. (Patrick Mueller) #6157 * stream: `Writable#setDefaultEncoding()` now returns `this`. (Alexander Makarenko) #5040 * util: Two new additions to `util.inspect()`: - Added a `maxArrayLength` option to truncate the formatting of Arrays. (James M Snell) #6334 - This is set to `100` by default. - Added a `showProxy` option for formatting proxy intercepting handlers. (James M Snell) #6465 - Inspecting proxies is non-trivial and as such this is off by default. PR-URL: #6557

* assert: `deep{Strict}Equal()` now works correctly with circular references. (Rich Trott) #6432 * debugger: Arrays are now formatted correctly in the debugger repl. (cjihrig) #6448 * deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu) #6550 - Please see our blog post for more info on the security contents of this release: - https://nodejs.org/en/blog/vulnerability/openssl-may-2016/ * net: Introduced a `Socket#connecting` property. (Fedor Indutny) #6404 - Previously this information was only available as the undocumented, internal `_connecting` property. * process: Introduced `process.cpuUsage()`. (Patrick Mueller) #6157 * stream: `Writable#setDefaultEncoding()` now returns `this`. (Alexander Makarenko) #5040 * util: Two new additions to `util.inspect()`: - Added a `maxArrayLength` option to truncate the formatting of Arrays. (James M Snell) #6334 - This is set to `100` by default. - Added a `showProxy` option for formatting proxy intercepting handlers. (James M Snell) #6465 - Inspecting proxies is non-trivial and as such this is off by default. PR-URL: #6557

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>

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>

pmuellr mentioned this pull request Apr 11, 2016

initial stab at process.cpuUsage() #5565

Closed

4 tasks

Fishrock123 reviewed Apr 11, 2016
View reviewed changes

Fishrock123 added semver-minor PRs that contain new features and should be released in the next minor version. process Issues and PRs related to the process subsystem. labels Apr 11, 2016

eljefedelrodeodeljefe reviewed Apr 11, 2016
View reviewed changes

jasnell reviewed Apr 12, 2016
View reviewed changes

trevnorris reviewed Apr 12, 2016
View reviewed changes

santigimeno reviewed Apr 29, 2016
View reviewed changes

jasnell closed this Apr 29, 2016

This was referenced May 4, 2016

Propose v6.1.0 wrong base please ignore #6556

Closed

Propose v6.1.0 (Openssl update) #6557

Merged

nickstanish mentioned this pull request Sep 7, 2016

Using process.cpuUsage to get percentage nodejs/help#283

Closed

nickstanish mentioned this pull request Sep 22, 2016

process.cpuUsage accuracy #8728

Closed

MylesBorins added lts-agenda labels Jan 6, 2017

MylesBorins mentioned this pull request Jan 6, 2017

Potential Semver Minor Backports nodejs/Release#177

Closed

9 tasks

MylesBorins mentioned this pull request Jan 14, 2017

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

Closed

4 tasks

sv2 mentioned this pull request Aug 2, 2017

Add memory and CPU stats slanatech/swagger-stats#8

Closed

+              argument to the function, to get a diff reading.
+              ```js
+              const startUsage  = process.cpuUsage();

process.cpuUsage() - implementation, doc, tests by pmuellr · Pull Request #6157 · nodejs/node · GitHub

Uh oh!

Uh oh!

process.cpuUsage() - implementation, doc, tests - #6157

process.cpuUsage() - implementation, doc, tests#6157pmuellr wants to merge 6 commits intonodejs:masternodejs/node:masterfrom pmuellr:cpuUsage2pmuellr/node:cpuUsage2Copy head branch name to clipboard

Conversation

pmuellr commented Apr 11, 2016

Checklist

current planned commit message

Affected core subsystem(s)

Description of change

Uh oh!

pmuellr commented Apr 11, 2016

Uh oh!

Fishrock123 Apr 11, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 11, 2016

Uh oh!

eljefedelrodeodeljefe Apr 11, 2016

Choose a reason for hiding this comment

Uh oh!

eljefedelrodeodeljefe commented Apr 11, 2016

Uh oh!

pmuellr commented Apr 11, 2016

Uh oh!

addaleax commented Apr 11, 2016

Uh oh!

mhdawson commented Apr 11, 2016

Uh oh!

eljefedelrodeodeljefe commented Apr 11, 2016

Uh oh!

Fishrock123 commented Apr 12, 2016

Uh oh!

mscdex commented Apr 12, 2016

Uh oh!

jasnell Apr 12, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 12, 2016

Uh oh!

jasnell Apr 12, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 12, 2016

Uh oh!

Fishrock123 commented Apr 12, 2016

Uh oh!

pmuellr commented Apr 12, 2016

Uh oh!

pmuellr commented Apr 12, 2016

Uh oh!

pmuellr commented Apr 12, 2016

Uh oh!

mscdex commented Apr 12, 2016

Uh oh!

pmuellr commented Apr 12, 2016

Uh oh!

pmuellr commented Apr 12, 2016

Uh oh!

mscdex commented Apr 12, 2016

Uh oh!

pmuellr commented Apr 12, 2016

Uh oh!

trevnorris Apr 12, 2016

Choose a reason for hiding this comment

Uh oh!

santigimeno Apr 29, 2016

Choose a reason for hiding this comment

Uh oh!

pmuellr commented Apr 29, 2016

Uh oh!

santigimeno commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

pmuellr commented Apr 29, 2016

Uh oh!

process.cpuUsage() - implementation, doc, tests#6157
pmuellr wants to merge 6 commits into
nodejs:masterfrom
pmuellr:cpuUsage2