From 2d6581f31fb7ad790138651801ba4ee9e8b646a4 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Sun, 17 May 2026 22:47:16 +0700 Subject: [PATCH] refactor(client): encapsulate SignedDuration and enhance its API Hide internal fields to improve type safety and provide helper methods for unit conversions and negative value checks. Fix several feature-gated test compilation issues. --- README.md | 2 +- crates/irtt-cli/src/lib.rs | 13 ++--- crates/irtt-cli/src/main.rs | 17 +++--- crates/irtt-cli/src/summary.rs | 8 +-- crates/irtt-client/src/client.rs | 5 +- .../tests/probes_replies/reply_metadata.rs | 1 + .../tests/probes_replies/timing_math.rs | 4 +- crates/irtt-client/src/event.rs | 39 ++++++++------ crates/irtt-client/tests/protocol_options.rs | 2 +- crates/irtt-stats/src/normalization.rs | 2 +- crates/irtt-stats/tests/common/mod.rs | 39 ++++++++++---- crates/irtt-stats/tests/stats.rs | 52 ++++++++++--------- 12 files changed, 107 insertions(+), 77 deletions(-) diff --git a/README.md b/README.md index 1f86aa8..443b6d2 100644 --- a/README.md +++ b/README.md @@ -129,7 +129,7 @@ fn main() -> Result<(), Box> { while let Ok(event) = events.recv() { match event { ClientEvent::EchoReply { seq, rtt, .. } => { - println!("seq={seq} rtt_us={}", rtt.effective.ns / 1_000); + println!("seq={seq} rtt_us={}", rtt.effective.as_micros()); } ClientEvent::SessionClosed { .. } => break, _ => {} diff --git a/crates/irtt-cli/src/lib.rs b/crates/irtt-cli/src/lib.rs index 8f57523..9c32ed6 100644 --- a/crates/irtt-cli/src/lib.rs +++ b/crates/irtt-cli/src/lib.rs @@ -774,7 +774,7 @@ fn duration_us(duration: Duration) -> u128 { } fn signed_duration_us(duration: SignedDuration) -> i128 { - duration.ns / 1_000 + duration.as_micros() } fn format_optional_duration(duration: Option) -> String { @@ -788,7 +788,7 @@ fn format_duration(duration: Duration) -> String { } fn format_signed_duration(duration: SignedDuration) -> String { - format_signed_ns(duration.ns as f64) + format_signed_ns(duration.as_nanos() as f64) } fn format_signed_ns(ns: f64) -> String { @@ -869,8 +869,8 @@ mod tests { received_at: test_timestamp(Duration::from_secs(1) + Duration::from_micros(1500)), rtt: RttSample { raw: Duration::from_micros(1500), - adjusted: Some(SignedDuration { ns: 1_200_000 }), - effective: SignedDuration { ns: 1_200_000 }, + adjusted: Some(SignedDuration::from_nanos(1_200_000)), + effective: SignedDuration::from_nanos(1_200_000), }, server_timing: Some(ServerTiming { receive_wall_ns: Some(1_000), @@ -894,6 +894,7 @@ mod tests { } } + #[cfg(feature = "stats")] fn negative_adjusted_reply_event() -> ClientEvent { ClientEvent::EchoReply { seq: 8, @@ -902,8 +903,8 @@ mod tests { received_at: test_timestamp(Duration::from_secs(1) + Duration::from_micros(500)), rtt: RttSample { raw: Duration::from_micros(500), - adjusted: Some(SignedDuration { ns: -1_500_000 }), - effective: SignedDuration { ns: -1_500_000 }, + adjusted: Some(SignedDuration::from_nanos(-1_500_000)), + effective: SignedDuration::from_nanos(-1_500_000), }, server_timing: Some(ServerTiming { receive_wall_ns: Some(1_000), diff --git a/crates/irtt-cli/src/main.rs b/crates/irtt-cli/src/main.rs index ccca3f7..6912d54 100644 --- a/crates/irtt-cli/src/main.rs +++ b/crates/irtt-cli/src/main.rs @@ -15,7 +15,7 @@ use irtt_cli::{ }; use irtt_client::{Client, ClientEvent, OpenOutcome, RecvBudget}; -#[cfg(test)] +#[cfg(all(test, feature = "stats"))] use irtt_client::ClientTimestamp; #[cfg(feature = "stats")] @@ -247,9 +247,14 @@ impl StreamOutput<'_, W> { let line = if self.mode == OutputMode::Human && !matches!(event, ClientEvent::EchoSent { .. }) { + #[cfg(feature = "stats")] + let human_stats = stats_update.into(); + #[cfg(not(feature = "stats"))] + let human_stats = stats_update; + Some(format_human_event_with_options( event, - Some(stats_update.into()), + Some(human_stats), self.human_options, )) } else { @@ -426,9 +431,7 @@ mod tests { rtt: RttSample { raw: Duration::from_micros(rtt_us), adjusted: None, - effective: SignedDuration { - ns: i128::from(rtt_us) * 1_000, - }, + effective: SignedDuration::from_nanos(i128::from(rtt_us) * 1_000), }, server_timing: None, one_way: None, @@ -466,7 +469,7 @@ mod tests { rtt: RttSample { raw: Duration::from_micros(1200), adjusted: None, - effective: SignedDuration { ns: 1_200_000 }, + effective: SignedDuration::from_nanos(1_200_000), }, server_timing: None, one_way: None, @@ -579,7 +582,7 @@ mod tests { rtt: RttSample { raw: Duration::from_micros(1), adjusted: None, - effective: SignedDuration { ns: 1_000 }, + effective: SignedDuration::from_nanos(1_000), }, server_timing: None, one_way: None, diff --git a/crates/irtt-cli/src/summary.rs b/crates/irtt-cli/src/summary.rs index 71d882f..aef978b 100644 --- a/crates/irtt-cli/src/summary.rs +++ b/crates/irtt-cli/src/summary.rs @@ -206,8 +206,8 @@ mod tests { received_at, rtt: RttSample { raw: Duration::from_micros(1500), - adjusted: Some(SignedDuration { ns: 1_200_000 }), - effective: SignedDuration { ns: 1_200_000 }, + adjusted: Some(SignedDuration::from_nanos(1_200_000)), + effective: SignedDuration::from_nanos(1_200_000), }, server_timing: Some(ServerTiming { receive_wall_ns: None, @@ -252,8 +252,8 @@ mod tests { received_at, rtt: RttSample { raw: Duration::from_micros(1500), - adjusted: Some(SignedDuration { ns: 1_200_000 }), - effective: SignedDuration { ns: 1_200_000 }, + adjusted: Some(SignedDuration::from_nanos(1_200_000)), + effective: SignedDuration::from_nanos(1_200_000), }, server_timing: None, one_way: None, diff --git a/crates/irtt-client/src/client.rs b/crates/irtt-client/src/client.rs index 036005e..e666677 100644 --- a/crates/irtt-client/src/client.rs +++ b/crates/irtt-client/src/client.rs @@ -727,9 +727,8 @@ fn compute_rtt( let server_processing = compute_server_processing(ts); - let adjusted = server_processing.map(|sp| SignedDuration { - ns: duration_ns_i128(raw) - duration_ns_i128(sp), - }); + let adjusted = server_processing + .map(|sp| SignedDuration::from_nanos(duration_ns_i128(raw) - duration_ns_i128(sp))); let effective = adjusted.unwrap_or_else(|| SignedDuration::from_duration(raw)); RttSample { diff --git a/crates/irtt-client/src/client/tests/probes_replies/reply_metadata.rs b/crates/irtt-client/src/client/tests/probes_replies/reply_metadata.rs index 8edcf53..a765bed 100644 --- a/crates/irtt-client/src/client/tests/probes_replies/reply_metadata.rs +++ b/crates/irtt-client/src/client/tests/probes_replies/reply_metadata.rs @@ -1,3 +1,4 @@ +#[cfg(all(target_os = "linux", feature = "ancillary"))] use super::*; #[test] diff --git a/crates/irtt-client/src/client/tests/probes_replies/timing_math.rs b/crates/irtt-client/src/client/tests/probes_replies/timing_math.rs index 52ac615..85b9acf 100644 --- a/crates/irtt-client/src/client/tests/probes_replies/timing_math.rs +++ b/crates/irtt-client/src/client/tests/probes_replies/timing_math.rs @@ -18,8 +18,8 @@ fn server_processing_greater_than_raw_does_not_underflow() { ..Default::default() }, ); - assert_eq!(rtt.adjusted, Some(SignedDuration { ns: -999_999_999 })); - assert_eq!(rtt.effective, SignedDuration { ns: -999_999_999 }); + assert_eq!(rtt.adjusted, Some(SignedDuration::from_nanos(-999_999_999))); + assert_eq!(rtt.effective, SignedDuration::from_nanos(-999_999_999)); } #[test] diff --git a/crates/irtt-client/src/event.rs b/crates/irtt-client/src/event.rs index 4bc34b9..08ce284 100644 --- a/crates/irtt-client/src/event.rs +++ b/crates/irtt-client/src/event.rs @@ -117,17 +117,37 @@ pub struct RttSample { pub effective: SignedDuration, } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub struct SignedDuration { - pub ns: i128, + ns: i128, } impl SignedDuration { + pub const fn from_nanos(ns: i128) -> Self { + Self { ns } + } + pub fn from_duration(duration: Duration) -> Self { Self { ns: i128::try_from(duration.as_nanos()).unwrap_or(i128::MAX), } } + + pub const fn as_nanos(self) -> i128 { + self.ns + } + + pub const fn as_micros(self) -> i128 { + self.ns / 1_000 + } + + pub const fn as_millis(self) -> i128 { + self.ns / 1_000_000 + } + + pub const fn is_negative(self) -> bool { + self.ns < 0 + } } impl From for SignedDuration { @@ -170,18 +190,3 @@ pub struct PacketMeta { pub ecn: Option, pub kernel_rx_timestamp: Option, } - -#[cfg(test)] -mod tests { - use super::PacketMeta; - - #[test] - fn packet_meta_default_is_unavailable() { - let meta = PacketMeta::default(); - - assert_eq!(meta.traffic_class, None); - assert_eq!(meta.dscp, None); - assert_eq!(meta.ecn, None); - assert_eq!(meta.kernel_rx_timestamp, None); - } -} diff --git a/crates/irtt-client/tests/protocol_options.rs b/crates/irtt-client/tests/protocol_options.rs index d2ca5a8..ac0f059 100644 --- a/crates/irtt-client/tests/protocol_options.rs +++ b/crates/irtt-client/tests/protocol_options.rs @@ -352,7 +352,7 @@ fn assert_processing_subtracted(reply: ReplyView<'_>) { assert_eq!(processing, Duration::from_nanos(1)); let adjusted = reply.rtt.adjusted.unwrap(); assert_eq!(reply.rtt.effective, adjusted); - assert!(adjusted.ns < SignedDuration::from_duration(reply.rtt.raw).ns); + assert!(adjusted < SignedDuration::from_duration(reply.rtt.raw)); } fn assert_one_way_presence( diff --git a/crates/irtt-stats/src/normalization.rs b/crates/irtt-stats/src/normalization.rs index e682354..4aafd74 100644 --- a/crates/irtt-stats/src/normalization.rs +++ b/crates/irtt-stats/src/normalization.rs @@ -203,7 +203,7 @@ fn duration_ns_i128(duration: Duration) -> i128 { } fn signed_duration_ns(duration: SignedDuration) -> i128 { - duration.ns + duration.as_nanos() } #[cfg(test)] diff --git a/crates/irtt-stats/tests/common/mod.rs b/crates/irtt-stats/tests/common/mod.rs index 6c3954f..678964f 100644 --- a/crates/irtt-stats/tests/common/mod.rs +++ b/crates/irtt-stats/tests/common/mod.rs @@ -12,14 +12,21 @@ pub fn ts(ms: u64) -> ClientTimestamp { } } -pub fn rtt(raw_ms: u64, effective_ms: i128) -> RttSample { - let effective = SignedDuration { - ns: effective_ms * 1_000_000, - }; +pub fn adjusted_rtt(raw_ms: u64, adjusted_ms: i128) -> RttSample { + let adjusted = SignedDuration::from_nanos(adjusted_ms * 1_000_000); RttSample { raw: Duration::from_millis(raw_ms), - adjusted: Some(effective), - effective, + adjusted: Some(adjusted), + effective: adjusted, + } +} + +pub fn unadjusted_rtt(raw_ms: u64) -> RttSample { + let raw = Duration::from_millis(raw_ms); + RttSample { + raw, + adjusted: None, + effective: SignedDuration::from_duration(raw), } } @@ -35,7 +42,15 @@ pub fn sent(seq: u32, sent_at: ClientTimestamp) -> ClientEvent { } } -pub fn reply(seq: u32, raw_ms: u64, effective_ms: i128) -> ClientEvent { +pub fn adjusted_reply(seq: u32, raw_ms: u64, adjusted_ms: i128) -> ClientEvent { + reply_with_rtt(seq, raw_ms, adjusted_rtt(raw_ms, adjusted_ms)) +} + +pub fn unadjusted_reply(seq: u32, raw_ms: u64) -> ClientEvent { + reply_with_rtt(seq, raw_ms, unadjusted_rtt(raw_ms)) +} + +fn reply_with_rtt(seq: u32, raw_ms: u64, rtt: RttSample) -> ClientEvent { let sent_at = ts(seq as u64 * 10); let received_at = ClientTimestamp { mono: sent_at.mono + Duration::from_millis(raw_ms), @@ -46,7 +61,7 @@ pub fn reply(seq: u32, raw_ms: u64, effective_ms: i128) -> ClientEvent { remote: "127.0.0.1:2112".parse().unwrap(), sent_at, received_at, - rtt: rtt(raw_ms, effective_ms), + rtt, server_timing: Some(ServerTiming { receive_wall_ns: Some(unix_time_ns_after_epoch(sent_at.wall) as i64 + 1_000_000), receive_mono_ns: Some(seq as i64 * 10_000_000 + 1_000_000), @@ -69,7 +84,11 @@ pub fn reply(seq: u32, raw_ms: u64, effective_ms: i128) -> ClientEvent { } } -pub fn late_reply(seq: u32, raw_ms: u64, effective_ms: i128) -> ClientEvent { +pub fn unadjusted_late_reply(seq: u32, raw_ms: u64) -> ClientEvent { + late_reply_from(unadjusted_reply(seq, raw_ms)) +} + +fn late_reply_from(reply: ClientEvent) -> ClientEvent { let ClientEvent::EchoReply { seq, remote, @@ -82,7 +101,7 @@ pub fn late_reply(seq: u32, raw_ms: u64, effective_ms: i128) -> ClientEvent { bytes, packet_meta, .. - } = reply(seq, raw_ms, effective_ms) + } = reply else { unreachable!(); }; diff --git a/crates/irtt-stats/tests/stats.rs b/crates/irtt-stats/tests/stats.rs index cebe997..ccccf6d 100644 --- a/crates/irtt-stats/tests/stats.rs +++ b/crates/irtt-stats/tests/stats.rs @@ -2,19 +2,20 @@ mod common; use std::time::{Duration, Instant}; -use common::{late_reply, reply, sent, ts}; +use common::{adjusted_reply, sent, ts, unadjusted_late_reply, unadjusted_reply}; use irtt_client::{ClientEvent, PacketMeta}; use irtt_stats::{EventStatsUpdate, IpdvPairUpdate, SampleMode, StatsCollector, StatsConfig}; #[test] fn running_only_samples_avoid_finite_retention() { let mut collector = StatsCollector::new(StatsConfig::continuous()); - collector.process(&reply(0, 10, 9)); - collector.process(&reply(1, 20, 19)); + collector.process(&unadjusted_reply(0, 10)); + collector.process(&unadjusted_reply(1, 20)); let snapshot = collector.snapshot(); assert_eq!(snapshot.rtt.primary.median_ns, None); assert_eq!(snapshot.rtt.raw.median_ns, None); + assert_eq!(snapshot.rtt.adjusted.count, 0); assert_eq!(snapshot.rtt.adjusted.median_ns, None); assert_eq!(snapshot.ipdv.round_trip.median_ns, None); assert_eq!(snapshot.one_way_delay.send_delay.median_ns, None); @@ -25,7 +26,7 @@ fn running_only_samples_avoid_finite_retention() { fn continuous_mode_tracks_running_samples_without_exact_medians() { let mut collector = StatsCollector::new(StatsConfig::continuous()); for seq in 0..4104 { - collector.process(&reply(seq, 10, 10)); + collector.process(&unadjusted_reply(seq, 10)); } let snapshot = collector.snapshot(); @@ -38,14 +39,15 @@ fn continuous_mode_tracks_running_samples_without_exact_medians() { #[test] fn cumulative_rtt_uses_signed_effective_and_tracks_raw() { let mut collector = StatsCollector::new(StatsConfig::finite()); - collector.process(&reply(0, 1, -2)); - collector.process(&reply(1, 10, 8)); + collector.process(&adjusted_reply(0, 1, -2)); + collector.process(&adjusted_reply(1, 10, 8)); let snapshot = collector.snapshot(); assert_eq!(snapshot.rtt.primary.count, 2); assert_eq!(snapshot.rtt.primary.min_ns, Some(-2_000_000)); assert_eq!(snapshot.rtt.primary.median_ns, Some(3_000_000.0)); assert_eq!(snapshot.rtt.raw.total_ns, 11_000_000); + assert_eq!(snapshot.rtt.adjusted.count, 2); } #[test] @@ -53,8 +55,8 @@ fn late_unique_counts_and_duplicates_do_not_update_duplicate_measurements() { let mut collector = StatsCollector::new(StatsConfig::finite()); collector.process(&sent(0, ts(0))); collector.process(&sent(1, ts(10))); - collector.process(&reply(1, 10, 9)); - collector.process(&late_reply(0, 20, 19)); + collector.process(&unadjusted_reply(1, 10)); + collector.process(&unadjusted_late_reply(0, 20)); collector.process(&ClientEvent::DuplicateReply { seq: 0, remote: "127.0.0.1:2112".parse().unwrap(), @@ -84,7 +86,7 @@ fn final_loss_uses_sent_minus_unique_replies_not_echo_loss_events() { sent_at: ts(0), timeout_at: Instant::now(), }); - collector.process(&late_reply(0, 10, 9)); + collector.process(&unadjusted_late_reply(0, 10)); let snapshot = collector.snapshot(); assert_eq!(snapshot.events.loss_events, 1); @@ -114,9 +116,9 @@ fn assert_one_ipdv_pair( #[test] fn ipdv_is_sequence_adjacent_and_gap_preserving() { let mut collector = StatsCollector::new(StatsConfig::finite()); - let first = collector.process(&reply(0, 10, 10)); - let gap = collector.process(&reply(2, 15, 15)); - let adjacent = collector.process(&reply(3, 12, 12)); + let first = collector.process(&unadjusted_reply(0, 10)); + let gap = collector.process(&unadjusted_reply(2, 15)); + let adjacent = collector.process(&unadjusted_reply(3, 12)); let snapshot = collector.snapshot(); assert!(first.contributed_sample); @@ -134,8 +136,8 @@ fn ipdv_is_sequence_adjacent_and_gap_preserving() { #[test] fn late_reply_can_complete_ipdv_pair() { let mut collector = StatsCollector::new(StatsConfig::finite()); - collector.process(&reply(1, 20, 20)); - let update = collector.process(&late_reply(0, 10, 10)); + collector.process(&unadjusted_reply(1, 20)); + let update = collector.process(&unadjusted_late_reply(0, 10)); let snapshot = collector.snapshot(); @@ -149,8 +151,8 @@ fn late_reply_can_complete_ipdv_pair() { #[test] fn update_exposes_directional_ipdv_when_available() { let mut collector = StatsCollector::new(StatsConfig::finite()); - collector.process(&reply(0, 10, 10)); - let update = collector.process(&reply(1, 13, 13)); + collector.process(&unadjusted_reply(0, 10)); + let update = collector.process(&unadjusted_reply(1, 13)); assert!(update.contributed_sample); @@ -163,9 +165,9 @@ fn update_exposes_directional_ipdv_when_available() { fn gap_fill_update_exposes_both_completed_ipdv_pairs() { let mut collector = StatsCollector::new(StatsConfig::finite()); - let first = collector.process(&reply(0, 10, 10)); - let gap = collector.process(&reply(2, 20, 20)); - let fill = collector.process(&reply(1, 13, 13)); + let first = collector.process(&unadjusted_reply(0, 10)); + let gap = collector.process(&unadjusted_reply(2, 20)); + let fill = collector.process(&unadjusted_reply(1, 13)); assert!(first.contributed_sample); assert!(first.ipdv_pairs.is_empty()); @@ -192,7 +194,7 @@ fn gap_fill_update_exposes_both_completed_ipdv_pairs() { #[test] fn server_processing_and_one_way_require_available_samples() { let mut collector = StatsCollector::new(StatsConfig::finite()); - collector.process(&reply(0, 10, 9)); + collector.process(&unadjusted_reply(0, 10)); collector.process(&ClientEvent::LateReply { seq: 9, highest_seen: 10, @@ -220,8 +222,8 @@ fn server_processing_and_one_way_require_available_samples() { #[test] fn older_cumulative_server_receive_count_does_not_regress() { let mut collector = StatsCollector::new(StatsConfig::finite()); - collector.process(&reply(1, 10, 10)); - collector.process(&late_reply(0, 10, 10)); + collector.process(&unadjusted_reply(1, 10)); + collector.process(&unadjusted_late_reply(0, 10)); let snapshot = collector.snapshot(); assert_eq!(snapshot.packets.server_packets_received, Some(2)); @@ -235,8 +237,8 @@ fn rolling_count_eviction_recomputes_from_bounded_events() { rolling_time: None, }); collector.process(&sent(0, ts(0))); - collector.process(&reply(0, 10, 10)); - collector.process(&reply(1, 20, 20)); + collector.process(&unadjusted_reply(0, 10)); + collector.process(&unadjusted_reply(1, 20)); let rolling = collector.rolling_count().unwrap(); assert_eq!(rolling.packets.packets_sent, 0); @@ -276,7 +278,7 @@ fn directional_loss_uses_server_received_count_when_available() { let mut collector = StatsCollector::new(StatsConfig::finite()); collector.process(&sent(0, ts(0))); collector.process(&sent(1, ts(10))); - collector.process(&reply(0, 10, 10)); + collector.process(&unadjusted_reply(0, 10)); let loss = collector.snapshot().loss; assert_eq!(loss.lost_packets, 1);