change STREAM and DATAGRAM frame definition to allow logging of frame payload
marten-seemann opened this issue · 3 comments
Currently the raw field includes the wire representation of the frame. This doesn't seem useful, and would mean that a qlog consumer would need to implement a frame parser. Instead, we should only log the payload.
This could be done by changing the raw field to payload.
So, I don't agree with this. The reason it includes the frame/packet headers currently is because we (intentionally) lose some information present in the binary form by representing the data in qlog/JSON. Users should have a way to get to the original data for low-level debugging if needed. An argument -might- be made for splitting it into a separate headers and payload (and then also trailers?) field, but not omitting headers.
Your proposal imo also only circumvents 1 layer of parsing at a time (e.g., not logging QUIC STREAM frames still requires implementing an H3/QPACK parser, not logging QUIC Packer Headers still required QUIC frame parser, etc.).
I would suggest closing this without action.
Are you still willing to fight for this @marten-seemann ?
I just also thought of another counter-argument: since the current RawInfo struct also contains the full length and the payload-only length, an implementation that just wants the payload can calculate the length of the headers and then skip that many bytes ahead in the payload hexstring to get to the payload start (it requires a little bit more logic in the case of trailers, but we really only have those for QUIC packets, and those trailers are 100% predictable in size).
If you do not provide push-back within 1 week, I'll close this without action.
Discussed during editors' meeting. Indeed makes sense to only log payload. If you need higher up headers, you log previous layer (e.g., if you need QUIC packet headers, you'd get them from the raw UDP packets).