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.

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

feat: add new rtt per op field to nfsclient #10787

Merged
merged 2 commits into from
Mar 11, 2022
Merged

feat: add new rtt per op field to nfsclient #10787

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66da75f
feat: add new rtt per op field to nfsclient
powersj Mar 7, 2022
3a144b2
update readme
powersj Mar 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugins/inputs/nfsclient/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, done


In addition enabling `fullstat` will make many more metrics available.

Expand Down
4 changes: 4 additions & 0 deletions plugins/inputs/nfsclient/nfsclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
tags["operation"] = first
acc.AddFields("nfsstat", fields, tags)
}
Expand Down
22 changes: 12 additions & 10 deletions plugins/inputs/nfsclient/nfsclient_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,12 @@ func TestNFSClientProcessStat(t *testing.T) {
require.NoError(t, err)

fieldsReadstat := map[string]interface{}{
"ops": uint64(600),
"retrans": uint64(1),
"bytes": uint64(1207),
"rtt": uint64(606),
"exe": uint64(607),
"ops": uint64(600),
"retrans": uint64(1),
"bytes": uint64(1207),
"rtt": uint64(606),
"exe": uint64(607),
"rtt_per_op": float64(1.01),
}

readTags := map[string]string{
Expand All @@ -119,11 +120,12 @@ func TestNFSClientProcessStat(t *testing.T) {
acc.AssertContainsTaggedFields(t, "nfsstat", fieldsReadstat, readTags)

fieldsWritestat := map[string]interface{}{
"ops": uint64(700),
"retrans": uint64(1),
"bytes": uint64(1407),
"rtt": uint64(706),
"exe": uint64(707),
"ops": uint64(700),
"retrans": uint64(1),
"bytes": uint64(1407),
"rtt": uint64(706),
"exe": uint64(707),
"rtt_per_op": float64(1.0085714285714287),
}

writeTags := map[string]string{
Expand Down