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

packet_size doesn't belong in the PacketHeader, but PacketType does #40

Closed
marten-seemann opened this issue Jan 19, 2020 · 2 comments
Closed

Comments

@marten-seemann
Copy link
Collaborator

In my interpretation, the PacketHeader is a QUIC packet header. Therefore, it should contain the QUIC packet type.
However, the packet_size (as opposed to the payload_length, which is the length of the QUIC payload e.g. in a coalesced packet), is a property of the UDP packet, and therefore should a property of the packet_sent / packet_received event.

@rmarx
Copy link
Contributor

rmarx commented Jan 19, 2020

While I -think- I agree with the general sentiment, this is a major departure from the current setup as implemented in most qlog setups. Additionally, many qvis visualizations actively use these fields.

I propose to keep the issue and PR open until when I can update qvis to deal with this change, so that I don't forget to do just that. Definitely before draft-02 lands of course.

Additionally, packet_size is intended to mean the entire size of the QUIC packet (header + payload), not the UDP datagram size (which can span multiple coalesced QUIC packets as you indicate). I felt the separate packet_size was needed to get an estimate of the header size, since that can differ quite a bit depending on the chosen encodings, pn-length etc. That does bring up the question if we need a datagram_size/length outside of the datagram_* events as well (though I personally would say not).

@rmarx
Copy link
Contributor

rmarx commented Nov 2, 2020

I initially moved packet_size to packet_sent and packet_received, which was the simplest solution.

However, when considering other issues around indicating packet and frame sizing, I ended up deciding to go for a generalized solution by using a shared RawInfo struct. This means that packet lengths are now logged as packet_sent:raw.length instead of packet_sent:packet_size. This isn't as clear, I agree, but it is consistent with how we log lengths for other things in qlog, which was more important for me at this time. Given that implementers have to move the packet_size field out of PacketHeader anyway, it shouldn't matter too much if they move it to raw.length I'd expect.

@rmarx rmarx closed this as completed Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants