Sys class net by klatys · Pull Request #82 · prometheus/procfs

klatys · 2018-03-03T13:37:39Z

SuperQ · 2018-03-03T21:12:12Z

Thanks for the contribution. I'm traveling right now, but will try and get this reviewed soon.

grobie

I'm generally not a fan of using reflect, it trades compile time checks for a few saved lines. Static code would be preferred.

I won't object merging a reflect based implementation, but please ensure there are no linter errors (e.g. ensure all types and functions are properly documented).

grobie · 2018-03-05T18:43:17Z

sysfs/net_class.go

+	"strings"
+)
+
+type interfaceNetClass struct {


The struct should be exported with a descriptive name, so that it'll be be included in the documentation https://godoc.org/github.com/prometheus/procfs/.

Ok, does NetClassInterface seem like a better name to you? I'm not sure whether using word interface is really a good idea - can be misleading language-wise, on the other hand something like device is misleading as well

I believe following the terminology chosen by the kernel developers makes the most sense. Reading the kernel documentation, it should be either iface or interface. I'm generally not a fan of abbreviations and would slightly lean towards interface. In the doc strings we could use interface (iface) to make the context clear.

grobie · 2018-03-05T18:46:37Z

sysfs/net_class.go

+)
+
+type interfaceNetClass struct {
+	Name             string `fileName:"name"`               // Interface name


I'm worried including this field can lead to confusion, as it's not actually an attribute in /sys/class/net/*/. I see the benefit of having it here though. I'd remove the filename attribute here to not further confuse a reader.

That is a correct note and mistake on my side, I'll remove the tag, thanks.

grobie · 2018-03-05T18:48:37Z

sysfs/net_class.go

+	CarrierChanges   uint64 `fileName:"carrier_changes"`    // /sys/class/net/<iface>/carrier_changes
+	CarrierUpCount   uint64 `fileName:"carrier_up_count"`   // /sys/class/net/<iface>/carrier_up_count
+	CarrierDownCount uint64 `fileName:"carrier_down_count"` // /sys/class/net/<iface>/carrier_down_count
+	DevId            uint64 `fileName:"dev_id"`             // /sys/class/net/<iface>/dev_id


DevID, see https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Did not know that. Thanks!

grobie · 2018-03-05T18:48:45Z

sysfs/net_class.go

+	IfIndex          uint64 `fileName:"ifindex"`            // /sys/class/net/<iface>/ifindex
+	IfLink           uint64 `fileName:"iflink"`             // /sys/class/net/<iface>/iflink
+	LinkMode         uint64 `fileName:"link_mode"`          // /sys/class/net/<iface>/link_mode
+	Mtu              uint64 `fileName:"mtu"`                // /sys/class/net/<iface>/link_mode


grobie · 2018-03-05T18:49:07Z

sysfs/net_class.go

+	NameAssignType   uint64 `fileName:"name_assign_type"`   // /sys/class/net/<iface>/name_assign_type
+	NetDevGroup      uint64 `fileName:"netdev_group"`       // /sys/class/net/<iface>/netdev_group
+	OperState        string `fileName:"operstate"`          // /sys/class/net/<iface>/operstate
+	PhysPortId       string `fileName:"phys_port_id"`       // /sys/class/net/<iface>/phys_port_id


PhysPortID

grobie · 2018-03-05T18:50:00Z

sysfs/net_class.go

+	Type             uint64 `fileName:"type"`               // /sys/class/net/<iface>/type
+}
+
+type NetClass map[string]interfaceNetClass


Please add a godoc comment for that type and explain the map key.

grobie · 2018-03-05T18:50:42Z

sysfs/net_class.go

+
+type NetClass map[string]interfaceNetClass
+
+// NewNetDev returns kernel/system statistics read from /proc/net/dev.


godoc comment is out of sync. Please check your code with golint sys/... and go vet ./....

I fixed the comment, but neither of those commands returned anything (neither before nor after the fix) not sure if that's correct or I'm doing sth wrong?

There are indeed no go vet errors in the packages fortunately. A couple of linter errors though in other files (nfs and bcache). Before your changes, golint also complained about the missing or wrong godoc strings. You should see some output with golint ./... executed in the procfs repository root.

grobie · 2018-03-05T18:53:57Z

sysfs/net_class_test.go

+		t.Errorf("want %d parsed class/net, have %d", want, have)
+	}
+
+	fields := []string{"AddrAssignType", "AddrLen", "Address", "Broadcast", "Carrier", "CarrierChanges", "CarrierUpCount", "CarrierDownCount", "DevId", "Dormant", "Duplex", "Flags", "IfAlias", "IfIndex", "IfLink", "LinkMode", "Mtu", "NameAssignType", "NetDevGroup", "OperState", "PhysPortId", "PhysPortName", "PhysSwitchId", "Speed", "TxQueueLen", "Type"}


Is all of this code necessary? Wouldn't it be enough to use reflect.DeepEqual(netClass, nc)?

absolutely, did not know that. thanks!

grobie · 2018-03-05T19:01:12Z

sysfs/net_class.go

+}
+
+func (nc NetClass) parseInterfaceNetClass(devicePath string) (*interfaceNetClass, error) {
+	fields := []string{"AddrAssignType", "AddrLen", "Address", "Broadcast", "Carrier", "CarrierChanges", "CarrierUpCount", "CarrierDownCount", "DevId", "Dormant", "Duplex", "Flags", "IfAlias", "IfIndex", "IfLink", "LinkMode", "Mtu", "NameAssignType", "NetDevGroup", "OperState", "PhysPortId", "PhysPortName", "PhysSwitchId", "Speed", "TxQueueLen", "Type"}


Why do you list all the field names again, given that you already use the reflect package? With reflect.ValueOf(&interfaceClass).NumField() you can get the number of fields and with reflect.ValueOf(&interfaceClass).Field(index) you can access a field by index.

much better solution, thanks!

grobie · 2018-03-05T19:02:02Z

sysfs/net_class.go

+		}
+		fieldValue := interfaceElem.FieldByName(fieldName)
+
+		fileContents, err := ioutil.ReadFile(devicePath + "/" + fieldType.Tag.Get("fileName"))


There should be a check whether fileName tag is set before trying to open such file.

definitely. Added, thanks for that

grobie

LGTM

grobie · 2018-03-06T00:07:56Z

sysfs/net_class_test.go

+	}
+
+	netClass := NetClass{
+		"eth0": {Name: "eth0", AddrAssignType: 3, AddrLen: 6, Address: "01:01:01:01:01:01", Broadcast: "ff:ff:ff:ff:ff:ff", Carrier: 1, CarrierChanges: 2, CarrierUpCount: 1, CarrierDownCount: 1, DevID: 32, Dormant: 1, Duplex: "full", Flags: 4867, IfAlias: "", IfIndex: 2, IfLink: 2, LinkMode: 1, MTU: 1500, NameAssignType: 2, NetDevGroup: 0, OperState: "up", PhysPortID: "", PhysPortName: "", PhysSwitchID: "", Speed: 1000, TxQueueLen: 1000, Type: 1},


It'd help readability to have a line break after each attribute/value pair.

grobie · 2018-03-06T00:09:45Z

sysfs/net_class.go

+	return newNetClass(fs.Path("class/net"))
+}
+
+// NewNetClass returns info for all net interfaces read from /sys/class/net/<interface>.


This should start with a lowercase n to document the private function. I wonder why not just merging this fuction with the public NewNetClass function, given it's the only caller.

absolutely, merging.

grobie · 2018-03-06T00:14:32Z

sysfs/net_class.go

+	"strings"
+)
+
+// NetClassInterface contains info from files in /sys/class/net/<interface>


The kernel documentation uses <iface>, what about using the same?

makes sense. Omits the possible confusion, thanks!

grobie · 2018-03-06T00:16:03Z

sysfs/net_class.go

+)
+
+// NetClassInterface contains info from files in /sys/class/net/<interface>
+// for single interface


What about adding for a single interface (iface). in parenthesis, to emphasize that it's not about a language/golang interface. Please end all comment sentences with full stops (following the golang style guide).

grobie · 2018-03-06T00:21:13Z

sysfs/net_class.go

+		}
+		value := strings.TrimSpace(string(fileContents))
+
+		if fieldValue.Kind() == reflect.Uint64 {


I'd suggest to use a switch statement here and error handling in the default case, to ensure all value types are handled.

switch fieldValue.Kind() { case reflect.Uint64: // ... default: return nil, fmt.Errorf("unhandled type %q", fieldValue.Kind()) }

grobie · 2018-03-06T00:23:41Z

sysfs/net_class.go

+}
+
+// NetClass is collection of info for every interface in /sys/class/net. The map keys
+// are interface names.


Probably here again to make it clearer interface (iface).

SuperQ · 2018-03-06T10:51:20Z

@grobie Do you have any suggestions to replace reflect to avoid the huge amount of copy-paste necessary for things like this vs with reflect?

SuperQ

LGTM

grobie

Thanks a lot for your contribution, looks great!

My only remaining question is around error handling. We currently ignore all errors and silently omit the data or even the whole interface. As procfs/sysfs are low level packages, I'd expect errors returned to be in that case, so that the caller can decide how to handle such situations.

One common error case I can imagine are non-existent files in older kernel versions (for attributes which were only added later). For that case you could use os.IsNotExist(err) and continue in that case without returning an error. For other file read errors or parser errors, I'd intuitively return an error. What do you think? Have you used the continue statements on purpose?

grobie · 2018-03-06T11:03:11Z

@SuperQ I often find static code easier to reason about then trying to abstract it away, my editor helps me with managing such problems. For a middleground between reflect usage and manually typed code I would look into go generate. I only have used it once in my life so far, but it should be possible to auto-generate your linked functions.

klatys · 2018-03-06T11:49:50Z

@grobie Thanks for your thorough code review, I'm really glad for that!

As for the error handling - yes I did use the continue on purpose, for two specific reasons:

When testing this on one of our debian machines, I got to a problem parsing the phys_* files which cannot be read (e.g. $ cat /sys/class/net/eth0/phys_port_id produces Operation not supported error, go returns error read ...: operation not supported). From what our linux guys are telling me this is probably caused by the driver of the device and might not be something that unusual. The other thing is, as you mentioned, nonexistent file due to older kernel (which I could find on any of our machines) and in my opinion should not produce error as well.

The other one is rather being cautious on my side, as I can imagine an interface dir not being accessible for some reason. In such case I don't think it is correct to return an error and ignoring all other ok ifaces, but that's really my point of view...

SuperQ · 2018-03-06T12:26:59Z

@klatys We had a similar IO problem with hwmon. Some kernels would return EAGAIN. We wrote a special reader function for this use case. We may want to do the same here.

grobie · 2018-03-06T12:42:24Z

In my experience strict error handling makes it easier to debug issues. I've witnessed many lengthy debug sessions due to silenced error swallowing. I'd suggest to handle the "file not exist" and "operation not supported" cases gracefully (and probably the EAGAIN error as well), but bubble up errors in all other cases.

klatys · 2018-03-06T20:21:20Z

Ok, np. I implemented that and changed the error handling. besides the three file errors I had to add one other - invalid argument:

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

Hopefully there won't be any other errors for these "files".

Also I ran this on several machines and discovered that some values (e.g. speed) can be negative (e.g. -1) thus the type is now Int64.

* add net class parsing * change tag, rename var * fix coding style * fix data types for name_assign_type and phys_switch_id * code review fixes * code review fixes - clearer naming * better error handling

klatys added 3 commits March 1, 2018 15:49

add net class parsing

14a25d7

change tag, rename var

7cfbc9b

fix coding style

d20b340

SuperQ requested review from SuperQ and mdlayher March 3, 2018 21:12

fix data types for name_assign_type and phys_switch_id

449a1e3

grobie reviewed Mar 5, 2018

View reviewed changes

code review fixes

a327c1e

grobie reviewed Mar 6, 2018

View reviewed changes

code review fixes - clearer naming

25ad479

SuperQ approved these changes Mar 6, 2018

View reviewed changes

grobie reviewed Mar 6, 2018

View reviewed changes

better error handling

4dcd48a

grobie approved these changes Mar 7, 2018

View reviewed changes

grobie merged commit 1c7ff3d into prometheus:master Mar 7, 2018

klatys deleted the sys-class-net branch April 17, 2018 14:51

klatys restored the sys-class-net branch April 17, 2018 14:51

klatys deleted the sys-class-net branch April 17, 2018 14:52

SuperQ mentioned this pull request May 31, 2018

adding a iscsi stats collector prometheus/node_exporter#961

Closed


		type NetClass map[string]interfaceNetClass

		// NewNetDev returns kernel/system statistics read from /proc/net/dev.

Comments

Conversation

klatys commented Mar 3, 2018

Uh oh!

SuperQ commented Mar 3, 2018

Uh oh!

grobie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grobie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperQ commented Mar 6, 2018

Uh oh!

SuperQ left a comment

Choose a reason for hiding this comment

Uh oh!

grobie left a comment

Choose a reason for hiding this comment

Uh oh!

grobie commented Mar 6, 2018

Uh oh!

klatys commented Mar 6, 2018

Uh oh!

SuperQ commented Mar 6, 2018

Uh oh!