Update diskstats for linux kernel 4.19

[pgier]

Handles new stats added in 4.19, along with some minor cleanup/refactoring
changes are base on proposal from @hhoffstaette in issue #1049 with some minor changes
fixes #1049

[SuperQ]

Nice, thanks.

I'm not sure adding the link to the kernel docs on every help line is necessary.

[pgier]

@SuperQ The link to the kernel docs seemed kind of randomly placed before. I wasn't sure which metrics should/shouldn't include it so I just decided to add it to all of them, but I'm happy to change it back if that's better.

[discordianfish]

Nice!

Agree with @SuperQ, let's remove the URL completely and add it as code comment.
If we feel like this needs to be more user facing, I'd rather put it in the README.

Beside that, LGTM

[SuperQ]

I've been thinking now that this exporter is getting closer to 1.0, we could start adding a per-collector documentation page.

[discordianfish]

That might make sense. Maybe though we should use code comments and refer to godoc in the README? Might make it easier to keep it up to date.

[SuperQ]

Yea, Godoc might be ok too. In theory some people use the collector package as a library.

[pgier]

Thanks for the feedback! I added a commit to remove the doc url from the individual metrics.

[discordianfish]

LGTM

[SuperQ]

LGTM

[bobrik]

I would suggest to have a more future proof version. It's ok to have stats that are not know to node_exporter (new kernel, old exporter), it's also on for node_exporter to know about more stats than kernel can report (old kernel, new exporter). Currently only the latter is supported.

How about something like this:

diff --git a/collector/diskstats_linux.go b/collector/diskstats_linux.go
index 8c1e5cc..e8a2237 100644
--- a/collector/diskstats_linux.go
+++ b/collector/diskstats_linux.go
@@ -164,7 +164,6 @@ func NewDiskstatsCollector() (Collector, error) {
 }

 func (c *diskstatsCollector) Update(ch chan<- prometheus.Metric) error {
-       procDiskStats := procFilePath("diskstats")
        diskStats, err := getDiskStats()
        if err != nil {
                return fmt.Errorf("couldn't get diskstats: %s", err)
@@ -176,16 +175,19 @@ func (c *diskstatsCollector) Update(ch chan<- prometheus.Metric) error {
                        continue
                }

-               if len(stats) != len(c.descs) {
-                       return fmt.Errorf("invalid line for %s for %s", procDiskStats, dev)
-               }
+               // Iterate over all known stats
+               for i, desc := range c.descs {
+                       // Stop if this stat is not reported
+                       if i >= len(stats) {
+                               break
+                       }

-               for i, value := range stats {
-                       v, err := strconv.ParseFloat(value, 64)
+                       v, err := strconv.ParseFloat(stats[i], 64)
                        if err != nil {
-                               return fmt.Errorf("invalid value %s in diskstats: %s", value, err)
+                               return fmt.Errorf("invalid value %s in diskstats: %s", stats[i], err)
                        }
-                       ch <- c.descs[i].mustNewConstMetric(v, dev)
+
+                       ch <- desc.mustNewConstMetric(v, dev)
                }
        }
        return nil

[pgier]

This sounds reasonable to me, see #1125

[SuperQ]

Yea, I ended up having to be a little more "future proof" with NFS metrics. The list of NFSv4 metrics varies wildly with kernel versions.

[dswarbrick]

Should that be "discarded" instead of "discard" ?

[pgier]

Good catch, I've updated pr #1125