diff --git a/noq-proto/src/connection/packet_builder.rs b/noq-proto/src/connection/packet_builder.rs index 338b3d758..5ba08c69e 100644 --- a/noq-proto/src/connection/packet_builder.rs +++ b/noq-proto/src/connection/packet_builder.rs @@ -281,6 +281,11 @@ impl<'a, 'b> PacketBuilder<'a, 'b> { let space_id = self.space; let (size, padded, sent) = self.finish(conn, now); + // Count only tracked sends. Off-path PATH_CHALLENGE/PATH_RESPONSE packets call + // `finish` directly and only share the path's logical identifier, not the + // network path itself, so they stay out of per-path stats. + conn.path_stats.for_path(path_id).sent_packets += 1; + let size = match padded || ack_eliciting { true => size as u16, false => 0, diff --git a/noq-proto/src/connection/stats.rs b/noq-proto/src/connection/stats.rs index dfaf02748..b70cb0414 100644 --- a/noq-proto/src/connection/stats.rs +++ b/noq-proto/src/connection/stats.rs @@ -229,6 +229,13 @@ pub struct PathStats { pub congestion_events: u64, /// Spurious congestion events on the connection. pub spurious_congestion_events: u64, + /// The number of packets sent on this path. + /// + /// This counts individual QUIC packets, which can differ from + /// [`UdpStats::datagrams`] when multiple packets are coalesced into a single UDP + /// datagram (e.g. during the handshake). Together with [`Self::lost_packets`] this + /// allows computing an accurate packet loss rate. + pub sent_packets: u64, /// The number of packets lost on this path. pub lost_packets: u64, /// The number of bytes lost on this path. @@ -262,6 +269,13 @@ pub struct ConnectionStats { pub frame_tx: FrameStats, /// Statistics about frames received on the connection. pub frame_rx: FrameStats, + /// The number of packets sent on the connection. + /// + /// This counts individual QUIC packets, which can differ from + /// [`UdpStats::datagrams`] when multiple packets are coalesced into a single UDP + /// datagram (e.g. during the handshake). Together with [`Self::lost_packets`] this + /// allows computing an accurate packet loss rate. + pub sent_packets: u64, /// The number of packets lost on the connection. pub lost_packets: u64, /// The number of bytes lost on the connection. @@ -283,6 +297,7 @@ impl std::ops::Add for ConnectionStats { cwnd: _, congestion_events: _, spurious_congestion_events: _, + sent_packets, lost_packets, lost_bytes, sent_plpmtud_probes: _, @@ -295,6 +310,7 @@ impl std::ops::Add for ConnectionStats { udp_rx: self.udp_rx + udp_rx, frame_tx: self.frame_tx + frame_tx, frame_rx: self.frame_rx + frame_rx, + sent_packets: self.sent_packets + sent_packets, lost_packets: self.lost_packets + lost_packets, lost_bytes: self.lost_bytes + lost_bytes, } @@ -314,6 +330,7 @@ impl std::ops::AddAssign for ConnectionStats { cwnd: _, congestion_events: _, spurious_congestion_events: _, + sent_packets: path_sent_packets, lost_packets: path_lost_packets, lost_bytes: path_lost_bytes, sent_plpmtud_probes: _, @@ -326,6 +343,7 @@ impl std::ops::AddAssign for ConnectionStats { udp_rx, frame_tx, frame_rx, + sent_packets, lost_packets, lost_bytes, } = self; @@ -333,6 +351,7 @@ impl std::ops::AddAssign for ConnectionStats { *udp_rx += path_udp_rx; *frame_tx += path_frame_tx; *frame_rx += path_frame_rx; + *sent_packets += path_sent_packets; *lost_packets += path_lost_packets; *lost_bytes += path_lost_bytes; } diff --git a/noq-proto/src/tests/multipath.rs b/noq-proto/src/tests/multipath.rs index c7ba73143..3569bde99 100644 --- a/noq-proto/src/tests/multipath.rs +++ b/noq-proto/src/tests/multipath.rs @@ -1161,6 +1161,99 @@ fn path_scheduling_path_status() -> TestResult { Ok(()) } +/// Tracks `sent_packets` per path and on the connection, and checks it against +/// `udp_tx.datagrams`. +/// +/// `sent_packets` counts individual QUIC packets, while `udp_tx.datagrams` counts UDP +/// datagrams. Since packets can be coalesced into a single datagram (e.g. during the +/// handshake), `sent_packets` must always be at least `udp_tx.datagrams`, and the +/// connection total must equal the sum across paths. See +/// . +#[test] +fn sent_packets_tracked_per_path() -> TestResult { + let _guard = subscribe(); + let mut pair = ConnPair::builder().enable_multipath().connect(); + + // After the handshake, path 0 has sent at least one packet, and the per-packet + // counter must be at least the per-datagram counter (they diverge when packets are + // coalesced into a single datagram). + let stats_path0 = pair.conn_mut(Client).path_stats(PathId::ZERO).unwrap(); + assert!( + stats_path0.sent_packets > 0, + "path 0 should have sent packets after the handshake" + ); + assert!( + stats_path0.sent_packets >= stats_path0.udp_tx.datagrams, + "sent_packets ({}) must be >= udp_tx.datagrams ({})", + stats_path0.sent_packets, + stats_path0.udp_tx.datagrams, + ); + + // Open a 2nd path and send some data on it. + let server_addr = pair.routes.public_server_addr(); + let path_1 = pair.open_path( + Client, + FourTuple::from_remote(server_addr), + PathStatus::Available, + )?; + pair.drive(); + assert_matches!( + pair.poll(Client), + Some(Event::Path(crate::PathEvent::Established { id })) if id == path_1 + ); + + // Put path 0 into Backup so the scheduler prefers the new path 1 for application + // data (mirrors `path_scheduling_path_status`). + pair.set_path_status(Client, PathId::ZERO, PathStatus::Backup)?; + pair.drive(); + + let path0_before = pair + .conn_mut(Client) + .path_stats(PathId::ZERO) + .unwrap() + .sent_packets; + let path1_before = pair + .conn_mut(Client) + .path_stats(path_1) + .unwrap() + .sent_packets; + + // Sending a STREAM frame goes out on path 1, so its counter should grow. Path 0 may + // still emit a few control/ACK packets, but the bulk of the new packets land on + // path 1. + let s = pair.streams(Client).open(Dir::Uni).unwrap(); + pair.send_stream(Client, s).write(b"hello").unwrap(); + pair.drive(); + + let path0_stats = pair.conn_mut(Client).path_stats(PathId::ZERO).unwrap(); + let path1_stats = pair.conn_mut(Client).path_stats(path_1).unwrap(); + let path0_delta = path0_stats.sent_packets - path0_before; + let path1_delta = path1_stats.sent_packets - path1_before; + assert!( + path1_delta > 0, + "path 1 sent_packets should grow after sending a STREAM frame" + ); + assert!( + path1_delta > path0_delta, + "the STREAM frame should drive more packets on path 1 ({path1_delta}) \ + than on path 0 ({path0_delta})" + ); + + // Per-packet counter stays consistent with per-datagram counter on each path. + assert!(path0_stats.sent_packets >= path0_stats.udp_tx.datagrams); + assert!(path1_stats.sent_packets >= path1_stats.udp_tx.datagrams); + + // The connection total must equal the sum across the live paths. + let conn_stats = pair.conn_mut(Client).stats(); + assert_eq!( + conn_stats.sent_packets, + path0_stats.sent_packets + path1_stats.sent_packets, + "connection sent_packets must equal the sum across paths" + ); + + Ok(()) +} + #[test] fn server_abandon_last_verified_path() -> TestResult { // The client abandons the last verified path the server has. The server is expected to