ref(relay): Use DateTime instead of Instant for start_time
#4184
Conversation
iambriccardo
changed the title
iambriccardo
changed the title
DateTime instead of Instant for start_time
| /// Returns the [`ReceivedAt`] corresponding to provided timestamp. | ||
| pub fn from_timestamp_millis(timestamp: i64) -> Self { | ||
| let datetime = DateTime::<Utc>::from_timestamp_millis(timestamp).unwrap_or_else(Utc::now); | ||
|
|
||
| Self(datetime) | ||
| } |
| fn start_time_from_timestamp() { | ||
| let elapsed = Duration::from_secs(10); | ||
| let now = Utc::now(); | ||
| let past = now - chrono::Duration::from_std(elapsed).unwrap(); | ||
| let start_time = ReceivedAt::from_timestamp_millis(past.timestamp_millis()).into_inner(); | ||
|
|
||
| // Check that the difference between the now and generated start_time is about 10s | ||
| let diff = now - start_time; | ||
| let diff_duration = diff.to_std().unwrap(); | ||
| assert!(diff_duration < elapsed + Duration::from_millis(50)); | ||
| assert!(diff_duration > elapsed - Duration::from_millis(50)); | ||
| } |
There was a problem hiding this comment.
This test was changed with the same logic.
| @@ -26,7 +29,7 @@ pub async fn metrics(mut request: Request, next: Next) -> Response { | |||
| let response = next.run(request).await; | |||
|
|
|||
| relay_statsd::metric!( | |||
| timer(RelayTimers::RequestsDuration) = start_time.into_inner().elapsed(), | |||
| timer(RelayTimers::RequestsDuration) = request_start.elapsed(), | |||
There was a problem hiding this comment.
Since we now use received_at as DateTime I wanted to still use an Instant for monitoring the request duration.
| sent_at, | ||
| source, | ||
| } = message; | ||
|
|
||
| let received_timestamp = UnixTimestamp::from_instant(start_time); | ||
| let received_timestamp = |
There was a problem hiding this comment.
I deemed the unwrap_or as a safe operation.
There was a problem hiding this comment.
What do you mean by this? Why wouldn't it be safe?
There was a problem hiding this comment.
By safe I meant semantically, meaning if it makes sense to default to now.
There was a problem hiding this comment.
Strictly speaking, defaulting to 1970-01-01 (UnixTimestamp::from_secs(0)) would make sense semantically, because from_datetime returns None only if the datetime is less than that. Either way it's a programming error or data corruption if it happens, since we control received_at ourselves.
This PR changes the
start_timefield of several components within Relay from typeInstanttoDateTime<Utc>and renames it toreceived_at.Closes: https://github.com/getsentry/team-ingest/issues/564