search

getsentry / sentry-cocoa

The official Sentry SDK for iOS, tvOS, macOS, watchOS.
https://sentry.io/for/cocoa/
MIT License
817 stars 330 forks source link

Use absolute time for tracing duration #2155

Open philipphofmann opened 2 years ago

philipphofmann commented 2 years ago

Description

Currently, our transactions and spans use NSDate, which is not a safe API for time-sensitive measurements because the time can be changed in iOS settings or by the system when it recalibrates with a remote source of truth. Instead, we could switch to absolute system timestamps as used by profiling. https://github.com/getsentry/sentry-cocoa/blob/eefdeedf0d7edd17ac03e89ab7a4da75f24bc1ff/Sources/Sentry/include/SentryTime.h#L10-L14

We could achieve this by internally keeping track of absolute timestamps and converting these to NSDate when finishing a transaction. We might also have to adjust span timestamps, as the system time could have changed during the lifetime of a transaction.

armcknight commented 2 years ago

See also https://github.com/getsentry/sentry-cocoa/discussions/1094#discussioncomment-3521972

philipphofmann commented 2 years ago

This only makes sense if we can map the absolute time back to NSDate. Consider updating the develop docs to use wall time for tracing.

A maybe useful resource https://linux.die.net/man/3/clock_gettime

github-actions[bot] commented 2 years ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

armcknight commented 2 years ago

The problem is that you cannot map system time to NSDate in the presence of the edge cases I've described. My proposition is that we keep both, using absolute time for anything requiring reliable timing like performance transactions and profiling. Then, when we go to construct an envelope, we'll record both, and perform checks so we can set a flag on any payload where we detect they have drifted due to NSDate being changed out from underneath. Then the NSDates can be used to answer customer questions like "when did this event occur" and absolute time to answer "how long did the event last." We essentially should never call -[NSDate timeIntervalSince...] to get the duration of an event.

Ideally we also validate the NSDate-provided timestamps in the payload on the backend, in case the sending device's system time was changed to something obviously wrong before the transaction was started or finished. Maybe we already do this today?

As for clock_gettime, we already use that API here: https://github.com/getsentry/sentry-cocoa/blob/411a940bcf9f7f6808f0282bc583827284d4e88b/Sources/Sentry/SentryTime.mm#L12-L15

armcknight commented 1 year ago

Just saw this bit of UI on a transaction, which is like what I had in mind to notify if we detect a jump in the time reported by NSDate:

image