Enable parsing of '/proc/[pid]/mountstats' #30
Conversation
|
Is it okay to make use of subtests now that Go 1.7 is out? Regarding the CI failure. Could update CI to Go 1.7. |
|
About the Go 1.7 features: This is a library, so we can expect it to be used in certain contexts where older Go compilers are used. Thus, I'd shy away from using Go-1.7 features if I can avoid it. @SuperQ @brian-brazil Is this PR doing what you discussed in prometheus/node_exporter#108 ? @grobie as you are the maintainer of this repo, could you signal if you are able/unable to review this? Thanks everybody. |
|
@beorn7 Yes, it looks OK to me for prometheus/node_exporter#108 |
|
@beorn7 Updated to remove subtests. It appears there is a failure unrelated to my change in CI for Go 1.5. |
|
I'll have some time the next 3 days.
…On Thu, Dec 1, 2016, 10:55 Matt Layher ***@***.***> wrote:
@beorn7 <https://github.com/beorn7> Updated to remove subtests. It
appears there is a failure unrelated to my change in CI for Go 1.5.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#30 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAANaC41zkIxsv2gLmIupDVC4rv-aKdaks5rDvwUgaJpZM4K_dQk>
.
|
|
Hey @grobie, any chance you can look this over soon? |
| fieldTransport11Len = 13 | ||
| ) | ||
|
|
||
| // A Mount is a device mount parsed from /proc/[pid]mountstats. |
There was a problem hiding this comment.
TIL (with optional leading article) is a thing now.
There was a problem hiding this comment.
Looks great! Thank you @mdlayher and sorry for the late review (/me waves from Mexico City).
I added a couple of questions and a few nits, but nothing serious really. Julius added a warning about the unstable API, so if anything comes up we're not in a bad situation anyway.
| fieldTransport11Len = 13 | ||
| ) | ||
|
|
||
| // A Mount is a device mount parsed from /proc/[pid]mountstats. |
| @@ -192,6 +192,18 @@ func (p Proc) FileDescriptorsLen() (int, error) { | |||
| return len(fds), nil | |||
| } | |||
|
|
|||
| // MountStats retrieves statistics and configuration for mount points in a | |||
| // process's name space. | |||
|
|
||
| // Does this mount also possess statistics information? | ||
| if len(ss) > deviceEntryLen { | ||
| // Only NFSv3 and v4 are supported for parsing statistics |
There was a problem hiding this comment.
Do you know other possible types providing statistics? I wonder if we should just leave Stats empty in this case, but being explicit is probably the right decision.
There was a problem hiding this comment.
The manpage mentions:
(4) Optional statistics and configuration information. Currently (as at Linux 2.6.26), only NFS filesystems export information
via this field.
So as far as I can tell, no other filesystem type exposes statistics here.
|
|
||
| // Check for specific words appearing at specific indices to ensure | ||
| // the format is consistent with what we expect | ||
| format := []struct { |
There was a problem hiding this comment.
What do you think, would it make sense to extract this struct to a global variable in order to not rebuild it on function invocation? Or is the compiler already smart enough to do that for us?
Have you thought about using fmt.Scanf here?
There was a problem hiding this comment.
I'll make some benchmarks for this logic and see which approach is fastest.
There was a problem hiding this comment.
https://gist.github.com/mdlayher/0e5db14dd6a2468bab343f2890d499d4
According to these results, moving this to a package variable results in about a 2 nanosecond savings per run. fmt.Sscanf() is nowhere near competitive.
My gut tells me that the very slight speedup is not worth adding a variable used in a single function to the package namespace, but I'd be okay with changing it if requested.
There was a problem hiding this comment.
Sounds good, thanks for checking.
|
Will rebase this once #31 is merged. |
|
Thanks! Given the build error is fixed already in master, in going to merge it. |
Well, or not. |
|
@grobie all fixed! |
|
Thanks again! |
Use `c_char`, which is the proper alias to use with `CStr`
Tested on my system using NFS mounts from my desktop to server. Reduced some of the per-operation stats in the fixture to save time. Kernel is 3.13.0-37.