[collectd 6] disk plugin: Align metrics with OpenTelemetry recommendations.

[octo]

• Rename labels to system.device and disk.io.direction.
• Rename system.disk.time to system.disk.operation_time.
• Add descriptions and units to all metric families.
• Add the "utilization" metric to FreeBSD.

ChangeLog: Disk plugin: The metric schema has been aligned with OpenTelemetry semantic conventions.

[eero-t]

In general code changes look OK, but:

• I think it would be better to handle disk: add /proc/diskstats fields 15 to 20 in KERNEL_LINUX #4242 in main, and merge related main changes to disk.c before this PR.

• Reporting (some) time units as seconds (s) is IMHO not good while collectd counters are integers i.e. do not support subsecond timings. Meaning that one does not see from counters very small usage spikes i.e. is disk completely idle or not.

• Time manipulation for values appended to fam_ops_time is confusing as they can be multiplied or divided by different values, and variable names do not indicate their time units (like they IMHO should).

[octo]

The v6 code is so different that I don't think resolving the resulting merge conflict is any easier than reimplementing the feature. Writing the metric families is more effort than plumbing the counters through.

Ok, fair enough.

I have changed all platforms to report microseconds, and changed the metric family unit to reflect that.
...
I have added appropriate suffixes to all variable and field names.

Thanks, it's much more readable now!

(I.e. only remaining item is odd fam_disk_io_time -> fam_ops_time change.)

[eero-t]

Value wrap-up calculations in disk.c are broken, see: grep UINT_MAX disk.c

They deduct 64-bit (signed) derive_t value from 32-bit (unsigned) UINT_MAX value.

Maybe collectd should have DERIVE_T_MAX define for this (both in v5 & v6 branches)?

[eero-t]

Approved. I'll file separate bug about the UINT_MAX vs derive_t issue, as it's already in the v5 version of disk plugin, and not introduced by these changes.

[octo]

Value wrap-up calculations in disk.c are broken, see: grep UINT_MAX disk.c

They deduct 64-bit (signed) derive_t value from 32-bit (unsigned) UINT_MAX value.

Maybe collectd should have DERIVE_T_MAX define for this (both in v5 & v6 branches)?

That's an excellent callout, thank you! We actually have a function that is doing this right, counter_diff(). I ended up refactoring a lot of the Linux code; being one of the oldest pieces of code in collectd it was quite in need of a facelift.

After a lot of fiddling I figured out that we calculated the average time per operation … just for that variable never to be read. I think the existing metrics are much better at conveying useful information about the disk, so I remove the calculcation and made the plugin a good deal simpler.

We also unnecessarily stored a bunch of stuff in global state, which I have now removed.

[eero-t]

Gah, read Linux diskstats documentation, and counters use native unsigned long (see the new bug).

I.e. they are indeed 64-bit, but only on 64-bit host. For 32-bit hosts, UINT_MAX was actually the correct maximum value.

[eero-t]

Nice, counter_diff() already handles transparently unsigned 32-bit vs 64-bit wrap up => it will fix #4245 for v6.

The very best changes are ones that remove lots of code, and still manage retain the (relevant) functionality. :-)

On (very) quick check of the changes, they seemed OK, i.e. from my side this still seems fine for merging.

[eero-t]

As to adding tests for disk.c (in another PR)...

I think that could be done for the Linux functionality by extracting that from disk_read() to another function, one that takes path to /proc/diskstats as an argument. Test code can then ask it to read pre-prepared regular file(s) instead, to verify that reported metric updates are as expected, and to verify that invalid file content is handled gracefully.

(CPU plugin test coverage could be similarly extended to parsing of /proc/stats file. At that point some helper functions for dealing with temporary test files could be useful.)

[octo]

As to adding tests for disk.c (in another PR)...

I think we need to separate I/O and parsing. Maybe a schema along the lines of:

int parse(char const *data, metric_family_t families[]);

int read_callback(void) {
  char *buffer = read_text_file_contents();
  parse(buffer);
}

This will allow us to test parse() without using I/O.

We should also split plugins into multiple files, one for each operating system we support. E.g. disk.c, disk_linux.c, disk_freebsd.c etc. The current approach is hard to read and hard to maintain.