Bump qlog to 0.9.0 #1454
Bump qlog to 0.9.0 #1454
Conversation
| @@ -698,7 +698,7 @@ fn client( | |||
| fn qlog_new(args: &Args, hostname: &str, cid: &ConnectionId) -> Res<NeqoQlog> { | |||
| if let Some(qlog_dir) = &args.qlog_dir { | |||
| let mut qlog_path = qlog_dir.to_path_buf(); | |||
| let filename = format!("{}-{}.qlog", hostname, cid); | |||
| let filename = format!("{}-{}.sqlog", hostname, cid); | |||
| early_data_enabled: None, | ||
| tls_cipher: None, | ||
| aead_tag_length: None, | ||
| original_destination_connection_id: remote |
There was a problem hiding this comment.
Do you implement Default for TransportParametersSet? Because then you could use ..Default::default() rather than providing all these None-valued arguments.
| /// If logging enabled, closure may generate an event to be logged. | ||
| pub fn add_event_data<F>(&mut self, f: F) | ||
| where | ||
| F: FnOnce() -> Option<qlog::events::EventData>, |
There was a problem hiding this comment.
I can't see any places where an instance of F returns None. So maybe you can just do:
| F: FnOnce() -> Option<qlog::events::EventData>, | |
| F: FnOnce() -> qlog::events::EventData, |
| let mut pacing_rate: Option<u64> = None; | ||
|
|
||
| for metric in updated_metrics { | ||
| match metric { | ||
| QlogMetric::MinRtt(v) => min_rtt = Some(u64::try_from(v.as_millis()).unwrap()), | ||
| QlogMetric::MinRtt(v) => min_rtt = Some(v.as_secs_f32() * 1000.0), |
There was a problem hiding this comment.
Feature request for you: can you take these as Duration rather than f32?
There was a problem hiding this comment.
The qlog spec data model defines the value as float32; see https://quicwg.org/qlog/draft-ietf-quic-qlog-quic-events.html#section-7.2
The qlog crate data model tries to be as close to the spec as possible. I don't see much value in e.g. turning the crate to support enum foo(spec_def(f32), my_own_thing(Duration) because it would have to be converted to f32 on way or another.
There was a problem hiding this comment.
Yeah, I get that, but Rust native time types are more ergonomic.
You could maybe double-dip by implementing a trait:
trait IntoQlogTime {
fn into_qlog_time(self) -> f32;
}
impl IntoQlogTime for f32 { ... }
impl IntoQlogTime for Duration { ... }Though that helps more with if you also provide functions for instantiating the objects rather than making the fields public as you do (because then you can do x: impl IntoQlogTime arguments).
There was a problem hiding this comment.
Ta, I'm happy to add some syntactic sugar as a feature request
|
I'll leave that last build/clippy error to you unless you would prefer that I fix it... |
Yeah saw this, I don't really have an opinion on solving it, do you? |
neqo-common/src/qlog.rs
Outdated
| datetime | ||
| .format(&time::format_description::well_known::Rfc3339) | ||
| .ok() // This is expected to never fail. | ||
| Some(datetime.unix_timestamp() as f64) |
There was a problem hiding this comment.
| Some(datetime.unix_timestamp() as f64) | |
| Some(f64::try_from(datetime.unix_timestamp()).unwrap()) |
This probably works well enough.
Or, if you prefer:
| Some(datetime.unix_timestamp() as f64) | |
| let mask = -1 >> (i64::BITS - f64::MANTISSA_DIGITS); | |
| Some(f64::try_from(datetime.unix_timestamp() & mask).unwrap()) |
|
Thanks for pushing this over the line. The suggestions for qlog changes are still on my todos list |
Later versions of the qlog crate provided a serializer that uses the JSON-SEQ streaming format, which uses a .sqlog file extension. The format helps the crate avoid needing to "lock" and "unlock" while writing events with frames. That clunkiness could make it hard for apps and might lead to mangled logs.
Later versions of the qlog crate also changed the data model, making it easier to interact directly with events and their data. This obviated the need for helper functions, so they were all removed and this PR needs to do the work to replace them.