-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUMM-655 / RUMM-689 Use NTP time for Logs, Spans and RUM events #321
RUMM-655 / RUMM-689 Use NTP time for Logs, Spans and RUM events #321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DateCorrection.toServerDate(Date) -> Date
looks confusing to me: DateCorrection
sounds more like a struct
containing correction data and toServerDate
looks more like an instance method for Date
type
also, do all Logger
, SpanBuilder
, RUMScopes
, etc. need to know DateCorrectionType
? i think they just need the correct date/time, they don't need to know more than DateProvider.now()
. we can correct the time within DateProvider
by injecting SystemTimeProvider
in it, otherwise DateCorrection
instances are not useful by themselves.
wdyt?
3bdab63
to
b648a5f
Compare
As explained:
Returning server date from There are also other examples where returning server date from
/// Adjusts device time to server time using the time difference calculated with NTP.
internal protocol DateCorrectionType {
/// Corrects given device time to server time using the last known time difference between the two.
func toServerDate(deviceDate: Date) -> Date
} Abstracting it with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see the point of using device time until the very end 👌
regarding the namings, of course it's very clear for you because you wrote it :)
i see a simpler implementation in Android: if you need to get time information you immediately know where to go.
i'm approving because it works ✅
but there seems to be some room for improvement here
What and why?
📦 This PR updates all 3 features to use the NTP server time for events (logs, span, rum events) creation time. This ensures time compatibility with other Datadog products, e.g. aligns client's portion of a distributed trace. It also help by reporting the real time value of events, instead of depending on the device clock, which may be adjusted manually by the user.
How?
Dedicated component is introduced for applying the time correction:
Internally, the
DateCorrection
uses lyft/Kronos to perform time sync over NTP.NTP synchronisation starts with
Datadog.initialize()
and may take from several to a dozen of seconds until it computes the precise time difference. This is notified by printing either.info
or.warning
message to user console (if usingDatadog.debug
flag).All events are collected using device time. The correction to server time is applied at the writing time. This is to preserve relative times within the events, i.e. in Tracing we correct only the "start" date, as the "finish" date is given by relative
span.duration
. Similar for RUM, where all events have theirdate
and other metrics use relative, nanosecond durations.Last, but not least, to not perform double-correction, the
&batch_time=
query item was removed from all products.Review checklist