Add qdisc collector for Linux by ema · Pull Request #580 · prometheus/node_exporter

ema · 2017-05-12T10:51:40Z

This collector gathers basic queueing discipline metrics via netlink,
similarly to what tc -s qdisc show does.

This collector gathers basic queueing discipline metrics via netlink, similarly to what `tc -s qdisc show` does.

SuperQ · 2017-05-12T11:23:45Z

collector/qdisc_linux.go

+
+type qdiscStatCollector struct {
+	bytes      typedDesc
+	pkts       typedDesc


No need to use shortened names here.

SuperQ · 2017-05-12T11:25:01Z

collector/qdisc_linux.go

+			[]string{"iface", "kind"}, nil,
+		), prometheus.CounterValue},
+		pkts: typedDesc{prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, "qdisc", "pkts"),


Same here, no need to shorten packets.

SuperQ · 2017-05-12T11:25:28Z

collector/qdisc_linux.go

+func NewQdiscStatCollector() (Collector, error) {
+	return &qdiscStatCollector{
+		bytes: typedDesc{prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, "qdisc", "bytes"),


If these are really counters, we can call them XXXX_total.

SuperQ · 2017-05-12T11:26:08Z

@mdlayher Hey, do we have any way to mock netlink so we can have tests and end-to-end testing here?

mdlayher · 2017-05-12T13:51:40Z

Yep! https://godoc.org/github.com/mdlayher/netlink/nltest

The tests show how to use it: https://github.com/mdlayher/netlink/blob/master/nltest/nltest_test.go.

The idea is that you check the parameters of the incoming message and pass back a message with whatever data you want via this mechanism.

mdlayher

Very cool, and I'm thrilled to see my netlink package being used. Left a few comments and questions regarding netlink specifics.

mdlayher · 2017-05-12T13:52:39Z

collector/qdisc_linux.go

+}
+
+// See if_indextoname(3)
+func ifIndexToName(index uint32) string {


Should be able to avoid Cgo by using: https://golang.org/pkg/net/#InterfaceByIndex.

Indeed! Done: https://github.com/ema/qdisc/blob/master/get.go#L197

mdlayher · 2017-05-12T13:54:22Z

collector/qdisc_linux.go

+}
+
+// See https://tools.ietf.org/html/rfc3549#section-3.1.3
+func parseMessage(msg netlink.Message) (Metric, error) {


Since you're handling netlink messages and the interesting cases that come with that, it might be worth making a separate package like github.com/ema/qdisc and then importing that for node_exporter. I like to promote code reusability wherever possible.

mdlayher · 2017-05-12T13:55:17Z

collector/qdisc_linux.go

+			Flags: netlink.HeaderFlagsRequest | netlink.HeaderFlagsDump,
+			Type:  38, // RTM_GETQDISC
+		},
+		Data: []byte{0},


Any particular reason why you have to pass a one byte payload? IIRC this will get padded up to 4 null bytes too. Wouldn't an empty payload work fine for a message with flags request|dump?

With an empty payload I get no response back.

mdlayher · 2017-05-12T13:57:46Z

collector/qdisc_linux.go

+	       __u32       tcm_info;
+	   };
+	*/
+	m.IfaceIdx = nlenc.Uint32(msg.Data[4:8])


You'll want to do check if len(msg.Data) < 20 or else any short messages will cause a panic. The attribute parsing routines will do bounds checking for you, so no need to check beyond that.

Done: https://github.com/ema/qdisc/blob/master/get.go#L153

- netlink-specific parts moved to github.com/ema/qdisc - avoid using shortened names - counters renamed into XXX_total

ema · 2017-05-14T12:01:21Z

Tests still TBD. Thanks for the quick reviews!

grobie

Please vendor your library with govendor.

Also it looks like you're ignoring the error in https://github.com/ema/qdisc/blob/ba575b63dffebb892b58c35c4124efe0ccb5bbbc/get.go#L205. What's the reason?

grobie · 2017-05-14T17:37:46Z

collector/qdisc_linux.go

+	}
+
+	for _, msg := range msgs {
+		// Only report root qdisc info


Prometheus comment guide line is to write them in full sentences ending on a period.

grobie · 2017-05-14T17:40:43Z

collector/qdisc_linux.go

+		), prometheus.CounterValue},
+		drops: typedDesc{prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, "qdisc", "drops_total"),
+			"Number of packets sent.",


The following help texts are copy&pasted and need to be adjusted. Ideally help texts don't just repeat the metric name, but provide actual additional information.

grobie · 2017-05-14T17:40:58Z

collector/qdisc_linux.go

+		ch <- c.drops.mustNewConstMetric(float64(msg.Drops), msg.IfaceName, msg.Kind)
+		ch <- c.requeues.mustNewConstMetric(float64(msg.Requeues), msg.IfaceName, msg.Kind)
+		ch <- c.overlimits.mustNewConstMetric(float64(msg.Overlimits), msg.IfaceName, msg.Kind)
+		//fmt.Printf("%+v\n", m)


ema · 2017-05-15T09:59:01Z

@grobie: thanks for your review! The error in ema/qdisc was ignored by mistake, that's fixed now.

grobie · 2017-05-15T10:25:30Z

Cool. As @mdlayher said, it would be great to add a test for this collector. Otherwise this looks good enough for me, but I don't really know qdisc.

mdlayher · 2017-05-15T15:13:19Z

collector/qdisc_linux.go

+		bytes: typedDesc{prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, "qdisc", "bytes_total"),
+			"Number of bytes sent.",
+			[]string{"iface", "kind"}, nil,


Looks like most network collectors use the label "device" instead of "iface". Can I get a confirmation from maintainers?

Yes, it looks like node_network_... uses device as the label. I would suggest using that for consistency.

mdlayher · 2017-05-15T15:13:48Z

Also this would need to be mentioned in the README.

Update github.com/ema/qdisc dependency to revision 2c7e72d, which includes unit testing.

ema · 2017-05-16T18:09:12Z

I've added unit tests for qdisc and end-to-end testing to the collector. Let me know if this looks sane! :)

SuperQ

LGTM!

grobie · 2017-05-17T12:20:36Z

I don't like the inclusion of test code in the collector itself. In this case, it's probably better to write tests in qdisc_linux_test.go and not use the e2e tests. I can propose something more specific tomorrow when I have more time.

ema · 2017-05-18T06:25:34Z

@grobie the idea of implementing e2e tests that way came from the wifi collector: https://github.com/prometheus/node_exporter/blob/master/collector/wifi_linux.go#L296

I'm open to alternative suggestions, but I'd like to know why you dislike the approach in this case.

Thanks!

grobie · 2017-05-18T10:28:35Z

@ema thanks for the link. I believe runtime code should not include any test code. It's a smell of bad interfaces if that's necessary. This statement also applies to wifi_linux, I wasn't aware of that collector.

Given we already include such testing style in master, I'm fine merging this PR for now. I'll open a new issue/PR to propose a different test setup.

mdlayher · 2017-05-18T15:53:48Z

@grobie I agree that the current setup isn't ideal, and would be happy to hear any suggestions you might have in mind. Because the collector constructor functions can't accept any arguments, it can be difficult to test them as of now.

grobie · 2017-05-18T21:38:57Z

The "factories" map expects any func() (Collector, error). The new function could take a wifistater as parameter, and we wrap that for the init call. Easiest will be independent go tests, without using e2e for now.

Let's tackle that in a follow up PR.

👍

ema · 2017-05-23T09:02:39Z

@SuperQ, @grobie: anything else left to do here or can the PR be merged? Thanks!

* Add qdisc collector for Linux This collector gathers basic queueing discipline metrics via netlink, similarly to what `tc -s qdisc show` does. * qdisc collector: nl-specific code moved, names fixed - netlink-specific parts moved to github.com/ema/qdisc - avoid using shortened names - counters renamed into XXX_total * Get rid of parseMessage error checking leftover * Add github.com/ema/qdisc to vendored packages * Update help texts and comments * Add qdisc collector to README file * qdisc collector end-to-end testing * Update qdisc dependency to latest version Update github.com/ema/qdisc dependency to revision 2c7e72d, which includes unit testing. * qdisc collector: rename "iface" label into "device"

) * Add commit_stats statistics --------- Signed-off-by: dongjiang1989 <dongjiang1989@126.com> Co-authored-by: Ben Kochie <superq@gmail.com>

Add qdisc collector for Linux

d909aeb

This collector gathers basic queueing discipline metrics via netlink, similarly to what `tc -s qdisc show` does.

SuperQ self-requested a review May 12, 2017 11:18

SuperQ requested changes May 12, 2017

View reviewed changes

mdlayher reviewed May 12, 2017

View reviewed changes

qdisc collector: nl-specific code moved, names fixed

dfc90ca

- netlink-specific parts moved to github.com/ema/qdisc - avoid using shortened names - counters renamed into XXX_total

Get rid of parseMessage error checking leftover

605be9f

grobie reviewed May 14, 2017

View reviewed changes

ema added 2 commits May 15, 2017 11:03

Add github.com/ema/qdisc to vendored packages

a656353

Update help texts and comments

45c98b1

mdlayher reviewed May 15, 2017

View reviewed changes

ema added 3 commits May 16, 2017 16:30

Add qdisc collector to README file

5845c3f

qdisc collector end-to-end testing

666b1ec

Update qdisc dependency to latest version

9ef83fd

Update github.com/ema/qdisc dependency to revision 2c7e72d, which includes unit testing.

qdisc collector: rename "iface" label into "device"

93ff913

SuperQ approved these changes May 17, 2017

View reviewed changes

SuperQ merged commit 047003b into prometheus:master May 23, 2017

Conversation

ema commented May 12, 2017

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 May 12, 2017

Uh oh!

mdlayher commented May 12, 2017

Uh oh!

mdlayher 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!

ema commented May 14, 2017

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!

ema commented May 15, 2017

Uh oh!

grobie commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdlayher commented May 15, 2017

Uh oh!

ema commented May 16, 2017

Uh oh!

SuperQ left a comment

Choose a reason for hiding this comment

Uh oh!

grobie commented May 17, 2017

Uh oh!

ema commented May 18, 2017

Uh oh!

grobie commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdlayher commented May 18, 2017

Uh oh!

grobie commented May 18, 2017

Uh oh!

ema commented May 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

grobie commented May 18, 2017 •

edited

Loading