fix(store): Apply clock drift correction based on received_at #580
Conversation
| @@ -31,6 +32,8 @@ pub struct StoreConfig { | |||
| pub protocol_version: Option<String>, | |||
| pub grouping_config: Option<Value>, | |||
| pub user_agent: Option<String>, | |||
| pub received_at: Option<DateTime<Utc>>, | |||
There was a problem hiding this comment.
This is only optional to retain Default for StoreConfig. Effectively, it is always set in the event processor. Internally, StoreProcessor defaults to Utc::now if we ever choose to not pass a value here.
| @@ -519,6 +520,7 @@ impl EventProcessor { | |||
| remove_other: Some(true), | |||
| normalize_user_agent: Some(true), | |||
| sent_at: envelope.sent_at(), | |||
| received_at: Some(relay_common::instant_to_date_time(start_time)), | |||
There was a problem hiding this comment.
We convert from Instant to DateTime as close as possible to normalization to ensure we have an accurate clock.
|
@jan-auer What was the reason for choosing 55 minutes as the minimum threshold for deciding a clock drift occurred? |
| use crate::protocol::Event; | ||
| use crate::types::{Error, ErrorKind, Meta, ProcessingResult, Timestamp}; | ||
|
|
||
| /// The minimum clock drift for correction to apply. |
There was a problem hiding this comment.
Would add the reasoning here or move the processor's docstring to the module-level
There was a problem hiding this comment.
This is already on the doc-string ClockDriftProcessor, which will be exported in a later PR (so that it can be used by non-processing Relays).
@dashed you can find the reasoning in that same docstring:
/// There is a minimum clock drift of _55 minutes_ to compensate for network latency and small clock
/// drift on the sender's machine, but allow to detect timezone differences. For differences lower
/// than that, no correction is performed.
Let me know if you'd like me to clarify.
| let timestamp_meta = event.timestamp.meta_mut(); | ||
| timestamp_meta.add_error(Error::with(ErrorKind::InvalidData, |e| { | ||
| let reason = format!( | ||
| "clock drift: all timestamps adjusted by {}", |
There was a problem hiding this comment.
Not a huge fan of producing error messages here, we might want to refactor _meta to emit reason codes (not related to this PR in particular, just for the future)
There was a problem hiding this comment.
The ErrorKind you saw is such a reason code, but I didn't want to touch Sentry for this now. If you like, we can follow up with a new error kind and then teach Sentry about it.
Moves clock drift correction into its own processor. The clock drift is now defined as
received_at - sent_at, where the two components are defined as:sent_at: The timestamp of the client at which the request was sent to the server.received_at: The timestamp when the request reached the Relay endpoint.Both timestamps are propagated into the store normalizer, where they are applied by a new
ClockDriftProcessor. The event's timestamp is no longer used, since event submission may be delayed. The minimum drift is 55 minutes; anything lower will skip clock drift correction.Fixes #534
Fixes #535