Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do we need an acked_packet event? #107

Closed
marten-seemann opened this issue Jul 13, 2020 · 3 comments
Closed

Do we need an acked_packet event? #107

marten-seemann opened this issue Jul 13, 2020 · 3 comments

Comments

@marten-seemann
Copy link
Collaborator

We have a lost_packet event, but no corresponding acked_packet event. That event would fire when a packet is acknowledged and removed from the map of sent packets.

A parser that wants to track when a packet is (first) acknowledged would have to build a map of sent packets, and remove packets from that map when they are either acknowledged, declared lost or when the packet number space is lost.

@rmarx
Copy link
Contributor

rmarx commented Jul 13, 2020

So this is similar to "do we need a connection_closed event?" in that there's a question of how many "redundant" ways we can have to log the same event.

For connection close and packet acked, the reasoning initially was that those could be fully parsed from the corresponding frames inside packet_sent and packet_received and thus didn't need an extra event.

I can now see the reasoning for wanting a specific connection_closed event (e.g., logging an internal reason that you don't send over the wire), but I'm not sure what a separate acked_packet event would give us. For some use cases it would make it easier (if you're not parsing frames inside the packet_* events), but in most, I feel it would make things more complicated (do I need to log both? Do I skip logging of ACK frames if I do acked_packet? etc.). I do see that receipt of an ACK frame is potentially different from adding/removing to the internal packet datastructure (given duplicate acks), but not sure that should be exposed in a default way in qlog (we have many more of those type of implied actions that are expected to happen upon frame processing. Though there is some precedent of exposing that type of thing, e.g., in data_moved). In most cases, it was expected that tools could derive this type of action from the logged frames (e.g., like how I track retransmissions and gaps in streams in qvis: those are also not explicitly logged).

@nibanks
Copy link
Member

nibanks commented Jul 13, 2020

Personally, I agree with @marten-seemann. MsQuic logs (to ETW) events or state changes for things. For qlog, we then have a converter. We don't log the contents of every packet or frame. For packets, we track when they're created, acknowledged, dropped, assumed lost, deemed spuriously lost, etc. IMO, tools that process the logs should not have to know how to process a logged ACK frame to then go calculate which in-flight packets have now been acknowledged. Another reason, is that our logging system doesn't assume full state was contained within the log file. The logs may have been started mid run or the fixed fixed log buffer rolled over. In those cases, you wouldn't necessarily have the created event for a packet and wouldn't know what is already in-flight.

@marten-seemann
Copy link
Collaborator Author

Paraphrasing an argument I made in a Slack conversation:

I can see @rmarx's argument that logging both the ACK frame and packet_acked is kind of redundant, given that a parser can build a copy of the packet map. However, I’d rather log a packet_acked event instaed of the content of ACK frames. I care more about the state changes than about what redundant ACK ranges get serialized on the wire.

rmarx pushed a commit that referenced this issue Nov 1, 2020
@rmarx rmarx closed this as completed Nov 1, 2020
@rmarx rmarx mentioned this issue Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants