feat: Expand qlog support #181
Conversation
Frando
changed the title
feat: add ConnectionStarted to qlog feat: track received packets in qlog fix: cid groups, padding size, cleanups fixup vantage_point method signature chore: clippy
Frando
force-pushed
the
Frando/qlog-qnt
branch
from
70cd464 to
b9e83dc
Compare
Frando
changed the title
flub
left a comment
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.
| use std::net::{IpAddr, SocketAddr}; | ||
| #[cfg(feature = "qlog")] | ||
| use std::sync::{Arc, Mutex}; |
There was a problem hiding this comment.
stdlib imports should be before 3rd party crates, no?
| #[cfg(feature = "qlog")] | ||
| impl QlogStream { | ||
| fn emit_event(&self, orig_rem_cid: ConnectionId, event: EventData, now: Instant) { | ||
| fn emit_event(&self, initial_dst_cid: ConnectionId, event: EventData, now: Instant) { |
There was a problem hiding this comment.
that is so much a better variable name for this
| ) { | ||
| #[cfg(feature = "qlog")] | ||
| { | ||
| use qlog::events::connectivity::ConnectionStarted; |
There was a problem hiding this comment.
Why is this not in the import block at the top of the file?
| let Some(stream) = self.stream.as_ref() else { | ||
| return; | ||
| }; | ||
| // TODO: Review fields. The standard has changed since. |
There was a problem hiding this comment.
Is it possible to say which draft the fields used here are in the comment? That would probably help the future.
| ..Default::default() | ||
| }; | ||
| #[derive(Default)] | ||
| pub(crate) struct QlogSentPacket { |
There was a problem hiding this comment.
Maybe add a doc comment that this is intentionally a noop struct if the cfg is disabled, and that the expectation is that the compiler completely removes it from function call parameters. Preferably somehow all on the first line so that IDEs show it nicely :)
| stream.emit_event(orig_rem_cid, EventData::PacketReceived(event), now); | ||
| #[cfg(feature = "qlog")] | ||
| impl QlogSentPacket { | ||
| pub(crate) fn header( |
There was a problem hiding this comment.
I know these are all internal APIs. But I love doc comments for internal stuff too.
| } | ||
|
|
||
| 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?
Expand qlog support:
Based on #177 because iroh feat-multipath is now based on that. Failures/lints are on
protocol-simplications- will rebase as soon as that branch becomes green.