-
Notifications
You must be signed in to change notification settings - Fork 99
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
fix: correct smartctl_device_bytes_written & smartctl_device_bytes_read for NVMe #211
Conversation
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.
Couple comments but LGTM otherwise.
…ad for NVMe The NVMe specification says that the controller is responsible for reporting "Data Units Read" & "Data Units Written" converted as needed for logicial block sizes other than 512-bytes. smartmontools already has the correct behavior. What is correct in this case? For now, track what smartmontools does: take the counter, multiply by 512*1000, report the value. We should be clear that it means the drive has read/written at most that many bytes. This has a few impacts: - NVME devices will now show these metrics, if they did not before. - NVME devices with blocksize other than 512-bytes may have previously reported inflated metrics, but are now corrected (is this worthy of larger notice in changelogs?) Reference: https://github.com/smartmontools/smartmontools/blob/11415ee0b9d5f4a22ddfb3722fdfb05e72372a03/smartmontools/nvmeprint.cpp#L394-L397 Closes: prometheus-community#122 Signed-off-by: Robin H. Johnson <[email protected]>
@SuperQ bump |
@SuperQ bump again |
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.
We could work around the precision by wrapping the int128 counter with a 2^53 modulus. I've done this for converting uint64 counters.
* [CHANGE] `slog` used for logging instead go logger prometheus-community#246 * [ENHANCEMENT] Added support for `megaraid` devices and device types prometheus-community#205 prometheus-community#257 * [BUGFIX] Better support for smartmontools < 7.3 prometheus-community#238 * [BUGFIX] Corrected NVMe read/write bytes to NVMe metrics prometheus-community#211 Signed-off-by: Konstantin Shalygin <[email protected]>
* [CHANGE] `slog` used for logging instead go logger #246 * [ENHANCEMENT] Added support for `megaraid` devices and device types #205 #257 * [BUGFIX] Better support for smartmontools < 7.3 #238 * [BUGFIX] Corrected NVMe read/write bytes to NVMe metrics #211 Signed-off-by: Konstantin Shalygin <[email protected]>
The NVMe specification says that the controller is responsible for reporting "Data Units Read" & "Data Units Written" converted as needed for logicial block sizes other than 512-bytes. smartmontools already has the correct** behavior.
What is correct in this case? For now, track what smartmontools does: take the counter, multiply by 512*1000, report the value.
We should be clear that it means the drive has read/written at most that many bytes.
This has a few impacts:
Reference: https://github.com/smartmontools/smartmontools/blob/11415ee0b9d5f4a22ddfb3722fdfb05e72372a03/smartmontools/nvmeprint.cpp#L394-L397
Closes: #122