feat(trace): add abstract Clock trait supporting custom timing impl - #710
feat(trace): add abstract Clock trait supporting custom timing impl#710Saplyn wants to merge 3 commits into
Clock trait supporting custom timing impl#710Conversation
| @@ -0,0 +1,77 @@ | |||
| use std::time::{Duration, Instant}; | |||
There was a problem hiding this comment.
I don't think that the clock belongs in trace but rather in a root module (as evidenced by on_early_drop importing it). You could re-export it from trace if desired.
| /// | ||
| /// The [`Clock`] trait's `Duration` associated type must implement this trait, | ||
| /// which provides conversion methods used by [`Latency`](super::Latency). | ||
| pub trait DurationExt { |
There was a problem hiding this comment.
This Duration trait creates a lot of ergonomic issues... it forces all of the downstream callbacks to specify generics, and I don't think it is particularly needed.
As best as I can tell, the wasm support issues are only about std::time::Instant/SystemTime. Duration itself seems fine (web-timejust re-exports thecore::time:Duration. Similarly other common clocks like tokio::time::Instantandquanta` use it).
Is there a reason we can't simplify to:
fn elapsed(&self, instant: Self::Instant) -> std::time::Duration
| /// The default implementation ([`DefaultClock`]) uses [`std::time::Instant`] | ||
| /// and [`std::time::Duration`], but custom clocks can be provided — for | ||
| /// example, to mock time in tests or to use a WASM-compatible time source. | ||
| pub trait Clock: Copy { |
There was a problem hiding this comment.
Why do we need Copy here? That makes it dyn-incompatible and also restricts all implementors to be Copy (which we can see the irritation in our own DummyClock mock, and any other stateful clock).
And then anyway TraceLayer and Trace derive Copy, so if the Clock impl is Copy, then those structs also remain Copy.
There was a problem hiding this comment.
And then, I would probably require Clone on the two use sites that need it, and use &self for access to make life better for stateful clocks.
There was a problem hiding this comment.
Also without Copy we can probably add some blanket implementations:
impl<C: Clock + ?Sized> Clock for &C, for Arc<C>, for Box<C>
| /// example, to mock time in tests or to use a WASM-compatible time source. | ||
| pub trait Clock: Copy { | ||
| /// An instant in time. | ||
| type Instant: Copy + Send; |
There was a problem hiding this comment.
I would probably keep Send on the use sites rather than the trait
|
|
||
| impl DefaultClock { | ||
| /// Create a new `DefaultClock`. | ||
| pub fn new() -> Self { |
There was a problem hiding this comment.
nit: seems redundant with the Default derive?
| /// [`OnEarlyDropLayer`]: super::OnEarlyDropLayer | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub struct EarlyDropsAsFailures<F> { | ||
| pub struct EarlyDropsAsFailures<F, Clk = DefaultClock> |
There was a problem hiding this comment.
nit: I would probably prefer either C or Clock
| .unwrap(); | ||
|
|
||
| assert!( | ||
| ON_RESPONSE_LATENCY.load(Ordering::SeqCst) > 0, |
There was a problem hiding this comment.
Here and in other places, we should be able to use exact assertions, right? A regular clock would also pass this, but exact assertions distinguish that we are using the deterministic clock.
| } | ||
|
|
||
| #[test] | ||
| fn dummy_clock_increments_and_elapsed() { |
There was a problem hiding this comment.
This is just testing the fixture, I'd ditch it.
| } | ||
|
|
||
| #[test] | ||
| fn dummy_clock_latency_display() { |
There was a problem hiding this comment.
Also would ditch this if we are dropping DurationExt
| Clk: Clock, | ||
| { | ||
| /// Wrap an [`OnFailure`] implementation with a custom [`Clock`]. | ||
| pub fn with_clock(on_failure: F, clock: Clk) -> Self { |
There was a problem hiding this comment.
This appears untested, there is enough plumbing with the callbacks that it would be good to test.
2 participants
Motivation
Adding a way to allow user to supply their own
Clock(time-recording) implementation if the default one (fromstd) isn't desired. Enabling deterministic testing with fake clocks and WASM-compatibility if the user has a WASM-compatible clock implementation.Closes #657
Solution
Added an abstract
Clocktrait for timing, replacing the original implementation.I decided to go with static dispatch (despite @jlizen suggest otherwise) because adding a
dyn Clockwould make trace layer and service non-copy, resulting a potential breaking API change.TODOs
Clock::now()andClock::elapsed()should take&selfor&mut self