collector/ethtool: Expose NIC channel configuration#3514
collector/ethtool: Expose NIC channel configuration#3514BarbarossaTM wants to merge 1 commit intoprometheus:masterfrom
Conversation
|
@SuperQ What do you think? :-) |
| return res, err | ||
| } | ||
|
|
||
| func (e *EthtoolFixture) GetChannels(intf string) (ethtool.Channels, error) { |
There was a problem hiding this comment.
The ethtool library already implements this. Please use it.
https://pkg.go.dev/github.com/safchain/ethtool#Ethtool.GetChannels
There was a problem hiding this comment.
Thanks for the review!
For querying the actual values, I'm using this, but I'm not sure how I should use that for the mock? There I adopted the existing approach also used to mock the other Ethtool methods.
Do you want me to rework the mock part? If so, could you please elaborate on what you have in mind?
There was a problem hiding this comment.
Ahh, sorry, I misunderstood the use here. I didn't realize that we had a complex mock test setup for this collector.
I'm not sure how useful this is, as it's not really mocking the syscalls that the real library does. I need to look over the ethtool_linux_test.go again, but it looks like it's not really a good way to test this.
There was a problem hiding this comment.
@SuperQ Did you have a chance to take a look and have thoughts on where to go from here?
collector/ethtool_linux.go
Outdated
| } | ||
|
|
||
| channels, err := c.ethtool.GetChannels(device) | ||
| if err == nil { |
There was a problem hiding this comment.
I'd suggest moving this to a function, then check for the error first and return early.
There was a problem hiding this comment.
Sure thing, like this?
When running a large fleet of machines running network-heavy loads and with different hardware configurations, it is important to get an overview about their configuration. Ethtool exporter already exposes a lot of useful info like port configuration, driver, and obviously stats, however did not expose the channel configuration. This commit adds two new metrics exposing the current and maximum supported channel configuration, with each metric exposing one instance for combined, other, rx, and tx. Signed-off-by: Maximilian Wilhelm <maximilian.wilhelm@hetzner-cloud.de>
BarbarossaTM
force-pushed
the
ethtool/channels
branch
from
d0555fb to
6084d55
Compare
|
Hm, is the FreeBSD test broken somehow? |
3 participants
When running a large fleet of machines running network-heavy loads and with different hardware configurations, it is important to get an overview about their configuration. Ethtool exporter already exposes a lot of useful info like port configuration, driver, and obviously stats, however did not expose the channel configuration.
This commit adds two new metrics exposing the current and maximum supported channel configuration, with each metric exposing one instance for combined, other, rx, and tx.
I'd consider this a fairly trivial improvement, so directly opening a PR, hope that's OK 😁