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

Add proportional resident memory size #142

Merged
merged 3 commits into from
May 18, 2020
Merged

Conversation

PierreF
Copy link
Contributor

@PierreF PierreF commented Apr 8, 2020

This PR fix #140 by adding two additional memory_bytes memtype:

  • proportionalResident
  • proportionalSwapped

I've added a command flag to enable/disable smaps information gathering as this cause notable CPU impact (when gathering all 480 processes - and 3000 threads - on my laptop: without process-exporter consume ~9% of CPU, with it consume ~30% of CPU). It's disabled by default.

Edit: updated to use prometheus/procfs/pull/286. This has much lower CPU impact.
Edit: It still include #141 so it may be merged after #141 (or supersede it).

This is WIP because:
* It include #141 which should be merged before (or #141 declined and only this one merged)
* It rely on prometheus/procfs#280 and prometheus/procfs#281. Before those PRs are merged, it's using my own fork of procfs.

I'll update this PR when other PRs get merged.

@PierreF
Copy link
Contributor Author

PierreF commented May 6, 2020

Updated to drop the replace in go.mod.
Like for #141 I switch to the commit which merged prometheus/procfs#286

I've kept the toggle flag, but the CPU cost if now much lower since procfs use smaps_rollup. I've run three process-exporter with and without the -gather-smaps flag and the last is before using smaps_rollup (e.g. using prometheus/procfs#281) :

  • Without gathering smaps, process-exported used 25.7 seconds of CPU (for 25 collections)
  • With gathering smaps (using smaps_rollup), it used 34.3 seconds of CPU (still for 25 collections)
  • With gathering smaps (not using smaps_rollup), it used 88.9 seconds of CPU (still for 25 collections)

Note that the last case should occur with older Linux kernel that don't implement smaps_rollup (before 4.15.x).
I would say since the toggle is implemented, let's keep it.

@PierreF PierreF changed the title [WIP] Add proportional resident memory size Add proportional resident memory size May 6, 2020
@ncabatoff
Copy link
Owner

This looks great, thanks! Please merge in master, then we can merge this and I'll build a new release.

@PierreF
Copy link
Contributor Author

PierreF commented May 17, 2020

Rebased (cherry-picked) the commit on master and fixed the two comment. Hope it's this way you though the options struct.

@ncabatoff ncabatoff merged commit d14dfc6 into ncabatoff:master May 18, 2020
@ncabatoff
Copy link
Owner

Thanks again @PierreF!

@ncabatoff
Copy link
Owner

I can't build a release yet because some target platforms aren't supported by the tip of procfs (see prometheus/procfs#294, https://app.circleci.com/pipelines/github/ncabatoff/process-exporter/8/workflows/97bdbcfe-2b97-464e-b846-60d327641e2f/jobs/57) but I'll keep an eye on it and hopefully have one out this week.

@PierreF PierreF mentioned this pull request Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose proportional resident memory size
2 participants