Multi partition support #32
Conversation
- Now, RoundFloatPtr uses the RoundFloat in itself
…and handle disk partitions (#7)
WalkthroughThe changes involve modifications to the disk metrics collection within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DiskMetricsCollector
participant DiskData
User->>DiskMetricsCollector: Request disk metrics
DiskMetricsCollector->>DiskData: Initialize metricsSlice
DiskMetricsCollector->>DiskMetricsCollector: Retrieve disk partitions
alt Error retrieving partitions
DiskMetricsCollector->>DiskMetricsCollector: Append CustomErr to diskErrors
DiskMetricsCollector->>User: Return default disk data and errors
else No errors
DiskMetricsCollector->>DiskData: Gather metrics for each partition
alt Error gathering metrics
DiskMetricsCollector->>DiskMetricsCollector: Append error to diskErrors
end
DiskMetricsCollector->>User: Return populated metrics slice
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
internal/metric/metric_math.go (1)
16-20: LGTM! Consider enhancing the documentation.The implementation is correct and efficient. The function properly handles rounding using
math.Roundandmath.Pow.Consider adding examples to the documentation:
// Round a float to a given precision and return the result +// Example: +// RoundFloat(3.14159, 2) // Returns 3.14 +// RoundFloat(3.14159, 4) // Returns 3.1416 func RoundFloat(val float64, precision uint) float64 {internal/metric/disk.go (1)
45-46: Plan implementation for disk I/O metrics.The TODOs indicate missing read/write speed implementations. These metrics would be valuable for monitoring disk performance.
Would you like me to help implement the disk I/O metrics collection using
disk.IOCounters()? I notice there's commented-out code at the bottom of the file that could be repurposed for this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
internal/metric/disk.go(2 hunks)internal/metric/metric.go(1 hunks)internal/metric/metric_math.go(1 hunks)
🔇 Additional comments (3)
internal/metric/metric_math.go (1)
12-14: LGTM! Clean implementation.
Good refactoring to reuse the RoundFloat function, following the DRY principle.
internal/metric/disk.go (1)
21-23: Good use of partitions(false) to filter unnecessary mounts.
The false parameter effectively filters out pseudo-filesystems like /run/user/1000, improving the relevance of collected metrics.
internal/metric/metric.go (1)
52-57: LGTM! DiskData structure properly updated for multi-partition support.
The addition of the MountPoint field and updated comments accurately reflect the multi-partition changes.
Let's verify the struct usage across the codebase:
✅ Verification successful
DiskData structure is correctly used with the new MountPoint field
The verification shows that all DiskData struct initializations in the codebase properly include the MountPoint field:
- Default initialization sets it to empty string:
MountPoint: "" - Actual usage correctly sets it to diskUsage.Path:
MountPoint: diskUsage.Path
The struct is used consistently and all required fields are properly initialized. No issues were found with the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct struct initialization that might need updating
# and verify JSON field usage in tests and documentation
# Search for DiskData struct initialization
rg "DiskData{" -A 5
# Search for mount_point field usage in tests
rg '"mount_point"' --type go
Length of output: 228
Script:
#!/bin/bash
# Let's fix the regex and try again with proper escaping
rg "DiskData\{" -A 5
# Also search for any DiskData variable declarations or assignments
ast-grep --pattern 'var $_ DiskData'
ast-grep --pattern '$_ := DiskData'
ast-grep --pattern '$_ = DiskData'
# Check for DiskData usage in function parameters and returns
ast-grep --pattern 'func $_($_: DiskData)'
ast-grep --pattern 'func $_($_) DiskData'
# Look for test files that might use DiskData
fd ".*_test.go$" --exec grep -l "DiskData" {}
Length of output: 948
|
|
||
| // Set all flag to false to get only necessary partitions | ||
| // Avoiding unnecessary partitions like /run/user/1000, /run/credentials | ||
| partitions, partErr := disk.Partitions(false) |
There was a problem hiding this comment.
This returns a lot of partitions that we aren't interested in, I think we need to refine how we parse out which partitions we want information on.
Is it possible to filter out all loopbacks? That would clean this up a lot.
Example, my current lsblk output which matches what disk.Partitions returns:
loop0 7:0 0 4K 1 loop /snap/bare/5
loop1 7:1 0 55.4M 1 loop /snap/core18/2846
loop2 7:2 0 274M 1 loop /snap/firefox/5361
loop3 7:3 0 273M 1 loop /snap/firefox/5273
loop4 7:4 0 245.5M 1 loop /snap/gitkraken/247
loop5 7:5 0 91.7M 1 loop /snap/gtk-common-themes/1535
loop6 7:6 0 504.2M 1 loop /snap/gnome-42-2204/172
loop7 7:7 0 73.9M 1 loop /snap/core22/1663
loop8 7:8 0 505.1M 1 loop /snap/gnome-42-2204/176
loop9 7:9 0 248.9M 1 loop /snap/gitkraken/249
loop10 7:10 0 73.9M 1 loop /snap/core22/1722
loop11 7:11 0 12.9M 1 loop /snap/snap-store/1113
loop12 7:12 0 38.8M 1 loop /snap/snapd/21759
loop13 7:13 0 12.2M 1 loop /snap/snap-store/1216
loop14 7:14 0 44.3M 1 loop /snap/snapd/23258
loop15 7:15 0 150.3M 1 loop /snap/thunderbird/581
loop16 7:16 0 150.6M 1 loop /snap/thunderbird/585
nvme0n1 259:0 0 1.8T 0 disk
├─nvme0n1p1 259:1 0 190M 0 part /boot/efi
├─nvme0n1p2 259:2 0 128M 0 part
├─nvme0n1p3 259:3 0 962G 0 part
We're actually only interested innvme0n1 here, all the loops can be ignored
There was a problem hiding this comment.
Now I'm thinking about reading only devices instead of mount points. This will show us
/dev/nvme0n1p2 => 0.80 usage
/dev/nvme0n1p1 => 0.30 usage
/dev/sda1 => 0.70 usage
I think this is more clear than the previous one. (home, boot, efi, dev ...)
Preview
{
"device": "/dev/sda1",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 30713544704,
"free_bytes": 6828425216,
"usage_percent": 0.7777
}Are you ok with that? @ajhollid
There was a problem hiding this comment.
@mertssmnoglu I think that makes more sense! Sounds good to me 👍 Thanks for making the changes!
There was a problem hiding this comment.
The current changes should work for it.
There was a problem hiding this comment.
@mertssmnoglu sorry for the late reply, just having a chance to look at this now.
This is better, but I still have all the loop backs in my response:
{
"data": [
{
"device": "/dev/nvme0n1p7",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 931258499072,
"free_bytes": 719357485056,
"usage_percent": 0.1862
},
{
"device": "/dev/loop0",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 131072,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop1",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 58064896,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop2",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 286261248,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop3",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 77463552,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop4",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 260964352,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop5",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 528744448,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop6",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 77463552,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop7",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 157941760,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop13",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 157679616,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop14",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 46530560,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop8",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 529661952,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop10",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 13631488,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop15",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 257556480,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop16",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 287440896,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop11",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 40763392,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop9",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 96206848,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/loop12",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 12845056,
"free_bytes": 0,
"usage_percent": 1
},
{
"device": "/dev/nvme0n1p1",
"read_speed_bytes": null,
"write_speed_bytes": null,
"total_bytes": 195035136,
"free_bytes": 66547712,
"usage_percent": 0.6588
}
],
"errors": null
}
There was a problem hiding this comment.
This mat be related to this issue, unless I'm reading that discussion incorreclty it seems that psutil ignores the all flag for all systems due to how BSD behaves 🤔
This seems ot be the case, the output is the same whether I pass true or false for the all flag.
There was a problem hiding this comment.
That's interesting. I added a new condition to if statement in 636c7e7
…of MountPoint and improve partition filtering
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
internal/metric/disk.go (3)
Line range hint
12-20: Consider meaningful defaults for error cases.While initializing fields as nil is valid, consider providing meaningful default values when returning this structure in error cases. This could help downstream consumers handle error scenarios more gracefully.
defaultDiskData := []*DiskData{ { - Device: "", + Device: "unknown", ReadSpeedBytes: nil, WriteSpeedBytes: nil, TotalBytes: nil, FreeBytes: nil, UsagePercent: nil, }, }
54-55: Address TODO comments for disk I/O metrics.The TODOs for read and write speed implementation should be tracked.
Would you like me to help create a GitHub issue to track the implementation of disk I/O metrics? I can provide example code for implementing these metrics using
gopsutil.
Line range hint
67-82: Consider preserving disk I/O implementation logic.While removing the commented trial code improves cleanliness, consider documenting the
IOCountersapproach in the GitHub issue for implementing read/write speeds, as it provides a good starting point for the TODO items.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
internal/metric/disk.go(2 hunks)internal/metric/metric.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/metric/metric.go
🔇 Additional comments (4)
internal/metric/disk.go (4)
4-7: LGTM! Clean import organization.
The imports are well-organized and the removal of the aliased import improves code readability.
25-27: Good implementation of partition filtering.
Setting all=false effectively addresses the previous review comments about filtering unnecessary partitions. This is a clean solution to avoid system-specific partitions like /run/user/1000.
38-40: Effective partition filtering logic.
The combination of checking for already processed devices and filtering non-device paths is a robust approach. The HasPrefix("/dev") check effectively implements the suggestion from the previous review to focus on actual devices.
36-60: Verify handling of different device types.
Let's ensure the implementation works correctly across different device types (nvme, sda, etc.).
…error handling in CollectDiskMetrics
This branch is based on develop
Rounding
There are math rounding functions for the metrics. All of the float numbers are rounded to 4 digit.
RoundFloatPtrfor the reference typesRoundFloatfor the primitive typeMulti Storage Support
Get all partitions as an array and get the disk usage metrics for the each one
CollectDiskMetricsfunction with multi partition support all useful partitions instead of only/MountPointfield to DiskData struct! Please make sure that this changes are works for the our test-setup.
Summary by CodeRabbit
New Features
Device, to the disk data structure for clearer representation of disk metrics.Bug Fixes
Documentation