feat: (s2n-dc-quic) Adds metrics to offloading feature - #3190
Conversation
d89dbf2 to
6ce6bbd
Compare
|
How hard would it be to enhance the existing s2n-quic-dc-metrics to give access to the numeric metrics you need (rather than the String)? |
It's not necessarily hard, I think it would just be kind of tedious. And I wanted to get some consensus before doing that. For reference, if you use the existing Registry to get TaskMetrics in our s2n-quic-dc-metrics crate, then you end up with this String as output. It's definitely more precise than averages, but the format makes me think these metrics are meant for some other purpose. Honestly I'm not sure why we even have the s2n-quic-dc-metrics crate. |
The internal usage directly ties events into a Registry which is how all aggregate metrics are emitted today, so it should be fine to just expect a Registry to be available in the psk code and register the tasks with it. Note that the approach here is (effectively) tracking a histogram of the means (sampled roughly once per second) whereas the dc-metrics implementation of task metrics tracks a histogram of the scheduled durations.
Say more? That format is just a dump of the underlying percentiles. I think it's intended for exactly the purpose we're interested in here.
I think having aggregate data is a good first step (we should get a version of that out quickly -- though I'd probably instrument the runtime and s2n-quic tasks too -- just to get everything we can easily get in one go), but I suspect it'll also be very valuable to capture per-handshake metrics. That's more work to wire up so I'd do that as fast follow up though. Non-aggregate data would allow us to definitively say that a given slow handshake spent X milliseconds scheduled in the Tokio runtime. With just aggregate data, I think we'd at least want a counter for how many times a given handshake's tasks are enqueued and waiting, otherwise moving from the aggregate scheduled/poll durations of the background runtime to the impact on handshake latency is quite hard (involves some extrapolating I think). |
| registry.instrument_runtime( | ||
| "offload runtime stats", | ||
| runtime.handle(), | ||
| Duration::new(1, 0), |
There was a problem hiding this comment.
We have to give an interval for the instrument_runtime function so I chose a second. Seems reasonable 🤷♀️
Alright. I changed the server builder to expect a registry and registered the offloading runtime stats with it.
I'd rather register s2n-quic tasks in a different PR. It's going to require a new API in s2n-quic and don't want to have that block this PR. |
Release Summary:
Resolved issues:
n/a
Description of changes:
dc-quic now takes in a Registry and uses it to instrument the offloading runtime and offloading tasks. Internally we will build dc-quic with a Registry and use that to emit aggregate stats about offloading.
Call-outs:
Testing:
n/a
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.