feat(proto): Add {Connection,Path}Stats::sent_{packets,bytes}#740
feat(proto): Add {Connection,Path}Stats::sent_{packets,bytes}#740matheus23 wants to merge 3 commits into
{Connection,Path}Stats::sent_{packets,bytes}#740Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/740/docs/noq/ Last updated: 2026-07-08T07:40:13Z |
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5321.4 Mbps | 7975.5 Mbps | -33.3% | 97.8% / 150.0% |
| medium-concurrent | 5282.1 Mbps | 7955.3 Mbps | -33.6% | 98.0% / 149.0% |
| medium-single | 3813.0 Mbps | 4595.2 Mbps | -17.0% | 91.5% / 100.0% |
| small-concurrent | 3707.2 Mbps | 5375.9 Mbps | -31.0% | 95.9% / 103.0% |
| small-single | 3424.0 Mbps | 4646.5 Mbps | -26.3% | 96.5% / 153.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3051.4 Mbps | 4092.7 Mbps | -25.4% |
| lan | 796.4 Mbps | 810.4 Mbps | -1.7% |
| lossy | 69.8 Mbps | 55.9 Mbps | +25.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 28.2% slower on average
3021d18e3915f2f61ed8c451d6ed0b25613f9674 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5288.9 Mbps | 7948.0 Mbps | -33.5% | 93.1% / 98.5% |
| medium-concurrent | 5334.6 Mbps | 7643.9 Mbps | -30.2% | 94.4% / 99.0% |
| medium-single | 3816.4 Mbps | 4749.3 Mbps | -19.6% | 93.1% / 101.0% |
| small-concurrent | 3760.1 Mbps | 5164.6 Mbps | -27.2% | 101.4% / 155.0% |
| small-single | 3384.9 Mbps | 4804.4 Mbps | -29.5% | 90.2% / 102.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3026.9 Mbps | 4078.6 Mbps | -25.8% |
| lan | 782.4 Mbps | 797.6 Mbps | -1.9% |
| lossy | 69.9 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 27.7% slower on average
5a668f96a2057f60a7cc471de7d9ddf3adf7927e - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5312.2 Mbps | 7999.5 Mbps | -33.6% | 96.6% / 147.0% |
| medium-concurrent | 5226.3 Mbps | 7629.7 Mbps | -31.5% | 91.7% / 97.1% |
| medium-single | 3352.2 Mbps | 4594.7 Mbps | -27.0% | 91.1% / 100.0% |
| small-concurrent | 3748.4 Mbps | 5331.2 Mbps | -29.7% | 95.6% / 102.0% |
| small-single | 3396.7 Mbps | 4810.4 Mbps | -29.4% | 95.8% / 152.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 2994.9 Mbps | 4051.0 Mbps | -26.1% |
| lan | 782.4 Mbps | 810.4 Mbps | -3.5% |
| lossy | 69.8 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 29.4% slower on average
divagant-martian
left a comment
There was a problem hiding this comment.
Changes to docs, this is nice otherwise
| /// 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*. |
There was a problem hiding this comment.
I'm having trouble understanding what this is trying to say. All sent packets are expected to be transmitted?
There was a problem hiding this comment.
Hm. I'm trying to put into words the fact that we count only a subset of packets that we build potentially in lost_packets and in sent_packets: Specifically, packets that are generally probes we don't count - NAT traversal probes & MTUD probes.
And I tried to put this into non-technical terms: These probes are packets we highly suspect can get lost, while other kinds of packets are packets we expect to be received.
(Apparently I forgot to push a change that address the code review from diva to change "expect to be transmitted" to "expect to be received")
There was a problem hiding this comment.
Yeah, current wording makes sense.
Allow me to think about this for a bit though. Why do we need this complex distinction? All packets are acknowledged. All packets are eventually either detected lost or not. So from that perspective you can have very simple and straightforward definitions of sent_packets and lost_packets.
If you want exact numbers there is also the sent_plmtud_probes and lost_plmtud_probes counters you can add into the mix. So perhaps the solution to the problem you're trying to solve here is to add sent & lost counters for the other kind of probes that we're sending as well?
There was a problem hiding this comment.
Well - for this PR I'm just describing the complex distinction that already existed in how lost_packets works, and my intention was to only add sent_packets in a way that makes sense relative to lost_packets.
I mean we could change that, but that goes a bit beyond the original issue I'm trying to solve. Could we do that as a follow-up?
There was a problem hiding this comment.
But did we create this complex distinction ourselves or was that already present in Quinn? It seems like that this points towards us forgetting to add counters for our new kinds of probes.
There was a problem hiding this comment.
The way that MTUD and "normal loss" was counted already existed in Quinn.
Obviously the same can't be said about NAT traversal probes.
There was a problem hiding this comment.
Sorry, I forgot sent_packets is new. To me it still makes more sense to increment that in PacketBuilder::finish so that it is incremented all the time.
I think that currently lost probes, of any kind, do still increment lost_packets. So if you had a path that sent more probes than other packets then you'd still end up with lost_packets > sent_packets.
It also means that finish_and_track does not have to figure out if something is an mtu probe since that can stay where it is. The "_track" bit refers to tracking it for congestion control I think.
flub
left a comment
There was a problem hiding this comment.
see continuation of previous thread. but github needs me to hit the "submit review" button for this PR to disappear out of my PR inbox :)
Can you point to the code that does this? |
oh, i found it. it does indeed do this. i'm still not convinced I like it though |
fwiw, all our other probes do not have this exception. including all our NAT traversal probes. They will all be declared lost and counted in lost_packets. |
Description
Closes #565
Breaking Changes
Only additions:
ConnectionStats::sent_packets,ConnectionStats::sent_bytes,PathStats::sent_packets,PathStast::sent_bytesNotes & open questions
The guiding principle behind this PR is a central use case of continually monitoring
sent_packetsandlost_packetsas an indicator of "how good" our connection (or path) is doing and how healthy it is.Previously, this was not possible, because
lost_packetscan't really be seen relative toUdpStats::datagramsdue to coalescing and generally some other inconsistencies.In this PR I'm taking great care to make sure
sent_packetsmatcheslost_packetsonce every single packet gets lost, and maintaining the invariant thatlost_packets <= sent_packetsalways (I had added adebug_assert!for this at some point, not sure if people would prefer adding that back?).Also,
lost_packetsonly counts packets that we had "expected to make it", but got lost. I.e. it does not count off-path/NAT traversal challenges and responses nor MTUD probes.I'm matching this behavior with
sent_packetsnow.Detecting if the current packet is an MTUD probe is somewhat ugly... I decided to use
conn.path_data(path_id).mtud.in_flight_mtu_probe() == Some(packet_number)for this, but could also have matched onPadDatagram::ToSize, as that's only used by MTUD probes, but the former seemed clearer.Change checklist