Update diskstats for linux kernel 4.19#1109
Conversation
pgier
force-pushed
the
update-diskstats-for-linux-kernel-4.19
branch
3 times, most recently
from
9f7590e to
6e691d7
Compare
Use a constant for the diskstats filename and for the iostats doc URL. Append the iostats doc URL consistently to the metric doc strings. Signed-off-by: Paul Gier <[email protected]>
The format of /proc/diskstats is changing in linux-4.19 to include some additional fields. See: https://www.kernel.org/doc/Documentation/iostats.txt Signed-off-by: Paul Gier <[email protected]>
Signed-off-by: Paul Gier <[email protected]>
pgier
force-pushed
the
update-diskstats-for-linux-kernel-4.19
branch
from
6e691d7 to
9141986
Compare
|
Nice, thanks. I'm not sure adding the link to the kernel docs on every help line is necessary. |
|
@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. |
|
Nice! Agree with @SuperQ, let's remove the URL completely and add it as code comment. Beside that, LGTM |
|
I've been thinking now that this exporter is getting closer to 1.0, we could start adding a per-collector documentation page. |
|
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. |
|
Yea, Godoc might be ok too. In theory some people use the collector package as a library. |
Signed-off-by: Paul Gier <[email protected]>
|
Thanks for the feedback! I added a commit to remove the doc url from the individual metrics. |
|
|
||
| if len(stats) != len(c.descs) { | ||
| return fmt.Errorf("invalid line for %s for %s", procDiskStats, dev) | ||
| if len(stats) > len(c.descs) { |
There was a problem hiding this comment.
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 nilThere was a problem hiding this comment.
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.
| { | ||
| desc: prometheus.NewDesc( | ||
| prometheus.BuildFQName(namespace, diskSubsystem, "discarded_sectors_total"), | ||
| "The total number of sectors discard successfully.", |
There was a problem hiding this comment.
Should that be "discarded" instead of "discard" ?
fixes, among others, prometheus/node_exporter#1109. Just backporting the patch failed on basically all hunks, so this was far easier. Seems to run stable.
…ormat of /proc/diskstats. Modified version of the upstream patch backported for the older node_exporter base: prometheus#1109
The format of /proc/diskstats is changing in linux-4.19 to include some additional fields. See: https://www.kernel.org/doc/Documentation/iostats.txt * collector/diskstats: use constants for some hard coded strings * collector/diskstats: update diskstats for linux-4.19 * collector/diskstats: remove kernel doc url from individual metrics Signed-off-by: Paul Gier <[email protected]>
5 participants
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