refactor: improve qlog frame recording for sent packets #207
Conversation
Frando
force-pushed
the
Frando/qlog-refactor
branch
from
c45f0ec to
6cfdcb7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## Frando/qlog-qnt #207 +/- ##
===================================================
- Coverage 76.71% 76.68% -0.04%
===================================================
Files 83 83
Lines 22689 22663 -26
===================================================
- Hits 17406 17378 -28
- Misses 5283 5285 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| length: None, | ||
| payload_length: None, | ||
| }); | ||
| qlog.frame(&Frame::ResetStream(frame)); |
There was a problem hiding this comment.
The function has an inner cfg now, I think you can also remove the cfg here and the compiler should figure out this is all a noop and remove the call?
Also in the other locations in this file.
| frame.write(buf); | ||
| #[cfg(feature = "qlog")] | ||
| qlog.unknown_frame(&frame.get_type()); | ||
| qlog.frame(&Frame::ObservedAddr(frame)); |
There was a problem hiding this comment.
Just occured to me that you might be able to write these all as &frame.into()? No need to do it, but you could if you like.
There was a problem hiding this comment.
I left it as-is, because the into is not available for all frame kinds (those that have primitive types are not covered).
| truncated.encode(buf); | ||
| self.stats.frame_tx.crypto += 1; | ||
|
|
||
| // The clone is cheap but we still cfg it out if qlog is disabled. |
There was a problem hiding this comment.
FWIW I don't think this matters: the way I understand it is that the compiler will always consider inlining functions from the same crate. It'll then see that this does nothing. And in turn it sees that this clone call is unused and remove it.
Description
Based on #179
This moves the QuicFrame generation into the
qlogmodule. The public interface for recording frames in sent packets now either takes acrate::Frame, or for frame types where constructing such a frame would have a cost, there's a specialized method on theQlogSentPacket.This keeps the mapping between frames and
qlog::QuicFramelocalized instead of spread out through the crate, and will make it much easier to add support for more frames.Breaking Changes
Notes & open questions