Skip to content

feat(load/peak_ewma): expose round-trip time estimate #871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cratelyn wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(load/peak_ewma): expose round-trip time estimate #871

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b2c3330
feat(load/peak_ewma): RttEstimate is `pub`
cratelyn Jun 4, 2026
fa0b122
feat(load/peak_ewma): RttEstimate is Clone
cratelyn Jun 4, 2026
3015e8c
feat(load/peak_ewma): add PeakEwma::rtt_estimate accessor
cratelyn Jun 4, 2026
708248a
feat(load/peak_ewma): add RttEstimate::{updated_at, rtt_ns}
cratelyn Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions tower/src/load/peak_ewma.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ pub struct Handle {
}

/// Holds the current RTT estimate and the last time this value was updated.
#[derive(Debug)]
struct RttEstimate {
#[derive(Clone, Debug)]
pub struct RttEstimate {
update_at: Instant,
rtt_ns: f64,
}
Expand Down Expand Up @@ -107,6 +107,18 @@ impl<S, C> PeakEwma<S, C> {
rtt_estimate: self.rtt_estimate.clone(),
}
}

/// Returns the current [`RttEstimate`] of the service.
///
/// # Panics
///
/// This value is stored in a mutex. If the mutex has become poisoned, this will panic.
pub fn rtt_estimate(&self) -> RttEstimate {
self.rtt_estimate
.lock()
.expect("mutex should not be poisoned")
.clone()
}
Comment thread
cratelyn marked this conversation as resolved.
}

impl<S, C, Request> Service<Request> for PeakEwma<S, C>
Expand Down Expand Up @@ -216,6 +228,16 @@ where
// ===== impl RttEstimate =====

impl RttEstimate {
/// Returns the [`Instant`] that this estimate was last updated.
pub fn updated_at(&self) -> Instant {
self.update_at
}

/// Returns the round-trip time estimate, in nanoseconds.
pub fn rtt_ns(&self) -> f64 {
self.rtt_ns
}
Comment on lines +231 to +239

@cratelyn cratelyn Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another way to do this would be making the inner update_at and rtt_ns fields public, but i didn't like that. introducing new fields would become a semver breaking change, and i'm fairly sure that the name update_at contains a typo (i would expect that this be named updated_at).

i went this route to provide a stable public interface.

@seanmonstar seanmonstar Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't even remember, why is nanoseconds stored in f64 instead of a u64 or u128? Is it to make use of the exponent to represent even bigger numbers in 64 bits?

Would it be worth exposing the value publicly as an unsigned int?

@seanmonstar seanmonstar Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, a different crazy idea: struct Rtt(_), so the exact repr inside is not public? Does the number truly matter, or do you just need an impl Ord?

@cratelyn cratelyn Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, a different crazy idea: struct Rtt(_), so the exact repr inside is not public? Does the number truly matter, or do you just need an impl Ord?

in our case, the number does matter. you're right that Ord is the way that tower::balance steers traffic given two Load::metrics (see https://github.com/cratelyn/tower/blob/251296dc54a044383dffd16d2179b443e2615672/tower/src/balance/p2c/service.rs#L168-L171), but i'm interested in providing a way for tower middleware to build atop this layer with their own Load implementations.

we'd like to be able to take this RTT estimate, while also taking into account other properties of the traffic like response status codes.

@cratelyn cratelyn Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't even remember, why is nanoseconds stored in f64 instead of a u64 or u128? Is it to make use of the exponent to represent even bigger numbers in 64 bits?

Would it be worth exposing the value publicly as an unsigned int?

my understanding is that this is stored as a floating-point number because of the math in the RttEstimate::update(..) method here: https://github.com/tower-rs/tower/blob/master/tower/src/load/peak_ewma.rs#L263-L277

// When an RTT is observed that is less than the estimated RTT, we decay the
// prior estimate according to how much time has elapsed since the last
// update. The inverse of the decay is used to scale the estimate towards the
// observed RTT value.
let elapsed = nanos(now.saturating_duration_since(self.update_at));
let decay = (-elapsed / decay_ns).exp();
let recency = 1.0 - decay;
let next_estimate = (self.rtt_ns * decay) + (rtt * recency);
trace!(
    "update rtt={:03.0}ms decay={:06.0}ns; next={:03.0}ms",
    rtt / NANOS_PER_MILLI,
    self.rtt_ns - next_estimate,
    next_estimate / NANOS_PER_MILLI,
);

because of this, when we observe requests taking less time than the original peak, we decay the estimate exponentially, which can lead to fractional estimates.

rounding to the nearest nanosecond, i.e. writing an inverse of the nanos() helper here, would let us return the estimate as a Duration (via Duration::from_nanos()), which does seem like a much more intuitive interface to expose publicly.

what do you think, @seanmonstar?

@seanmonstar seanmonstar Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, gotcha. So, we don't have to do RTT math with floating points. The Linux kernel does TCP RTT/EWMA with integers. 🤷

But, it also could be just whatever, I guess if we change the types internally, the numbers will never be so large that a cast will lose anything.

@cratelyn cratelyn Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a great point. i can refactor this middleware to switch away from floating point numbers to integers, which would make exposing the current estimate more natural.

i'll do that as a separate pull request, to keep each change easily reviewable.

Comment on lines +236 to +239

@cratelyn cratelyn Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i toyed briefly with finding a way to convert this into a Duration, but it seems like a fundamentally lossy translation that's best handled by callers. we could add a rtt that rounds to the nearest nanosecond, if that'd be welcome.


fn new(rtt_ns: f64) -> Self {
debug_assert!(0.0 < rtt_ns, "rtt must be positive");
Self {
Expand Down