qlog: Update ConnectionClose with CryptoError #1734
Conversation
This makes the qlog crate fit the specification laid out in quicwg/qlog#392
| #[derive(Serialize, Deserialize, Clone, PartialEq, Debug)] | ||
| #[serde(untagged)] | ||
| pub enum ConnectionCloseErrorCode { | ||
| CryptoError(CryptoError), |
There was a problem hiding this comment.
Need to add all the variants in https://quicwg.org/qlog/draft-ietf-quic-qlog-quic-events.html#name-connectioncloseframe
| #[derive(Serialize, Deserialize, Clone, PartialEq, Debug)] | ||
| #[serde(untagged)] | ||
| pub enum ConnectionCloseErrorCode { | ||
| CryptoError(CryptoError), |
There was a problem hiding this comment.
The CryptoError type also needs to be updated to reflect the latest qlog spec - https://quicwg.org/qlog/draft-ietf-quic-qlog-quic-events.html#section-8.12.25
| error_code_value: None, // raw error is no different for us | ||
| // TODO: determine if connection is established so we can log | ||
| // CryptoErrors | ||
| error_code: Some(ConnectionCloseErrorCode::Numeric(*error_code)), |
There was a problem hiding this comment.
I think what we want to do instead convert all the known error codes to actual qlog transport or crypto errors, rather than a numeric one
| // TODO: determine if connection is established so we can log | ||
| // CryptoErrors | ||
| error_code: Some(ConnectionCloseErrorCode::Numeric( | ||
| *error_code, | ||
| )), |
There was a problem hiding this comment.
CryptoErrors cannot be sent in application close. And at the point we log things here, we don't know the application protocol. So I think its safe to just log the Numeric variant without a comment.
This makes the qlog crate fit the specification laid out in quicwg/qlog#392.
This yields the following
ConnectionCloseframe:We won't be able to log
CryptoErrors without a larger change that pushes detection of connection establishment into the qlog crate. We could also explore logging one of two differentConnectionCloseevents on close, one each for pre- and post-establishment, but that seemed messy. I can implement that if we do want to go down that route.