Skip to content
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

feat(diskstats): add metric for bare disk capacity #3068

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

fs185143
Copy link

@fs185143 fs185143 commented Jul 2, 2024

work in progress PR to add a metric for bare disk capacity that gets the values of /sys/block/<device_name>/size

related issue: #2595

@dswarbrick
Copy link
Contributor

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.

@fs185143
Copy link
Author

fs185143 commented Jul 2, 2024

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)

@fs185143 fs185143 requested a review from dswarbrick July 2, 2024 13:09
@dswarbrick
Copy link
Contributor

are you happy with using a popular library such as https://github.com/jaypipes/ghw?

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.

@fs185143
Copy link
Author

fs185143 commented Jul 2, 2024

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

@fs185143
Copy link
Author

fs185143 commented Jul 3, 2024

added a PR for a new method here: prometheus/procfs#650 prometheus/procfs#658

updated this PR to take advantage of the new method, but obviously it will not work until that is merged

level.Error(c.logger).Log("msg", "Failed to get device size", "err", err)
continue
}
sizeBytes := size * queueStats.LogicalBlockSize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated that PR, as well as this one to use the new method

d8e74d6

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy with the change? wondering if we can try get these PRs moving

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but obviously this is contingent on the procfs PR being accepted first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants