feat: add sent_packets to PathStats and ConnectionStats#696
Conversation
You should use |
I'm not sure this is desired, the path id is just a logical identifier. Off-path PATH_CHALLENGEs and PATH_RESPONSEs don't actually share the same full network path over which bytes flow as those that are on-path, and counting them towards loss rate is amiss. They just happen to share the same logical identifier. |
|
This is an agent and not a person. I worry that they are just collecting "trophies" of merged PRs. Doesn't seem like they actually care about the work. 
See also the user profile's list of "contributed to". |
Off-path PATH_CHALLENGE/PATH_RESPONSE packets share the path's logical identifier but not the network path itself, so counting them toward per-path stats would skew loss-rate math. The counter moves from PacketBuilder::finish to finish_and_track.
|
@divagant-martian you're right - the logical path id isn't the network path, and off-path probes have no business in per-path loss math. Moved the counter to finish_and_track in f205bec so only tracked sends count. @matheus23 fair question. I do use agent tooling, openly. What I'd point to is the follow-through: feedback on this PR got engaged and fixed, not dropped - that's the part I care about and the part I think you should judge. If this project would rather not take contributions made this way, tell me and I'll respect that. |
| 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, |
There was a problem hiding this comment.
I wonder if this is good enough. These are connection wide so I'm on the fence on whether off-path packets should also taken into account. I'm tempted to say yes, but also it's probably convoluted to implement. What do you all think about this?
There was a problem hiding this comment.
I think the important part is that lost_packets and sent_packets make sense in relation to each other. We should never have lost_packets > sent_packets, for example.
If off-path packets are taken into account for lost_packets (which I haven't checked!), then they should be taken into account for sent_packets.
There was a problem hiding this comment.
@mvanhorn this is solid advice. Would you mind checking that this is consistent so that we can move forward with this PR?
4 participants
Description
Adds a
sent_packetscounter toPathStatsandConnectionStatsinnoq-proto, counting individual QUIC packets sent (per path, and aggregated for the connection). Resolves #565.PathStatsalready exposeslost_packets, but without asent_packetscounterpart downstream consumers cannot derive an accurate packet loss rate (lost_packets / sent_packets).udp_tx.datagramsis only an approximation: it counts UDP datagrams, which diverge from packet counts whenever packets are coalesced into a single datagram (notably during the handshake), as flub noted on the issue. This adds a precise per-packet counter, mirroringquinn::PathStats::sent_packets.Changes:
noq-proto/src/connection/stats.rs: addpub sent_packets: u64toPathStatsandConnectionStats, with docs mirroring the existinglost_packetsfield; fold it into the path-to-connection aggregation in both theAdd<PathStats>andAddAssign<PathStats>impls forConnectionStats.noq-proto/src/connection/packet_builder.rs: increment the per-path counter inPacketBuilder::finish, the common packet-build path. Placing it here (rather than only infinish_and_track) ensures off-pathPATH_CHALLENGE/PATH_RESPONSEpackets, which callfinishdirectly, are also counted. The value flows out unchanged through the existingConnection::path_stats()andConnection::stats()getters, and discarded-path counts are preserved viapartial_stats.noq-proto/src/tests/multipath.rs: addsent_packets_tracked_per_path, asserting per-path tracking, thatsent_packets >= udp_tx.datagrams(the coalescing case), that sending on a second path grows that path's counter, and that the connection total equals the sum across paths.Verified locally:
cargo build -p noq-proto,cargo clippy -p noq-proto --all-targets, andcargo test -p noq-proto --lib(375 passed, including the new test) all pass;cargo fmt --checkclean for the changed files.Breaking Changes
None.
PathStatsandConnectionStatsare#[non_exhaustive], so adding a field is additive. There is no behavior change beyond populating the new counter.Notes & open questions
The counter is incremented once per finished QUIC packet, so it is genuinely per-packet and intentionally diverges from
udp_tx.datagramsin the coalesced cases flub described.Change checklist
Fixes #565
AI was used for assistance.