Add ARP collector for Linux #540

skottler · 2017-04-02T18:43:21Z

This introduces a simple ARP collector with initial support for Linux. It parses /proc/net/arp and emits a count of ARP entries on a per-device basis.

/cc #535

grobie

Thanks, looks good in general. The consensus has been to handle procfs parsing in https://github.com/prometheus/procfs and use that implementation here though.

grobie · 2017-04-02T19:14:48Z

collector/arp_linux.go

+	scanner := bufio.NewScanner(data)
+	entries := make(map[string]uint32)
+
+	for scanner.Scan() {


scanner.Scan() can fail. You need to check scanner.Err() and handle that.

grobie · 2017-04-02T19:16:57Z

collector/arp_linux.go

+	return parseArpEntries(file), nil
+}
+
+func parseArpEntries(data *os.File) map[string]uint32 {


The only requirement is to pass something implementing the io.Reader interface, so I'd reflect that in the function signature.

grobie · 2017-04-02T19:17:57Z

collector/arp_linux.go

+	entries := make(map[string]uint32)
+
+	for scanner.Scan() {
+		columns := strings.Split(string(scanner.Text()), " ")


scanner.Text() returns a string. The type conversion is unnecessary.

grobie · 2017-04-02T19:18:44Z

collector/arp_linux.go

+	entries := make(map[string]uint32)
+
+	for scanner.Scan() {
+		columns := strings.Split(string(scanner.Text()), " ")


Also, check strings.Fields()

skottler · 2017-04-02T19:39:27Z

Thanks for the insanely quick feedback @grobie! 🙇‍♂️ I've adopted the changes you suggested.

The consensus has been to handle procfs parsing in https://github.com/prometheus/procfs and use that implementation here though.

This PR implements only a tiny portion of what potentially needs to get parsed from /proc/net/arp as it only covers what's needed to expose the count per device. Is it okay to add a very much incomplete API to the procfs package or would you prefer that more complete parsing support gets implemented first?

grobie · 2017-04-02T19:47:50Z

You're lucky I had to push my hiking trip back by one day. Now I'm bored in Patagonia and have time to review PRs ;-) I'm fine with either merging the current state with a comment saying something along the lines of "if more /proc/net/arp parsing support is needed, move this to prometheus/procfs" or by adding full parsing support to procfs now. Shouldn't be to hard to parse the whole line and return a slice like []ARPEntry.

…

On Sun, Apr 2, 2017 at 4:39 PM Sam Kottler ***@***.***> wrote: Thanks for the insanely quick feedback @grobie <https://github.com/grobie>! 🙇 I've adopted the changes you suggested. The consensus has been to handle procfs parsing in https://github.com/prometheus/procfs and use that implementation here though. This PR implements only a tiny portion of what potentially needs to get parsed from /proc/arp/net as it only covers what's needed to expose the count per device. Is it okay to add a very much incomplete API to the procfs package or would you prefer that more complete parsing support gets implemented first? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#540 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaGCHJLZgjnK7MJcKmrMSgmEGoFuHks5rr_lvgaJpZM4Mw4_K> .

grobie · 2017-04-02T20:11:20Z

collector/arp_linux.go

+	entries := make(map[string]uint32)
+
+	for scanner.Scan() {
+		if err := scanner.Err(); err != nil {


Err() will always be nil if Scan() is not nil. Check the example in https://golang.org/pkg/bufio/#example_Scanner_custom

skottler · 2017-04-02T20:30:36Z

I'm fine with either merging the current state with a comment saying something along the lines of "if more /proc/net/arp parsing support is needed, move this to prometheus/procfs" or by adding full parsing support to procfs now.

I added a comment in that vain to the current parsing function. If you're comfortable merging this in its current state I'm happy to follow up with /proc/net/arp support in the procfs package and then to switch node_exporter to use that. Doing that now or after merge are both fine with me.

skottler · 2017-04-02T20:47:22Z

# netbsd-armv5

>> building binaries

 >   node_exporter

# github.com/prometheus/node_exporter

/usr/local/go/pkg/tool/linux_amd64/link: running arm-linux-gnueabi-gcc failed: exit status 1

/tmp/go-link-863345215/000001.o: In function `_cgo_41b2da71c790_C2func_getaddrinfo':

/usr/local/go/src/net/cgo-gcc-prolog:45: warning: Using 'getaddrinfo' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

/usr/lib/gcc-cross/arm-linux-gnueabi/6/../../../../arm-linux-gnueabi/lib/crt1.o: In function `_start':

(.text+0x34): undefined reference to `main'

collect2: error: ld returned 1 exit status

The tests look to be failing on netbsd with ARMv5. I hope that's a CI or build issue rather than actual failure because debugging that is gonna be...challenging 😂

juliusv · 2017-04-02T21:00:04Z

collector/arp_linux.go

+	Factories["arp"] = NewArpCollector
+}
+
+// NewArpCollector returns a new Collector exposing ARP stats.


Arp -> ARP (https://github.com/golang/go/wiki/CodeReviewComments#initialisms)

Same below.

juliusv · 2017-04-02T21:03:20Z

collector/arp_linux.go

+	return &arpCollector{
+		count: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, "arp",
+				"count"), "ARP entries by device",


This is kind of a confusing indentation/line break combination. If a function call is broken into multiple lines, I wouldn't then expect some other argument to follow it on the same line...

How about:

count: prometheus.NewDesc( prometheus.BuildFQName(Namespace, "arp", "count"), "ARP entries by device", []string{"device"}, nil, ),

juliusv · 2017-04-02T21:03:54Z

collector/arp_linux.go

+	return entries, nil
+}
+
+// This should get extracted to the github.com/prometheus/procfs package to


Add a TODO: to the beginning of this.

juliusv · 2017-04-02T21:05:24Z

@skottler Regarding the test errors, I don't see how you could be causing them, given that you are not introducing any architecture-specific functionality, or even cgo stuff...

mdlayher · 2017-04-02T21:22:25Z

collector/arp_linux.go

+	for scanner.Scan() {
+		columns := strings.Fields(scanner.Text())
+
+		if columns[0] != "IP" {


This will panic if strings.Fields returns an empty slice, so it's probably worth adding a bounds check too.

https://play.golang.org/p/09sEiI2gEN

👋 Hey Sam!

Yes, always bounds check first!

skottler · 2017-04-02T23:44:17Z

@juliusv I've addressed your feedback in 43c7480, c1e6b6a, and eb3934a.

@mdlayher 👋 hope you're doing well! I addressed your comment in b5ba102.

grobie

A few more comments ;-)

grobie · 2017-04-02T23:41:59Z

collector/arp_linux.go

+func NewARPCollector() (Collector, error) {
+	return &arpCollector{
+		count: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, "arp", "count"),


We don't include things like count in gauge metric names. I'd suggest node_arp_entries or maybe node_arp_hosts?

grobie · 2017-04-02T23:51:25Z

collector/arp_linux.go

+	for scanner.Scan() {
+		columns := strings.Fields(scanner.Text())
+
+		if len(columns) > 0 {


Sorry for all the nitpicking. Idiomatic go tries to reduce the amount of nesting as much as possible, so if len(columns) == 0 { continue } would be more common here. Though, I think we should actually return an error here, such line would be completely unexpected and tell us about either a broken or changed ARP file. So I suggest:

if len(columns) < 6 { return nil, fmt.Errorf("unexpected ARP table file") } // ...

grobie · 2017-04-02T23:51:54Z

collector/arp_linux.go

+// to support more complete parsing of /proc/net/arp. Instead of adding
+// more fields to this function's return values it should get moved and
+// changed to support each field.
+func parseArpEntries(data io.Reader) (map[string]uint32, error) {


grobie · 2017-04-02T23:52:49Z

collector/arp_linux.go

+	}, nil
+}
+
+func getArpEntries() (map[string]uint32, error) {


grobie · 2017-04-02T23:53:59Z

collector/arp_linux.go

+		return fmt.Errorf("could not get ARP entries: %s", err)
+	}
+
+	for device, entryCount := range entries {


As a note, it's common in go to use single letter variables in such short loops. I'm fine with the descriptive names though.

skottler · 2017-04-03T00:06:34Z

@grobie thanks! I've updated this including your feedback in 532677e.

mdlayher · 2017-04-03T00:19:34Z

Seems like this would be a reasonable collector to enable by default as well. Thoughts on that from anyone?

grobie · 2017-04-03T00:23:35Z

👍 Please also add it to the readme.

…

On Sun, Apr 2, 2017, 21:19 Matt Layher ***@***.***> wrote: Seems like this would be a reasonable collector to enable by default as well. Thoughts on that from anyone? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#540 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaOGIDJtC4cUvesSdBasE9llh7T1_ks5rsDsWgaJpZM4Mw4_K> .

skottler · 2017-04-03T00:27:11Z

Seems like this would be a reasonable collector to enable by default as well.

Done in 5ebb24e.

Please also add it to the readme.

Done in 2a82e3d.

grobie · 2017-04-03T01:51:36Z

I believe the test failures are the result of @juliusv latest change to build Prometheus components with go1.8 by default. Maybe our build yoda @sdurrheimer can help here? :-)

discordianfish

This is great, thanks!

grobie · 2017-04-11T15:45:05Z

Build issues fixed in master.

* Implement commonalities and linux support for ARP collection * Add ARP collector to fixtures and run as part of e2e tests * Bubble up scanner errors * Use single return values where it makes sense * Add missing annotation * Move arp_common into arp_linux * Add license header to arp_linux.go * Address initial feedback * Use strings.Fields instead of strings.Split * Deal with scanner.Err() rather than throwing away errors * Check for scan errors in-line before interacting with the entries map * Don't interact with potentially empty text from scan * Check for scan errors outside the scan loop * Add comment about moving procfs parsing * Add more direct comment * Update initialism style to match go style guide * Put function args on the same line * Add TODO in front of comment about procfs extraction * Guard against strings.Fields returning an empty slice * Be more defensive about ARP table format and use upcase more broadly * Enable the ARP collector by default * Add ARP collector to the README * Remove 'entry'

Sam Kottler added 2 commits April 2, 2017 14:16

Implement commonalities and linux support for ARP collection

93353b9

Add ARP collector to fixtures and run as part of e2e tests

2e736f7

skottler mentioned this pull request Apr 2, 2017

Add neighbor stats #535

Open

Sam Kottler added 5 commits April 2, 2017 14:52

Bubble up scanner errors

5582792

Use single return values where it makes sense

1474e2f

Add missing annotation

a494342

Move arp_common into arp_linux

9fb9c96

Add license header to arp_linux.go

5aa46a6

grobie reviewed Apr 2, 2017

View reviewed changes

Sam Kottler added 3 commits April 2, 2017 15:23

Address initial feedback

6f83d85

Use strings.Fields instead of strings.Split

8879321

Deal with scanner.Err() rather than throwing away errors

0f9471a

Sam Kottler added 2 commits April 2, 2017 15:43

Check for scan errors in-line before interacting with the entries map

58a9830

Don't interact with potentially empty text from scan

8cce0e2

grobie reviewed Apr 2, 2017

View reviewed changes

Sam Kottler added 2 commits April 2, 2017 16:25

Check for scan errors outside the scan loop

d21c5f9

Add comment about moving procfs parsing

6686259

Add more direct comment

ab20f5d

juliusv reviewed Apr 2, 2017

View reviewed changes

mdlayher reviewed Apr 2, 2017

View reviewed changes

Sam Kottler added 3 commits April 2, 2017 19:37

Update initialism style to match go style guide

eb3934a

Put function args on the same line

c1e6b6a

Add TODO in front of comment about procfs extraction

43c7480

Guard against strings.Fields returning an empty slice

b5ba102

grobie reviewed Apr 2, 2017

View reviewed changes

Be more defensive about ARP table format and use upcase more broadly

532677e

Sam Kottler added 2 commits April 2, 2017 20:26

Enable the ARP collector by default

5ebb24e

Add ARP collector to the README

2a82e3d

Remove 'entry'

c89fb35

rtreffer mentioned this pull request Apr 3, 2017

Add cpufreq node_exporter collector #538

Closed

discordianfish approved these changes Apr 7, 2017

View reviewed changes

grobie merged commit 6eafa51 into prometheus:master Apr 11, 2017

skottler deleted the arp branch April 11, 2017 16:48

skottler mentioned this pull request Aug 18, 2018

Initial support for parsing /proc/net/arp prometheus/procfs#105

Closed

TheMeier mentioned this pull request Aug 3, 2019

rebase Initial support for parsing /proc/net/arp #105 from @skottler prometheus/procfs#199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ARP collector for Linux #540

Add ARP collector for Linux #540

skottler commented Apr 2, 2017 •

edited

Loading

grobie left a comment

grobie Apr 2, 2017

grobie Apr 2, 2017

grobie Apr 2, 2017

grobie Apr 2, 2017

skottler commented Apr 2, 2017 •

edited

Loading

grobie commented Apr 2, 2017 via email

grobie Apr 2, 2017

skottler commented Apr 2, 2017 •

edited

Loading

skottler commented Apr 2, 2017

juliusv Apr 2, 2017

juliusv Apr 2, 2017

juliusv Apr 2, 2017

juliusv commented Apr 2, 2017

mdlayher Apr 2, 2017

juliusv Apr 2, 2017

skottler commented Apr 2, 2017

grobie left a comment

grobie Apr 2, 2017

grobie Apr 2, 2017

grobie Apr 2, 2017

grobie Apr 2, 2017

grobie Apr 2, 2017

skottler commented Apr 3, 2017

mdlayher commented Apr 3, 2017

grobie commented Apr 3, 2017 via email

skottler commented Apr 3, 2017

grobie commented Apr 3, 2017

discordianfish left a comment

grobie commented Apr 11, 2017

Add ARP collector for Linux #540

Add ARP collector for Linux #540

Conversation

skottler commented Apr 2, 2017 • edited Loading

grobie 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

skottler commented Apr 2, 2017 • edited Loading

grobie commented Apr 2, 2017 via email

Choose a reason for hiding this comment

skottler commented Apr 2, 2017 • edited Loading

skottler commented Apr 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusv commented Apr 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skottler commented Apr 2, 2017

grobie 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

skottler commented Apr 3, 2017

mdlayher commented Apr 3, 2017

grobie commented Apr 3, 2017 via email

skottler commented Apr 3, 2017

grobie commented Apr 3, 2017

discordianfish left a comment

Choose a reason for hiding this comment

grobie commented Apr 11, 2017

skottler commented Apr 2, 2017 •

edited

Loading

skottler commented Apr 2, 2017 •

edited

Loading

skottler commented Apr 2, 2017 •

edited

Loading