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

Consider leaving meminfo values as kilobytes/kibibytes #282

Closed
smalis-msft opened this issue Aug 16, 2023 · 3 comments
Closed

Consider leaving meminfo values as kilobytes/kibibytes #282

smalis-msft opened this issue Aug 16, 2023 · 3 comments

Comments

@smalis-msft
Copy link

smalis-msft commented Aug 16, 2023

Currently all values reported in the Meminfo type are converted to bytes from their reported value in kibibytes. This can be confusing for users who expect the values reported to be unchanged from the values shown in /proc/meminfo. Given that 0.16.0 is a breaking change release coming soon, could this behavior be changed? Would you be open to a PR making this change?

@eminence
Copy link
Owner

Hi, thanks for the comment and the PR. At the moment, I'm not inclined to change this (even though we have a breaking change coming up).

I think the values are well-documented as being in bytes (though if you disagree, I'm happy to work on improving the docs). I like that procfs knows that the values are in kibibytes (despite appearing to be in kilobytes) so that procfs users don't have to know about this subtlety. And also changing the meaning of a u64 field (from bytes to kibibytes) seems like the type of change that people won't notice, and would be a big source of downstream bugs.

@smalis-msft
Copy link
Author

smalis-msft commented Aug 18, 2023

I don't disagree with any of your points honestly, and the docs are fine I think, my point is more that people tend to not read the docs, and I've had people surprised that the crate was performing any conversion at all. It's not as if doing the conversion is increasing the precision of the values, they're still the same values. It also means that if people want to use this crate to generate and compare data with something they had previously been generating by reading /proc/meminfo directly they need to undo the conversion themselves, and doing that for every single field is a real pain (and a tiny bit tricky considering the few that are unitless and don't get converted).

@eminence
Copy link
Owner

Hey @smalis-msft, sorry about my poor communication here, in letting this sit for months.

I recognize that some confusion can arise if a user is not reading procfs docs (even though you should be reading the docs 😄). And I probably could have been convinced to leave the original values back when MemInfo was added. But at this point, I don't think such an ABI change is worth the churn.

Thanks for the issue and the PR, though.

@eminence eminence closed this as not planned Won't fix, can't repro, duplicate, stale Oct 29, 2023
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 a pull request may close this issue.

2 participants