Add flag to ignore network speed if it is unknown

[SuperQ]

Some devices (ex virtual) don't have a speed and report -1 as the
speed value. Add a flag to allow ignoring speed on these devices.

Fixes: #1967

Signed-off-by: Ben Kochie superq@gmail.com

[SuperQ]

CC @svalouch

[roidelapluie]

Thanks, do you think it is test-worthy?

[SuperQ]

Added to the end-to-end test.

[roidelapluie]

LGTM

[svalouch]

This will cause metrics to disappear once the cable has been unplugged from a bare-metal server, which means that existing alerts that (ab)use this metric to report both wrong speed (e.g. drop from 10→1Gbit/s or lower due to faulty cable) as well as cable being unplugged / transceiver becoming faulty (value <= 0) need to be adjusted, I'm fine with that (just stating it in case someone does this like we did previously and finds this PR).

However, this is not consistent with what ethtool reports, which returns Unknown! for both 0 and -1. I'd prefer to have a consistent result from node_exporter if it starts to interpret the value (as opposed to simply applying math as before) that is consistent with what standard-tools report. So, either omit the metric or report a value that can be easily identified as "unknown" for value <= 0. The value was consistently 0 before release 1.0.0-rc.0, but I'm fine with omitting it for both values.

I've just found a couple of VMs that report 0 and I have no clue as to why (comparing them to others uncovered no obvious (virtual) hardware differences), so there indeed are cases where it does report 0 in the wild. So with this patch applied in its current state, we'd have the majority of VMs without that metric and some with that metric being 0.

[SuperQ]

We already have node_network_carrier for explicit link checks. By making the metric vanish, you can now do queries like node_network_carrier unless node_network_speed_bytes to find interfaces that have an "unknown" value.

[SuperQ]

A good example of this is lo. When you try and access the speed info, you get an error.

$ cat /sys/class/net/lo/speed 
cat: /sys/class/net/lo/speed: Invalid argument

The node_exporter sees this as a nil from the procfs library and doesn't expose a speed metric value.

[svalouch]

By making the metric vanish, you can now do queries like node_network_carrier unless node_network_speed_bytes to find interfaces that have an "unknown" value.

Ah, sounds like making it vanish is the superior solution, cool! Still, I'd like to see 0 treated the same for reasons stated above:

-if ifaceInfo.Speed != nil && *ifaceInfo.Speed >= 0 {
+if ifaceInfo.Speed != nil && *ifaceInfo.Speed > 0 {

[SuperQ]

Alright, let me look over a few things. You're probably right that we should treat 0 as unknown.

[discordianfish]

LGTM but technically, this again is a breaking change.. As @svalouch said, I can definitely see people alerting on this.

[SuperQ]

So, this sucks a bit. We can't fix this without breaking existing use of the output. The only way to make it possible is to either cut this as part of node_exporter 2.0. Or add a feature flag to enable this change.

[discordianfish]

Sounds like a good compromise!