Add ZFS Collector + review feedback from 213 #369
Conversation
It is tested on FreeBSD 10.2-RELEASE and Linux (ZFS on Linux 0.6.5.4). On FreeBSD, Solaris, etc. ZFS metrics are exposed through sysctls. ZFS on Linux exposes the same metrics through procfs `/proc/spl/...`. In addition to sysctl metrics, 'computed metrics' are exposed by the collector, which are based on several sysctl values. There is some conditional logic involved in computing these metrics which cannot be easily mapped to PromQL. Not all 92 ARC sysctls are exposed right now but this can be changed with one additional LOC each.
Use ConstMetric with UntypedValue. Handle nozfs build flag. Remove computed values.
collector now does not expose metrics if stats cannot be fetched.
Instantiate a zfsMetricProvider per Update() call.
Remove predefined list of metrics. Remove C code for accessing sysctls on FreeBSD, use sysctl(8) instead.
This patch makes stylistic changes to error strings, unexports method names by lower casing them, removes unused dataSetMetric, and adds copyright/licence information. Signed-Off-By: Corey Stewart <stewa169@purdue.edu>
Signed-Off-By: Corey Stewart <stewa169@purdue.edu>
| } | ||
|
|
||
| func (c *zfsCollector) Update(ch chan<- prometheus.Metric) (err error) { | ||
| err = c.zfsAvailable() |
There was a problem hiding this comment.
This causes the stats file to be opened twice. This can be skipped and just use the error returned by updateArcstats()
|
@SuperQ Thanks for taking a look. @cbstewart9393's internship ended a few days ago, but I'll ping him and see if he can make the requested update and squash it with his fixup patch. Do you have any thoughts on the other question he asked about the commit count? It's quite high, but the original commits aren't his. I'm not sure if he can squash down those commits and still maintain @problame's ownership of the work. If y'all don't care about the commit count, then we'll leave it as is. If you do, we need some feedback from @problame on how to proceed...or mess around with rebasing and see if we can maintain ownership appropriately. |
|
At a minimum I would squash with |
|
Shouldn't be a problem to squash @problame's commits, as long as you don't squash them together with @cbstewart9393 |
|
592c716 removes FreeBSD support (which I would be willing to add back if/when the PR is merged), but leaves behind FreeBSD build tags and test files. The current version of the PR likely doesn't compile on FreeBSD at all. |
|
@discordianfish @dominikh @SuperQ Thanks for taking a look again, and sorry for the delay (winter holiday/vacation). To distill down into concrete steps:
Idea: Should we just squash ALL commits from @problame into a single commit and call it a day? I personally like commits that actually implement an entire feature, but we'll go with whatever strategy you all prefer. Note on number 3: I took a look at the tags on @cbstewart9393's fork and didn't notice any tags that we had created...can you offer a pointer to what you want removed, @dominikh? |
Yes, that would be best. The goal is simply to have a commit history where every commit is can be built and used without errors.
I think we all agree that this is the way to go: Every commit should implement something useful on it's own. No need to overthink this, just try to have the commit history make sense. |
|
@dominikh Ah, I didn't even know that was there. Thank you for catching that, and we'll remove it in the FreeBSD removal commit. @discordianfish We'll squash away all fixes then and leave some smaller number of feature commits from @problame (4 or 5 I would guess, based on a quick look at the commit history). Thanks! |
Remove FreeBSD support for zfs. Signed-Off-By: Corey Stewart <stewa169@purdue.edu> Remove FreeBSD files
|
@SuperQ @discordianfish @dominikh @cbstewart9393 I am in favor of closing this PR and resuming any further discussions in PR #410. |
|
SGTM |
Add check for sysfs build tag
This is a follow on to PR #213, it includes all commits from that PR submitted by plus additional commits addressing feedback from that PR. Is it necessary to bring the commit count down? If so, how many commits would it be appropriate to get down to?