Complete code refactor #49

simonepri · 2018-03-17T22:53:43Z

Changes

Sorry for this but I've almost rewrote the entire package. 🙈❤️

Now windows time readings are now correct (time is 100-ns units not in ms)
The history for wmic readings now is a LRU cache that auto-expire after 1 min of inactivity to avoid memory leaks
CPU usage on windows should now be more accurate. If there isn't any history for a given pid, then the measurement is took twice.
The start property of the statistics object has been removed. Actually it was completely wrong. (BREAKING CHANGE)
The time property has been renamed to ctime that means cpu time. (BREAKING CHANGE)
The elapsed has been added. It's the time in ms since the process has been started. (BREAKING CHANGE)
The timestamp property has been added. It's the time at which we took the measure. (BREAKING CHANGE)
The unmonitor and _history functions exported from the index.js has been complitely removed. Now we export just the stat funtion (BREAKING CHANGE)
Tests are re-implemented and improved using ava
Coverage report with nyc added
Over-simplified the readme.md (We may want to re-add some documentation on how it works)
Update .gitignore file
Cleaned package.json file
procfile implementation has been completely removed (We may consider to re-introduce this in the future if we will need it, but now is useless)
The ppid property has been added.

I can understand that maybe is too much but I need this package to work.
If you are unhappy with the changes let me know! Thanks!

You can walk though the code here

+ fix darwin

Fix array when pid has not been found

simonepri · 2018-03-18T21:26:25Z

this package in v1 is working just fine

I don't think so. (Sorry to be rude)
I must agree that your work is the best so far on the npm, but also now after the refactoring is far to be perfect.

and is required by a lot of modules :)

This is the reason why we need to have rock-solid tests for this fundamental module

didn't we agreed on an array?

See https://github.com/soyuka/pidusage/pull/49/files#r175304248

I think that we should make some errors graceful

I've tried do it when possible.
The problem is the nature of pids to disappear on a system, throwing an error is not appropriate.
If the user didn't get a reading then it can understand that that pid is not present in the system.
Obvs we need to document that behaviour!

I'm not fond of the wmic/history thing with the self-invalidation.
This should be left to the developer and it's out of scope.

The developer doesn't know when it's appropriate to clean the history.
That's the reason why it should be completely hidden to the developer.

You should consider history as an internal optimization of the problem that you should need to call wmic twice. Then instead of calling twice always you save the last reading.
But you do it intelligently without wasting memory.

We may want to discuss about the max age time, but I believe this is the way to go.

Last but not least, please don't make me learn a new test framework

What are the advantages of tap?
I believe that the tests would be more complicated to write.

Can you please squash your commits?

Let's end the work and the I will take care of that.
Anyway I believe that most of them are useful to understand the changes.

soyuka · 2018-03-19T08:54:31Z

The problem is the nature of pids to disappear on a system, throwing an error is not appropriate.
If the user didn't get a reading then it can understand that that pid is not present in the system.
Obvs we need to document that behaviour!

Mhh so, if I call pidusage with an array of pids and one of them isn't found. Will this give back partial results AND an error?
I ask because I really need pidusage to return empty statistics if the process has not been found, not to send an error.
Maybe we can pass an option to still get an error when the process does not exist though.

What are the advantages of tap?
I believe that the tests would be more complicated to write.

Less noise, less fancy just good ol' non-breaking working for years stuff :). Sure thing let's keep this fancy ava thing...

Good work on the README it works like that for me!

So left to do:

see the parseTime stuff
Add the wmic link to the comment
Question about the statistics when pid has not been found

Thanks for your work, looks great so far!

soyuka · 2018-03-19T10:03:37Z

lib/ps.js

@@ -7,7 +7,7 @@ var PLATFORM = os.platform()

 function parseTime (timestr, fraction) {
  var time = 0
-  var tpart = timestr.replace(/-|\./g, ':').split(':')
+  var tpart = timestr.split(/-|:|\./)


now yours faster :D

🎊How much is improved?

Close #50

soyuka · 2018-03-19T11:03:45Z

index.js

+  return pify(stats, pids)
+}
+
+module.exports = pidusage


I just did that as well haha. let me rebase... We could've done this in another patch

Sorry! 🙈
This is the biggest PR I ever done.
It was really a pleasure to collaborate with u

simonepri · 2018-03-19T11:07:36Z

Good work on the README it works like that for me!

Should we need to document how it works under the hood?
Like in the previous version?

I really need pidusage to return empty statistics if the process has not been found, not to send an error.

That's the current behaviour if you pass an array of pids.
Instead if you pass a single pid then the error 'No maching pid found' is given.

That's the default behaviour of ps and wmic also

simonepri · 2018-03-19T11:11:38Z

Also lets consider to enables services like codecov and snyk and add their badges to the readme

EG:
https://codecov.io/gh/simonepri/pidtree
https://snyk.io/test/github/simonepri/pidtree?targetFile=package.json

- Now windows time readings are now correct (time is 100-ns units not in ms) - The history for wmic readings now is a LRU cache that auto-expire after 1 min of inactivity to avoid memory leaks - CPU usage on windows should now be more accurate. If there isn't any history for a given pid, then the measurement is took twice. - The start property of the statistics object has been removed. Actually it was completely wrong. (BREAKING CHANGE) - The time property has been renamed to ctime that means cpu time. (BREAKING CHANGE) - The elapsed has been added. It's the time in ms since the process has been started. (BREAKING CHANGE) - The timestamp property has been added. It's the time at which we took the measure. (BREAKING CHANGE) The unmonitor and _history functions exported from the index.js has been complitely removed. Now we export just the stat funtion (BREAKING CHANGE) - Tests are re-implemented and improved using ava - Coverage report with nyc added - Over-simplified the readme.md (We may want to re-add some documentation on how it works) - Update .gitignore file - Cleaned package.json file - procfile implementation has been completely removed (We may consider to re-introduce this in the future if we will need it, but now is useless) - The ppid property has been added. Await for child to be spawned before start the test Extend old readings when reading twice with wmic Improve windows spawn options Sort array before comparison in test Fix child script Add legend to the compatibility table Fix child script Fix unresolved promis in tests Fix test Normalize cpu usage Add PPID Fix timezone calc Add missing base to parseInt and parseFloat Improve parseTime performance Improve bench creation performance Add missing 'use strict' Use single interval for history expiration Fix clearInterval for history Remove duplicated benchmark Improve lint script Fix codestyle Fix compatibility table Fix heading Update authors section Update API section Update usage section Update dependencies constraints Update api section Update readme Update readme Fix related section Fix error message Improve error messages Improve error messages Improve error messages Improve error messages Improve stdout parsing performance Improve documentation Return promise if no callback is provided Close #50

simonepri · 2018-03-19T11:25:02Z

@soyuka can you create a PR with the changes you want on my fork instead of working on your branch
In that way I can follow the changes.
https://github.com/simonepri/pidusage/tree/refactor

Then we will squash commits while merging this.

- Now windows time readings are now correct (time is 100-ns units not in ms) - The history for wmic readings now is a LRU cache that auto-expire after 1 min of inactivity to avoid memory leaks - CPU usage on windows should now be more accurate. If there isn't any history for a given pid, then the measurement is took twice. - The start property of the statistics object has been removed. Actually it was completely wrong. (BREAKING CHANGE) - The time property has been renamed to ctime that means cpu time. (BREAKING CHANGE) - The elapsed has been added. It's the time in ms since the process has been started. (BREAKING CHANGE) - The timestamp property has been added. It's the time at which we took the measure. (BREAKING CHANGE) The unmonitor and _history functions exported from the index.js has been complitely removed. Now we export just the stat funtion (BREAKING CHANGE) - Tests are re-implemented and improved using ava - Coverage report with nyc added - Over-simplified the readme.md (We may want to re-add some documentation on how it works) - Update .gitignore file - Cleaned package.json file - procfile implementation has been completely removed (We may consider to re-introduce this in the future if we will need it, but now is useless) - The ppid property has been added. Commits: Extend old readings when reading twice with wmic Normalize cpu usage Add PPID Fix timezone calc Add missing base to parseInt and parseFloat Improve parseTime performance Improve bench creation performance Use single interval for history expiration Improve lint script Update readme Improve error messages Improve stdout parsing performance Improve documentation Return promise if no callback is provided Close #50

soyuka · 2018-03-19T11:40:00Z

I squashed and merged in #51 !! Thank you very much for the work, I'll open maybe a couple more merge requests this week to keep improving while I'm working on Unitech/pm2#3546 !

simonepri · 2018-03-19T11:54:15Z

@soyuka I cannot see the changes for the readme. I believe that something went wrong while merging

soyuka · 2018-03-19T12:11:27Z

Oh yes thanks, was a mess to rebase because you merged master back to your branch. Prefer rebase next time :). All good now I think!

Note that I removed benchmarks from CI because it was 1,2 minute per number of test versions and that's a lot.

soyuka · 2018-03-19T12:13:00Z

I activated https://codecov.io/gh/soyuka/pidusage not sure if I should configure something

simonepri · 2018-03-19T12:13:52Z

Yes you should add
npm run coverage
inside travis.yml and appveyor.yml
Like this:
https://github.com/simonepri/pidtree/blob/master/.travis.yml#L21
https://github.com/simonepri/pidtree/blob/master/.appveyor.yml#L22
And setup the coverage script
https://github.com/simonepri/pidtree/blob/master/package.json#L49

abluchet and others added 17 commits March 17, 2018 00:57

Fix array when pid has not been found

ba8f4cd

+ fix darwin

doc [ci skip]

0b738bf

Merge pull request #48 from soyuka/fix-missing-pid

2c8e7c2

Fix array when pid has not been found

2.0.1

13c7704

fix case where no output from ps

86f780e

2.0.2

ed4e7dc

test

d886ead

Explicit error

49203ec

2.0.3

e640eae

Update .gitignore

92d6d32

Disable lockfile

f46b8aa

Remove example folder

0e5c7a8

Remove old tests

95eae19

Update package.json

c4d8250

Refactor implementation

fdd08fe

Refactor tests

44663c4

Update documentation

bdfd144

simonepri added the enhancement label Mar 17, 2018

simonepri requested a review from soyuka March 17, 2018 22:53

simonepri added 11 commits March 18, 2018 00:01

Merge remote-tracking branch 'upstream/master' into refactor

2cfb800

Improve badges

eb2bf16

Await for child to be spawned before start the test

a12f910

Extend old readings when reading twice with wmic

9a16ca0

Improve windows spawn options

88d2f9d

Sort array before comparison in test

76f4b4b

Fix child script

6d4d8da

Add legend to the compatibility table

4ef2f1f

Fix child script

8796eed

Fix unresolved promis in tests

c2a3a74

Fix test

be225a0

simonepri added 9 commits March 18, 2018 22:55

Update api section

8fbb23d

Update readme

90b44cf

Update readme

8213557

Fix related section

09b64c3

Fix error message

c6717b8

Improve error messages

c41df56

Improve error messages

e89bbd3

Improve error messages

50de8d4

Improve error messages

2cd6750

simonepri mentioned this pull request Mar 19, 2018

Merge pidtree back into ps-tree? indexzero/ps-tree#30

Open

Improve stdout parsing performance

2807071

soyuka reviewed Mar 19, 2018

View reviewed changes

Improve documentation

6c38476

soyuka approved these changes Mar 19, 2018

View reviewed changes

Return promise if no callback is provided

fb64d82

Close #50

soyuka reviewed Mar 19, 2018

View reviewed changes

soyuka force-pushed the master branch from e640eae to 8dbad5b Compare March 19, 2018 11:37

soyuka closed this Mar 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete code refactor #49

Complete code refactor #49

simonepri commented Mar 17, 2018 •

edited

Loading

simonepri commented Mar 18, 2018 •

edited

Loading

soyuka commented Mar 19, 2018 •

edited

Loading

soyuka Mar 19, 2018

simonepri Mar 19, 2018

soyuka Mar 19, 2018

simonepri Mar 19, 2018 •

edited

Loading

simonepri commented Mar 19, 2018

simonepri commented Mar 19, 2018

simonepri commented Mar 19, 2018 •

edited

Loading

soyuka commented Mar 19, 2018

simonepri commented Mar 19, 2018

soyuka commented Mar 19, 2018 •

edited

Loading

soyuka commented Mar 19, 2018

simonepri commented Mar 19, 2018 •

edited

Loading

Complete code refactor #49

Complete code refactor #49

Conversation

simonepri commented Mar 17, 2018 • edited Loading

Changes

simonepri commented Mar 18, 2018 • edited Loading

soyuka commented Mar 19, 2018 • edited Loading

soyuka Mar 19, 2018

Choose a reason for hiding this comment

simonepri Mar 19, 2018

Choose a reason for hiding this comment

soyuka Mar 19, 2018

Choose a reason for hiding this comment

simonepri Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

simonepri commented Mar 19, 2018

simonepri commented Mar 19, 2018

simonepri commented Mar 19, 2018 • edited Loading

soyuka commented Mar 19, 2018

simonepri commented Mar 19, 2018

soyuka commented Mar 19, 2018 • edited Loading

soyuka commented Mar 19, 2018

simonepri commented Mar 19, 2018 • edited Loading

simonepri commented Mar 17, 2018 •

edited

Loading

simonepri commented Mar 18, 2018 •

edited

Loading

soyuka commented Mar 19, 2018 •

edited

Loading

simonepri Mar 19, 2018 •

edited

Loading

simonepri commented Mar 19, 2018 •

edited

Loading

soyuka commented Mar 19, 2018 •

edited

Loading

simonepri commented Mar 19, 2018 •

edited

Loading