-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add raw bytes field support to CONNECTION_CLOSE. #412
base: main
Are you sure you want to change the base?
Conversation
draft-ietf-quic-qlog-quic-events.md
Outdated
@@ -2051,6 +2051,11 @@ error. For known frame types, the appropriate string value is used. For unknown | |||
frame types, the numerical value without variable-length integer encoding is | |||
used. | |||
|
|||
The CONNECTION_CLOSE reason is a byte sequences, that may be possible to | |||
present as UTF-8. The `reason_text` and `reason_bytes` fields provide support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not how I read RFC 9000. The field SHOULD be UTF-8, so the expectation is that it actually is. We're just creating an escape hatch here in case this assumption doesn't hold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - need better wording alignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're logging a reason sent by a peer you don't have any relationship with, there is no guarantee it is following the SHOULD. Therefore, it is only a possibility that the reason is presentable as UTF-8. I can't think of a more succint way to describe this possibility than the current text. If you have concrete suggestions for alternative text, please propose them :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke offline with Marten and came up with a slight variant
I'm not the biggest fan of this not using the same pattern as we did for ALPNInformation... which would be more like this:
Is there a particular reason to change from that established precedent? |
The main reason (pun intended) is that the I won't defend that choice with much vigor if others prefer a Structured approach. |
I don't have a super-strong opinion on this, as this approach also makes sense. Especially seeing another need for this in #414, we could reason this approach is the default, and ALPNInformation deviates from that for good reasons™ |
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.