infiniband: Add new collector for InfiniBand statistics #450

roclark · 2017-02-02T19:40:20Z

Add new metrics for the InfiniBand network protocol including the amount of packets sent and received, the number of times the link has been downed and how many times the link has recovered from an error state.

Signed-Off-By: Robert Clark robert.d.clark@hpe.com

mdlayher · 2017-02-02T19:48:29Z

Nice! Though I would guess that InfiniBand is probably uncommon enough that it shouldn't be enabled by default. I'll review the rest now.

roclark · 2017-02-02T19:51:56Z

Good point! I made sure that it works regardless of whether or not InfiniBand is installed, but I can certainly make that change if desired.

mdlayher

A lot of my comments involve general Go stylistic suggestions. As always, if one of the maintainers of the project (or you, the author) disagree, please feel free to reply to my suggestions.

mdlayher · 2017-02-02T19:49:27Z

collector/infiniband.go

+// limitations under the License.
+
+// +build linux
+// +build !noinfiniband


Some of the collectors have this tag while others do not. Question for maintainers: are these collector-specific tags still being used in practice anywhere?

mdlayher · 2017-02-02T19:50:43Z

collector/infiniband.go

+	i.infinibandCounters = map[string]string{
+		"link_downed":            "Number of times the link failed to recover from an error state and went down",
+		"link_error_recovery":    "Number of times the link successfully recovered from an error state",
+		"multicast_rcv_packets":  "Number of multicast packets received (including errors)",


I believe the precedent is to spell out "receive".

I definitely agree with you that spelling out "receive" would be beneficial, but multicast_rcv_packets is the name of the file I read the metric from, hence the use here. I extract every metric's filename from the key in this map.

mdlayher · 2017-02-02T19:50:55Z

collector/infiniband.go

+		"link_downed":            "Number of times the link failed to recover from an error state and went down",
+		"link_error_recovery":    "Number of times the link successfully recovered from an error state",
+		"multicast_rcv_packets":  "Number of multicast packets received (including errors)",
+		"multicast_xmit_packets": "Number of multicast packets transmitted (including errors)",


Same as above, but "transmit" instead.

See above comment for my reasoning.

mdlayher · 2017-02-02T19:51:54Z

collector/infiniband.go

+	for metric, description := range i.infinibandCounters {
+		i.metricDescs[metric] = prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, metric+"_total"),
+			fmt.Sprintf(description),


I'd remove the fmt.Sprintf wrapper. No need and I'd imagine that descriptions probably should not be variable anyway.

mdlayher · 2017-02-02T19:53:00Z

collector/infiniband.go

+	subsystem := "infiniband"
+	i.metricDescs = make(map[string]*prometheus.Desc)
+	i.infinibandPath = "class/infiniband"
+	i.infinibandNoDevicesFound = errors.New("InfiniBand / No devices detected")


Seems unusual to pass around errors as struct members like this. I'd put them up above in a var() block and just reference them by name that way.

For these pieces of data, I had Robert pull them into the structure based on the feedback I received from my previous pull requests about global variables. Totally cool if any of the maintainers want us to pull them back out.

mdlayher · 2017-02-02T19:57:08Z

collector/infiniband.go

+
+	subsystem := "infiniband"
+	i.metricDescs = make(map[string]*prometheus.Desc)
+	i.infinibandPath = "class/infiniband"


Since I doubt this will change, it may also be easier to just put this in a const() block up top and reference it that way, instead of passing it to functions.

mdlayher · 2017-02-02T19:57:40Z

collector/infiniband.go

+
+	// Extract just the filenames which equates to the port numbers.
+	for i, port := range ports {
+		_, ports[i] = filepath.Split(port)


filepath.Base(port).

mdlayher · 2017-02-02T20:01:45Z

collector/infiniband_linux_test.go

+	"testing"
+)
+
+func TestDevices(t *testing.T) {


These test names should be namespaced something like TestInfiniBand* for ease of finding failing tests.

mdlayher · 2017-02-02T20:02:08Z

collector/infiniband_linux_test.go

+	}
+
+	if len(devices) != 1 {
+		t.Fatal(err)


This will just do t.Fatal(nil) in effect. Should add a more descriptive error message.

mdlayher · 2017-02-02T20:04:00Z

collector/infiniband_linux_test.go

+	}
+}
+
+func TestLinkDowned(t *testing.T) {


Because of the end to end tests, the remaining tests in this file seem unnecessary, as they're essentially just reading the value of a fixture and confirming it. They aren't actually testing that the value is appropriately output with other metrics.

mdlayher · 2017-02-02T20:06:08Z

collector/infiniband.go

@@ -0,0 +1,166 @@
+// Copyright 2017 The Prometheus Authors


This file should be named infiniband_linux.go.

roclark · 2017-02-02T23:10:03Z

@mdlayher thank you very much for the feedback! I am a recent Go convert from python and am still getting used to the new environment. I just pushed the changes you suggested - let me know if there is anything else you see that needs to be changed.

mdlayher · 2017-02-03T00:16:21Z

No problem at all, I'm always happy to help. I don't have time to review this again tonight but I'm happy to take another look tomorrow. Or, if someone from the Prometheus organization would like to take a look, that would be great too!

mdlayher · 2017-02-03T17:30:09Z

Okay! Starting in on another review.

mdlayher

Getting there! More nitpicky Go type things and a couple requests to bubble up errors and provide more descriptive test failures.

mdlayher · 2017-02-03T17:30:47Z

collector/infiniband_linux.go

+const infinibandPath = "class/infiniband"
+
+var (
+	infinibandNoDevicesFound = errors.New("InfiniBand / No devices detected")


Errors should start with err*, so errInfinibandDevicesNotFound or similar. Same for next error.

mdlayher · 2017-02-03T17:31:08Z

collector/infiniband_linux.go

+	i.counters = map[string]string{
+		"link_downed":            "Number of times the link failed to recover from an error state and went down",
+		"link_error_recovery":    "Number of times the link successfully recovered from an error state",
+		"multicast_rcv_packets":  "Number of multicast packets received (including errors)",


Expand "rcv" to "receive" and "xmit" to "transmit" on all metric names.

@mdlayher As @roclark said in his previous response on this subject, he's using those names because those are the actual metric names used for those metrics in /sys. I took at look at the existing e2e-output.txt file and saw four other metrics from the netstat collector using "Rcv" within the metric name as well (no cases of xmit though). We're open to the most correct answer being to expand these names, but it seemed like the status quo was to maintain the source location's naming (that's what I did for all the ZFS metrics). I figure we'll need input from @discordianfish or @SuperQ on this.

Oh sorry, I must have missed that reply with the new commit. Yeah, sounds good.

@mdlayher It's a pretty busy thread, no worries!

I was using node_network_{receive,transmit}_* as a reference for what it's worth.

Gotcha. @roclark and I were talking about some of the inconsistency across metrics in the collectors here (certainly not saying our own choices are perfect, mind you), maybe at some point it would make sense to try to standardize within this exporter about things like when to expand abbreviation, use camelcase, etc.

I totally agree. At this point I am just curious if it's okay to go back and "fix" metrics names since that would break metrics for previous timeseries when the node_exporter is upgraded.

I was working on a tool at one point that would lint metric names and tell you what the conventions were, but I haven't gotten back to it recently.

Filed #453 about it.

From skimming through the PR, giving it 'better' names would require storing some mapping between the filename and the metric, correct? That's what we want to avoid and rather keep the filename as metric name.
Arguably it's somewhat in between, since you store a list of files that might change in the collector anyway, so could also make it a mapping and use names following https://prometheus.io/docs/practices/naming/.

I'm personally fine either way.

If you're going to explicitly list every file, then you should also fix the names while you're at it. If you were automatically finding the files, then doing a straight pass through would be fine.

mdlayher · 2017-02-03T17:31:56Z

collector/infiniband_linux.go

+// Retrieve a list of InfiniBand devices.
+func infinibandDevices(infinibandPath string) ([]string, error) {
+	devices, err := filepath.Glob(filepath.Join(infinibandPath, "/*"))
+


It's a little unusual to have a blank line before an error check like this.

mdlayher · 2017-02-03T17:32:49Z

collector/infiniband_linux.go

+			prometheus.BuildFQName(Namespace, subsystem, metric+"_total"),
+			description,
+			[]string{"device", "port"},
+			nil)


Total nitpick, but if you break the parenthesis onto its own line, things line up nicely and it becomes easy to add or remove arguments in one line later.

Ex:

m := foo( "bar", "baz", )

mdlayher · 2017-02-03T17:33:24Z

collector/infiniband_linux.go

+// Retrieve a list of ports for the InfiniBand device.
+func infinibandPorts(infinibandPath, device string) ([]string, error) {
+	ports, err := filepath.Glob(filepath.Join(infinibandPath, device, "ports/*"))
+


Remove blank line.

mdlayher · 2017-02-03T17:39:44Z

collector/infiniband_linux_test.go

+
+func TestInfiniBandDevices(t *testing.T) {
+	devices, err := infinibandDevices("fixtures/sys/class/infiniband")
+


Remove blank line.

mdlayher · 2017-02-03T17:39:47Z

collector/infiniband_linux_test.go

+
+func TestInfiniBandPorts(t *testing.T) {
+	ports, err := infinibandPorts("fixtures/sys/class/infiniband", "mlx4_0")
+


Remove blank line.

mdlayher · 2017-02-03T17:40:42Z

collector/infiniband_linux_test.go

+	}
+
+	if len(devices) != 1 {
+		t.Fatal("Retrieved an unexpected number of InfiniBand devices")


Would be nice to specify how many devices were found. I typically do:

if l := len(devices); l != 1 { t.Fatalf("unexpected number of InfiniBand devices: %d", l) }

mdlayher · 2017-02-03T17:40:54Z

collector/infiniband_linux_test.go

+		t.Fatal(err)
+	}
+
+	if len(ports) != 2 {


See above about a more descriptive error for failing the test.

mdlayher · 2017-02-03T17:41:42Z

collector/infiniband_linux.go

+	return &i, nil
+}
+
+// Retrieve a list of InfiniBand devices.


Go comments should be of the form:

// infinibandDevices retrieves a list of InfiniBand devices.

roclark · 2017-02-03T21:04:51Z

Thanks again for the reviews Matt! Always good to get some helpful tips from an expert. Hopefully I got everything sorted out with my latest push but as always, let me know if otherwise. I look forward to getting feedback from some of the other maintainers.

mdlayher

LGTM with one final comment about format of error messages. There is also the outstanding question of whether or not this should be on by default.

I'll leave the rest for @SuperQ and @discordianfish . Thanks for bearing with my reviews!

mdlayher · 2017-02-03T21:25:07Z

collector/infiniband_linux.go

+const infinibandPath = "class/infiniband"
+
+var (
+	errInfinibandNoDevicesFound = errors.New("InfiniBand / No devices detected")


One thing I forgot to mention last time (sorry!), error messages should have text that starts lowercase (so they can be chained if needed). You probably want some text like no InfiniBand devices detected. Same goes for the ports error.

An example of why it's typically done this way (with three different errors that each add context):

metrics collection failed: infiniband collector failed: no InfiniBand devices detected".

No worries! I'd rather get all of these sorted out now instead of causing headaches later.

mdlayher · 2017-02-04T15:37:30Z

Appears this needs a rebase on master now.

roclark · 2017-02-06T19:16:33Z

Rebased off master so all merge conflicts are gone again.

mdlayher · 2017-02-06T19:33:03Z

No further feedback from me. Will let @SuperQ , @discordianfish , and @brian-brazil look this over and also determine if it should be enabled by default.

brian-brazil · 2017-02-06T20:14:57Z

collector/infiniband_linux.go

+		"link_error_recovery":    "Number of times the link successfully recovered from an error state",
+		"multicast_rcv_packets":  "Number of multicast packets received (including errors)",
+		"multicast_xmit_packets": "Number of multicast packets transmitted (including errors)",
+		"port_rcv_data":          "Number of data octets received on all links",


bytes should be in this name

See discussion above: Using good names if we have to track them here anyway or us the filenames here. If we add bytes here, we should also change rcv to receive etc etc, right?
I lean towards making all names follow our best practices but fine either way.

roclark · 2017-02-07T16:54:13Z

I edited the names to hopefully be more in line with the Prometheus standards. Let me know if you have any thoughts! And if not, are we good to merge this? Thanks everyone for the help!

mdlayher · 2017-02-07T16:57:33Z

collector/infiniband_linux.go

+	var i infinibandCollector
+
+	// Filenames of all InfiniBand counter metrics including a detailed description.
+	i.counters = map[string][]string{


For a little more type safety and readability, I'd probably go with a new type for the value in this map:

type infinibandMetric struct { File string Help string }

Alternatively, I suppose you could do an array ([2]string), but a struct feels cleaner to me than either a slice or an array.

I pushed another update using a struct. It works as I intend, but as always, let me know if I can make it more Go-idiomatic.

Add new metrics for the InfiniBand network protocol including the amount of packets sent and received, the number of times the link has been downed and how many times the link has recovered from an error state. Signed-Off-By: Robert Clark <robert.d.clark@hpe.com>

Signed-Off-By: Robert Clark <robert.d.clark@hpe.com>

mdlayher

LGTM again. Up to maintainers to determine if this should be on by default.

discordianfish

LGTM. In general we enable collectors by default that won't generate any metrics or significant load if run on a system without support for whatever the collector is doing. That's the case here so it can be enabled by default.

roclark · 2017-02-09T15:11:22Z

@brian-brazil @SuperQ any additional thoughts from either of you?

joehandzik · 2017-02-16T13:24:16Z

@SuperQ How about this one? Or are we waiting to pull it in until @brian-brazil re-reviews?

SuperQ · 2017-02-16T13:32:07Z

I was trying to hold off a lot of things so that we could cut 0.14. I guess we can add this to the pile. 😄

SuperQ

LGTM

joehandzik · 2017-02-16T13:55:49Z

@SuperQ Ya know, I was starting to think that was the plan (hold some PRs back to cut a release)...thanks for pulling this in! We're done at HPE for now until we get Omnipath cards in, so it's helpful for us to have all of our contributed code merged ahead of 0.14.

roclark · 2017-02-16T15:08:35Z

Thanks @SuperQ for the review and merge! As Joe mentioned, once I get my hands on some Omni-Path cards, I will crank out another collector.

Allow parsing multiple `xprt` fields present in the same NFS transport stats data. Fixes: prometheus#450 Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

mdlayher reviewed Feb 2, 2017

View reviewed changes

roclark force-pushed the add-infiniband branch from 9c74ef8 to 7886d4c Compare February 2, 2017 21:58

mdlayher reviewed Feb 3, 2017

View reviewed changes

roclark force-pushed the add-infiniband branch from 7886d4c to 36df228 Compare February 3, 2017 20:59

roclark force-pushed the add-infiniband branch from 36df228 to 61cb3f7 Compare February 3, 2017 21:07

mdlayher reviewed Feb 3, 2017

View reviewed changes

mdlayher approved these changes Feb 3, 2017

View reviewed changes

mdlayher mentioned this pull request Feb 3, 2017

Standardizing metric names #453

Closed

roclark force-pushed the add-infiniband branch from 61cb3f7 to c392648 Compare February 3, 2017 21:59

roclark force-pushed the add-infiniband branch 4 times, most recently from 3bef2cf to f0e6fc1 Compare February 4, 2017 18:36

brian-brazil reviewed Feb 6, 2017

View reviewed changes

roclark force-pushed the add-infiniband branch 2 times, most recently from eb77cd5 to 95c5c82 Compare February 7, 2017 16:52

mdlayher reviewed Feb 7, 2017

View reviewed changes

Robert Clark added 2 commits February 7, 2017 11:09

Add unit tests for InfiniBand collector

36f8128

Signed-Off-By: Robert Clark <robert.d.clark@hpe.com>

Robert Clark added 3 commits February 7, 2017 11:09

Enable InfiniBand by default

b0c9133

Signed-Off-By: Robert Clark <robert.d.clark@hpe.com>

Update README to include InfiniBand collector

b2d2b69

Signed-Off-By: Robert Clark <robert.d.clark@hpe.com>

Update authors after InfiniBand commit

f809bfd

Signed-Off-By: Robert Clark <robert.d.clark@hpe.com>

roclark force-pushed the add-infiniband branch from 95c5c82 to f809bfd Compare February 7, 2017 17:09

mdlayher approved these changes Feb 7, 2017

View reviewed changes

discordianfish approved these changes Feb 7, 2017

View reviewed changes

SuperQ approved these changes Feb 16, 2017

View reviewed changes

SuperQ changed the title ~~Add new collector for InfiniBand statistics~~ infiniband: Add new collector for InfiniBand statistics Feb 16, 2017

SuperQ merged commit 38cd07e into prometheus:master Feb 16, 2017

grobie mentioned this pull request Mar 7, 2017

Release v0.14.0-rc.2 #441

Merged


		func TestInfiniBandDevices(t *testing.T) {
		devices, err := infinibandDevices("fixtures/sys/class/infiniband")


		func TestInfiniBandPorts(t *testing.T) {
		ports, err := infinibandPorts("fixtures/sys/class/infiniband", "mlx4_0")

infiniband: Add new collector for InfiniBand statistics #450

infiniband: Add new collector for InfiniBand statistics #450

Conversation

roclark commented Feb 2, 2017

mdlayher commented Feb 2, 2017

roclark commented Feb 2, 2017

mdlayher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roclark commented Feb 2, 2017

mdlayher commented Feb 3, 2017

mdlayher commented Feb 3, 2017

mdlayher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roclark commented Feb 3, 2017

mdlayher left a comment

Choose a reason for hiding this comment

mdlayher Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdlayher commented Feb 4, 2017

roclark commented Feb 6, 2017

mdlayher commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roclark commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdlayher left a comment

Choose a reason for hiding this comment

discordianfish left a comment

Choose a reason for hiding this comment

roclark commented Feb 9, 2017

joehandzik commented Feb 16, 2017

SuperQ commented Feb 16, 2017

SuperQ left a comment

Choose a reason for hiding this comment

joehandzik commented Feb 16, 2017

roclark commented Feb 16, 2017

mdlayher Feb 3, 2017 •

edited

Loading