From caf6d015de49a8fe6ecf4022d6033e79b3f404c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Fri, 11 Sep 2026 15:39:59 +0200 Subject: [PATCH 1/4] feat(proto): Add `PathStats::sent_packets` --- noq-proto/src/connection/mod.rs | 2 -- noq-proto/src/connection/packet_builder.rs | 14 ++++++++ noq-proto/src/connection/stats.rs | 41 ++++++++++++++++++++++ noq-proto/src/tests/mod.rs | 24 +++++++++++++ 4 files changed, 79 insertions(+), 2 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 5a89682297..7078be5a13 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -1871,8 +1871,6 @@ impl Connection { builder.finish_and_track(now, self, path_id, PadDatagram::ToSize(probe_size)); - self.path_stats.get_mut(path_id).sent_plpmtud_probes += 1; - Some(self.build_transmit(path_id, transmit)) } diff --git a/noq-proto/src/connection/packet_builder.rs b/noq-proto/src/connection/packet_builder.rs index 59bab52083..683a0dfccc 100644 --- a/noq-proto/src/connection/packet_builder.rs +++ b/noq-proto/src/connection/packet_builder.rs @@ -286,6 +286,20 @@ impl<'a, 'b> PacketBuilder<'a, 'b> { false => 0, }; + // `lost_packets` is not incremented for MTUD probes, so we avoid incrementing + // `sent_packets` for it as well. + if conn.path_data(path_id).mtud.in_flight_mtu_probe() == Some(packet_number) { + conn.path_stats.get_mut(path_id).sent_plpmtud_probes += 1; + } else { + // Needs to be incremented in the same place that constructs `SentPacket`s, + // to ensure it matches `lost_packets` (`lost_packets <= sent_packets`). + // Similarly, `sent_bytes` needs to match how `lost_bytes` are incremented. + // Hence, the weird case of sent_packets increasing, but sent_bytes not, + // because the `size` above is what makes it into `lost_bytes` eventually. + conn.path_stats.get_mut(path_id).sent_packets += 1; + conn.path_stats.get_mut(path_id).sent_bytes += size as u64; + } + let packet = SentPacket { path_generation: conn.paths.get_mut(&path_id).unwrap().data.generation(), largest_acked: sent.largest_acked, diff --git a/noq-proto/src/connection/stats.rs b/noq-proto/src/connection/stats.rs index a5121a46de..c6ffa596ae 100644 --- a/noq-proto/src/connection/stats.rs +++ b/noq-proto/src/connection/stats.rs @@ -232,6 +232,29 @@ pub struct PathStats { pub congestion_events: u64, /// Spurious congestion events on the connection. pub spurious_congestion_events: u64, + /// The number of QUIC packets sent on this path. + /// + /// The intention for this stat is to capture all the packets we send that are congestion controlled + /// and which we *expect to be transmitted*. + /// + /// This does *not* count off-path path challenges, off-path path responses, any path challenges + /// or responses sent for NAT traversal or MTUD probes, for which the above is not the case (we + /// expect these packets to not make it in many cases). + /// + /// This value should be meaningful relative to [`Self::lost_packets`], packets this way can get lost + /// and will be counted in lost packets. + /// + /// This counts individual QUIC packets, which may differ from [`UdpStats::datagrams`] when + /// packets are coalesced into a single UDP datagram. + pub sent_packets: u64, + /// The total number of QUIC bytes sent on this path (sum of all sent packet sizes). + /// + /// This counts only the QUIC packet payload bytes, not UDP/IP header bytes. + /// + /// This also does not count coalesced packets. + /// + /// Caveats similar to the ones in [`Self::sent_packets`] apply. + pub sent_bytes: u64, /// The number of packets lost on this path. pub lost_packets: u64, /// The number of bytes lost on this path. @@ -265,6 +288,14 @@ pub struct ConnectionStats { pub frame_tx: FrameStats, /// Statistics about frames received on the connection. pub frame_rx: FrameStats, + /// The number of QUIC packets sent on the connection (sum across all paths). + /// + /// See also [`PathStats::sent_packets`]. + pub sent_packets: u64, + /// The total number of QUIC bytes sent on the connection (sum across all paths). + /// + /// See also [`PathStats::sent_bytes`]. + pub sent_bytes: u64, /// The number of packets lost on the connection. pub lost_packets: u64, /// The number of bytes lost on the connection. @@ -290,6 +321,8 @@ impl std::ops::Add for ConnectionStats { cwnd: _, congestion_events: _, spurious_congestion_events: _, + sent_packets, + sent_bytes, lost_packets, lost_bytes, sent_plpmtud_probes: _, @@ -302,6 +335,8 @@ 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, + sent_bytes: self.sent_bytes + sent_bytes, lost_packets: self.lost_packets + lost_packets, lost_bytes: self.lost_bytes + lost_bytes, #[cfg(test)] @@ -323,6 +358,8 @@ impl std::ops::AddAssign for ConnectionStats { cwnd: _, congestion_events: _, spurious_congestion_events: _, + sent_packets: path_sent_packets, + sent_bytes: path_sent_bytes, lost_packets: path_lost_packets, lost_bytes: path_lost_bytes, sent_plpmtud_probes: _, @@ -335,6 +372,8 @@ impl std::ops::AddAssign for ConnectionStats { udp_rx, frame_tx, frame_rx, + sent_packets, + sent_bytes, lost_packets, lost_bytes, #[cfg(test)] @@ -344,6 +383,8 @@ 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; + *sent_bytes += path_sent_bytes; *lost_packets += path_lost_packets; *lost_bytes += path_lost_bytes; } diff --git a/noq-proto/src/tests/mod.rs b/noq-proto/src/tests/mod.rs index 50ad621e56..8a694edcb0 100644 --- a/noq-proto/src/tests/mod.rs +++ b/noq-proto/src/tests/mod.rs @@ -5111,3 +5111,27 @@ fn regression_initial_coalescing_large_cid() { pair.time += Duration::from_secs(5); pair.drive_client(); // this used to try to build a packet without enough datagram space } + +/// Snapshot test to prevent accidentally skipping code-paths related to `sent_packets` stats. +#[test] +fn sent_packets_stats_snapshot_test() { + let _guard = subscribe(); + let mut pair = Pair::default(); + let (client_ch, server_ch) = pair.connect(); + pair.drive(); + + let client_stats = pair.client_conn_mut(client_ch).stats(); + let server_stats = pair.server_conn_mut(server_ch).stats(); + assert_eq!(client_stats.sent_packets, 9); + assert_eq!(server_stats.sent_packets, 6); + + let s = pair.client_streams(client_ch).open(Dir::Uni).unwrap(); + const MSG: &[u8] = b"Hello, World!"; + pair.client_send(client_ch, s).write(MSG).unwrap(); + pair.drive(); + + let client_stats2 = pair.client_conn_mut(client_ch).stats(); + let server_stats2 = pair.server_conn_mut(server_ch).stats(); + assert_eq!(client_stats2.sent_packets, 10); + assert_eq!(server_stats2.sent_packets, 7); +} From 1b62ec187159b65c3ded6d6ddaefbf3b2571fa24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Tue, 7 Jul 2026 11:41:53 +0200 Subject: [PATCH 2/4] cr: Adjust documentation --- noq-proto/src/connection/stats.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/noq-proto/src/connection/stats.rs b/noq-proto/src/connection/stats.rs index c6ffa596ae..76390fd61f 100644 --- a/noq-proto/src/connection/stats.rs +++ b/noq-proto/src/connection/stats.rs @@ -235,7 +235,7 @@ pub struct PathStats { /// The number of QUIC packets sent on this path. /// /// The intention for this stat is to capture all the packets we send that are congestion controlled - /// and which we *expect to be transmitted*. + /// and which we *expect to be received*. /// /// This does *not* count off-path path challenges, off-path path responses, any path challenges /// or responses sent for NAT traversal or MTUD probes, for which the above is not the case (we @@ -250,8 +250,11 @@ pub struct PathStats { /// The total number of QUIC bytes sent on this path (sum of all sent packet sizes). /// /// This counts only the QUIC packet payload bytes, not UDP/IP header bytes. + /// It also doesn't count ACK-only packets in an effort to stay consistent with + /// [`Self::lost_bytes`], which doesn't count ACK-only packets either. /// - /// This also does not count coalesced packets. + /// If you're interested in the full amount of bytes transmitted, consider looking + /// at [`ConnectionStats::udp_tx`]. /// /// Caveats similar to the ones in [`Self::sent_packets`] apply. pub sent_bytes: u64, @@ -290,11 +293,13 @@ pub struct ConnectionStats { pub frame_rx: FrameStats, /// The number of QUIC packets sent on the connection (sum across all paths). /// - /// See also [`PathStats::sent_packets`]. + /// This does not count bytes for some kinds of probing packets, for more information + /// see [`PathStats::sent_packets`]. pub sent_packets: u64, /// The total number of QUIC bytes sent on the connection (sum across all paths). /// - /// See also [`PathStats::sent_bytes`]. + /// This does not count bytes for some kinds of probing packets, for more information + /// see [`PathStats::sent_bytes`] and [`PathStats::sent_packets`]. pub sent_bytes: u64, /// The number of packets lost on the connection. pub lost_packets: u64, From ad431800b2c7c1bf1f4bf4f0169711f7b41353ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Wed, 8 Jul 2026 09:37:42 +0200 Subject: [PATCH 3/4] Don't claim that MTUD probes are not congestion controlled --- noq-proto/src/connection/stats.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/noq-proto/src/connection/stats.rs b/noq-proto/src/connection/stats.rs index 76390fd61f..48cdaaa0ba 100644 --- a/noq-proto/src/connection/stats.rs +++ b/noq-proto/src/connection/stats.rs @@ -238,8 +238,8 @@ pub struct PathStats { /// and which we *expect to be received*. /// /// This does *not* count off-path path challenges, off-path path responses, any path challenges - /// or responses sent for NAT traversal or MTUD probes, for which the above is not the case (we - /// expect these packets to not make it in many cases). + /// or responses sent for NAT traversal or MTUD probes (we expect these packets to not make it in + /// many cases). /// /// This value should be meaningful relative to [`Self::lost_packets`], packets this way can get lost /// and will be counted in lost packets. From b3e608e406f0cab82bdd3715a645ffd585f480c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Fri, 11 Sep 2026 15:04:50 +0200 Subject: [PATCH 4/4] Count *all* packets in sent_packets and lost_packets --- noq-proto/src/connection/mod.rs | 13 +++++-- noq-proto/src/connection/packet_builder.rs | 19 ++++------ noq-proto/src/connection/stats.rs | 41 ++++++++++++---------- noq-proto/src/tests/mod.rs | 8 ++--- 4 files changed, 44 insertions(+), 37 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 7078be5a13..2460bc554e 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -3354,8 +3354,10 @@ impl Connection { if packet_too_old || largest_acked_packet_pn >= packet + packet_threshold { // The packet should be declared lost. if Some(packet) == in_flight_mtu_probe { - // Lost MTU probes are not included in `lost_packets`, because they - // should not trigger a congestion control response + // MTU probes are handled separately: they should not trigger + // retransmission or a congestion control response. They are still + // counted in the `lost_packets`/`lost_bytes` stats (in + // `handle_lost_packets`), consistent with `sent_packets`. lost_mtu_probe = in_flight_mtu_probe; } else { lost_packets.push(packet); @@ -3559,7 +3561,12 @@ impl Connection { .unwrap() .remove_in_flight(&info); self.path_data_mut(path_id).mtud.on_probe_lost(); - self.path_stats.get_mut(path_id).lost_plpmtud_probes += 1; + let path_stats = self.path_stats.get_mut(path_id); + path_stats.lost_plpmtud_probes += 1; + // MTUD probes are also counted in the general lost_packets/lost_bytes + // counters, consistent with sent_packets/sent_bytes counting all packets. + path_stats.lost_packets += 1; + path_stats.lost_bytes += info.size as u64; } } diff --git a/noq-proto/src/connection/packet_builder.rs b/noq-proto/src/connection/packet_builder.rs index 683a0dfccc..d219aaab20 100644 --- a/noq-proto/src/connection/packet_builder.rs +++ b/noq-proto/src/connection/packet_builder.rs @@ -286,18 +286,13 @@ impl<'a, 'b> PacketBuilder<'a, 'b> { false => 0, }; - // `lost_packets` is not incremented for MTUD probes, so we avoid incrementing - // `sent_packets` for it as well. - if conn.path_data(path_id).mtud.in_flight_mtu_probe() == Some(packet_number) { - conn.path_stats.get_mut(path_id).sent_plpmtud_probes += 1; - } else { - // Needs to be incremented in the same place that constructs `SentPacket`s, - // to ensure it matches `lost_packets` (`lost_packets <= sent_packets`). - // Similarly, `sent_bytes` needs to match how `lost_bytes` are incremented. - // Hence, the weird case of sent_packets increasing, but sent_bytes not, - // because the `size` above is what makes it into `lost_bytes` eventually. - conn.path_stats.get_mut(path_id).sent_packets += 1; - conn.path_stats.get_mut(path_id).sent_bytes += size as u64; + let is_mtud_probe = + conn.path_data(path_id).mtud.in_flight_mtu_probe() == Some(packet_number); + { + let path_stats = conn.path_stats.get_mut(path_id); + path_stats.sent_packets += 1; + path_stats.sent_bytes += size as u64; + path_stats.sent_plpmtud_probes += is_mtud_probe as u64; } let packet = SentPacket { diff --git a/noq-proto/src/connection/stats.rs b/noq-proto/src/connection/stats.rs index 48cdaaa0ba..708f7a1269 100644 --- a/noq-proto/src/connection/stats.rs +++ b/noq-proto/src/connection/stats.rs @@ -234,15 +234,14 @@ pub struct PathStats { pub spurious_congestion_events: u64, /// The number of QUIC packets sent on this path. /// - /// The intention for this stat is to capture all the packets we send that are congestion controlled - /// and which we *expect to be received*. + /// This counts all packets that are tracked for acknowledgement, including MTUD probes + /// and other probes. It does *not* count off-path packets (e.g. off-path path challenges, + /// off-path path responses, or NAT traversal probes) which are sent via a different code + /// path and are not tracked for acknowledgement. /// - /// This does *not* count off-path path challenges, off-path path responses, any path challenges - /// or responses sent for NAT traversal or MTUD probes (we expect these packets to not make it in - /// many cases). - /// - /// This value should be meaningful relative to [`Self::lost_packets`], packets this way can get lost - /// and will be counted in lost packets. + /// More specific counters such as [`Self::sent_plpmtud_probes`] allow breaking this number + /// down further. To get the number of non-probe packets sent, subtract + /// [`Self::sent_plpmtud_probes`] from this value. /// /// This counts individual QUIC packets, which may differ from [`UdpStats::datagrams`] when /// packets are coalesced into a single UDP datagram. @@ -250,25 +249,29 @@ pub struct PathStats { /// The total number of QUIC bytes sent on this path (sum of all sent packet sizes). /// /// This counts only the QUIC packet payload bytes, not UDP/IP header bytes. - /// It also doesn't count ACK-only packets in an effort to stay consistent with - /// [`Self::lost_bytes`], which doesn't count ACK-only packets either. + /// It does not count bytes for ACK-only (non-ack-eliciting, non-padded) packets, in an + /// effort to stay consistent with [`Self::lost_bytes`]. /// /// If you're interested in the full amount of bytes transmitted, consider looking /// at [`ConnectionStats::udp_tx`]. - /// - /// Caveats similar to the ones in [`Self::sent_packets`] apply. pub sent_bytes: u64, /// The number of packets lost on this path. + /// + /// This counts all packets declared lost, including MTUD probes. More specific counters + /// such as [`Self::lost_plpmtud_probes`] allow breaking this number down further. pub lost_packets: u64, /// The number of bytes lost on this path. + /// + /// This does not count bytes for ACK-only (non-ack-eliciting, non-padded) packets. pub lost_bytes: u64, /// The number of PLPMTUD probe packets sent on this path. /// - /// These are also counted by [`UdpStats::datagrams`]. + /// These are also counted by [`Self::sent_packets`] and [`Self::sent_bytes`]. + /// They are also counted by [`UdpStats::datagrams`]. pub sent_plpmtud_probes: u64, /// The number of PLPMTUD probe packets lost on this path. /// - /// These are not included in [`Self::lost_packets`] and [`Self::lost_bytes`]. + /// These are also counted by [`Self::lost_packets`] and [`Self::lost_bytes`]. pub lost_plpmtud_probes: u64, /// The number of times a black hole was detected in the path. pub black_holes_detected: u64, @@ -293,17 +296,19 @@ pub struct ConnectionStats { pub frame_rx: FrameStats, /// The number of QUIC packets sent on the connection (sum across all paths). /// - /// This does not count bytes for some kinds of probing packets, for more information - /// see [`PathStats::sent_packets`]. + /// See also [`PathStats::sent_packets`]. pub sent_packets: u64, /// The total number of QUIC bytes sent on the connection (sum across all paths). /// - /// This does not count bytes for some kinds of probing packets, for more information - /// see [`PathStats::sent_bytes`] and [`PathStats::sent_packets`]. + /// See also [`PathStats::sent_bytes`]. pub sent_bytes: u64, /// The number of packets lost on the connection. + /// + /// See also [`PathStats::lost_packets`]. pub lost_packets: u64, /// The number of bytes lost on the connection. + /// + /// See also [`PathStats::lost_bytes`]. pub lost_bytes: u64, /// Number of [`super::Transmit`] produced by this connection. diff --git a/noq-proto/src/tests/mod.rs b/noq-proto/src/tests/mod.rs index 8a694edcb0..8925293716 100644 --- a/noq-proto/src/tests/mod.rs +++ b/noq-proto/src/tests/mod.rs @@ -5122,8 +5122,8 @@ fn sent_packets_stats_snapshot_test() { let client_stats = pair.client_conn_mut(client_ch).stats(); let server_stats = pair.server_conn_mut(server_ch).stats(); - assert_eq!(client_stats.sent_packets, 9); - assert_eq!(server_stats.sent_packets, 6); + assert_eq!(client_stats.sent_packets, 13); + assert_eq!(server_stats.sent_packets, 10); let s = pair.client_streams(client_ch).open(Dir::Uni).unwrap(); const MSG: &[u8] = b"Hello, World!"; @@ -5132,6 +5132,6 @@ fn sent_packets_stats_snapshot_test() { let client_stats2 = pair.client_conn_mut(client_ch).stats(); let server_stats2 = pair.server_conn_mut(server_ch).stats(); - assert_eq!(client_stats2.sent_packets, 10); - assert_eq!(server_stats2.sent_packets, 7); + assert_eq!(client_stats2.sent_packets, 14); + assert_eq!(server_stats2.sent_packets, 11); }