Add a new ethtool stats collector#1832
Merged
SuperQ merged 5 commits intoprometheus:masterfrom Jun 16, 2021
ventifus:master
Merged
Add a new ethtool stats collector#1832SuperQ merged 5 commits intoprometheus:masterfrom ventifus:master
SuperQ merged 5 commits intoprometheus:masterfrom
ventifus:master
Conversation
Member
|
Interesting, last time I looked into ethtool metrics, everything required root access. We don't allow collectors that require privileged system access in the node_exporter. It's expected to work fine To answer your other questions:
|
Contributor
Author
|
Thanks for the feedback, I'll modify it so to include a dependency on safchain/ethtool. Likewise switch to UntypedValue. I tested non-root access and the only system I found where it didn't work was CentOS 6. On CentOS 7 and 8 it works fine unprivileged. I'll see if I can find out specifically in which kernel version this changed. |
Member
|
Thanks, documenting where it does/doesn't work is probably good enough. When you think this is ready for more testing, let me know. One thing that would be nice is if we could have tests for this. |
SuperQ
reviewed
Sep 2, 2020
Contributor
Author
|
The removal of CAP_NET_ADMIN from querying ethtool was in commit: 0fdc100bdc4b7ab61ed632962c76dfe539047296 ethtool: allow non-netadmin to query settings The mentioned commit landed first in the 2.6.37, so CentOS 6 just missed out. It's only going to be with us until November, and no other still supported LTS distro I checked (Ubuntu, Debian, SUSE) still runs a 2.x kernel. I'll look into writing tests next. |
Closed
|
This would be a nice feature to have indeed! Would be really nice if it was integrated directly into node_exporter. |
SuperQ
requested changes
Mar 3, 2021
Member
|
Thinking about this some more, I don't like the use of dumping random stats as different labels. Labels are meant to have aggregation meanings, not just tag random things. Similar to other collectors, I think we need to parse the list of returned stats and convert them into meaningful metric names. |
Contributor
Author
|
Thank you for the feedback! Would just concatenating metric names like node_ethtool_stats_d_rx_extra_frags_ring2 be acceptable? It would be challenging to make them conform to the OpenMetrics style suffixes (_count, _sum, etc) since they are driver-dependent. |
Contributor
Author
|
I've changed the metric names to now be a concatenation of "node_ethtool" plus ethtool's stat name. Metrics now look like: How does it look to you now? |
Member
|
How do all metrics look like now? Even better, can you add tests for this? |
Contributor
Author
|
And now there are tests! You can see what the metrics look like in |
|
Almost there folks, lets make it happen! |
…htool -S") Signed-off-by: W. Andrew Denton <git@flying-snail.net>
Contributor
Author
|
I've refactored the tests to use an interface to ethtool to allow swapping out the real tool for the test fixture. What do you think now? The bulk of the fixture stuff does still need to live with the main code to allow it to be activated at runtime for |
|
FYI, kernel 5.13 will support standardized ethtool statistics: It makes use of the new ethtool netlink interface, not the ioctl interface used here. |
Member
|
This still leaks test stuff into the main binary which I'd like to avoid. I don't have time to provide more detailed guides though. Maybe best to ask on the mailinglist for help. |
Contributor
Author
|
Hi @discordianfish , including test stuff into the main binary is required for "end-to-end-test.sh" support. I can remove that code and make the tests entirely separate if that's preferable. |
Signed-off-by: W. Andrew Denton <git@flying-snail.net>
Contributor
Author
|
@idosch thanks for the note, I'll see about getting support for that into mdlayher/ethtool which will let me add them here. |
Member
|
Just realized we have prior art for this. So guess that's fine then for now: node_exporter/collector/wifi_linux.go Line 52 in 46cdf61 |
Contributor
Author
|
Right, the wifi and qdisc collectors are like that. Anything that wants to be runtime testable and needs data outside the /proc and /sys filesystems will need something similar. |
Signed-off-by: W. Andrew Denton <git@flying-snail.net>
Signed-off-by: W. Andrew Denton <git@flying-snail.net>
|
Any ETA this can be pulled into master? |
Member
mrueg
reviewed
Jun 11, 2021
Signed-off-by: W. Andrew Denton <git@flying-snail.net> Co-authored-by: Manuel Rüger <manuel@rueg.eu>
|
Getting this error when I'm trying to build it: |
Contributor
Author
SuperQ
added a commit
that referenced
this pull request
Jul 12, 2021
NOTE: Ignoring invalid network speed will be the default in 2.x NOTE: Filesystem collector flags have been renamed. `--collector.filesystem.ignored-mount-points` is now `--collector.filesystem.mount-points-exclude` and `--collector.filesystem.ignored-fs-types` is now `--collector.filesystem.fs-types-exclude`. The old flags will be removed in 2.x. * [CHANGE] Rename filesystem collector flags to match other collectors #2012 * [CHANGE] Make node_exporter print usage to STDOUT #2039 * [FEATURE] Add conntrack statistics metrics #1155 * [FEATURE] Add ethtool stats collector #1832 * [FEATURE] Add flag to ignore network speed if it is unknown #1989 * [FEATURE] Add tapestats collector for Linux #2044 * [ENHANCEMENT] Add ErrorLog plumbing to promhttp #1887 * [ENHANCEMENT] Add time zone offset metric #2060 * [BUGFIX] Add ErrorLog plumbing to promhttp #1887 * [BUGFIX] Handle errors from disabled PSI subsystem #1983 * [BUGFIX] Fix panic when using backwards compatible flags #2000 * [BUGFIX] Only initiate collectors once #2048 * [BUGFIX] Handle small backwards jumps in CPU idle #2067 Signed-off-by: Ben Kochie <superq@gmail.com>
Merged
SuperQ
added a commit
that referenced
this pull request
Jul 15, 2021
NOTE: Ignoring invalid network speed will be the default in 2.x NOTE: Filesystem collector flags have been renamed. `--collector.filesystem.ignored-mount-points` is now `--collector.filesystem.mount-points-exclude` and `--collector.filesystem.ignored-fs-types` is now `--collector.filesystem.fs-types-exclude`. The old flags will be removed in 2.x. * [CHANGE] Rename filesystem collector flags to match other collectors #2012 * [CHANGE] Make node_exporter print usage to STDOUT #2039 * [FEATURE] Add conntrack statistics metrics #1155 * [FEATURE] Add ethtool stats collector #1832 * [FEATURE] Add flag to ignore network speed if it is unknown #1989 * [FEATURE] Add tapestats collector for Linux #2044 * [FEATURE] Add nvme collector #2062 * [ENHANCEMENT] Add ErrorLog plumbing to promhttp #1887 * [ENHANCEMENT] Add more Infiniband counters #2019 * [ENHANCEMENT] netclass: retrieve interface names and filter before parsing #2033 * [ENHANCEMENT] Add time zone offset metric #2060 * [BUGFIX] Handle errors from disabled PSI subsystem #1983 * [BUGFIX] Fix panic when using backwards compatible flags #2000 * [BUGFIX] Fix wrong value for OpenBSD memory buffer cache #2015 * [BUGFIX] Only initiate collectors once #2048 * [BUGFIX] Handle small backwards jumps in CPU idle #2067 Signed-off-by: Ben Kochie <superq@gmail.com>
oblitorum
pushed a commit
to shatteredsilicon/node_exporter
that referenced
this pull request
Apr 9, 2024
NOTE: Ignoring invalid network speed will be the default in 2.x NOTE: Filesystem collector flags have been renamed. `--collector.filesystem.ignored-mount-points` is now `--collector.filesystem.mount-points-exclude` and `--collector.filesystem.ignored-fs-types` is now `--collector.filesystem.fs-types-exclude`. The old flags will be removed in 2.x. * [CHANGE] Rename filesystem collector flags to match other collectors prometheus#2012 * [CHANGE] Make node_exporter print usage to STDOUT prometheus#2039 * [FEATURE] Add conntrack statistics metrics prometheus#1155 * [FEATURE] Add ethtool stats collector prometheus#1832 * [FEATURE] Add flag to ignore network speed if it is unknown prometheus#1989 * [FEATURE] Add tapestats collector for Linux prometheus#2044 * [FEATURE] Add nvme collector prometheus#2062 * [ENHANCEMENT] Add ErrorLog plumbing to promhttp prometheus#1887 * [ENHANCEMENT] Add more Infiniband counters prometheus#2019 * [ENHANCEMENT] netclass: retrieve interface names and filter before parsing prometheus#2033 * [ENHANCEMENT] Add time zone offset metric prometheus#2060 * [BUGFIX] Handle errors from disabled PSI subsystem prometheus#1983 * [BUGFIX] Fix panic when using backwards compatible flags prometheus#2000 * [BUGFIX] Fix wrong value for OpenBSD memory buffer cache prometheus#2015 * [BUGFIX] Only initiate collectors once prometheus#2048 * [BUGFIX] Handle small backwards jumps in CPU idle prometheus#2067 Signed-off-by: Ben Kochie <superq@gmail.com>
oblitorum
pushed a commit
to shatteredsilicon/node_exporter
that referenced
this pull request
Apr 9, 2024
NOTE: Ignoring invalid network speed will be the default in 2.x NOTE: Filesystem collector flags have been renamed. `--collector.filesystem.ignored-mount-points` is now `--collector.filesystem.mount-points-exclude` and `--collector.filesystem.ignored-fs-types` is now `--collector.filesystem.fs-types-exclude`. The old flags will be removed in 2.x. * [CHANGE] Rename filesystem collector flags to match other collectors prometheus#2012 * [CHANGE] Make node_exporter print usage to STDOUT prometheus#2039 * [FEATURE] Add conntrack statistics metrics prometheus#1155 * [FEATURE] Add ethtool stats collector prometheus#1832 * [FEATURE] Add flag to ignore network speed if it is unknown prometheus#1989 * [FEATURE] Add tapestats collector for Linux prometheus#2044 * [FEATURE] Add nvme collector prometheus#2062 * [ENHANCEMENT] Add ErrorLog plumbing to promhttp prometheus#1887 * [ENHANCEMENT] Add more Infiniband counters prometheus#2019 * [ENHANCEMENT] netclass: retrieve interface names and filter before parsing prometheus#2033 * [ENHANCEMENT] Add time zone offset metric prometheus#2060 * [BUGFIX] Handle errors from disabled PSI subsystem prometheus#1983 * [BUGFIX] Fix panic when using backwards compatible flags prometheus#2000 * [BUGFIX] Fix wrong value for OpenBSD memory buffer cache prometheus#2015 * [BUGFIX] Only initiate collectors once prometheus#2048 * [BUGFIX] Handle small backwards jumps in CPU idle prometheus#2067 Signed-off-by: Ben Kochie <superq@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a new collector for Linux to gather "ethtool" stats, the same as "ethtool -S" provides. This is collected by using the
SIOCETHTOOLioctl.Some questions and comments:
Here is some example output:
Signed-off-by: W Andrew Denton ventifus@flying-snail.net