qlog: add delivery_rate, send_rate and ack_rate to MetricsUpdated via ex_data #2337
Conversation
|
Theres a lot of changes in a single commit. I would recommend splitting out the qlog crate changes to a separate commit and probably it's own PR |
| /// The maximum send rate observed across all acked packets in this event. | ||
| /// Computed as bytes_sent_delta / time_delta between packet send times. | ||
| pub sample_max_send_rate: Option<Bandwidth>, | ||
| /// The ack rate for the last acked packet in this event. |
There was a problem hiding this comment.
Why not also the max for consistency with send_rate?
|
|
||
| // New fields for qlog rate metrics | ||
| // These reuse the BandwidthSampler's existing computations instead of | ||
| // duplicating the calculation in GRecovery |
There was a problem hiding this comment.
I would consider removing the previous 3 lines and replace them with some usage comment on these 2 new fields.
- Application should look at sample_max_bandwidth for bandwidth estimates, not these new fields.
- The purpose of these fields is to provide additional debug info mostly for qlog.
We could consider only computing these fields if the qlog feature is enabled.
| self.min_rtt_filter.update(rtt_sample, event_time); | ||
| } | ||
|
|
||
| // Store latest rate samples for qlog exposure |
There was a problem hiding this comment.
Comment referring to qlog likely to become out of date.
| // bandwidth_latest is the max bandwidth for the round, but to allow | ||
| // fast, conservation style response to loss, use the last sample. | ||
| last_bandwidth = sample_max_bandwidth; | ||
| last_bandwidth = congestion_event.sample_max_bandwidth.unwrap(); |
There was a problem hiding this comment.
Not sure what happened here.
The original if let version is more correct. Did you accidentally undo a change in the main branch?
| /// Avoid Overestimation in Bandwidth Sampler with ack aggregation. | ||
| /// This is an old experiment that we have found to under-perform the | ||
| /// algorithm described in the spec. Use is not recommended. | ||
| /// Avoid Overestimation in Bandwidth Sampler with ack aggregation |
There was a problem hiding this comment.
I think you lost recent changes to comments in this file as you tried to create this PR.
| /// slightly over estimated. The pacer can only schedule packets | ||
| /// 1/8th of an RTT into the future, so the error introduced by | ||
| /// setting `time_sent` to `now` is bounded. | ||
| time_sent_set_to_now: bool, |
antoniovicente
changed the title
estcarisimo
force-pushed
the
add-qlog-send-ack-rates
branch
2 times, most recently
from
bffd9d5 to
0e70c88
Compare
antoniovicente
left a comment
antoniovicente
left a comment
There was a problem hiding this comment.
Looks good otherwise.
| ex_data | ||
| .insert("delivery_rate".to_string(), serde_json::json!(rate)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we also suppress events when the latest measurement is None? Something like:
// Build ex_data for rate metrics
let mut ex_data = qlog::events::ExData::new();
if self.delivery_rate != latest.delivery_rate {
- self.delivery_rate = latest.delivery_rate;
- emit_event = true;
- if let Some(rate) = latest.delivery_rate {
+ if let Some(rate) = latest.delivery_rate {
+ self.delivery_rate = latest.delivery_rate;
+ emit_event = true;
ex_data
.insert("delivery_rate".to_string(), serde_json::json!(rate));
}
}
3 participants
No description provided.