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

process: expose uv_rusage on process.resourceUsage() #28018

Open
wants to merge 1 commit into
base: master
from

Conversation

@vmarchaud
Copy link
Contributor

commented Jun 2, 2019

As discussed in nodejs/diagnostics#161,
the core should expose important metrics about the runtime, this PR's
goal is to let user get the number of io request made, and lower level
metrics like the page faults and context switches.

The make -j4 test are not passing locally because of a failing test test/parallel/test-worker-prof.js but i believe it's not related to my changes.
Concerning the tests, i didn't really tested the output of the function mainly because it might make the test flaky depending on the platform and runtime behavior.

This is my first PR on the core, so i might have forgotten some requirements.

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
Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved src/node_process_methods.cc Outdated
@Trott

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

This is my first PR on the core, so i might have forgotten some requirements.

Looks like you didn't forget anything. Thanks for the pull request and welcome!

@nodejs-github-bot

This comment has been minimized.

@vmarchaud vmarchaud force-pushed the vmarchaud:resources-usage branch 3 times, most recently from dfdcae7 to 2eff3d1 Jun 2, 2019

@addaleax addaleax removed the errors label Jun 2, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

Any reason this would make test-worker-prof less reliable? It's failing a lot in CI here, but I'm not sure if it's because of the changes here or something else?

Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved doc/api/process.md
-->

* Returns: {Object}
* `minflt` {integer}

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jun 2, 2019

Contributor

I'm not sure if it matters at the JS level, but these are all unsigned integers.

Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved test/parallel/test-resources-usage.js Outdated
Show resolved Hide resolved test/parallel/test-resources-usage.js Outdated

@vmarchaud vmarchaud force-pushed the vmarchaud:resources-usage branch 4 times, most recently from 315c0e1 to 3f0d72b Jun 3, 2019

@bnoordhuis
Copy link
Member

left a comment

LGTM. My comments are not blockers.

Show resolved Hide resolved doc/api/process.md
Show resolved Hide resolved doc/api/process.md Outdated
@cjihrig

cjihrig approved these changes Jun 3, 2019

Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved doc/api/process.md Outdated
Show resolved Hide resolved test/parallel/test-resource-usage.js Outdated

@vmarchaud vmarchaud changed the title process: expose uv_rusage on process.resourcesUsage() process: expose uv_rusage on process.resourceUsage() Jun 5, 2019

@vmarchaud vmarchaud force-pushed the vmarchaud:resources-usage branch from 3f0d72b to 68947b7 Jun 5, 2019

@vmarchaud

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I've switched to full names, added the mapping between node names and libuv ones. Also exposed all the fields even if not supported as @bnoordhuis said.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

What benefit do we have by exposing entries that are not supported on any platform?

@addaleax

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@BridgeAR Future-proofing and standards conformance would be the two big pro arguments, I imagine?

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

@vmarchaud vmarchaud force-pushed the vmarchaud:resources-usage branch 2 times, most recently from d6374b6 to 7bed043 Jun 5, 2019

@vmarchaud

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I believe the PR is good to be merged expect if someone has a blocker on the exposed fields ?

fields[12] = rusage.ru_msgrcv;
fields[13] = rusage.ru_nsignals;
fields[14] = rusage.ru_nvcsw;
fields[15] = rusage.ru_nivcsw;

This comment has been minimized.

Copy link
@jasnell

jasnell Jun 10, 2019

Member

nit: this would be a bit more readable/maintainable if these were named constants instead of just literal index numbers

This comment has been minimized.

Copy link
@vmarchaud

vmarchaud Jun 13, 2019

Author Contributor

Not sure how to do that exactly, i never really coded in C++

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

@vmarchaud vmarchaud force-pushed the vmarchaud:resources-usage branch from 7bed043 to b582b90 Jun 13, 2019

process: expose uv_rusage on process.resourcesUsage()
As discussed in nodejs/diagnostics#161,
the core should expose important metrics about the runtime, this PR's
goal is to let user get the number of io request made, and lower level
mertrics like the page faults and context switches.

@vmarchaud vmarchaud force-pushed the vmarchaud:resources-usage branch from b582b90 to d9579de Jun 18, 2019

@vmarchaud

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

I've rebased on master hoping that the test on worker-prof will pass now

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@vmarchaud

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

I've checked the test failures and it doesn't seems to be related to this PR, what are the next steps in this case ?

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

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.