add timeTaken to query response
#1270
Conversation
WalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant QH as Query Handler
participant QE as Query Execution
C->>QH: Send query request
QH->>QH: Start timer (Instant::now())
QH->>QE: Execute query
QE-->>QH: Return query results
QH->>QH: Compute elapsed time (total_time)
QH->>C: Send response with total_time included
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/handlers/http/query.rs (1)
69-153: 💡 Verification agent🧩 Analysis chain
Verify timing function with automated tests.
The PR objectives mention that testing has not been completed. Adding unit tests to verify that the timing information is correctly included in the response would help ensure the functionality works as expected.
Consider adding a test that verifies:
- The response JSON contains the
totalTimefield- The value is properly formatted
- The time reported is reasonable for the executed query
🌐 Web query:
What's the best way to test timing functionality in Rust?💡 Result:
Testing timing functionality in Rust requires approaches that ensure determinism and avoid real-time delays. Here's a concise guide to best practices:
1. Mocking Time in Unit Tests
For synchronous code, use conditional compilation and thread-local storage to override time functions:
#[cfg(test)] mod mock_time { use std::cell::RefCell; thread_local! { static MOCK_TIME: RefCell<Option<SystemTime>> = RefCell::new(None); } pub fn now() -> SystemTime { /* ... */ } pub fn set_mock_time(time: SystemTime) { /* ... */ } }
- Non-test code uses
SystemTime::now()- Tests control mocked time via
set_mock_time()
Gotcha:#[cfg(test)]doesn’t propagate across crates—useRUSTFLAGSor dedicated crates likefaketime[1].
2. Async Runtime Time Control
For async code (e.g., Tokio timers), use runtime instrumentation:
#[tokio::test(start_paused = true)] async fn test_timer() { let start = Instant::now(); sleep(Duration::from_secs(10)).await; assert_eq!(start.elapsed().as_secs(), 10); // Instant result }
start_paused = truefreezes time until manually advanced [7]- Use
time::advance(Duration).awaitto trigger timers deterministically [4]
3. Preventing Optimizations
For benchmarking loops, use
std::hint::black_boxto prevent compiler optimizations:let start = Instant::now(); for _ in 0..100_000 { std::hint::black_box(test_function()); } let elapsed = start.elapsed();Avoid
SystemTimefor microbenchmarks—useInstantfor monotonic timing [6][12].
Key Libraries
Use Case Crate/Feature Notes Async time mocking tokio::test-utilBuilt-in with Tokio runtime Deterministic test runs ditto_timeCustom time control for async workflows Performance benchmarks criterionStatistical analysis of timings
Common Pitfalls
- Flaky Tests: Avoid real-time sleeps; use mockable time sources [4]
- Cross-Crate Mocking: Use
RUSTFLAGS='--cfg feature_flag'for shared time logic [1]- Doc Tests:
RUSTDOCFLAGSmay not honorRUSTFLAGSfor mocked time [1]For complex async systems, combine thread-local time controls with runtime instrumentation to fully isolate timing behavior in tests [4][7].
Citations:
- 1: https://blog.iany.me/2019/03/how-to-mock-time-in-rust-tests-and-cargo-gotchas-we-met/
- 2: Test case timing information and ordering rust-lang/cargo#4674
- 3: https://users.rust-lang.org/t/how-to-test-speed/70605
- 4: https://www.ditto.com/blog/mocking-time-in-async-rust
- 5: https://blog.waleedkhan.name/rust-incremental-test-times/
- 6: https://www.simonwenkel.com/notes/programming_languages/rust/measure_time_elapsed_with_rust.html
- 7: https://tokio.rs/tokio/topics/testing
- 8: https://doc.rust-lang.org/book/ch11-03-test-organization.html
- 9: https://rustwasm.github.io/book/game-of-life/time-profiling.html
- 10: https://users.rust-lang.org/t/limit-execution-time-of-test/6687
- 11: https://users.rust-lang.org/t/best-practices-for-managing-test-data/18979
- 12: https://users.rust-lang.org/t/what-is-a-proper-way-to-measure-time-execution/91680
- 13: https://docs.rs/function-timer
- 14: https://rustc-dev-guide.rust-lang.org/tests/best-practices.html
- 15: https://zerotomastery.io/blog/complete-guide-to-testing-code-in-rust/
- 16: https://doc.rust-lang.org/book/ch11-01-writing-tests.html
Action: Add Automated Timing Tests Using Deterministic Time Control
Please add unit tests to verify the timing logic in the query handler. In these tests, ensure that:
- The response JSON includes the
total_time(or equivalent) field.- The timing value is properly formatted (e.g., matches the expected duration string format).
- The measured elapsed time is within a logically reasonable range.
For asynchronous code, consider using deterministic time control techniques (like Tokio’s
#[tokio::test(start_paused = true)]or appropriate time-mocking libraries) to avoid real delays and ensure reproducible results.
🧹 Nitpick comments (2)
src/handlers/http/query.rs (2)
136-136: Consider using a more consistent time format for API consumers.The debug formatting (
{:?}) produces human-readable output but might be difficult to parse programmatically. Consider a more structured approach like providing milliseconds as a number or using a standardized time format string.-let total_time = format!("{:?}", start.elapsed()); +let total_time = format!("{:.3}s", start.elapsed().as_secs_f64());Alternatively, if you prefer a more machine-parseable format:
-let total_time = format!("{:?}", start.elapsed()); +let total_time = start.elapsed().as_millis().to_string();
69-143: Consider adding documentation for the new timing functionality.Adding a brief comment explaining the purpose of the timing measurement would improve code maintainability for future developers.
pub async fn query(req: HttpRequest, query_request: Query) -> Result<HttpResponse, QueryError> { + // Start timing the query execution to track performance let start = Instant::now();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/query.rs(2 hunks)src/response.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (5)
src/response.rs (3)
31-31: Field addition looks good.The addition of the
total_timefield to theQueryResponsestruct is appropriate for storing the query execution time.
50-55: Consistent field inclusion with proper JSON structure.The
totalTimefield is correctly added to the JSON response whenwith_fieldsis true, maintaining consistent camelCase naming convention.
57-60: Proper handling of the alternate response format.The
totalTimefield is also correctly included whenwith_fieldsis false, ensuring the timing information is always present in the response regardless of the format requested.src/handlers/http/query.rs (2)
70-70: Timer initialization at function start is correct.Using
Instant::now()at the beginning of the function ensures the timing captures the entire query execution process.
137-143: The QueryResponse struct is correctly populated with timing information.The
total_timefield is properly included in theQueryResponsestruct, ensuring that the timing information will be available in the HTTP response.
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit