8000 processes plugin: Code cleanup + add per-process open files count by rpv-tomsk · Pull Request #1989 · collectd/collectd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

processes plugin: Code cleanup + add per-process open files count #1989

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 29, 2017
Merged

processes plugin: Code cleanup + add per-process open files count #1989

merged 3 commits into from
Mar 29, 2017

Conversation

rpv-tomsk
Copy link
Contributor
@rpv-tomsk rpv-tomsk commented Oct 12, 2016 •
edited
Loading

The procstat_entry_s used in improper way.
Added new type and simplified the code - removed unneeded assignments and values copying.

@rpv-tomsk
8000 Copy link
Contributor Author
rpv-tomsk commented Oct 12, 2016 •
edited
Loading

That is too hard to make changes in this plugin. There is too much unused fields in structures and many unnecessary actions before this cleanup.

What do you think, @octo?

@rpv-tomsk rpv-tomsk force-pushed the processes branch 2 times, most recently from 2d53270 to 9bbcd5f Compare October 12, 2016 16:59
@rpv-tomsk
Copy link
Contributor Author
rpv-tomsk commented Oct 12, 2016 •
edited
Loading

The second commit fixes gaps, caused by terminated processes.
When process terminates, huge value is decreased from counter, causing negative derived value.

1
2

After upgrade (at 07:33):

3

That commit also fix monitoring processes which does not write nothing:

# sleep 100 &
[1] 29996

# cat /proc/29996/io
rchar: 1948
wchar: 0
syscr: 7
syscw: 0
read_bytes: 0
write_bytes: 0
cancelled_write_bytes: 0

In that case ps->io_wchar will be left in -1 value in:

ps->io_wchar   += ((pse->io_wchar == -1)?0:pse->io_wchar);

Later, in ps_submit_proc_list, related type will not be submitted due to ps->io_wchar == -1.

@rpv-tomsk
Copy link
Contributor Author

Added commit here for 'Add per-process open files count #836'.

@rpv-tomsk rpv-tomsk changed the title processes plugin: Code cleanup processes plugin: Code cleanup + add per-process open files count Oct 13, 2016
@rpv-tomsk
Copy link
Contributor Author
rpv-tomsk commented Oct 13, 2016 •
edited
Loading

After I had put this on my production, I found a drop on one of charts (Updated at 07:33):

4

Avg value before update was around ~65 and around ~40 after upgrade.
I have checked this with # while true; do cat /proc/2317/stat && sleep 10; done and "spreadsheet" and found what current value is right. Before upgrade there was collectd-5.5.2 code running.
That change is only at one chart. I have two mysql instances (the second one has less load) monitored same way and by same collectd process, and there is not such drop or rise. Quite strange.

@rpv-tomsk
Copy link
Contributor Author

Changed files to file_handles.

Work finished, please review/merge it.

@rpv-tomsk
Copy link
Contributor Author

ping

@mfournier mfournier self-assigned this Jan 10, 2017
@ggrossetie
Copy link

This feature would be really useful to us.
I can't review the code because I don't know C but let me know if you need help in order to get it merged.

@rpv-tomsk
Copy link
Contributor Author
rpv-tomsk commented Mar 29, 2017 •
edited
Loading

Hi!

I'm just use this (like other my PR) in my production environment, so I don't experience any problems with code quality.

Thanks for your attention and intereset to this patch request.
I will update it to fix merge conflict, then you can try to get it merged.

Pavel Rochnyack added 2 commits March 29, 2017 18:04
@rpv-tomsk
processes plugin: Code cleanup
4d769ad
@rpv-tomsk
processes plugin: Update all counters via ps_update_counter()
68557b1 
Fix counting when matched processes are started and terminated often.
@rpv-tomsk
Copy link
Contributor Author
rpv-tomsk commented Mar 29, 2017 •
edited
Loading

Rebased

Taken the src/processes.c file from cb8683c, 36cfb41, 7c33e5d commits, passed throught clang-format and done new commits.

@rubenk
Copy link
Contributor
rubenk commented Mar 29, 2017

This is Linux-only right? If so, please add that fact to the docs and commit message.

@rpv-tomsk
Copy link
Contributor Author

Hi Ruben!

That change would be enough?

@rubenk
Copy link
Contributor
rubenk commented Mar 29, 2017

Enough for what?

@rpv-tomsk
Copy link
Contributor Author

For this:

This is Linux-only right? If so, please add that fact to the docs and commit message.

@rubenk
Copy link
Contributor
rubenk commented Mar 29, 2017

Ah, I didn't see your change. Github lag I guess. Yes, it is fine.
Thanks Pavel!

@rubenk rubenk merged commit 6c97677 into collectd:master Mar 29, 2017
@rpv-tomsk rpv-tomsk deleted the processes branch March 29, 2017 14:46
@ggrossetie
Copy link

Awesome, thanks @rpv-tomsk and @rubenk

@octo octo added this to the 5.8 milestone Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mfournier mfournier

Labels
Pending feedback
Projects
None yet
Milestone
5.8
Development

Successfully merging this pull request may close these issues.
5 participants
@rpv-tomsk @ggrossetie @rubenk @mfournier @octo
0