quicwg / qlog

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

Add raw bytes field support to CONNECTION_CLOSE. #412

Closed LPardue closed 5 months ago

LPardue commented 8 months ago

Fixes #411

Same approach as the one we look for ALPNInformation. Allows endpoints to log either, because sometimes it is more useful to print and consume a string like "It's borked" than it's hex encoded equivalent.

rmarx commented 8 months ago

I'm not the biggest fan of this not using the same pattern as we did for ALPNInformation... which would be more like this:

...
? reason: CloseReason
...

CloseReason = {
    ? byte_value: hexstring
    ? string_value: text
}

Is there a particular reason to change from that established precedent?

LPardue commented 8 months ago

The main reason (pun intended) is that the QUICALPNInformation event contains 3 instances of ALPNInformation, which would make a flatter structure really hard to describe. Here, there is only one instance of the reason, so it seems of less value to create another structure type to hold it.

I won't defend that choice with much vigor if others prefer a Structured approach.

rmarx commented 8 months ago

I don't have a super-strong opinion on this, as this approach also makes sense. Especially seeing another need for this in https://github.com/quicwg/qlog/pull/414, we could reason this approach is the default, and ALPNInformation deviates from that for good reasons™