search

eminence / procfs

Rust library for reading the Linux procfs filesystem
Other
358 stars 105 forks source link

Consider leaving meminfo values as kilobytes/kibibytes #282

Closed smalis-msft closed 9 months ago

smalis-msft commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 9 months ago

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 :smile:). 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.