feat(s2n-quic-dc): emit cleaner events #2392
Conversation
dc/s2n-quic-dc/events/map.rs
Outdated
|
|
||
| /// The utilization percentage of the available number of entries before the cycle | ||
| #[measure("utilization.initial", Percent)] | ||
| initial_utilization: f32, |
There was a problem hiding this comment.
Are we missing a utilization for the addresses (or the IDs)? I wonder if we should have a consistent prefix/suffix to make it clearer that you're not looking at an aggregate and need to inspect both metrics and not just one of them?
There was a problem hiding this comment.
Yeah this is from the IDs. But I can change it to emit for both with the prefixes.
| 60 | ||
| } else { | ||
| rand::thread_rng().gen_range(5..60) | ||
| }; |
There was a problem hiding this comment.
We're sort of not deterministic anyway since this is wall clock time... but I guess this seems fine.
There was a problem hiding this comment.
For sure. But this is at least better than what we had before 😄. The snapshots were quite flaky.
| let map = Map::new(signer, capacity, NoopClock, sub); | ||
|
|
||
| // sleep so the cleaner has time to emit events | ||
| std::thread::sleep(core::time::Duration::from_millis(100)); |
There was a problem hiding this comment.
Should we run the cleaner.clean() on the primary thread at Drop time or similar?
There was a problem hiding this comment.
You're saying just to get the events?
camshaft
force-pushed
the
camshaft/dc-utilization-events
branch
from
ca54f60 to
427b901
Compare
dc/s2n-quic-dc/events/map.rs
Outdated
| retired_entries: usize, | ||
|
|
||
| /// The utilization percentage of the available number of entries after the cycle | ||
| #[measure("entries.utilization", Percent)] |
There was a problem hiding this comment.
I think this still feels like a metric that at first glance describes aggregate utilization, rather than of a single map of many. Maybe we can name both kinds something like entries.ids.utilization and entries.addresses.utilization? That makes it much clearer IMO that someone looking at that metric should expect other stuff in the entries. category for other kinds of entries.
There was a problem hiding this comment.
makes sense. i can change it
Description of changes:
This change adds an event for the
s2n_quic_dc::path::secret::map::Cleanerbackground thread. This allows a subscriber to get insight into how often the cleaner thread is running and what kind of work is being done. It also emits map utilization metrics.Call-outs:
I renamed the
lenmethod in thefixed_mapmodule to becount, since it iterates over all of the slots and sums up thelen.The stable clippy task is failing due to a new version being released over the weekend. That will be fixed in another PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.