add sys/class/net parsing from procfs and expose its metrics #851

klatys · 2018-03-08T16:03:55Z

related to #300
Hi @SuperQ , I just implemented the mentioned funcionality from procfs into node_exporter, but have several questions:

I'm not sure where did we land on the reflect/noreflect discussion with @grobie . Here it's used again, but I can write it into static calls, if you wish so
Since the class/net is a linux feature, how to handle other collector files? I came up with several solutions, but none of them seems quite right:
- check arch in common and call the function then (though I think the compiler would see through that)
- add empty function in other platforms's files (e.g. netdev_bsd.go)
- collect the data in getNetDevStats
- introduce separate collector (this one seems like the best choice, but not sure whether it is overkill?)
What naming should be used for metrics? Atm filenames are used, but then again the naming is based on sys/class/net structure, which might be different for other platforms (if there were any).
- Also speed is in bits (but per second), so not sure whether any suffix is correct here
I had to update vendor for procfs, mostly it's just licence headers, but since it's WIP, I'm not sure whether this is a correct way.

SuperQ · 2018-04-12T09:50:18Z

Ping, this needs some cleanup.

klatys · 2018-04-13T10:14:34Z

@SuperQ Absolutely, though I'd be glad for pointing me in the right direction in this point mostly

Since the class/net is a linux feature, how to handle other collector files? I came up with several solutions, but none of them seems quite right

discordianfish · 2018-04-13T10:48:41Z

yes, better avoid reflect
I think a separate collector, maybe netclass, would make sense
Naming: Good question.. In general we either expose raw values by just iterating over lines/directories so we don't need to maintain a mapping which might change between kernel versions etc. Or, if we need to liste all names anyway, we would make sure they are following our best practices. I think the later one would apply here more. So make sure to follow https://prometheus.io/docs/practices/naming/
Not really sure about the speed name.. I think speed_bits would work, since it somewhat implies that it's bits per second (since everything is suppose to in seconds). But maybe being explicit and call it speed_bits_per_second is better. @SuperQ preferences?

brian-brazil · 2018-04-13T10:50:53Z

bytes_per_second would be better I think, as that's what the output of rate() on the bytes sent will be.

klatys · 2018-04-13T11:11:45Z

thanks for the answers guys! I'll get on it soon.

SuperQ · 2018-04-13T13:10:22Z

Yes to:

netclass collector
Either speed_bytes or bytes_per_second. We do want bytes so it's the same unit as node_network_receive_bytes_total.
Avoid reflect, at the cost of a little more maintenance / copy-paste.

discordianfish · 2018-04-14T11:56:11Z

Makes sense, let's go with bytes

poblahblahblah · 2018-05-22T23:50:22Z

I'm interested in getting this into node_exporter. Is there anything I can do to move this along?

klatys · 2018-05-23T08:02:11Z

Yes, currently this is waiting for prometheus/procfs#87 can we move on with this guys?

SuperQ · 2018-05-30T08:44:47Z

collector/netdev_common.go

@@ -73,5 +73,6 @@ func (c *netDevCollector) Update(ch chan<- prometheus.Metric) error {
 			ch <- prometheus.MustNewConstMetric(desc, prometheus.CounterValue, v, dev)
 		}
 	}
+


This change seems unrelated.

SuperQ · 2018-05-30T08:45:18Z

collector/netclass_linux.go

@@ -0,0 +1,169 @@
+// Copyright 2015 The Prometheus Authors


Since this is a new file, let's make this 2018.

SuperQ · 2018-05-30T08:47:45Z

Let's add some fixtures to this, and add netclass to the end-to-end test.

SuperQ · 2018-05-30T09:43:39Z

The pushMetric() function needs to have the option to push the type. Maybe add a param for prometheus.ValueType and pass prometheus.GaugeValue or prometheus.CounterValue as appropriate.

Also, it looks like this PR needs to be rebased against master.

klatys · 2018-05-30T10:48:48Z

@SuperQ I guess your point is to the
node_network_carrier_changes_total non-counter metrics should not have "_total" suffix
fail in tests which I think is rather bad metric name (although the file name is literally carrier_changes_total).

Every metric is changed with each restart of the machine, so counter type is imo wrong here (that's why gauge is used).

So I suppose using sth like node_network_carrier_changes , ..carrier_up_changes , carrier_down_changes should be enough?

SuperQ · 2018-05-30T11:10:41Z

Counters being reset to 0 on machine restart is completely fine and normal from a Prometheus perspective. The system is designed to handle this automatically. So if these only go up for the lifetime of a device, and only reset to 0, they are counters.

SuperQ · 2018-05-30T11:23:33Z

Also, it looks like you need to reword your early commit with the DCO signoff.

Signed-off-by: Jan Klat <jenik@klatys.cz>

…eparate collector, change metric naming Signed-off-by: Jan Klat <jenik@klatys.cz>

Signed-off-by: Jan Klat <jenik@klatys.cz>

klatys · 2018-05-30T11:46:06Z

Oh ok, I honestly got a different view from docs, especially

Counters should not be used to expose current counts of items whose number can also go down

Signed off, but the test is failing on changed values for network interfaces... Is there a way to omit this somehow?

-node_network_iface_id{interface="eth0"} 605
+node_network_iface_id{interface="eth0"} 1337
-node_network_iface_link{interface="eth0"} 606
+node_network_iface_link{interface="eth0"} 1338
-node_network_up{address="02:42:ac:13:00:03",broadcast="ff:ff:ff:ff:ff:ff",duplex="full",ifalias="",interface="eth0",operstate="up"} 1
+node_network_up{address="02:42:ac:18:00:03",broadcast="ff:ff:ff:ff:ff:ff",duplex="full",ifalias="",interface="eth0",operstate="up"} 1

SuperQ · 2018-05-30T12:39:51Z

I think the test diff is due to the fact that the fixtures do not include the fixture data, and you're getting leaked info from the docker container.

Signed-off-by: Jan Klat <jenik@klatys.cz>

SuperQ · 2018-05-30T14:13:41Z

It looks like the fixtures need an iface_id and iface_link file.

klatys · 2018-05-31T09:49:38Z

The fixtures are passed as a option(--path.sysfs=) thats what I was missing... While testing that I also discovered bug in procfs which is referenced above. Hopefuly after that fixtures will work correctly

discordianfish · 2018-05-31T11:18:40Z

Tests are still failing though:

>> running end-to-end tests
./end-to-end-test.sh
--- collector/fixtures/e2e-output.txt	2018-05-30 13:45:44.663039377 +0000
+++ /tmp/node_exporter_e2e_test.cnTQB1/e2e-output.txt	2018-05-30 13:47:11.255262772 +0000
@@ -1690,11 +1690,11 @@
 node_network_flags{interface="lo"} 9
 # HELP node_network_iface_id iface_id value of /sys/class/net/<iface>.
 # TYPE node_network_iface_id gauge
-node_network_iface_id{interface="eth0"} 605
+node_network_iface_id{interface="eth0"} 9393
 node_network_iface_id{interface="lo"} 1
 # HELP node_network_iface_link iface_link value of /sys/class/net/<iface>.
 # TYPE node_network_iface_link gauge
-node_network_iface_link{interface="eth0"} 606
+node_network_iface_link{interface="eth0"} 9394
 node_network_iface_link{interface="lo"} 1
 # HELP node_network_iface_link_mode iface_link_mode value of /sys/class/net/<iface>.
 # TYPE node_network_iface_link_mode gauge
LOG =====================

Signed-off-by: Jan Klat <jenik@klatys.cz>

SuperQ · 2018-05-31T11:53:37Z

collector/netclass_linux.go

+)
+
+var (
+	netclassIgnoredDevices = kingpin.Flag("collector.netclass.ignored-devices", "Regexp of net devices to ignore for netclass collector.").Default("^$").String()


Do we want to include some interfaces in this default list? Maybe lo and/or docker.*?

Signed-off-by: Jan Klat <jenik@klatys.cz>

klatys · 2018-05-31T13:08:36Z

Can someone please let me know what is failing in buildkite? I don't have access to that org. Thanks!

SuperQ · 2018-05-31T13:33:34Z

You probably need to update the collector/fixtures/e2e-64k-page-output.txt to match the other fixture update.

Signed-off-by: Jan Klat <jenik@klatys.cz>

klatys · 2018-05-31T13:57:47Z

Thanks! Should I set the default interface list as well or do you want approve from other Members?

SuperQ

LGTM

klatys · 2018-07-11T09:05:47Z

ping @discordianfish I'd be happy to have these in node exporter, the sooner the better :) thanks a lot!

discordianfish

LGTM

discordianfish · 2018-07-16T13:08:09Z

Sorry, this slipped through. Merging now!

klatys · 2018-07-16T13:09:04Z

Thanks guys for all the help. Appreciate it!

socketpair · 2019-08-21T17:05:33Z

collector/fixtures/e2e-64k-page-output.txt

+# HELP node_network_carrier carrier value of /sys/class/net/<iface>.
+# TYPE node_network_carrier gauge
+node_network_carrier{interface="eth0"} 1
+# HELP node_network_carrier_changes_total carrier_changes_total value of /sys/class/net/<iface>.


carrier_changes_total value of -> carrier_changes value of please fix it. And others in this PR

…eus#851) * add sys/class/net parsing from procfs and expose its metrics Signed-off-by: Jan Klat <jenik@klatys.cz> * change code to use int pointers per procfs change, move netclass to separate collector, change metric naming Signed-off-by: Jan Klat <jenik@klatys.cz> * bump year in licence, remove redundant newline, correct fixtures Signed-off-by: Jan Klat <jenik@klatys.cz> * fix style Signed-off-by: Jan Klat <jenik@klatys.cz> * change carrier changes to counter type Signed-off-by: Jan Klat <jenik@klatys.cz> * fix e2e output Signed-off-by: Jan Klat <jenik@klatys.cz> * add fixtures Signed-off-by: Jan Klat <jenik@klatys.cz> * update vendor, use fixtures correctly Signed-off-by: Jan Klat <jenik@klatys.cz> * change fixtures (device in /sys/class/net should be symlinked) Signed-off-by: Jan Klat <jenik@klatys.cz> * correct fixtures for 64k page, updated readme Signed-off-by: Jan Klat <jenik@klatys.cz>

klatys force-pushed the net-class branch from 90a43e1 to 302325c Compare March 9, 2018 09:10

klatys mentioned this pull request Apr 17, 2018

change values to pointers to differ missing value from 0 prometheus/procfs#87

Merged

SuperQ reviewed May 30, 2018

View reviewed changes

klatys force-pushed the net-class branch from 1117c26 to 7345529 Compare May 30, 2018 10:04

klatys added 6 commits May 30, 2018 13:38

add sys/class/net parsing from procfs and expose its metrics

07b5255

Signed-off-by: Jan Klat <jenik@klatys.cz>

change code to use int pointers per procfs change, move netclass to s…

162502a

…eparate collector, change metric naming Signed-off-by: Jan Klat <jenik@klatys.cz>

bump year in licence, remove redundant newline, correct fixtures

9d74d5c

Signed-off-by: Jan Klat <jenik@klatys.cz>

fix style

fbe5465

Signed-off-by: Jan Klat <jenik@klatys.cz>

change carrier changes to counter type

37340d1

Signed-off-by: Jan Klat <jenik@klatys.cz>

fix e2e output

dd92399

Signed-off-by: Jan Klat <jenik@klatys.cz>

klatys force-pushed the net-class branch from 75ff863 to dd92399 Compare May 30, 2018 11:39

add fixtures

9c4ca36

Signed-off-by: Jan Klat <jenik@klatys.cz>

klatys mentioned this pull request May 31, 2018

netclass: parse only directories prometheus/procfs#90

Merged

update vendor, use fixtures correctly

1a1db7b

Signed-off-by: Jan Klat <jenik@klatys.cz>

SuperQ reviewed May 31, 2018

View reviewed changes

change fixtures (device in /sys/class/net should be symlinked)

d55d863

Signed-off-by: Jan Klat <jenik@klatys.cz>

correct fixtures for 64k page, updated readme

b93c6a2

Signed-off-by: Jan Klat <jenik@klatys.cz>

SuperQ approved these changes May 31, 2018

View reviewed changes

SuperQ requested a review from discordianfish May 31, 2018 14:55

SuperQ mentioned this pull request May 31, 2018

adding a iscsi stats collector #961

Closed

discordianfish approved these changes Jul 16, 2018

View reviewed changes

discordianfish merged commit c4102f1 into prometheus:master Jul 16, 2018

klatys mentioned this pull request Jul 16, 2018

Expose network link speed #300

Closed

klatys deleted the net-class branch July 19, 2018 11:13

SuperQ mentioned this pull request Jan 5, 2019

standardize between interface and device for network metric labels #1223

Closed

socketpair reviewed Aug 21, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add sys/class/net parsing from procfs and expose its metrics #851

add sys/class/net parsing from procfs and expose its metrics #851

klatys commented Mar 8, 2018

SuperQ commented Apr 12, 2018

klatys commented Apr 13, 2018

discordianfish commented Apr 13, 2018

brian-brazil commented Apr 13, 2018

klatys commented Apr 13, 2018

SuperQ commented Apr 13, 2018

discordianfish commented Apr 14, 2018

poblahblahblah commented May 22, 2018

klatys commented May 23, 2018

SuperQ May 30, 2018

SuperQ May 30, 2018

SuperQ commented May 30, 2018

SuperQ commented May 30, 2018

klatys commented May 30, 2018

SuperQ commented May 30, 2018

SuperQ commented May 30, 2018

klatys commented May 30, 2018

SuperQ commented May 30, 2018

SuperQ commented May 30, 2018

klatys commented May 31, 2018 •

edited

Loading

discordianfish commented May 31, 2018

SuperQ May 31, 2018

klatys commented May 31, 2018

SuperQ commented May 31, 2018

klatys commented May 31, 2018

SuperQ left a comment

klatys commented Jul 11, 2018

discordianfish left a comment

discordianfish commented Jul 16, 2018

klatys commented Jul 16, 2018

socketpair Aug 21, 2019

add sys/class/net parsing from procfs and expose its metrics #851

add sys/class/net parsing from procfs and expose its metrics #851

Conversation

klatys commented Mar 8, 2018

SuperQ commented Apr 12, 2018

klatys commented Apr 13, 2018

discordianfish commented Apr 13, 2018

brian-brazil commented Apr 13, 2018

klatys commented Apr 13, 2018

SuperQ commented Apr 13, 2018

discordianfish commented Apr 14, 2018

poblahblahblah commented May 22, 2018

klatys commented May 23, 2018

SuperQ May 30, 2018

Choose a reason for hiding this comment

SuperQ May 30, 2018

Choose a reason for hiding this comment

SuperQ commented May 30, 2018

SuperQ commented May 30, 2018

klatys commented May 30, 2018

SuperQ commented May 30, 2018

SuperQ commented May 30, 2018

klatys commented May 30, 2018

SuperQ commented May 30, 2018

SuperQ commented May 30, 2018

klatys commented May 31, 2018 • edited Loading

discordianfish commented May 31, 2018

SuperQ May 31, 2018

Choose a reason for hiding this comment

klatys commented May 31, 2018

SuperQ commented May 31, 2018

klatys commented May 31, 2018

SuperQ left a comment

Choose a reason for hiding this comment

klatys commented Jul 11, 2018

discordianfish left a comment

Choose a reason for hiding this comment

discordianfish commented Jul 16, 2018

klatys commented Jul 16, 2018

socketpair Aug 21, 2019

Choose a reason for hiding this comment

klatys commented May 31, 2018 •

edited

Loading