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

Allow more wire-image indicators #54

Closed
rmarx opened this issue Feb 26, 2020 · 8 comments
Closed

Allow more wire-image indicators #54

rmarx opened this issue Feb 26, 2020 · 8 comments

Comments

@rmarx
Copy link
Contributor

rmarx commented Feb 26, 2020

Currently, we abstract out a lot of the on-the-wire specifics.

A good example is in the STREAM frame: there, the length and offset fields are optional, depending on how the frame should be handled. Simply having the same fields optional in qlog doesn't convey quite the same semantics: did the implementation simply not log them or were they not present from the start?

More importantly though: if no length is set, the frame extends to the end of the packet, so it does have a length, which you'd probably want to log (that's the way it's currently designed), but so you loose the info that the length field wasn't encoded on the wire.

This is just one example of similar problems across the board. I rather like the simplicity of the current setup, but we should have ways to signal the explicit wire image as well.

cc @huitema

@rmarx rmarx added the future-versions issue will be tackled but not for the current iteration label Oct 31, 2020
@rmarx
Copy link
Contributor Author

rmarx commented Aug 18, 2021

I can see three options:

  1. provide a more fine-grained, optional raw_frame_type field that encodes the actual stream type number instead of the conceptual name we have in frame_type (like how UnknownFrame works today)
  2. require people to log the (partial) (STREAM) frame header in the raw field to allow for later re-parsing
  3. do nothing

Proposal: use 1.

@rmarx rmarx added current-version design quic-http3-fields and removed future-versions issue will be tackled but not for the current iteration labels Aug 18, 2021
@LPardue
Copy link
Member

LPardue commented Aug 18, 2021

Can you elaborate on 1 more? By the time my parser informs me I have a STREAM frame, I might have discarded the on-wire bytes

@rmarx
Copy link
Contributor Author

rmarx commented Aug 18, 2021

This would be an -extra- option to log besides the frame_type in case you really need to know if you got 0x08, 0x09, 0x0A ... or 0x0F (at least for STREAM, for ACK it'd be 0x02 or 0x03).

This could be parsed if you logged the first few bytes in raw for example, but having a separate field would be more direct. We have precedent for this in e.g., ConnectionCloseFrame.raw_error_code.

If you don't have that information available at the point you're logging but still need it well... you'd need to log somewhere closer to the wire? In the ACK case, it is implicit through the logging of ECN info, for stream it is somewhat for the presence of the FIN bool, but for length and offset, it's more difficult to know sometimes.

This is not crucial information, but could be good to know in some highly specific edge case debugging.

Got a bug in pcap2qlog about this just the other day, where I forgot to extrapolate the length field from the QUIC packet length if it wasn't present in the STREAM frame itself (quiclog/pcap2qlog#7)

@LPardue
Copy link
Member

LPardue commented Aug 18, 2021

This could be parsed if you logged the first few bytes in raw for example, but having a separate field would be more direct. We have precedent for this in e.g., ConnectionCloseFrame.raw_error_code.

Some the the question here is what do you mean by wire image. Should something like raw_error_code be raw bytes underlying the varint (maintaining the information on how the number was encoded on the wire), or the parsed varint.

Since raw_error_code a single int, it must be the parsed varint, which is slightly easier. But even today I can't populate that because my implementation has discarded the data by the time I come to generate the qlog. Changing the code would boil down to me changing the logging pipeline, parsing the same value twice, and/or storing the data twice just for qlog.

I'll note that for H3 this is even harder, because we have to handle non-atomic varint reads as STREAM data comes.

@rmarx
Copy link
Contributor Author

rmarx commented Aug 19, 2021

So the idea was:
Option 1: parsed varint (so literally the 0x08 - 0x0F for STREAM)
Option 2: raw wire image (so "unparsed" varint)

I do see the confusion... probably we should use another word than raw here, since raw_error_code is intended to also be the parsed varint, but the raw:RawInfo fields are intended to be pure wire bits.

With regards to your implementation not having that info available... I'm not sure what you want me to say... these are optional fields that you can output if needed/available, but not something I would expect many implementations to do (by default). I don't see a way of solving the original issue here (representing all the wire image nuances in some form) without having access to those nuances at the logging location :)

Could you elaborate on because we have to handle non-atomic varint reads as STREAM data comes with an example maybe? Not sure what you mean there?

@LPardue
Copy link
Member

LPardue commented Aug 19, 2021

The main point is that it's fairly straightforward to log the unprocessed bytes of a QUIC packet or QUIC frame, that's because an implementation will have received those in whole and know their length. E.g. Read UDP, everything in the datagram.

For things on top of QUIC streams it becomes harder because they can span lots of packets. A streaming parser will possibly not have an entire frame available when it want to log an event.

If the proposal is to log the parsed varint, it makes things slightly easier

@LPardue
Copy link
Member

LPardue commented Sep 7, 2022

how about an optional field frame_type_value: uin64, which can be used in any frame type that spans a range of possible values. Then we'll update UnknownFrame s/raw_frame_type/frame_type_value.

@rmarx
Copy link
Contributor Author

rmarx commented Jan 19, 2023

Fixed by #274

@rmarx rmarx closed this as completed Jan 19, 2023
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