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 statistics from /proc/self/mountstats for nfs clients (like iostat -n) #188

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ymettier
Copy link
Contributor

Hello,

On a nfs client, run "iostat -n" and have a look on /proc/self/mountstats...

This patch parses /proc/self/mountstats and get statistics about all NFS mountpoints.

Note : patch against release 5.1.1 is in perfwatcher/collectd-pw in branch patch-nfs_stats_with_network_plugin/5.1.1 if you are also interested in it.

Regards,
Yves

@jgoldschrafe
Copy link

I'm running this fella now, and one thing I should note is that collectd configuration tends to CamelCase, while your configuration uses lowercase_with_underscores -- this should probably be cleaned up from a user experience perspective. enable_client_stats_per_mountpoint is a really verbose config option too -- maybe something more like EnableMountStats would be better?

@jgoldschrafe
Copy link

This patch is causing coredumps for me on RHEL 6.4:

(gdb) backtrace
#0  0x0000003b8ec328a5 in raise () from /lib64/libc.so.6
#1  0x0000003b8ec34085 in abort () from /lib64/libc.so.6
#2  0x0000003b8ec6ffe7 in __libc_message () from /lib64/libc.so.6
#3  0x0000003b8ed01d47 in __fortify_fail () from /lib64/libc.so.6
#4  0x0000003b8ecffc30 in __chk_fail () from /lib64/libc.so.6
#5  0x0000003b8ecfeefb in __strncpy_chk () from /lib64/libc.so.6
#6  0x00002b79fbf21d3d in strncpy (m=0x2b7a101fdc40) at /usr/include/bits/string3.h:121
#7  duplicate_mountstats (m=0x2b7a101fdc40) at nfs.c:826
#8  dispatch_mountstats (m=0x2b7a101fdc40) at nfs.c:911
#9  0x00002b79fbf227a0 in parse_proc_self_mountstats () at nfs.c:965
#10 0x00002b79fbf2314e in nfs_read () at nfs.c:1346
#11 0x000000000040ee6a in plugin_read_thread (args=<value optimized out>) at plugin.c:454
#12 0x0000003b8f807851 in start_thread () from /lib64/libpthread.so.0
#13 0x0000003b8ece811d in clone () from /lib64/libc.so.6

@ymettier
Copy link
Contributor Author

@jgoldschrafe : could you provide more information ? What signal number does it crash with ? Does it crash after a long time (more than 10 minuntes) or when you start it ? Does it crash after you just mounted/unmounted some NFS share ?
If you know gdb a little, could you check how strncpy was called ? Here is the code :

strncpy(dest->op[dest->nb_op].op_name, src->op[i].op_name, sizeof(src->op[i].op_name));

What do those vars contain ? dest, dest->nb_op, dest->op[dest->nb_op].op_name, src, i, src->op[i].op_name ?

Regards,
Yves

@ymettier
Copy link
Contributor Author

@jgoldschrafe : I agree with you about configuration file in CamelCase style. Please check commit 27dc489.

@kronn
Copy link

kronn commented Jan 23, 2014

I am very much looking forward to this. Can anyone give an update on the status of this PR?

@pyr
Copy link
Member

pyr commented Jul 28, 2014

This patch cannot be merged due to conflict, closing it for now, do no hesitate to re-open when you have a mergeable patch, thanks again!

@pyr pyr closed this Jul 28, 2014
@ymettier
Copy link
Contributor Author

ymettier commented Aug 4, 2014

Hello,

I updated my patch (https://github.com/ymettier/collectd/tree/ym/nfs_self_mountstats) and it does not conflict any more.

It seems that I'm not allowed to reopen. Could you do it for me ? Or should I create a new PR ?

@mfournier mfournier reopened this Aug 4, 2014
@mfournier
Copy link

Thanks for updating the code @ymettier ! I reopened the issue so that the conversation isn't lost.

@mfournier mfournier removed the Patch label Aug 4, 2014
@jedblack
Copy link

Any chance this could get merged? This is /exactly/ what we are searching to accomplish. NFS stats per mount point on the client side. Would make life much easier to have this merged upstream.

@jgoldschrafe
Copy link

@jedblack: If it's any help, Diamond has a collector that can do this in the meantime.

@ymettier
Copy link
Contributor Author

ymettier commented Jul 3, 2015

Hello,

I updated my patch. You can merge again on master branch.
Note to maintainers (@mfournier ?) : in PR #1109, I forked master and applied the same patch. So maybe you prefer to merge PR #1109 instead of this one.

Regards,
Yves

@mfournier mfournier modified the milestone: Features Jan 21, 2016
@jakubjedelsky
Copy link

Hi there, is there any chance to merge this feature to upstream? We will test it in a near future, so I can post some info if needed.

@collectd-bot collectd-bot changed the base branch from master to main July 3, 2020 09:41
@octo
Copy link
Member

octo commented Aug 3, 2020

Hi,

there is clearly interest in this to get merged. Is anybody willing to brush up the pull request so it merges cleanly and passes CI?

Thanks and best regards,
—octo

@ymettier
Copy link
Contributor Author

ymettier commented Jan 1, 2021

Hello,

Sorry to answer so late. Sorry too because I'm not working on collectd and NFS any more : I will not be able to update my patch.

Please feel free to fork it and update it to make it work.

Regards,
Yves

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants