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
This commit is contained in:
Marshall Bowers 2024-02-23 20:07:13 -05:00 committed by GitHub
parent 9b44ba9382
commit e06ff5f507
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 62 additions and 47 deletions

View file

@ -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,
}));

View file

@ -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<dyn SystemClock>,
state: Option<PeriodData>,
}
impl EventCoalescer {
pub fn new() -> Self {
Self { state: None }
pub fn new(clock: Arc<dyn SystemClock>) -> Self {
Self { clock, state: None }
}
pub fn log_event(
&mut self,
environment: &'static str,
) -> Option<(DateTime<Utc>, DateTime<Utc>, &'static str)> {
self.log_event_with_time(Utc::now(), environment)
}
// pub fn close_current_period(&mut self) -> Option<(DateTime<Utc>, DateTime<Utc>)> {
// self.environment.map(|env| self.log_event(env)).flatten()
// }
fn log_event_with_time(
&mut self,
log_time: DateTime<Utc>,
environment: &'static str,
) -> Option<(DateTime<Utc>, DateTime<Utc>, &'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