Feat: Current Read/Write Data #54
Conversation
WalkthroughThe changes update the disk metrics collection by enhancing the Changes
Sequence Diagram(s)sequenceDiagram
participant CM as CollectDiskMetrics
participant IOC as disk.IOCounters
participant DD as DiskData
participant Logger as Error Logger
CM->>IOC: Request I/O stats for disk device
alt Error occurs
IOC-->>CM: Return error
CM->>Logger: Append CustomErr with device info
Note over CM: Continue to next partition
else Success
IOC-->>CM: Return I/O stats\n(ReadBytes, WriteBytes, ReadTime, WriteTime)
CM->>DD: Update DiskData with new I/O fields
end
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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: 0
🧹 Nitpick comments (1)
internal/metric/disk.go (1)
45-55: Check map key existence before accessingThe code correctly collects disk I/O metrics, but doesn't verify if the device exists in the diskIOCounts map before accessing it at line 54. Map access returns zero values when keys don't exist, but it's better to check explicitly.
- stats, _ := diskIOCounts[p.Device] + stats, exists := diskIOCounts[p.Device] + if !exists { + diskErrors = append(diskErrors, CustomErr{ + Metric: []string{"disk.read_bytes", "disk.write_bytes", "disk.read_time", "disk.write_time"}, + Error: "Device not found in IO counters: " + p.Device, + }) + continue + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/metric/disk.go(3 hunks)internal/metric/metric.go(1 hunks)
🔇 Additional comments (3)
internal/metric/metric.go (1)
55-58: Implementation of new disk I/O metrics looks good!The newly added fields for tracking disk I/O operations (ReadBytes, WriteBytes, ReadTime, WriteTime) are well-defined and properly documented. The use of pointer types is consistent with the existing pattern for fields like TotalBytes and FreeBytes.
internal/metric/disk.go (2)
16-20: Proper initialization of new fieldsThe initialization of the new fields in the defaultDiskData structure matches the existing pattern and is correctly set to nil.
70-74: Consider handling zero values for Mac environmentThe PR objective mentions that on Mac environments, these values were recorded as zero. Consider adding a comment about platform-specific behavior or check if these values are zero and add appropriate handling.
Could you verify if the gopsutil library supports these metrics across different operating systems? If not, it might be worth handling this case explicitly in the code or documenting this limitation.
There was a problem hiding this comment.
Thank you so much @Br0wnHammer. We should remove the /dev/ prefix to get real data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/metric/disk.go (1)
16-19: Consider adding unit tests for the new metrics.The PR adds several new fields to collect disk I/O metrics. Consider adding unit tests to verify that these metrics are correctly collected and reported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/metric/disk.go(3 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 (5)
internal/metric/disk.go (5)
16-16: Fix indentation inconsistency.There's a tab indentation inconsistency in the
ReadBytesfield declaration compared to the other fields that use spaces.- ReadBytes: nil, + ReadBytes: nil,
45-52: LGTM! Good error handling for I/O metrics.The error handling for disk I/O metrics is well implemented. The code correctly captures errors when attempting to collect read/write metrics and provides clear error information including both the error message and the device name.
54-55: LGTM! Correctly handles device name prefix.The code properly handles the device name by trimming the "/dev/" prefix before accessing the diskIOCounts map, which is the correct approach since the library stores data without this prefix.
71-74: LGTM! Properly populates the new disk I/O metrics.The implementation correctly assigns pointers to the disk I/O statistics to the corresponding fields in the DiskData structure.
16-19:❓ Verification inconclusive
Verify metrics collection works across different operating systems.
The PR objectives mentioned that testing in a Mac environment resulted in zero values for these metrics. Consider adding cross-platform testing to ensure these metrics work as expected across different operating systems.
🏁 Script executed:
#!/bin/bash # Check how different OS platforms handle disk I/O metrics with gopsutil # Print Go version and OS information go version uname -a # Create a simple test program to test disk I/O metrics collection cat > test_disk_io.go << 'EOF' package main import ( "fmt" "github.com/shirou/gopsutil/v4/disk" "strings" ) func main() { partitions, err := disk.Partitions(true) if err != nil { fmt.Printf("Error getting partitions: %v\n", err) return } for _, p := range partitions { if !strings.HasPrefix(p.Device, "/dev") || strings.HasPrefix(p.Device, "/dev/loop") { continue } fmt.Printf("Device: %s\n", p.Device) // Get disk I/O counters diskIOCounts, err := disk.IOCounters(p.Device) if err != nil { fmt.Printf(" Error getting I/O counters: %v\n", err) continue } deviceName := strings.TrimPrefix(p.Device, "/dev/") stats, exists := diskIOCounts[deviceName] if !exists { fmt.Printf(" No stats found for device: %s\n", deviceName) // Try to print all available keys in the map fmt.Printf(" Available keys: ") for k := range diskIOCounts { fmt.Printf("%s ", k) } fmt.Println() continue } fmt.Printf(" ReadBytes: %d\n", stats.ReadBytes) fmt.Printf(" WriteBytes: %d\n", stats.WriteBytes) fmt.Printf(" ReadTime: %d\n", stats.ReadTime) fmt.Printf(" WriteTime: %d\n", stats.WriteTime) } } EOF # Build and run the test program go mod init diskiotest go get github.com/shirou/gopsutil/v4/disk go run test_disk_io.goLength of output: 1076
Action Required: Enhance cross-platform handling for disk I/O metrics.
The test run indicates that on Linux (in the current jail/sandbox environment), the disk metrics collection fails due to an inability to open
/proc/self/mounts, resulting in an error instead of returning metrics. Since the PR noted that testing on macOS produced zero values for these metrics, it's clear that the current approach may not be robust across different operating systems.Please consider the following:
Cross-Platform Testing:
Augment the test suite to cover multiple OS environments (including macOS and Linux) to confirm the behavior and reliability of disk I/O metrics collection.Error Handling:
Implement or improve error handling ininternal/metric/disk.go(around lines 16–19) to gracefully address cases where system-specific files (e.g.,/proc/self/mounts) are unavailable.Fallback Mechanisms:
Consider adding fallback logic or corrections for environments where the expected metrics cannot be retrieved due to OS limitations.
|
Hey @mertssmnoglu, have implemented the said changes. Should be fine for merging now. |
Description
This PR introduces metrics such as
ReadBytes,WriteBytes,ReadTimeandWriteTimein the disk api.This api was tested on a Mac environment. Hence the values are 0.
Fixes
#8
Summary by CodeRabbit