A collector for DRBD#365
Conversation
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.
collector/drbd_linux.go
Outdated
| log.Infof("Don't know how to process string %s", field) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
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.
collector/drbd_linux.go
Outdated
|
|
||
| func (c *drbdCollector) Update(ch chan<- prometheus.Metric) (err error) { | ||
| statsFile := procFilePath("drbd") | ||
| f, err := os.Open(statsFile) |
There was a problem hiding this comment.
Just a nit, but the collectors tend to use the full variable name file for these type of uses.
collector/drbd_linux.go
Outdated
| var ( | ||
| drbdNumericalMetrics = map[string]drbdNumericalMetric{ | ||
| "ns": newDrbdNumericalMetric( | ||
| "network_sent_bytes", |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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/'
collector/drbd_linux.go
Outdated
| multiplier float64 | ||
| } | ||
|
|
||
| func newDrbdNumericalMetric(name string, desc string, valueType prometheus.ValueType, multiplier float64) drbdNumericalMetric { |
There was a problem hiding this comment.
newDrbdNumericalMetric -> newDRBDNumericalMetric (keep case consistent within acronyms, see https://github.com/golang/go/wiki/CodeReviewComments#initialisms)
Same for other occurrences below.
collector/drbd_linux.go
Outdated
| 1), | ||
| "oos": newDrbdNumericalMetric( | ||
| "out_of_sync_bytes", | ||
| "Amount of data known to be out of sync.", |
There was a problem hiding this comment.
Include the unit (bytes) in the help string as well.
collector/drbd_linux.go
Outdated
| 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) |
There was a problem hiding this comment.
This has two %s placeholders, but only one argument.
There was a problem hiding this comment.
(btw., either golint or go vet will complain about things like this)
There was a problem hiding this comment.
Nice catch! Thanks for pointing me to golint. Will use that from now on. :+1
collector/drbd_linux.go
Outdated
| 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) |
There was a problem hiding this comment.
Pro tip: use %q if you want the string to be automatically quoted in the output.
|
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
left a comment
discordianfish
left a comment
There was a problem hiding this comment.
Looking good, just some nitpicks.
collector/drbd_linux.go
Outdated
| 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]) |
There was a problem hiding this comment.
Just a question: Is this unexpected? Than info is okay, if this is expected and might happen everytime it should be logged with Debug.
There was a problem hiding this comment.
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().
collector/drbd_linux.go
Outdated
| "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.", |
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
Sure, sounds good. Fixed!
collector/drbd_linux.go
Outdated
| "pe": newDrbdNumericalMetric( | ||
| "pe": newDRBDNumericalMetric( | ||
| "remote_pending", | ||
| "Number of requests sent to the partner, but that have not yet been answered by the latter.", |
There was a problem hiding this comment.
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.
|
Looks good to me now, thanks! I'll let fish approve + merge. |
|
Looking great! Thanks a lot @EdSchouten! |
…freebsd Mark sysfs as Linux-only
4 participants
In addition to the NFS stats collector, we also wrote a similar one for parsing
/proc/drbdto 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.