nfs plugin: Add config options to ignore versions #2430
Conversation
(implemented with commit 64446a6)
|
At my day job I was looking for a way to avoid metrics for NFSv2 hitting a loaded graphite server, today. Looking at src/nfs.c, I thought, I might as well open a PR for this functionality. This is my first PR for collectd, so please have patience and review carefully ;-) |
There was a problem hiding this comment.
Hi @usev6,
thank you very much for opening this PR! Code-wise your change looks good (some nit-picky comments are inline), but in order to have a consistent user-experience I'd like to you rename the IgnoreV _x_ option (details inline).
Best regards,
—octo
src/collectd.conf.pod
Outdated
|
|
||
| =over 4 | ||
|
|
||
| =item B<IgnoreV2> B<false>|B<true> |
There was a problem hiding this comment.
We avoid using negated config options, because the resulting double-negations ("ignore: false!") are unnecessarily hard to read. The typical naming for such options is "Report…", i.e. in your case ReportV2, etc.
src/nfs.c
Outdated
| @@ -31,6 +31,12 @@ | |||
| #include <kstat.h> | |||
| #endif | |||
|
|
|||
| static const char *config_keys[] = {"IgnoreV2", "IgnoreV3", "IgnoreV4"}; | |||
| static int config_keys_num = STATIC_ARRAY_SIZE(config_keys); | |||
| static int ignore_v2 = 0; | |||
There was a problem hiding this comment.
These variables should be of type _Bool.
src/nfs.c
Outdated
| @@ -295,6 +301,19 @@ static kstat_t *nfs4_ksp_client; | |||
| static kstat_t *nfs4_ksp_server; | |||
| #endif | |||
|
|
|||
| static int nfs_config(const char *key, const char *value) { | |||
| if (strcasecmp(key, "IgnoreV2") == 0 && IS_TRUE(value)) | |||
| ignore_v2 = 1; | |||
There was a problem hiding this comment.
Please just assign the valur of IS_TRUE(), i.e. ignore_v2 = IS_TRUE(value). This allows (convoluted but correct) configs such as
<Plugin nfs>
IgnoreV2 true
IgnoreV2 false
</Plugin>
src/nfs.c
Outdated
| @@ -495,18 +514,18 @@ static void nfs_read_linux(FILE *fh, const char *inst) { | |||
| if (fields_num < 3) | |||
| continue; | |||
|
|
|||
| if (strcmp(fields[0], "proc2") == 0) { | |||
| if (strcmp(fields[0], "proc2") == 0 && ignore_v2 == 0) { | |||
There was a problem hiding this comment.
With boolean variables (_Bool), please use !ignore_v2 instead. Here and below.
* Avoid negated config options (s/IgnoreV/ReportV/) * Use _Bool instead of int * Handle multiple occurences of config option correctly
|
Hi @octo , thanks a lot for your comments! I changed my PR accordingly and I fully agree that the resulting code is clearer. Best regards, |
No description provided.