Conversation
70cd464 to
b9e83dc
Compare
flub
left a comment
There was a problem hiding this comment.
Generally looks great, but let's add good doc comments. Even for internal APIs.
The other main comment is that the qlog module is cfg-disabling all the bodies of functions so that the callsites don't have to have cfg everywhere. I think you should continue that style for the newly added methods. That would get rid of a lot of cfg lines.
quinn-proto/src/connection/qlog.rs
Outdated
| } | ||
|
|
||
| stream.emit_event(orig_rem_cid, EventData::PacketReceived(event), now); | ||
| #[cfg(feature = "qlog")] |
There was a problem hiding this comment.
I think the philosophy of this module is to keep all/most the cfgs inside the function bodies. That way the call sites are not littered with cfgs. And the compiler is expected to optimise out the noop function calls. So you probably should follow the same pattern for these new impls?
There was a problem hiding this comment.
Yes, maybe. For QlogRecvPacket it's straightforward, will change. I kept the cfg initially for QlogSentPacket::frame, which takes a qlog::QuicFrame. Constructing this from the raw data sometimes involves calculations, so to make sure to not do these when qlog is disabled I made the fn be configured out in that case. Maybe I just keep the cfg for that one only.
b9e83dc to
9760ab4
Compare
feat: add ConnectionStarted to qlog feat: track received packets in qlog fix: cid groups, padding size, cleanups fixup vantage_point method signature chore: clippy
53824d3 to
4179cd2
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main-iroh #181 +/- ##
=============================================
+ Coverage 76.38% 76.68% +0.30%
=============================================
Files 83 83
Lines 22286 22663 +377
=============================================
+ Hits 17023 17379 +356
- Misses 5263 5284 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
## Description Based on #179 This moves the QuicFrame generation into the `qlog` module. The public interface for recording frames in sent packets now either takes a `crate::Frame`, or for frame types where constructing such a frame would have a cost, there's a specialized method on the `QlogSentPacket`. This keeps the mapping between frames and `qlog::QuicFrame` localized instead of spread out through the crate, and will make it much easier to add support for more frames. ## Breaking Changes <!-- Optional, if there are any breaking changes document them, including how to migrate older code. --> ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. -->
This expands the qlog support: