qlog,quiche,tokio-quiche: add QlogSink abstraction for custom qlogs by strtok · Pull Request #2500 · cloudflare/quiche

strtok · 2026-05-29T19:36:56Z

Add a QlogSink trait that allows overriding qlog behavior. Previously QlogStreamer was instantiated with a writer object. Instead, it is instantiated with a QlogSink object which allows more flexibility. A QlogWriterSink implementation provides backwards compatibility.

Implementations have new methods for overriding qlog behavior:

Connection::set_qlog_sink and set_qlog_sink_with_level allow setting a custom qlog sink. The existing set_qlog and set_qlog_with_level exist for backwards compatibility and call these new functions with a QlogWriterSink object.
A new ConnectionHook::create_qlog_sink hook can be implemented to install a sink at connection accept/connect time. If a sink is returned it overrides any qlog_dir behavior.

Add QlogStreamer::should_log(EventType) that allows high volume events to be skipped without construction. Modified both the recv and write path to make use of this.

meha23

strtok:ebremen/qlog

gregor-cf

This is great. I did a quick pass and had some minor comments and nits.

gregor-cf · 2026-06-01T02:55:37Z

+    fn add_event(&mut self, event: events::Event) -> Result<()>;
+
+    /// Add a pretty-printed native qlog event to the stream.
+    fn add_event_pretty(&mut self, event: events::Event) -> Result<()> {


I would remove the `pretty functions. The sink might not actually have a serialization or similar that can do pretty printing. IMHO if pretty printing is desired, then we that should be handled by the sink / writer internally, but it shouldn't be exposed to callers of QlogSink.

But I'll let the folks who orginally added the QlogStreamer chime in.

I think it really depends on what we are trying to abstract.

The qlog spec only defines two serialization formats, JSON and JSON-SEQ, the latter being a streaming format. Other serialization formats can be used, they just aren't specified (yet).

This trait is describing itself as "/// A destination for sequential qlog events." but its unclear to me what that means. qlog events are don't have a strict sequential requirement - the spec says

Events in each individual trace SHOULD be logged in strictly ascending timestamp order

but this is just a recommendation because in practice achieving that in the logger might be difficult.

So some clarification questions:

Is the trait intended for "streaming" qlog events at the unit of a single event?

Are we intending to want to support other qlog serialization formats? For example, writing out events as CBOR of BSON.

Are we intending to want to support transforms on the serialized data. For example, conventional gzip stream on the text output?

if the answer to (2) is yes, then I'd probably agree with Gregor's point - the "pretty" is a facet of the serializer not the qlog event. However, it is useful to have the flexibility for a single JSON-SEQ log to mix and match condensed vs. pretty printed JSON. We do this with h3i today. That suggests you might need a add_event_with_customization (or pick a snazzier name) that would allow per-event sink-specific serialization customization.

gregor-cf · 2026-06-01T03:07:29Z

    /// Writes a serializable to a pretty-printed JSON-SEQ record using
    /// [std::time::Instant::now()].
-    pub fn add_event_now_pretty<E: Serialize + Eventable>(
+    pub fn add_event_now_pretty<E: Into<crate::QlogEvent> + Eventable>(


nit: I'd remove the _pretty functions here too....

LPardue

Thanks for the PR.

Overall I'm a big fan of adding more flexibility to what events are included in a serialized qlog, and how it is precisely written out. This makes a good start on it.

That said, its not 100% clear to me on the abstraction model being used. I've asked some clarification questions in-band to try and understand more.

Its made a bit more complicated because there is a QlogStreamer that now seems to wrap a sink and some of the contracts documented at https://docs.rs/qlog/latest/qlog/#streaming-traces-with-json-seq no longer holds with the change. For example, we can't say its always JSON-SEQ if the sink allows it to be something different 😄 The docs should be updated once we have hammered out the design questions a little

There's a lot of changes in a single commit. While its great to see how tokio-quiche can be updated to use the new API. I think it would help to break this work out over multiple commits or probably even multiple PRs. I think that will help to let us iterate on the qlog sink details and resolve those independent of tokio-quiche issues. This also lets us prove the change is backwards compatible with our own stack.

LPardue · 2026-07-10T01:07:27Z

+            .qlog
+            .streamer
+            .as_ref()
+            .is_some_and(|q| q.should_log(QLOG_PACKET_TX))
+            .then(|| Vec::with_capacity(frames.len()));


This seems to be a slight variant of qlog_with_type!, consider adding a companion macro that returns bool so you can do

let mut qlog_frames = if should_qlog_with_type!(....) { Some(Vec::with_capacity(frames.len()) } else { None };

LPardue · 2026-07-10T01:08:22Z

                    EventData::QuicPacketSent(qlog::events::quic::PacketSent {
                        header,
-                        frames: Some(qlog_frames),
+                        frames: Some(qlog_frames.unwrap_or_default()),


qlog_frames is an option with this change

Suggested change

frames: Some(qlog_frames.unwrap_or_default()),

frames: qlog_frames

LPardue · 2026-07-10T01:41:58Z

+    fn add_event(&mut self, event: events::Event) -> Result<()>;
+
+    /// Add a pretty-printed native qlog event to the stream.
+    fn add_event_pretty(&mut self, event: events::Event) -> Result<()> {


I think it really depends on what we are trying to abstract.

The qlog spec only defines two serialization formats, JSON and JSON-SEQ, the latter being a streaming format. Other serialization formats can be used, they just aren't specified (yet).

This trait is describing itself as "/// A destination for sequential qlog events." but its unclear to me what that means. qlog events are don't have a strict sequential requirement - the spec says

Events in each individual trace SHOULD be logged in strictly ascending timestamp order

but this is just a recommendation because in practice achieving that in the logger might be difficult.

So some clarification questions:

Is the trait intended for "streaming" qlog events at the unit of a single event?

Are we intending to want to support other qlog serialization formats? For example, writing out events as CBOR of BSON.

Are we intending to want to support transforms on the serialized data. For example, conventional gzip stream on the text output?

if the answer to (2) is yes, then I'd probably agree with Gregor's point - the "pretty" is a facet of the serializer not the qlog event. However, it is useful to have the flexibility for a single JSON-SEQ log to mix and match condensed vs. pretty printed JSON. We do this with h3i today. That suggests you might need a add_event_with_customization (or pick a snazzier name) that would allow per-event sink-specific serialization customization.

LPardue · 2026-07-10T02:15:39Z

+    fn start_log(&mut self, qlog: &QlogSeq) -> Result<()>;
+
+    /// Add a native qlog event to the stream.
+    fn add_event(&mut self, event: events::Event) -> Result<()>;


It's not clear to me why the sink trait has both add_event() and add_json_event(), when there is a QlogEvent type defined. Wouldn't it be easier to have a single add_event(&mut self, event: QlogEvent) method?

LPardue · 2026-07-10T02:17:00Z

+}
+
+/// A destination for sequential qlog events.
+pub trait QlogSink: Send + Sync {


This name doesn't seem completely accurate, consider StreamingQlogSink to make it clear what it is (although see my clarification questions below).

LPardue · 2026-07-10T02:20:09Z

+}
+
+/// A [`QlogSink`] that writes JSON-SEQ qlog records to a [`Write`].
+pub struct QlogWriterSink<W: Write + Send + Sync> {


Strictly, per https://quicwg.org/qlog/draft-ietf-quic-qlog-main-schema.html#section-11.2 you'd want include QlogFileSeq in the name for clarity. Something like QlogFileSeqWriterSink etc.

LPardue · 2026-07-10T02:24:10Z

+    fn add_event(&mut self, event: events::Event) -> Result<()> {
+        self.writer.write_all(b"\x1e")?;
+        serde_json::to_writer(&mut self.writer, &event)
+            .map_err(|_| Error::Done)?;
+        self.writer.write_all(b"\n")?;
+
+        Ok(())
+    }
+
+    fn add_event_pretty(&mut self, event: events::Event) -> Result<()> {
+        self.writer.write_all(b"\x1e")?;
+        serde_json::to_writer_pretty(&mut self.writer, &event)
+            .map_err(|_| Error::Done)?;
+        self.writer.write_all(b"\n")?;
+
+        Ok(())
+    }
+
+    fn add_json_event(&mut self, event: events::JsonEvent) -> Result<()> {
+        self.writer.write_all(b"\x1e")?;
+        serde_json::to_writer(&mut self.writer, &event)
+            .map_err(|_| Error::Done)?;
+        self.writer.write_all(b"\n")?;
+
+        Ok(())
+    }
+
+    fn add_json_event_pretty(&mut self, event: events::JsonEvent) -> Result<()> {
+        self.writer.write_all(b"\x1e")?;
+        serde_json::to_writer_pretty(&mut self.writer, &event)
+            .map_err(|_| Error::Done)?;
+        self.writer.write_all(b"\n")?;
+
+        Ok(())


There's tonnes of duplication across these functions. The old serializer code was able to condense it all down into a single write_event function, I can't see a good reason we can't do the same.

…iting Add a QlogSink trait that allows overriding qlog behavior. Previously QlogStreamer was instantiated with a writer object. Instead, it is instantiated with a QlogSink object which allows more flexibility. A QlogWriterSink implementation provides backwards compatibility. Implementations have new methods for overriding qlog behavior: - Connection::set_qlog_sink and set_qlog_sink_with_level allow setting a custom qlog sink. The existing set_qlog and set_qlog_with_level exist for backwards compatibility and call these new functions with a QlogWriterSink object. - A new ConnectionHook::create_qlog_sink hook can be implemented to install a sink at connection accept/connect time. If a sink is returned it overrides any qlog_dir behavior. Add QlogStreamer::should_log(EventType) that allows high volume events to be skipped without construction. Modified both the recv and write path to make use of this. Remove QlogStreamer::writer. A QlogStreamer is no longer guaranteed to be backed by a Writer. The underlying QlogSink is exposed via QlogStreamer::sink(), where it's used by tests to access the underlying test sink.

LPardue · 2026-07-10T02:48:34Z

@strtok FYI I rebased the PR to also let CI have a go and find any other latent issues that might be hiding.

strtok requested a review from a team as a code owner May 29, 2026 19:36

meha23 approved these changes May 29, 2026

View reviewed changes

gregor-cf reviewed Jun 1, 2026

View reviewed changes

csujedihy reviewed Jun 1, 2026

View reviewed changes

Comment thread quiche/src/lib.rs

strtok force-pushed the ebremen/qlog branch from a287cf5 to 3deccff Compare June 3, 2026 00:26

LPardue suggested changes Jul 10, 2026

View reviewed changes

LPardue force-pushed the ebremen/qlog branch from 3deccff to e680eed Compare July 10, 2026 02:46

	frames: Some(qlog_frames.unwrap_or_default()),
	frames: qlog_frames

Uh oh!

Conversation

strtok commented May 29, 2026

Uh oh!

meha23 left a comment

Choose a reason for hiding this comment

Uh oh!

gregor-cf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LPardue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LPardue commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants