From e06ff5f50763e41f999542c71991d95d12125aca Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Fri, 23 Feb 2024 20:07:13 -0500 Subject: [PATCH] Use `SystemClock` in `EventCoalescer` (#8317) This PR updates the `EventCoalescer` to use the `SystemClock` trait to abstract over the clock. This allows us to test the advancement of time without relying on the caller passing in the current time. Release Notes: - N/A --- crates/client/src/telemetry.rs | 2 +- .../client/src/telemetry/event_coalescer.rs | 107 ++++++++++-------- 2 files changed, 62 insertions(+), 47 deletions(-) diff --git a/crates/client/src/telemetry.rs b/crates/client/src/telemetry.rs index 2bee99073c..7d1e248ec2 100644 --- a/crates/client/src/telemetry.rs +++ b/crates/client/src/telemetry.rs @@ -96,7 +96,7 @@ impl Telemetry { log_file: None, is_staff: None, first_event_date_time: None, - event_coalescer: EventCoalescer::new(), + event_coalescer: EventCoalescer::new(clock.clone()), max_queue_size: MAX_QUEUE_LEN, })); diff --git a/crates/client/src/telemetry/event_coalescer.rs b/crates/client/src/telemetry/event_coalescer.rs index f0efeb38e6..33bcf492f6 100644 --- a/crates/client/src/telemetry/event_coalescer.rs +++ b/crates/client/src/telemetry/event_coalescer.rs @@ -1,6 +1,9 @@ -use chrono::{DateTime, Duration, Utc}; +use std::sync::Arc; use std::time; +use chrono::{DateTime, Duration, Utc}; +use clock::SystemClock; + const COALESCE_TIMEOUT: time::Duration = time::Duration::from_secs(20); const SIMULATED_DURATION_FOR_SINGLE_EVENT: time::Duration = time::Duration::from_millis(1); @@ -12,30 +15,20 @@ struct PeriodData { } pub struct EventCoalescer { + clock: Arc, state: Option, } impl EventCoalescer { - pub fn new() -> Self { - Self { state: None } + pub fn new(clock: Arc) -> Self { + Self { clock, state: None } } pub fn log_event( &mut self, environment: &'static str, ) -> Option<(DateTime, DateTime, &'static str)> { - self.log_event_with_time(Utc::now(), environment) - } - - // pub fn close_current_period(&mut self) -> Option<(DateTime, DateTime)> { - // self.environment.map(|env| self.log_event(env)).flatten() - // } - - fn log_event_with_time( - &mut self, - log_time: DateTime, - environment: &'static str, - ) -> Option<(DateTime, DateTime, &'static str)> { + let log_time = self.clock.utc_now(); let coalesce_timeout = Duration::from_std(COALESCE_TIMEOUT).unwrap(); let Some(state) = &mut self.state else { @@ -78,18 +71,22 @@ impl EventCoalescer { #[cfg(test)] mod tests { use chrono::TimeZone; + use clock::FakeSystemClock; use super::*; #[test] fn test_same_context_exceeding_timeout() { + let clock = Arc::new(FakeSystemClock::new( + Utc.with_ymd_and_hms(1990, 4, 12, 0, 0, 0).unwrap(), + )); let environment_1 = "environment_1"; - let mut event_coalescer = EventCoalescer::new(); + let mut event_coalescer = EventCoalescer::new(clock.clone()); assert_eq!(event_coalescer.state, None); - let period_start = Utc.with_ymd_and_hms(1990, 4, 12, 0, 0, 0).unwrap(); - let period_data = event_coalescer.log_event_with_time(period_start, environment_1); + let period_start = clock.utc_now(); + let period_data = event_coalescer.log_event(environment_1); assert_eq!(period_data, None); assert_eq!( @@ -102,12 +99,12 @@ mod tests { ); let within_timeout_adjustment = Duration::from_std(COALESCE_TIMEOUT / 2).unwrap(); - let mut period_end = period_start; // Ensure that many calls within the timeout don't start a new period for _ in 0..100 { - period_end += within_timeout_adjustment; - let period_data = event_coalescer.log_event_with_time(period_end, environment_1); + clock.advance(within_timeout_adjustment); + let period_data = event_coalescer.log_event(environment_1); + let period_end = clock.utc_now(); assert_eq!(period_data, None); assert_eq!( @@ -120,10 +117,12 @@ mod tests { ); } + let period_end = clock.utc_now(); let exceed_timeout_adjustment = Duration::from_std(COALESCE_TIMEOUT * 2).unwrap(); // Logging an event exceeding the timeout should start a new period - let new_period_start = period_end + exceed_timeout_adjustment; - let period_data = event_coalescer.log_event_with_time(new_period_start, environment_1); + clock.advance(exceed_timeout_adjustment); + let new_period_start = clock.utc_now(); + let period_data = event_coalescer.log_event(environment_1); assert_eq!(period_data, Some((period_start, period_end, environment_1))); assert_eq!( @@ -138,13 +137,16 @@ mod tests { #[test] fn test_different_environment_under_timeout() { + let clock = Arc::new(FakeSystemClock::new( + Utc.with_ymd_and_hms(1990, 4, 12, 0, 0, 0).unwrap(), + )); let environment_1 = "environment_1"; - let mut event_coalescer = EventCoalescer::new(); + let mut event_coalescer = EventCoalescer::new(clock.clone()); assert_eq!(event_coalescer.state, None); - let period_start = Utc.with_ymd_and_hms(1990, 4, 12, 0, 0, 0).unwrap(); - let period_data = event_coalescer.log_event_with_time(period_start, environment_1); + let period_start = clock.utc_now(); + let period_data = event_coalescer.log_event(environment_1); assert_eq!(period_data, None); assert_eq!( @@ -157,8 +159,9 @@ mod tests { ); let within_timeout_adjustment = Duration::from_std(COALESCE_TIMEOUT / 2).unwrap(); - let period_end = period_start + within_timeout_adjustment; - let period_data = event_coalescer.log_event_with_time(period_end, environment_1); + clock.advance(within_timeout_adjustment); + let period_end = clock.utc_now(); + let period_data = event_coalescer.log_event(environment_1); assert_eq!(period_data, None); assert_eq!( @@ -170,10 +173,12 @@ mod tests { }) ); + clock.advance(within_timeout_adjustment); + // Logging an event within the timeout but with a different environment should start a new period - let period_end = period_end + within_timeout_adjustment; + let period_end = clock.utc_now(); let environment_2 = "environment_2"; - let period_data = event_coalescer.log_event_with_time(period_end, environment_2); + let period_data = event_coalescer.log_event(environment_2); assert_eq!(period_data, Some((period_start, period_end, environment_1))); assert_eq!( @@ -188,13 +193,16 @@ mod tests { #[test] fn test_switching_environment_while_within_timeout() { + let clock = Arc::new(FakeSystemClock::new( + Utc.with_ymd_and_hms(1990, 4, 12, 0, 0, 0).unwrap(), + )); let environment_1 = "environment_1"; - let mut event_coalescer = EventCoalescer::new(); + let mut event_coalescer = EventCoalescer::new(clock.clone()); assert_eq!(event_coalescer.state, None); - let period_start = Utc.with_ymd_and_hms(1990, 4, 12, 0, 0, 0).unwrap(); - let period_data = event_coalescer.log_event_with_time(period_start, environment_1); + let period_start = clock.utc_now(); + let period_data = event_coalescer.log_event(environment_1); assert_eq!(period_data, None); assert_eq!( @@ -207,9 +215,10 @@ mod tests { ); let within_timeout_adjustment = Duration::from_std(COALESCE_TIMEOUT / 2).unwrap(); - let period_end = period_start + within_timeout_adjustment; + clock.advance(within_timeout_adjustment); + let period_end = clock.utc_now(); let environment_2 = "environment_2"; - let period_data = event_coalescer.log_event_with_time(period_end, environment_2); + let period_data = event_coalescer.log_event(environment_2); assert_eq!(period_data, Some((period_start, period_end, environment_1))); assert_eq!( @@ -221,22 +230,26 @@ mod tests { }) ); } - // // 0 20 40 60 - // // |-------------------|-------------------|-------------------|------------------- - // // |--------|----------env change - // // |------------------- - // // |period_start |period_end - // // |new_period_start + + // 0 20 40 60 + // |-------------------|-------------------|-------------------|------------------- + // |--------|----------env change + // |------------------- + // |period_start |period_end + // |new_period_start #[test] fn test_switching_environment_while_exceeding_timeout() { + let clock = Arc::new(FakeSystemClock::new( + Utc.with_ymd_and_hms(1990, 4, 12, 0, 0, 0).unwrap(), + )); let environment_1 = "environment_1"; - let mut event_coalescer = EventCoalescer::new(); + let mut event_coalescer = EventCoalescer::new(clock.clone()); assert_eq!(event_coalescer.state, None); - let period_start = Utc.with_ymd_and_hms(1990, 4, 12, 0, 0, 0).unwrap(); - let period_data = event_coalescer.log_event_with_time(period_start, environment_1); + let period_start = clock.utc_now(); + let period_data = event_coalescer.log_event(environment_1); assert_eq!(period_data, None); assert_eq!( @@ -249,9 +262,10 @@ mod tests { ); let exceed_timeout_adjustment = Duration::from_std(COALESCE_TIMEOUT * 2).unwrap(); - let period_end = period_start + exceed_timeout_adjustment; + clock.advance(exceed_timeout_adjustment); + let period_end = clock.utc_now(); let environment_2 = "environment_2"; - let period_data = event_coalescer.log_event_with_time(period_end, environment_2); + let period_data = event_coalescer.log_event(environment_2); assert_eq!( period_data, @@ -270,6 +284,7 @@ mod tests { }) ); } + // 0 20 40 60 // |-------------------|-------------------|-------------------|------------------- // |--------|----------------------------------------env change