Complete code refactor - Githubissues

simonepri commented 6 years ago

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

simonepri commented 6 years ago

@soyuka the PR should be ready. Tests are now solid and passing. Maybe we should consider to add tests using Docker to test all the other non tested platforms.

You can find benchmarks for the various platform at the bottom of the test output.

simonepri commented 6 years ago

I've done some performance related improvements. Let me know your opinion.

soyuka commented 6 years ago

I can understand that maybe is too much but I need this package to work.

Everything is fine, note that this package in v1 is working just fine and is required by a lot of modules :).

I like the work thank you for putting time into this! A few general remarks:

didn't we agreed on an array?
I think that we should make some errors graceful, for example if the pid has not been found

With these things in mind a given array should give out the same lengthed array with statistics:

pidusage([1,2,3], function (err, data) {
  // data 
  [
    {pid: 1, memory: 0 ...}, // assuming this pid has not been found, return blank statistics
    ...
  ]
})

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.

Last but not least, please don't make me learn a new test framework :D. I'd be glad to change them back to using tape if you don't mind (I can take of the work no problem!). Can you please squash your commits?

simonepri commented 6 years ago

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 commented 6 years ago

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!

simonepri commented 6 years ago

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 commented 6 years ago

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

simonepri commented 6 years ago

@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.

soyuka commented 6 years ago

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 https://github.com/Unitech/pm2/pull/3546 !

simonepri commented 6 years ago

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

soyuka commented 6 years ago

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 commented 6 years ago

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

simonepri commented 6 years ago

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

soyuka / pidusage

Complete code refactor #49

Changes