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

Indicate (encoded) sizes #97

Closed
rmarx opened this issue Jul 8, 2020 · 3 comments
Closed

Indicate (encoded) sizes #97

rmarx opened this issue Jul 8, 2020 · 3 comments

Comments

@rmarx
Copy link
Contributor

rmarx commented Jul 8, 2020

Currently we do not have many fields for indicating (compressed) sizes of things.

We have the packet_size, but this is in the wrong place (#40) and payload_length (though it's not well-defined whether packet_size includes the TLS suffix).

However, we lack size indicators for datagrams and, especially frames. When creating the qvis packetization diagram, this was very annoying, as I'd have to reconstruct the encoded frame headers to properly render those (and even then there might be rounding issues if the sender didn't encode things in the absolute minimum of bytes allowed).

In comparison, wireshark output does include exact sizes for these fields for TCP/TLS/H2 and this was quite handy.

Another point: need some way to indicate the used TLS AEAD tag length (though that should be constant for the entire connection and depends on the used cipher IIUC. For TCP+TLS from Wireshark I also had to manually calculate this from the cipher definitions though, which was a PITA).

Finally, make terminology consistent (_size vs _length)

@rmarx
Copy link
Contributor Author

rmarx commented Aug 6, 2020

From a discussion on slack, most are in favor of adding AEAD length to the general packet_size field (one reasons is because the tag also contributes to cwnd calculation)

@rmarx
Copy link
Contributor Author

rmarx commented Sep 7, 2020

We have a bit of a weird situation with packet_size, payload_size and header_size, in that if you have 2, you can calculate the third from those... so do we want to specify all three?

I would reckon that packet_size and payload_size will typically be readily available to implementations, but header_size might be more difficult to come by (as headers can be of varying sizes, while payload is "the rest of the packet after the header has been processed").

Finally, there is overlap with what we currently call "raw_length" if we're logging the full raw byte contents of the packet (in the "raw" field, which we currently name "raw" across all events for consistency). To keep naming consistent, you'd want this "raw_length" here as well, but you also kind of want "packet_size" to show the core semantics (especially as I'd suspect most won't log the raw values (and accompanying "raw_length") in most places, but do log packet_size). A similar remark for datagram_size in the datagram_* events... do we live with having the same field defined twice with different names or do we remove packet_size and datagram_size and explain that people should use raw_length for those instead (I'm personally tending to this latter option).

Could use some input on this @marten-seemann, @huitema, @dtikhonov

rmarx pushed a commit that referenced this issue Sep 7, 2020
rmarx pushed a commit that referenced this issue Nov 2, 2020
@rmarx
Copy link
Contributor Author

rmarx commented Nov 2, 2020

Ended up not logging the header_length and writing out how it can be calculated from total length and payload length and aead length.

Currently, aead length is logged separately in transport:parameters_set, which is not optimal, but good enough for now until we figure out what to do with that event and how to split it up (see #85)

Finally, I ended up keeping explicit length fields when they are used in the QUIC/H3 drafts. This is not the case for the packet_size field, so that has now been changed to raw.length instead. I'm not entirely enamored by this, as arguably .packet_size was clearer, but since we're moving packet_size out of Header anyway (see #40), it felt best to keep things consistent and punt this to the new RawInfo approach.

@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

No branches or pull requests

1 participant