search

quicwg / qlog

The IETF I-D documents for the qlog format
Other
86 stars 13 forks source link

QPACK: check data types #115

Closed rmarx closed 4 years ago

rmarx commented 4 years ago

Commit 128f3840cf951e8032c25896683b369cb92d13e6 adds proper data type definitions (e.g., uint64 instead of just "number", bytes instead of "hex string") to the qlog event fields.

These were a bit difficult to figure out for QPACK events (with the whole "prefixed integer" stuff), so I've put everything at uint64 for now. While this is the "safest" option, elsewhere in the document I've taken the approach of choosing the smallest "likely" integer size for a given value. One example is the datagram_size: it's unlikely we'll see datagrams larger than uint32 in practice (you could even argue uint16?), so this field is not a uint64. Similarly, the length of a QUIC STREAM frame is also just uint32, not 64. This approach should help make binary qlog derivations more optimized.

As such, I was hoping someone with more QPACK experience could help indicate the "minimal" data type size for the different fields.

A secondary aspect is the values logged for "name" and "value" (e.g., in InsertWithoutNameReferenceInstruction). I'm not sure if these should be readable strings (e.g., "Content-Encoding" or their hex/byte equivalents (e.g., "abcde1234"). For debugging it makes more sense if they are the readable strings, though I'm not sure it's logical to treat them that way inside a QPACK implementation. Alternative is the make the datatype for those string | bytes (one of both) and let the logger/tools figure it out.

So, TLDR:

  1. Which QPACK number-esque fields can do with less than 64 bits in practice?
  2. Which data type should name and value fields have in qlog?

I was hoping for some input on this from maybe @dtikhonov, @lpardue, @afrind. Thanks in advance!

LPardue commented 4 years ago

I don't have answers sorry, only a question. Have you considered making the name string and value string optional?

rmarx commented 4 years ago

@LPardue yes, and most already are, but it seems I missed a few. Will be fixed, thanks for reporting.

dtikhonov commented 4 years ago

Similarly, the length of a QUIC STREAM frame is also just uint32, not 64.

I looked at the latest version of the draft and stream IDs in the "qpack" section are all uint64...

rmarx commented 4 years ago

Similarly, the length of a QUIC STREAM frame is also just uint32, not 64.

I looked at the latest version of the draft and stream IDs in the "qpack" section are all uint64...

yes, because the IDs can conceivably go higher than 32. However, the raw_length of the STREAM frame (https://quiclog.github.io/internet-drafts/draft-marx-qlog-event-definitions-quic-h3.html#name-streamframe) can never span more than just the single packet, and I assume individual packets won't be larger than 32 bits in practice. (the "length" field there should then probably also be uint32 instead of 64, though the offset should remain at 64).

dtikhonov commented 4 years ago

Oh, I see where I went wrong! :-)

The updated "qpack" types in the draft look OK.

afrind commented 4 years ago

Seems like you could probably reduce the table capacity and table size to 32 bits, as I have a hard time seeing someone reserving up to 4GB for this purpose. That said, it is possible in the protocol, so if you want to be able to handle even the crazies I guess you have to stick to 64.

rmarx commented 4 years ago

Thanks all for the review.

Name and value (and their lengths) are now always optional and some uint64's have become uint32's (we do not want to encourage the crazies ;))