New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor diskstats #2141
Refactor diskstats #2141
Conversation
Signed-off-by: W. Andrew Denton <git@flying-snail.net>
Signed-off-by: W. Andrew Denton <git@flying-snail.net>
Signed-off-by: W. Andrew Denton <git@flying-snail.net>
) | ||
|
||
func TestDiskStats(t *testing.T) { | ||
file, err := os.Open("fixtures/proc/diskstats") | ||
type testDiskStatsCollector struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might make sense to have a package for this style of testing that works for other collectors as well, but that's all good for now I'd say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized I had mostly reimplemented testutil.GatherAndCompare(), so I dropped most of this code in favor of that. My plan is to refactor at least ethtool_linux_test to use this method, I'll include a common mechanism then. It doesn't belong in e.g. exporter-toolkit though because most of this code is just a wrapper for node_exporter's Collector type (Update()
) to make it compatible with prometheus' Collector type (Describe()
and Collect()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Just a minor nit, otherwise this looks good to me!
Signed-off-by: W. Andrew Denton <git@flying-snail.net>
…most of what was here before. Signed-off-by: W. Andrew Denton <git@flying-snail.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks
* Refactor diskstats_linux to use procfs. * Add `node_disk_info` metric. Signed-off-by: W. Andrew Denton <git@flying-snail.net> Co-authored-by: W. Andrew Denton <git@flying-snail.net>
* Refactor diskstats_linux to use procfs. * Add `node_disk_info` metric. Signed-off-by: W. Andrew Denton <git@flying-snail.net> Co-authored-by: W. Andrew Denton <git@flying-snail.net>
This refactors diskstats to use procfs. It is laying the groundwork for adding device-mapper metadata from #885 using prometheus/procfs/pull/412.
There is no user-visible change except for the addition of a new node_disk_info metric. I'm undecided if this should even be here at this point. Major/minor numbers aren't very useful. And maybe it should be
{dev="252:2"}
instead? Opinions?Also of note, the tests have been rewritten in a way I'm finally happy with. What do you think of the approach to fully render all the metrics to text and assert that the strings match?
Signed-off-by: W. Andrew Denton git@flying-snail.net