feat: add more socket features #4
Conversation
WalkthroughThe changes introduce advanced socket monitoring and analysis capabilities, expanding the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SocketMonitor
participant OS
participant SocketInfo
participant SocketStats
User->>SocketMonitor: Start monitoring (interval=500ms)
loop Every 500ms
SocketMonitor->>OS: Query sockets info
OS-->>SocketMonitor: Return sockets list & stats
SocketMonitor->>SocketInfo: Construct with metadata & stats
SocketMonitor->>User: Emit event (Opened/Closed/StateChanged) with SocketInfo
User->>SocketStats: Analyze stats (if present)
SocketStats-->>User: Provide health report & metrics
end
User->>SocketMonitor: Stop monitoring after 30s
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 6
🔭 Outside diff range comments (2)
src/socket/monitor.rs (1)
122-124:⚠️ Potential issueFix incorrect protocol fallback mapping.
The fallback logic incorrectly maps
Protocol::Icmp,Protocol::Raw, andProtocol::Other(_)toProtocol::Tcp. This could mislead consumers about the actual protocol type.protocol: match socket.protocol { Protocol::Tcp => Protocol::Tcp, Protocol::Udp => Protocol::Udp, - Protocol::Icmp | Protocol::Raw | Protocol::Other(_) => { - Protocol::Tcp - } + other => other, },This same issue exists in lines 161-163 and 203-205.
src/socket/platform.rs (1)
710-720:⚠️ Potential issueTests reference the wrong platform module – unconditional compile error
Within the
#[cfg(target_os = "linux")]block the test still callsmacos::get_process_info, which is unavailable on Linux builds; the same pattern exists in the Windows branch. This will not compile:let process_info = macos::get_process_info(pid);Replace with the platform-appropriate module (or gate the inner call with the same
cfg):- let process_info = macos::get_process_info(pid); + let process_info = linux::get_process_info(pid);Repeat for the Windows section.
🧹 Nitpick comments (5)
src/socket/monitor.rs (1)
8-8: Add missing SocketFlags import for consistency.The import is missing
SocketFlagswhich is used in similar files (tcp.rs,udp.rs) for socket metadata. While not used here, it should be included for consistency.-use crate::socket::platform::{SocketFamily, SocketInfo, SocketType}; +use crate::socket::platform::{SocketFamily, SocketFlags, SocketInfo, SocketType};examples/socket_analysis.rs (2)
79-84: Add safety checks for potential division by zero in time calculations.The RTT calculations could potentially panic if the time values are unexpected or if the conversion operations result in division by zero.
Consider adding safety checks:
if let Some(rtt) = stats.rtt { - println!(" RTT: {:.2}ms", rtt.as_secs_f32() * 1000.0); + let rtt_ms = rtt.as_secs_f32() * 1000.0; + if rtt_ms.is_finite() { + println!(" RTT: {:.2}ms", rtt_ms); + } if let Some(rtt_var) = stats.rtt_variance { - println!(" RTT variance: {:.2}ms", rtt_var.as_secs_f32() * 1000.0); + let var_ms = rtt_var.as_secs_f32() * 1000.0; + if var_ms.is_finite() { + println!(" RTT variance: {:.2}ms", var_ms); + } } }
41-45: Consider making the monitoring duration configurable.The example runs for a fixed 30 seconds without user control. For a more flexible demonstration, consider making this configurable via command-line arguments.
+use std::env; fn main() -> pree::Result<()> { + let duration = env::args() + .nth(1) + .and_then(|s| s.parse().ok()) + .unwrap_or(30); + // Create a socket monitor with a shorter interval for more frequent updates let mut monitor = SocketMonitor::new().interval(Duration::from_millis(500)); // ... existing code ... - // Monitor for 30 seconds - thread::sleep(Duration::from_secs(30)); + // Monitor for specified duration + println!("Monitoring sockets for {} seconds...", duration); + thread::sleep(Duration::from_secs(duration));src/socket/platform.rs (2)
395-405:window_utilizationsilently divides by zero
send_windowmay legally be zero during zero-window events. Guard before division:- if cwnd > 0 { + if cwnd > 0 && send_win > 0 {(or return
None). PreventsNaNpropagation and keeps semantics consistent with the other helper methods.
182-213:calculate_quality_scorecan underflow below 0With large penalties the score may drop below 0 before the final
clamp, causing unnecessary negative intermediate values. That’s harmless but avoidable; compute each penalty withmin(remaining, …)or track remaining budget to keep the mental model clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/socket_analysis.rs(1 hunks)src/socket/monitor.rs(4 hunks)src/socket/platform.rs(8 hunks)src/socket/tcp.rs(2 hunks)src/socket/udp.rs(2 hunks)
🔇 Additional comments (3)
src/socket/tcp.rs (1)
1-1: LGTM: Import additions are appropriate.The added imports for
SocketFamily,SocketFlags, andSocketTypeare necessary for the enrichedSocketInfometadata.src/socket/udp.rs (1)
3-3: LGTM: Import additions are appropriate.The added imports for
SocketFamily,SocketFlags, andSocketTypeare necessary for the enrichedSocketInfometadata.examples/socket_analysis.rs (1)
1-48: LGTM: Well-structured example demonstrating new socket features.The example effectively demonstrates the new socket monitoring and analysis capabilities. The code is well-organized with clear separation of concerns and proper error handling for the main monitoring operations.
| socket_family: Some(SocketFamily::Inet), | ||
| socket_type: Some(SocketType::Stream), | ||
| socket_flags: Some(SocketFlags { | ||
| non_blocking: false, | ||
| close_on_exec: false, | ||
| }), | ||
| socket_options: None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider retrieving actual socket flags instead of hard-coding defaults.
The socket metadata fields are correctly set for TCP sockets (Inet family, Stream type), but the socket_flags are hard-coded to default values (non_blocking: false, close_on_exec: false). This may not accurately reflect the actual socket state.
Consider retrieving the actual socket flags from the system or making this configurable:
- socket_flags: Some(SocketFlags {
- non_blocking: false,
- close_on_exec: false,
- }),
+ socket_flags: None, // TODO: Retrieve actual flags from systemAlternatively, implement platform-specific code to query the actual socket flags using system calls like fcntl() on Unix systems.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| socket_family: Some(SocketFamily::Inet), | |
| socket_type: Some(SocketType::Stream), | |
| socket_flags: Some(SocketFlags { | |
| non_blocking: false, | |
| close_on_exec: false, | |
| }), | |
| socket_options: None, | |
| socket_family: Some(SocketFamily::Inet), | |
| socket_type: Some(SocketType::Stream), | |
| socket_flags: None, // TODO: Retrieve actual flags from system | |
| socket_options: None, |
🤖 Prompt for AI Agents
In src/socket/tcp.rs around lines 160 to 166, the socket_flags are currently
hard-coded to non_blocking: false and close_on_exec: false, which may not
reflect the actual socket state. To fix this, replace the hard-coded values by
retrieving the actual socket flags from the system, for example by using
platform-specific system calls like fcntl() on Unix to query the socket's
current flags, and set socket_flags accordingly. This ensures the metadata
accurately represents the socket's real configuration.
| socket_type: match socket.protocol { | ||
| Protocol::Tcp => Some(SocketType::Stream), | ||
| Protocol::Udp => Some(SocketType::Datagram), | ||
| _ => None, | ||
| }, | ||
| socket_family: Some(SocketFamily::Inet), | ||
| socket_flags: None, | ||
| socket_options: None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract socket metadata creation to reduce code duplication.
The socket metadata assignment logic is duplicated three times (here, lines 168-175, and 210-217). This violates the DRY principle and makes maintenance harder.
Consider extracting this logic into a helper function:
+fn create_socket_metadata(protocol: Protocol) -> (Option<SocketType>, Option<SocketFamily>, Option<SocketFlags>, Option<SocketOptions>) {
+ let socket_type = match protocol {
+ Protocol::Tcp => Some(SocketType::Stream),
+ Protocol::Udp => Some(SocketType::Datagram),
+ _ => None,
+ };
+ (socket_type, Some(SocketFamily::Inet), None, None)
+}Then use it in all three locations:
+ let (socket_type, socket_family, socket_flags, socket_options) = create_socket_metadata(socket.protocol);
socket_type: match socket.protocol {
Protocol::Tcp => Some(SocketType::Stream),
Protocol::Udp => Some(SocketType::Datagram),
_ => None,
},
- socket_family: Some(SocketFamily::Inet),
- socket_flags: None,
- socket_options: None,
+ socket_type,
+ socket_family,
+ socket_flags,
+ socket_options,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| socket_type: match socket.protocol { | |
| Protocol::Tcp => Some(SocketType::Stream), | |
| Protocol::Udp => Some(SocketType::Datagram), | |
| _ => None, | |
| }, | |
| socket_family: Some(SocketFamily::Inet), | |
| socket_flags: None, | |
| socket_options: None, | |
| let (socket_type, socket_family, socket_flags, socket_options) = | |
| create_socket_metadata(socket.protocol); | |
| socket_type, | |
| socket_family, | |
| socket_flags, | |
| socket_options, |
🤖 Prompt for AI Agents
In src/socket/monitor.rs around lines 129 to 136, the socket metadata creation
code is duplicated in three places, violating the DRY principle. Extract this
socket metadata assignment logic into a separate helper function that returns
the constructed metadata struct. Replace the duplicated code in all three
locations with calls to this new helper function to centralize and simplify
maintenance.
| socket_family: Some(SocketFamily::Inet), | ||
| socket_type: Some(SocketType::Datagram), | ||
| socket_flags: Some(SocketFlags { | ||
| non_blocking: false, | ||
| close_on_exec: false, | ||
| }), | ||
| socket_options: None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider retrieving actual socket flags instead of hard-coding defaults.
The socket metadata fields are correctly set for UDP sockets (Inet family, Datagram type), but the socket_flags are hard-coded to default values. This mirrors the same issue in tcp.rs.
For consistency with the TCP implementation, consider the same approach:
- socket_flags: Some(SocketFlags {
- non_blocking: false,
- close_on_exec: false,
- }),
+ socket_flags: None, // TODO: Retrieve actual flags from systemThis should be addressed consistently across both TCP and UDP implementations.
🤖 Prompt for AI Agents
In src/socket/udp.rs around lines 149 to 155, the socket_flags field is
hard-coded with default values instead of retrieving the actual socket flags. To
fix this, modify the code to obtain the real socket flags from the socket
configuration or system call, similar to the approach used in tcp.rs, ensuring
consistency between UDP and TCP implementations.
| if let Some(util) = stats.bandwidth_utilization() { | ||
| println!(" Bandwidth Utilization: {util:.1}%"); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify bandwidth_utilization() method handles edge cases properly.
The code assumes bandwidth_utilization() returns a valid percentage, but should verify the method handles cases where bandwidth calculation might fail or return invalid values.
🏁 Script executed:
#!/bin/bash
# Description: Check if bandwidth_utilization method has proper error handling
# Expected: Method should handle edge cases like zero bandwidth or division by zero
ast-grep --pattern $'impl SocketStats {
$$$
fn bandwidth_utilization($$$) -> Option<$_> {
$$$
}
$$$
}'Length of output: 115
🏁 Script executed:
#!/bin/bash
# Locate the SocketStats implementation and the bandwidth_utilization method
rg -n "impl SocketStats" -R .
rg -n "fn bandwidth_utilization" -R .Length of output: 881
🏁 Script executed:
#!/bin/bash
# Locate and display the bandwidth_utilization implementation
rg -n 'fn bandwidth_utilization' -g '*.rs' .
rg -n -C5 'fn bandwidth_utilization' -g '*.rs' .Length of output: 936
🏁 Script executed:
#!/bin/bash
# Display the full bandwidth_utilization implementation for review
sed -n '220,280p' src/socket/platform.rsLength of output: 1955
Guard against zero connection_duration to avoid division by zero
The current bandwidth_utilization() checks rtt_secs > 0.0 but still divides by self.connection_duration?.as_secs_f32() without verifying that the duration is non-zero. If connection_duration is zero, this yields Inf/NaN which then gets clamped to 100%, masking the invalid case.
• File: src/socket/platform.rs, method bandwidth_utilization
• Add a check for duration.as_secs_f32() > 0.0 (similar to the rtt_secs guard), returning None when it’s zero
Suggested diff:
pub fn bandwidth_utilization(&self) -> Option<f32> {
if let (Some(cwnd), Some(mss)) = (self.congestion_window, self.snd_mss) {
if let Some(rtt) = self.rtt {
let rtt_secs = rtt.as_secs_f32();
if rtt_secs > 0.0 {
- let max_bandwidth = (cwnd * mss) as f32 / rtt_secs;
- let actual_bandwidth =
- self.bytes_sent as f32 / self.connection_duration?.as_secs_f32();
- Some((actual_bandwidth / max_bandwidth * 100.0).min(100.0))
+ let max_bandwidth = (cwnd * mss) as f32 / rtt_secs;
+ // Ensure we don't divide by zero on connection_duration
+ if let Some(duration) = self.connection_duration {
+ let duration_secs = duration.as_secs_f32();
+ if duration_secs > 0.0 {
+ let actual_bandwidth = self.bytes_sent as f32 / duration_secs;
+ return Some((actual_bandwidth / max_bandwidth * 100.0).min(100.0));
+ }
+ }
+ None
} else {
None
}
} else {
None
}
} else {
None
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/socket/platform.rs around the bandwidth_utilization method, add a check
to ensure that connection_duration.as_secs_f32() is greater than 0.0 before
performing the division. If the duration is zero, return None to avoid division
by zero and prevent returning invalid bandwidth utilization values. This will
improve the method's robustness by properly handling edge cases where connection
duration might be zero.
| // Get socket options using lsof | ||
| for socket in &mut sockets { | ||
| if let Some(pid) = socket.process_id { | ||
| if let Ok(output) = Command::new("lsof") | ||
| .args([ | ||
| "-p", | ||
| &pid.to_string(), | ||
| "-i", | ||
| &format!("{}:{}", socket.local_addr.ip(), socket.local_addr.port()), | ||
| "-F", | ||
| "n", | ||
| ]) | ||
| .output() | ||
| { | ||
| let output = String::from_utf8_lossy(&output.stdout); | ||
| socket.socket_options = Some(parse_socket_options(&output)); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
lsof executed per-socket → severe performance hit
Inside the macOS collector a separate lsof command is spawned for every socket. On busy machines this will:
- Fork hundreds/thousands of processes.
- Introduce large latency and CPU spikes.
- Hammer the kernel with redundant identical queries.
Refactor to a single call (e.g. lsof -p <pid> -F n) per process and post-process in memory, or drop to libproc/sysctl APIs.
🤖 Prompt for AI Agents
In src/socket/platform.rs around lines 1316 to 1335, the current code runs a
separate lsof command for each socket, causing severe performance issues.
Refactor this by running a single lsof command per unique process ID with
arguments like `lsof -p <pid> -F n` to get all socket info at once. Then, parse
the combined output in memory to assign socket options to each socket,
eliminating redundant lsof calls and reducing CPU and latency overhead.
| fn parse_tcp_stats_line(line: &str, target_inode: u64) -> Option<SocketStats> { | ||
| let parts: Vec<&str> = line.split_whitespace().collect(); | ||
| if parts.len() < 17 { | ||
| return None; | ||
| } | ||
|
|
||
| // Extract inode | ||
| let inode = parts[9].parse::<u64>().ok()?; | ||
| if inode != target_inode { | ||
| return None; | ||
| } | ||
|
|
||
| let mut stats = SocketStats { | ||
| bytes_sent: 0, | ||
| bytes_received: 0, | ||
| packets_sent: 0, | ||
| packets_received: 0, | ||
| errors: 0, | ||
| retransmits: 0, | ||
| rtt: None, | ||
| congestion_window: None, | ||
| send_queue_size: None, | ||
| receive_queue_size: None, | ||
| slow_start_threshold: None, | ||
| send_window: None, | ||
| receive_window: None, | ||
| rtt_variance: None, | ||
| min_rtt: None, | ||
| max_rtt: None, | ||
| rtt_samples: None, | ||
| retransmit_timeout: None, | ||
| snd_mss: None, | ||
| rcv_mss: None, | ||
| snd_una: None, | ||
| snd_nxt: None, | ||
| rcv_nxt: None, | ||
| congestion_control: None, | ||
| congestion_state: None, | ||
| sack_enabled: false, | ||
| ecn_enabled: false, | ||
| ecn_ce_count: None, | ||
| sack_blocks: None, | ||
| sack_reordering: None, | ||
| out_of_order_packets: None, | ||
| duplicate_acks: None, | ||
| zero_window_events: None, | ||
| connection_duration: None, | ||
| }; | ||
|
|
||
| // Parse TCP stats | ||
| if let (Ok(send_queue), Ok(receive_queue)) = | ||
| (parts[4].parse::<u32>(), parts[5].parse::<u32>()) | ||
| { | ||
| stats.send_queue_size = Some(send_queue); | ||
| stats.receive_queue_size = Some(receive_queue); | ||
| } | ||
|
|
||
| if let (Ok(snd_una), Ok(snd_nxt)) = (parts[10].parse::<u32>(), parts[11].parse::<u32>()) { | ||
| stats.snd_una = Some(snd_una); | ||
| stats.snd_nxt = Some(snd_nxt); | ||
| } | ||
|
|
||
| if let (Ok(rcv_nxt), Ok(rcv_mss)) = (parts[12].parse::<u32>(), parts[13].parse::<u32>()) { | ||
| stats.rcv_nxt = Some(rcv_nxt); | ||
| stats.rcv_mss = Some(rcv_mss); | ||
| stats.snd_mss = Some(rcv_mss); // MSS is same for both directions | ||
| } | ||
|
|
||
| if let (Ok(rtt), Ok(rttvar)) = (parts[14].parse::<u32>(), parts[15].parse::<u32>()) { | ||
| stats.rtt = Some(Duration::from_micros(rtt as u64)); | ||
| stats.rtt_variance = Some(Duration::from_micros(rttvar as u64)); | ||
| } | ||
|
|
||
| if let Ok(cwnd) = parts[16].parse::<u32>() { | ||
| stats.congestion_window = Some(cwnd); | ||
| } | ||
|
|
||
| Some(stats) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compile-time breakage: missing fields in SocketStats initialisers
Every manual construction of SocketStats (here in linux::parse_tcp_stats_line, windows::get_tcp_stats, and windows::parse_netstat_line) omits the newly-added fields connection_quality_score and state_history (and in some blocks several more).
Rust’s struct-literal syntax is non-exhaustive: all fields must be provided (or ..Default::default() used).
The current code will fail to compile with E0063: missing field on every platform.
Minimal, uniform fix:
- let mut stats = SocketStats {
- bytes_sent: 0,
- /* … many fields … */
- zero_window_events: None,
- connection_duration: None,
- };
+ let mut stats = SocketStats {
+ // explicitly initialised fields…
+ ..Default::default()
+ };and add
#[derive(Debug, Clone, Default)]
pub struct SocketStats { … }This removes a huge amount of repetition and guarantees future additions won’t silently break other modules.
Also applies to: 983-1020, 1078-1102
🤖 Prompt for AI Agents
In src/socket/platform.rs around lines 833 to 912, the manual construction of
the SocketStats struct is missing newly added fields like
connection_quality_score and state_history, causing compile errors due to
non-exhaustive struct initialization. To fix this, replace the explicit field
initializations with the struct literal followed by ..Default::default() to fill
in all other fields with their default values. This approach should be applied
uniformly in all similar SocketStats constructions in this file, including lines
983-1020 and 1078-1102, to prevent future breakage when fields are added.
|
Closing in favour of #5 |
Summary by CodeRabbit
New Features
Documentation