feat: add new rtt per op field to nfsclient #10787
Conversation
This field will provide the calculation for round-trip time per operation. The current rtt field is the total number of time taken, which is not very useful.
telegraf-tiger
bot
added
the
feat
powersj
added
the
ready for final review
|
Is this PR supposed to be linked to an issue? It seems like a specific fix but I don't see a link |
plugins/inputs/nfsclient/README.md
Outdated
| @@ -62,7 +62,8 @@ If `fullstat` is set, a great deal of additional metrics are collected, detailed | |||
| - ops (integer, count) - The number of operations of this type executed. | |||
| - retrans (integer, count) - The number of times an operation had to be retried (retrans = OP_trans - OP_ops. See nfs_ops below) | |||
| - exe (integer, miliseconds) - The number of miliseconds it took to process the operations. | |||
| - rtt (integer, miliseconds) - The round-trip time for operations. | |||
| - rtt (integer, miliseconds) - The total round-trip time for all operations. | |||
| - rtt_per_op (float, miliseconds) - The round-trip time per operation. | |||
There was a problem hiding this comment.
When I first read this I thought it rtt_per_op would give an exact value for each operation, but after reading the code I understand this is an average rtt per op. It might be more clear for users to add in the docs that this is an average.
| @@ -190,6 +190,10 @@ func (n *NFSClient) parseStat(mountpoint string, export string, version string, | |||
| fields["bytes"] = nline[3] + nline[4] | |||
| fields["rtt"] = nline[6] | |||
| fields["exe"] = nline[7] | |||
| fields["rtt_per_op"] = 0.0 | |||
| if nline[0] > 0 { | |||
| fields["rtt_per_op"] = float64(nline[6]) / float64(nline[0]) | |||
There was a problem hiding this comment.
rtt_per_op is just rtt/ops, right? This is the kind of thing processors are meant to handle. Why not use starlark or something to do this?
There was a problem hiding this comment.
If someone is using the NFS client input plugin, they are probably monitoring their system and are interested in a number of key metrics.
Similarly, in the memory input plugin we calculate used and available percents, in the CPU plugin we calculate all sorts of usage percentages, and in the LVM plugins, we calculate used percentages as well. I feel that the NFS client, knowing your average RTT falls into this same category of something pretty much everyone is going to want to do. Asking every user to do this calculation, when we could add it seems to not be the right user experience.
I can understand not wanting Telegraf to calculate lots of different averages and other metrics, but this seems to fall in line with other monitoring and reporting of key metrics that make it hard not to have it included.
EAR |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 👍 This pull request doesn't change the Telegraf binary size 📦 Click here to get additional PR build artifactsArtifact URLs |
This field will provide the calculation for round-trip time per
operation. The current rtt field is the total number of time taken,
which is not very useful.