-
Notifications
You must be signed in to change notification settings - Fork 104
fix(timestamps): Deal with sub-microsecond inaccuracies on parse #5002
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
Conversation
f117a67 to
ae0f0cb
Compare
| return { | ||
| "sentry.observed_timestamp_nanos": { | ||
| "stringValue": time_within(ts, expect_resolution="ns", precision="s") | ||
| "stringValue": time_within(ts, expect_resolution="ns") |
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.
This was incorrect, before, the observed_timestamp_nanos is set from the received timestamp, which is derived from the current time in Relay, not an input.
Fixed here because I used these tests to verify the flakiness is gone (which was observable under repeated execution).
7acf98c to
d153981
Compare
d153981 to
3597ba3
Compare
12cf442 to
9452280
Compare
9452280 to
32ba503
Compare
| fn test_timestamp_to_datetime() { | ||
| assert_eq!(timestamp_to_datetime(0.), Utc.timestamp_opt(0, 0)); | ||
| assert_eq!(timestamp_to_datetime(1000.), Utc.timestamp_opt(1000, 0)); | ||
| assert_eq!(timestamp_to_datetime(-1000.), Utc.timestamp_opt(-1000, 0)); | ||
| assert_eq!( | ||
| timestamp_to_datetime(1.234_567), | ||
| Utc.timestamp_opt(1, 234_567_000) | ||
| ); | ||
| assert_eq!(timestamp_to_datetime(2.999_999_51), Utc.timestamp_opt(3, 0)); | ||
| assert_eq!( | ||
| timestamp_to_datetime(2.999_999_45), | ||
| Utc.timestamp_opt(2, 999_999_000) | ||
| ); | ||
| assert_eq!( | ||
| timestamp_to_datetime(-0.000_001), | ||
| Utc.timestamp_opt(-1, 999_999_000) | ||
| ); | ||
| assert_eq!( | ||
| timestamp_to_datetime(-3.000_000_49), | ||
| Utc.timestamp_opt(-3, 0) | ||
| ); | ||
| assert_eq!( | ||
| timestamp_to_datetime(-3.000_000_51), | ||
| Utc.timestamp_opt(-4, 999_999_000) | ||
| ); | ||
| } |
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.
Very nice!
This also deals with floating point sub-microsecond inaccuracies on parsing, instead of only on serialization.
Logs access the nanoseconds of the extracted timestamp and persist these into the payload, this leads to subtle inaccuracies because it never goes through the floating point serialization code which would deal with these inaccuracies.
As it turns out this also applies to calculated span durations which were off.
Instead account for these inaccuracies when parsing from a float, but also when serializing, as the timestamp may have been constructed from something with 'perfect' accuracy.
Also fixes this:
