A collector for DRBD #365

EdSchouten · 2016-12-11T11:02:26Z

In addition to the NFS stats collector, we also wrote a similar one for parsing /proc/drbd to extract DRBD replication stats. This allows us to keep an eye on whether our systems are still in sync with each other, the amount of network/disk I/O, etc.

This collector exposes most of the useful information that can be found in /proc/drbd. Sizes are normalised to be in bytes, as /proc/drbd uses kibibytes.

SuperQ · 2016-12-15T20:59:20Z

collector/drbd_linux.go

+			log.Infof("Don't know how to process string %s", field)
+		}
+	}
+	return nil


Please catch scanner.Scan() errors. See https://github.com/prometheus/node_exporter/blob/master/collector/meminfo_numa_linux.go#L164-L181 for a good example.

SuperQ

LGTM.

SuperQ · 2016-12-16T10:43:48Z

collector/drbd_linux.go

+
+func (c *drbdCollector) Update(ch chan<- prometheus.Metric) (err error) {
+	statsFile := procFilePath("drbd")
+	f, err := os.Open(statsFile)


Just a nit, but the collectors tend to use the full variable name file for these type of uses.

discordianfish · 2016-12-21T11:52:22Z

collector/drbd_linux.go

+var (
+	drbdNumericalMetrics = map[string]drbdNumericalMetric{
+		"ns": newDrbdNumericalMetric(
+			"network_sent_bytes",


Counter metrics should always get prefixed with _total, see: https://prometheus.io/docs/practices/naming/. This applies to all other counters too.

Also think that something like "Total bytes sent via network." might be a better help string (same for similar metrics).

Adding the _total suffix: yes! That sounds like a good idea.

Regarding changing the documentation string: I made sure that all of the documentation strings are literal copies of how they are officially documented, except that I dropped the "in Kibyte" part. I've now restored those parts to say "in bytes".

To summarize: the strings are now the same as upstream | sed -e 's/Kibyte/bytes/'

juliusv · 2016-12-21T21:36:47Z

collector/drbd_linux.go

+	multiplier float64
+}
+
+func newDrbdNumericalMetric(name string, desc string, valueType prometheus.ValueType, multiplier float64) drbdNumericalMetric {


newDrbdNumericalMetric -> newDRBDNumericalMetric (keep case consistent within acronyms, see https://github.com/golang/go/wiki/CodeReviewComments#initialisms)

Same for other occurrences below.

juliusv · 2016-12-21T21:41:55Z

collector/drbd_linux.go

+			1),
+		"oos": newDrbdNumericalMetric(
+			"out_of_sync_bytes",
+			"Amount of data known to be out of sync.",


Include the unit (bytes) in the help string as well.

juliusv · 2016-12-21T21:44:47Z

collector/drbd_linux.go

+	file, err := os.Open(statsFile)
+	if err != nil {
+		if os.IsNotExist(err) {
+			log.Debugf("Not collecting DRBD statistics, as %s does not exist: %s", statsFile)


This has two %s placeholders, but only one argument.

(btw., either golint or go vet will complain about things like this)

Nice catch! Thanks for pointing me to golint. Will use that from now on. :+1

juliusv · 2016-12-21T21:46:35Z

collector/drbd_linux.go

+				log.Infof("Don't know how to process key-value pair [%s: %s]", kv[0], kv[1])
+			}
+		} else {
+			log.Infof("Don't know how to process string %s", field)


Pro tip: use %q if you want the string to be automatically quoted in the output.

juliusv · 2016-12-21T21:49:41Z

Thanks, looks pretty cool to me besides mine and @discordianfish's comments!

- Use the right number of printf() arguments. Use %q where it makes sense. - Use "DRBD" instead of "Drbd", per Go's style guide. - Add _total suffixes to counter metrics. - Mention the unit (bytes) in documentation strings once more.

discordianfish

Looking good, just some nitpicks.

discordianfish · 2016-12-22T15:55:44Z

collector/drbd_linux.go

 				if kv[1] == "Connected" {
 					connected = 1
 				}
 				ch <- prometheus.MustNewConstMetric(
 					drbdConnected, prometheus.GaugeValue,
 					connected, device)
 			} else {
-				log.Infof("Don't know how to process key-value pair [%s: %s]", kv[0], kv[1])
+				log.Infof("Don't know how to process key-value pair [%s: %q]", kv[0], kv[1])


Just a question: Is this unexpected? Than info is okay, if this is expected and might happen everytime it should be logged with Debug.

Good that you've brought this up. The /proc file does expose some tags that we don't process at all. Changing this to Debugf().

discordianfish · 2016-12-22T16:01:27Z

collector/drbd_linux.go

-			"Volume of net data sent to the partner via the network connection.",
+		"ns": newDRBDNumericalMetric(
+			"network_sent_bytes_total",
+			"Volume of net data sent to the partner via the network connection; in bytes.",


Sorry for nitpicking, but I find the wording a bit odd and redundant. What do you think about

"Total number of bytes sent via the network"?

Same for received and similar for disk_ metrics ("Total number of bytes read from/written to disk")

Sure, sounds good. Fixed!

discordianfish · 2016-12-22T16:02:42Z

collector/drbd_linux.go

 			"local_pending",
 			"Number of open requests to the local I/O sub-system.",
 			prometheus.GaugeValue,
 			1),
-		"pe": newDrbdNumericalMetric(
+		"pe": newDRBDNumericalMetric(
 			"remote_pending",
 			"Number of requests sent to the partner, but that have not yet been answered by the latter.",


I'd say peer instead of partner, that's more common.

They get printed all the time, as there are some tokens in the /proc file that we simply don't support. It's better to keep these as debugging messages, which may come in useful if new tags start to appear.

juliusv · 2016-12-23T17:00:30Z

Looks good to me now, thanks! I'll let fish approve + merge.

discordianfish · 2016-12-25T10:04:46Z

Looking great! Thanks a lot @EdSchouten!

…freebsd Mark sysfs as Linux-only

Ed Schouten added 2 commits December 11, 2016 11:55

Add a collector for DRBD.

6269f75

This collector exposes most of the useful information that can be found in /proc/drbd. Sizes are normalised to be in bytes, as /proc/drbd uses kibibytes.

Add myself to the AUTHORS file.

8c720ba

SuperQ requested changes Dec 15, 2016

View reviewed changes

Properly propagate parse errors.

6ff620e

SuperQ approved these changes Dec 16, 2016

View reviewed changes

Use a descriptive name for the file descriptor.

d1fa279

discordianfish requested changes Dec 21, 2016

View reviewed changes

juliusv reviewed Dec 21, 2016

View reviewed changes

Process feedback from the code review.

b7daf27

- Use the right number of printf() arguments. Use %q where it makes sense. - Use "DRBD" instead of "Drbd", per Go's style guide. - Add _total suffixes to counter metrics. - Mention the unit (bytes) in documentation strings once more.

discordianfish requested changes Dec 22, 2016

View reviewed changes

Ed Schouten added 2 commits December 23, 2016 15:55

Improve the help strings, as proposed in the code review.

4adf7fa

Reduce the severity of these messages.

b0d15ea

They get printed all the time, as there are some tokens in the /proc file that we simply don't support. It's better to keep these as debugging messages, which may come in useful if new tags start to appear.

discordianfish approved these changes Dec 25, 2016

View reviewed changes

discordianfish merged commit 71ea379 into prometheus:master Dec 25, 2016

SuperQ mentioned this pull request Jan 15, 2017

Release v0.14.0-rc.1. #423

Merged

tamcore pushed a commit to gitgrave/node_exporter that referenced this pull request Oct 22, 2024

Merge pull request prometheus#365 from rfratto/class_thermal-exclude-…

b565fef

…freebsd Mark sysfs as Linux-only

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A collector for DRBD #365

A collector for DRBD #365

EdSchouten commented Dec 11, 2016

SuperQ Dec 15, 2016

EdSchouten Dec 16, 2016

SuperQ left a comment

SuperQ Dec 16, 2016

discordianfish Dec 21, 2016

EdSchouten Dec 22, 2016

juliusv Dec 21, 2016

juliusv Dec 21, 2016

juliusv Dec 21, 2016

juliusv Dec 21, 2016

EdSchouten Dec 22, 2016

juliusv Dec 21, 2016

juliusv commented Dec 21, 2016

discordianfish left a comment

discordianfish Dec 22, 2016

EdSchouten Dec 23, 2016

discordianfish Dec 22, 2016

EdSchouten Dec 23, 2016

discordianfish Dec 22, 2016

EdSchouten Dec 23, 2016

juliusv commented Dec 23, 2016

discordianfish commented Dec 25, 2016

A collector for DRBD #365

A collector for DRBD #365

Conversation

EdSchouten commented Dec 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ 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

juliusv commented Dec 21, 2016

discordianfish 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

juliusv commented Dec 23, 2016

discordianfish commented Dec 25, 2016