add optional qlogging to quiche #379

LPardue · 2020-02-26T15:50:33Z

This is still WIP but ready for review. I need to add tests for the new capability but will await design feedback.

The PR contains 2 commits, the older one uses a buffered serialization approach, the newer one replaces this with a streaming serialized approach. I left these so that we could compare the two, but we'd definitely squash this down before any merge.

To test this out, you can run quiche-client or quiche-server with the QLOGDIR environment pointing at a folder. The resulting trace will be written to a file such as client-{scid}.qlog which can then be imported to the visualization tool https://qvis.edm.uhasselt.be.

ghedo

Haven't really finished reviewing the whole thing, but I think there are enough comments for now (mostly about code structure, though we might need to tweak the public API a bit).

include/quiche.h

Cargo.toml

src/lib.rs

ghedo · 2020-03-13T12:41:08Z

src/ranges.rs

@@ -244,7 +244,8 @@ mod tests {
    fn insert_non_overlapping() {
        let mut r = RangeSet::default();
        assert_eq!(r.inner.len(), 0);
-        assert_eq!(&r.flatten().collect::<Vec<u64>>(), &[]);


These changes look pretty suspicious TBH. Like, why are they needed in this branch but not master given that there are no other changes to this file?

I don't know!!! (but in seriousness I agree, so let's see if I can bisect the root cause)

caused by serde_json. Even though only qlog imports that crate, it seems to leak into the quiche library

src/frame.rs

src/lib.rs

ghedo

I didn't quite look at the qlog code before, which I just did and added a few comments.

The quiche code itself looks mostly good now though. just a few small things remaining.

ghedo · 2020-03-18T16:57:36Z

tools/qlog/src/lib.rs

+        // trace, which might be undesirable.
+        match serde_json::to_string(&self.qlog) {
+            Ok(mut out) => {
+                if let Some(index) = out.find("\"events\":[") {


Is there no better way to do this? Maybe defining the Serialize trait manually for these types rather than deriving one automatically?

At the very least we should probably avoid scanning the whole string for the specific substring. If I understand it correctly this string would always be at the end of the output, so maybe we can just match the suffix itself directly?

So far with these changes, they allow both a buffered and a streaming mode of using the qlog crate. Modifying Serialize risks breaking that. Furthermore, the generated serialization code for Trace is 150 lines of code, maintaining a manual equivalent code is a bit brittle.

Reverse-scanning is going to be an improvement. Additionally, if it scans for end of the array, it can avoid blatting over any existing events, which is a problem highlighted in the TODO.

tools/qlog/src/lib.rs

ghedo · 2020-03-18T17:01:34Z

tools/apps/src/bin/quiche-client.rs

@@ -139,6 +139,21 @@ fn main() {
    let mut conn =
        quiche::connect(connect_url.domain(), &scid, &mut config).unwrap();

+    // Only bother with qlog if the user specified it.
+    #[cfg(feature = "qlog")]


FTR, rust-lang/rust#69201 will be in Rust 1.43, so we can probably simplify this a bit in the future.

tools/apps/src/bin/quiche-server.rs

src/lib.rs

src/recovery.rs

tools/apps/Cargo.toml

tools/qlog/src/lib.rs

Cargo.toml

src/ffi.rs

src/frame.rs

tools/apps/Cargo.toml

tools/apps/src/lib.rs

tools/qlog/src/lib.rs

src/ffi.rs

LPardue · 2020-03-20T19:01:24Z

Okay... I've addressed a lot of the feedback comments, thanks!

This is now ready for another review. In the meantime I'll start on updating the qlog documentation ready to publish the new v0.2 of the crate before this PR lands.

LPardue · 2020-03-24T18:49:59Z

Docs updated and comments addressed, PTAL

junhochoi

🚢

ghedo · 2020-03-26T15:40:39Z

@LPardue looks like this needs to be rebased on master as there is a conflict. While you are at it, can you also submit the new qlog crate to crates.io and squash the commits in the PR? So we can finally merge this.

Previously, qlog API was designed to operate in a buffered mode. This required users to create a trace and hold it in memory, append events to it, and at the end serialize the whole thing. This change adds a streaming mode API that serializes qlog events immediately to a `Write` trait. A few quality-of-life changes have also been made.

This change introduces qlog support into the quiche library as a new optional feature that is disabled by default. qlog occurs in addition to conventional Rust logging. To build quiche with qlog support do: cargo build --features qlog Rust applications can enable qlog on a per-connection basis via the `set_qlog()` method. C applications can use `quiche_conn_set_qlog_fd()`. Once qlog is enabled, qlog events are seralized in a streaming fashion to the target Write trait or file descriptor.

LPardue requested a review from a team as a code owner February 26, 2020 15:50

LPardue force-pushed the quiche-qlog branch 8 times, most recently from b02d0de to 7b0fbb2 Compare March 11, 2020 12:16

ghedo requested changes Mar 13, 2020

View reviewed changes

LPardue force-pushed the quiche-qlog branch 3 times, most recently from cd271d0 to 4a87e28 Compare March 16, 2020 13:20

ghedo requested changes Mar 18, 2020

View reviewed changes