diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 5a89682297..2460bc554e 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)) } @@ -3356,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); @@ -3561,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 59bab52083..d219aaab20 100644 --- a/noq-proto/src/connection/packet_builder.rs +++ b/noq-proto/src/connection/packet_builder.rs @@ -286,6 +286,15 @@ impl<'a, 'b> PacketBuilder<'a, 'b> { false => 0, }; + 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 { 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..708f7a1269 100644 --- a/noq-proto/src/connection/stats.rs +++ b/noq-proto/src/connection/stats.rs @@ -232,17 +232,46 @@ 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. + /// + /// 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. + /// + /// 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. + 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. + /// 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`]. + 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, @@ -265,9 +294,21 @@ 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. + /// + /// 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. @@ -290,6 +331,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 +345,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 +368,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 +382,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 +393,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..8925293716 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, 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!"; + 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, 14); + assert_eq!(server_stats2.sent_packets, 11); +}