[823] add detailed disk metrics #827
Conversation
archeoss
changed the title
bob/src/api/mod.rs
Outdated
| pub(crate) struct SpaceInfo { | ||
| #[serde(flatten)] | ||
| total_space: Space, | ||
| disk_space_by_disk: HashMap<DiskName, Space>, |
There was a problem hiding this comment.
Unfortunately, this is a breaking change.
@idruzhitskiy can we support both old and this new formats in OldPartitionsRemover?
There was a problem hiding this comment.
There is another problem here: you try to combine logical and physical structure in this map. Probably, it is better to split them. occupied_disk_space_by_disk is created to represent space info within logical structure. I suggest to keep it without changes (occupied_disk_space_by_disk: HashMap<DiskName, u64>) and add additional disk_space_by_disk for physical structure (its key will be a mount point of the disk, not its name)
pub(crate) struct SpaceInfo {
#[serde(flatten)]
total_space: Space,
occupied_disk_space_by_disk: HashMap<DiskName, u64>, // key is a Bob disk name
disk_space_by_disk: HashMap<String, Space> // Key is mount point
}
bob/src/api/mod.rs
Outdated
| @@ -145,12 +145,18 @@ pub(crate) struct NodeConfiguration { | |||
| } | |||
|
|
|||
| #[derive(Debug, Serialize)] | |||
| pub(crate) struct SpaceInfo { | |||
| struct Space { | |||
| total_disk_space_bytes: u64, | |||
| free_disk_space_bytes: u64, | |||
| used_disk_space_bytes: u64, | |||
| occupied_disk_space_bytes: u64, | |||
There was a problem hiding this comment.
If we will decide to allow breaking changes, then this field should be renamed to occupied_by_bob_disk_space_bytes. Such a name will better reflect the meaning of the field
| occupied_disk_space_by_disk: HashMap<DiskName, u64>, | ||
|
|
||
| /// Key - Mount point or Bob disk name if no mount point was found | ||
| disk_space_by_disk: HashMap<String, Space>, |
There was a problem hiding this comment.
I just thought that it would be better to use disk name as the key (on Linux it is sda1, sdb1, on Windows it is C:, D:), because this is the common way of identification of disks in OS
There was a problem hiding this comment.
I think, it would be better to do it later in another request. Right now, mount point would be enough here
bob/src/hw_metrics_collector.rs
Outdated
| for mount_point in disks.keys() { | ||
| /// Maps [`DiskSpaceMetrics`] to the corresponding mount point. | ||
| /// | ||
| /// `f_fsid` field of [`statvfs`] is used to ensure that disks are unique. |
There was a problem hiding this comment.
According to the code of the function, you return mount point as the key. Please, fix the comment
bob/src/hw_metrics_collector.rs
Outdated
| pub(crate) fn update_space_metrics(&self) -> DiskSpaceMetrics { | ||
| /// Returns the updated space metrics of this [`HWMetricsCollector`]. | ||
| /// | ||
| /// Key -- `f_fsid` of underlying [`statvfs`] function, represents file system ID |
There was a problem hiding this comment.
Same here. Comment is incorrect
bob/src/hw_metrics_collector.rs
Outdated
| let bsize = stat.f_bsize; | ||
| let blocks = stat.f_blocks; | ||
| let bavail = stat.f_bavail; | ||
| let bfree = stat.f_bfree; |
There was a problem hiding this comment.
Please, return back explicit type declaration, because it is required for proper calculations. On some targets the type of all this values can be 'u32' instead of 'u64', so it is better to explicitly convert it to u64 here to avoid overflow on multiplication operation later
There was a problem hiding this comment.
nerd mode on
Theoretically, if that's the case, it shouldn't even build at this point. Multiplication of two u32 numbers produces another u32 number, but DiskSpaceMetrics has u64 field, and rust prohibits any implicit type conversions, thus the compiler should produce the error
In order to convert the type we must use either as <type> syntax or .into() method
nerd mode off
But maybe I'm missing something
In any case, the type annotations are placed back, they wouldn't hurt
| } | ||
|
|
||
| #[derive(Debug, Serialize)] | ||
| pub(crate) struct SpaceInfo { |
There was a problem hiding this comment.
Please update openapi in config-examples
There was a problem hiding this comment.
I think it would be good to integrate utoipa for openapi auto-gen at some point
Closes #823