Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
========================================
Coverage 100.0% 100.0%
========================================
Files 187 210 +23
Lines 14618 15514 +896
========================================
+ Hits 14618 15514 +896 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Moves an async timing helper from cachet’s telemetry extension into the shared tick crate, making it available as an inherent Clock::timed API for measuring future execution duration (useful for telemetry and tests with controlled time).
Changes:
- Added
tick::Timed/tick::TimedResultand implementedClock::timed. - Added tests and an example demonstrating the timing API.
- Updated
cachetto useClock::timedand removed the old telemetry extension module.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tick/src/timed.rs | New Timed future + TimedResult output type for measuring elapsed time. |
| crates/tick/src/lib.rs | Wires the new module and re-exports Timed / TimedResult. |
| crates/tick/src/clock.rs | Adds Clock::timed and associated tests. |
| crates/tick/examples/timed_result.rs | Adds a runnable example using Clock::timed. |
| crates/tick/Cargo.toml | Registers the new example (Tokio-gated). |
| crates/cachet/src/wrapper.rs | Switches from timed_async extension to Clock::timed. |
| crates/cachet/src/telemetry/mod.rs | Removes telemetry ext module from the module tree. |
| crates/cachet/src/telemetry/ext.rs | Deletes the old ClockExt/Timed implementation previously local to cachet. |
| crates/cachet/src/refresh.rs | Switches to Clock::timed. |
| crates/cachet/src/fallback.rs | Switches to Clock::timed (including promotion insert timing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn timed<F>(&self, f: F) -> Timed<F> | ||
| where | ||
| F: Future, | ||
| { |
There was a problem hiding this comment.
Clock::timed uses F: Future, but Future isn’t in scope in this module (no use std::future::Future; and it’s not in the prelude). This will not compile as-is; import std::future::Future or fully qualify the bound.
| /// use std::time::Duration; | ||
| /// |
There was a problem hiding this comment.
The doctest imports std::time::Duration but doesn’t use it. This produces an unused_imports warning when running doctests; consider removing the import to keep the example warning-free.
| /// use std::time::Duration; | |
| /// |
| /// use std::time::Duration; | ||
| /// |
There was a problem hiding this comment.
This doc example imports std::time::Duration but doesn’t use it, which causes an unused_imports warning in doctest compilation. Consider removing the unused import so the example stays warning-free.
| /// use std::time::Duration; | |
| /// |
| impl<F: Future> Future for Timed<F> { | ||
| type Output = TimedResult<F::Output>; |
There was a problem hiding this comment.
Timed's Future implementation uses the Future trait (impl<F: Future> Future for Timed<F>) but Future isn’t imported or fully qualified, which will fail to compile. Add use std::future::Future; (or fully qualify std::future::Future in both places).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[tokio::test] | ||
| async fn timed_handles_pending() { | ||
| use std::pin::Pin; | ||
| use std::sync::Arc; |
There was a problem hiding this comment.
This new #[tokio::test] is missing #[cfg_attr(miri, ignore)]. CI runs Miri for the workspace, and Tokio-based async tests are typically ignored under Miri elsewhere in this file.
| .clock | ||
| .timed_async(self.inner.primary.insert(key.clone(), v.clone())) | ||
| .await; | ||
| let timed_insert = self.inner.clock.timed(self.inner.primary.insert(key.clone(), v.clone())).await; |
There was a problem hiding this comment.
This statement is formatted as a single very long line. Please run rustfmt (or break the call across lines) to match the project's formatting expectations and avoid rustfmt CI failures.
| let timed_insert = self.inner.clock.timed(self.inner.primary.insert(key.clone(), v.clone())).await; | |
| let timed_insert = self | |
| .inner | |
| .clock | |
| .timed(self.inner.primary.insert(key.clone(), v.clone())) | |
| .await; |
| /// | ||
| /// # async fn timed_example(clock: &Clock) { | ||
| /// let TimedResult { result, duration } = clock.timed(async { 42 }).await; | ||
| /// assert_eq!(result, 42); |
There was a problem hiding this comment.
The doctest example introduces unused bindings/imports (Duration import and the duration binding from TimedResult). This can produce warnings in doctest builds; consider removing the unused import and/or using _duration (or asserting on duration) to keep the example warning-free.
| /// assert_eq!(result, 42); | |
| /// assert_eq!(result, 42); | |
| /// assert!(duration >= Duration::from_millis(0)); |
| /// use std::time::Duration; | ||
| /// | ||
| /// use tick::{Clock, TimedResult}; | ||
| /// | ||
| /// # async fn example(clock: &Clock) { | ||
| /// let TimedResult { result, duration } = clock.timed(async { 42 }).await; |
There was a problem hiding this comment.
This doctest example has unused items (the Duration import and the duration binding). Consider removing the unused import and/or renaming duration to _duration (or asserting on it) so the example stays warning-free when compiled as a doctest.
| /// use std::time::Duration; | |
| /// | |
| /// use tick::{Clock, TimedResult}; | |
| /// | |
| /// # async fn example(clock: &Clock) { | |
| /// let TimedResult { result, duration } = clock.timed(async { 42 }).await; | |
| /// use tick::{Clock, TimedResult}; | |
| /// | |
| /// # async fn example(clock: &Clock) { | |
| /// let TimedResult { result, duration: _duration } = clock.timed(async { 42 }).await; |
| #[tokio::test] | ||
| async fn timed_measures_duration() { | ||
| let control = ClockControl::new(); | ||
| let clock = control.to_clock(); |
There was a problem hiding this comment.
This new #[tokio::test] is not annotated with #[cfg_attr(miri, ignore)], unlike other Tokio-based tests in this module. Since CI runs cargo miri test --all-features, this test should be ignored under Miri (or rewritten to avoid Tokio).
crates/tick/src/timed.rs
Outdated
| /// | ||
| /// # async fn example(clock: &Clock) { | ||
| /// let TimedResult { result, duration } = clock.timed(async { 42 }).await; | ||
| /// println!("Result: {}, Duration: {:?}", result, duration); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Created via [`Clock::timed`][crate::Clock::timed]. | ||
| #[must_use = "futures do nothing unless polled"] | ||
| pub struct Timed<F> { | ||
| #[pin] | ||
| pub(crate) inner: F, | ||
| pub(crate) watch: Stopwatch, | ||
| } |
There was a problem hiding this comment.
Timed is a public type but doesn’t implement Debug. With the workspace missing_debug_implementations = "warn" lint (and CI running with -D warnings), this will fail the build. Derive/implement Debug for Timed (matching other public futures like Timeout).
| pub(crate) mod ext; | ||
| #[cfg(any(feature = "metrics", test))] | ||
| pub(crate) mod metrics; | ||
|
|
There was a problem hiding this comment.
With telemetry::ext removed, cachet no longer appears to use pin-project-lite anywhere. Because CI runs cargo clippy --all-targets --all-features -- -D warnings and the workspace enables Clippy’s cargo lint group, this is likely to trigger clippy::unused_crate_dependencies. Remove pin-project-lite from crates/cachet/Cargo.toml (or add an explicit allow if it’s intentionally kept).
| // Ensure `pin-project-lite` is considered used so that | |
| // `clippy::unused_crate_dependencies` does not trigger for this crate. | |
| use pin_project_lite as _; |
For cachet this helper function was added to a
Clockextension to time async futures and return the result and duration (primarily for telemetry). I'm proposing moving this to the tick crate.