Skip to content

feat: add sent_packets to PathStats and ConnectionStats#696

Open
mvanhorn wants to merge 2 commits into
n0-computer:mainfrom
mvanhorn:feat/565-sent-packets-pathstats
Open

feat: add sent_packets to PathStats and ConnectionStats#696
mvanhorn wants to merge 2 commits into
n0-computer:mainfrom
mvanhorn:feat/565-sent-packets-pathstats

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jun 9, 2026

Copy link
Copy Markdown

Description

Adds a sent_packets counter to PathStats and ConnectionStats in noq-proto, counting individual QUIC packets sent (per path, and aggregated for the connection). Resolves #565.

PathStats already exposes lost_packets, but without a sent_packets counterpart downstream consumers cannot derive an accurate packet loss rate (lost_packets / sent_packets). udp_tx.datagrams is 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, mirroring quinn::PathStats::sent_packets.

Changes:

  • noq-proto/src/connection/stats.rs: add pub sent_packets: u64 to PathStats and ConnectionStats, with docs mirroring the existing lost_packets field; fold it into the path-to-connection aggregation in both the Add<PathStats> and AddAssign<PathStats> impls for ConnectionStats.
  • noq-proto/src/connection/packet_builder.rs: increment the per-path counter in PacketBuilder::finish, the common packet-build path. Placing it here (rather than only in finish_and_track) ensures off-path PATH_CHALLENGE/PATH_RESPONSE packets, which call finish directly, are also counted. The value flows out unchanged through the existing Connection::path_stats() and Connection::stats() getters, and discarded-path counts are preserved via partial_stats.
  • noq-proto/src/tests/multipath.rs: add sent_packets_tracked_per_path, asserting per-path tracking, that sent_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, and cargo test -p noq-proto --lib (375 passed, including the new test) all pass; cargo fmt --check clean for the changed files.

Breaking Changes

None. PathStats and ConnectionStats are #[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.datagrams in the coalesced cases flub described.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.

Fixes #565

AI was used for assistance.

)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@flub

flub commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Verified locally: cargo build -p noq-proto, cargo clippy -p noq-proto --all-targets, and cargo test -p noq-proto --lib (375 passed, including the new test) all pass; cargo fmt --check clean for the changed files.

You should use cargo make as the instructions in the PR template indicate. And please write things yourself. This is not stuff people used to write before, it is not valuable information how you develop stuff. Just follow up on the CI failures.

@divagant-martian

Copy link
Copy Markdown
Collaborator

increment the per-path counter in PacketBuilder::finish, the common packet-build path. Placing it here (rather than only in finish_and_track) ensures off-path PATH_CHALLENGE/PATH_RESPONSE packets, which call finish directly, are also counted.

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.

@matheus23

matheus23 commented Jun 10, 2026

Copy link
Copy Markdown
Member

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.

image

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.
@mvanhorn

Copy link
Copy Markdown
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚑 Needs Triage

Development

Successfully merging this pull request may close these issues.

feature request: add sent_packets field to noq-proto PathStats and ConnectionStats

4 participants