feat(diskstats): add metric for bare disk capacity#3068
feat(diskstats): add metric for bare disk capacity#3068fs185143 wants to merge 13 commits intoprometheus:masterfrom
Conversation
|
Any code that adds new functionality for parsing procfs / sysfs ought to be added to https://github.com/prometheus/procfs. There you will also find helper functions for reading / parsing files, so you won't need to implement your own. |
are you happy with using a popular library such as https://github.com/jaypipes/ghw? amended the PR to make use of that here 672a9cf (and a couple of subsequent commits) |
I am not a maintainer of this project, merely an occasional contributor, so I cannot make that decision. I doubt that the maintainers will be too excited about adding yet another third-party dependency for something as trivially implemented as this however. |
that's reasonable - the ghw library does have some neat abstractions that could be used in future, and it seems actively maintained. however, if the maintainers would prefer an update to https://github.com/prometheus/procfs then i can add a utility function there instead |
|
added a PR for a new method here: updated this PR to take advantage of the new method, but obviously it will not work until that is merged |
collector/diskstats_linux.go
Outdated
| level.Error(c.logger).Log("msg", "Failed to get device size", "err", err) | ||
| continue | ||
| } | ||
| sizeBytes := size * queueStats.LogicalBlockSize |
There was a problem hiding this comment.
As I wrote in prometheus/procfs#650 (comment), this will be incorrect for Advanced Format (AF / 4K sector) drives, since /sys/block/<dev>/size, which your proposed SysBlockDeviceSize method returns, is always reported in units of 512-byte sectors.
There was a problem hiding this comment.
updated that PR, as well as this one to use the new method
There was a problem hiding this comment.
happy with the change? wondering if we can try get these PRs moving
There was a problem hiding this comment.
LGTM but obviously this is contingent on the procfs PR being accepted first.
|
Is there any plan to further modify the merge to the main branch? @fs185143 @dswarbrick |
work in progress PR to add a metric for bare disk capacity that gets the values of
/sys/block/<device_name>/sizerelated issue: #2595