feat(clock-drift): Apply clock drift normalization only if `sent_at` is defined #3405

iambriccardo · 2024-04-10T07:42:33Z

This PR applies clock drift normalization only when sent_at is defined in the envelope headers.

The previous implementation was falling back to event.timestamp if the incoming event was a transaction. That was expected behavior for old SDKs which didn't send sent_at however we do not have anymore SDKs using the /store endpoint, which means that we can assume that sent_at is sent for each envelope. If not sent, we won't perform clock drift normalization instead of using event.timestamp like before.

Closes: #1625

iambriccardo · 2024-04-10T07:52:31Z

relay-event-normalization/src/validation.rs

@@ -308,30 +314,30 @@ fn normalize_timestamps(
 ) {
    let received_at = received_at.unwrap_or_else(Utc::now);

-    let mut sent_at = None;
+    let mut event_timestamp = None;


Renamed because I didn't want confusion with the new has_sent_at property.

iker-barriocanal · 2024-04-10T14:58:04Z

That was expected behavior for old SDKs but now it should not be needed anymore.

What are the SDK versions where this is fixed? Do we have a big event influx from these outdated versions? Bonus points if this is documented in develop docs!

Dav1dde · 2024-04-11T07:19:23Z

    // TODO: Temporary workaround before processing. Experimental SDKs relied on a buggy
    // clock drift correction that assumes the event timestamp is the sent_at time. This
    // should be removed as soon as legacy ingestion has been removed.

Curious what this has to do with the legacy ingestion (I assume directly to Sentry).

iambriccardo · 2024-04-11T11:25:13Z

    // TODO: Temporary workaround before processing. Experimental SDKs relied on a buggy
    // clock drift correction that assumes the event timestamp is the sent_at time. This
    // should be removed as soon as legacy ingestion has been removed.

Curious what this has to do with the legacy ingestion (I assume directly to Sentry).

@jan-auer mentioned this in one of our private discussions. From what I understood, the store endpoint is used by old SDKs that send envelopes without the sent_at header assuming that the event timestamp is the same as the time the envelope is sent. New SDKs, are sending envelopes with the sent_at which differs from event timestamp.

jjbayer · 2024-04-11T11:27:52Z

CHANGELOG.md

 - Allow spans with no `exclusive_time`. ([#3385](https://github.com/getsentry/relay/pull/3385))
+- Perform clock drift normalization only when `sent_at` is set in the `Envelope` headers. ([#3405](https://github.com/getsentry/relay/pull/3405))


Please move Bug Fixes to above Features, it's the first thing people should read IMO.

Yep, will do.

jjbayer · 2024-04-11T11:29:54Z

tests/integration/test_clock_drift.py

+        timeout=1
+    ).get_transaction_event()
+    error_name, error_metadata = transaction_event["_meta"]["timestamp"][""]["err"][0]
+    assert error_name == "clock_drift"


Should we also assert on the actual timestamps in the event? (Same for tests below).

I was considering it, my only problem was that received_at seems to be inferred by Relay on receive time, so I might have to either mock things or freeze time but it seems like this is not possible in an integration test.

What I know is that sdk_time will be one_month_ago and server_time will be received_at and the new timestamp is + the difference of the above.

I opted for a middle ground, I do test for > in the timestamps which should be normalized, and = when the timestamps must remain the same.

The assert could be a much smaller window though, right? transaction_event["timestamp"] > now.timestamp()

If there was a way to freeze or hardcode the received_at time, I could compute the exact test expectations.

If we can't, to make the test more precise, we could assert transaction_event["timestamp"] == now within some bounds but I am not super confident to define them since they are dependent on the time it takes for the integration test to run and I would not want to introduce a flaky test.

I wouldn't exactly assert, but if you compare to the now value taken at the beginning of the test, that should be good, additionally you could also take another "now" value at the end and check whether the expected value is between those. I wouldn't try to freeze time etc. that will just break at some point. Being within a few seconds should be good enough.

jjbayer · 2024-04-11T11:31:41Z

@iambriccardo in the PR description, could you elaborate a bit more on why this is now OK? (Transactions no longer sent to /store endpoint...).

Dav1dde · 2024-04-11T12:39:09Z

Re store endpoint, but these old (broken?) SDKs are still sending data, why can we change it now?

jan-auer · 2024-04-11T12:58:02Z

This was used just for beta SDKs while Performance monitoring was experimental. As we launched the first stable version of Performance Monitoring to general availability, we required an SDK upgrade. Hence this is safe to remove.

Dav1dde · 2024-04-11T13:05:17Z

Nice, thanks!

Dav1dde · 2024-04-11T13:07:10Z

tests/integration/test_clock_drift.py

+        timeout=1
+    ).get_transaction_event()
+    error_name, error_metadata = transaction_event["_meta"]["timestamp"][""]["err"][0]
+    assert error_name == "clock_drift"


The assert could be a much smaller window though, right? transaction_event["timestamp"] > now.timestamp()

iambriccardo commented Apr 10, 2024

View reviewed changes

Fix

4ec8bdc

iambriccardo force-pushed the riccardo/fix/correctly-apply-clock-drift branch from cd61803 to 4ec8bdc Compare April 10, 2024 13:28

iambriccardo changed the title ~~feat(clock-drift): Do not apply clock drift normalization if is not sent~~ feat(clock-drift): Remove event.timestamp from clock normalization Apr 10, 2024

iambriccardo changed the title ~~feat(clock-drift): Remove event.timestamp from clock normalization~~ feat(clock-drift): Apply clock drift normalization only if sent_at is defined Apr 10, 2024

Fix

1b0adb7

iambriccardo marked this pull request as ready for review April 10, 2024 14:18

iambriccardo requested a review from a team as a code owner April 10, 2024 14:18

iker-barriocanal assigned iambriccardo Apr 10, 2024

iambriccardo added 2 commits April 11, 2024 13:19

Add tests

975f04d

Update changelog

c653c34

Merge

0609ce7

jjbayer reviewed Apr 11, 2024

View reviewed changes

Fix

1dc4a6f

Improve

ae04f5f

iambriccardo requested a review from jjbayer April 11, 2024 12:11

Dav1dde approved these changes Apr 11, 2024

View reviewed changes

Improve

66341a2

iambriccardo merged commit 368ec8c into master Apr 12, 2024
20 checks passed

iambriccardo deleted the riccardo/fix/correctly-apply-clock-drift branch April 12, 2024 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clock-drift): Apply clock drift normalization only if `sent_at` is defined #3405

feat(clock-drift): Apply clock drift normalization only if `sent_at` is defined #3405

iambriccardo commented Apr 10, 2024 •

edited

Loading

iambriccardo Apr 10, 2024

iker-barriocanal commented Apr 10, 2024

Dav1dde commented Apr 11, 2024

iambriccardo commented Apr 11, 2024

jjbayer Apr 11, 2024

iambriccardo Apr 11, 2024

jjbayer Apr 11, 2024

iambriccardo Apr 11, 2024

iambriccardo Apr 11, 2024

iambriccardo Apr 11, 2024

Dav1dde Apr 11, 2024

iambriccardo Apr 11, 2024

Dav1dde Apr 11, 2024

jjbayer commented Apr 11, 2024

Dav1dde commented Apr 11, 2024

jan-auer commented Apr 11, 2024

Dav1dde commented Apr 11, 2024

Dav1dde Apr 11, 2024

		- Allow spans with no `exclusive_time`. ([#3385](https://github.com/getsentry/relay/pull/3385))
		- Perform clock drift normalization only when `sent_at` is set in the `Envelope` headers. ([#3405](https://github.com/getsentry/relay/pull/3405))

feat(clock-drift): Apply clock drift normalization only if sent_at is defined #3405

feat(clock-drift): Apply clock drift normalization only if sent_at is defined #3405

Conversation

iambriccardo commented Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

iker-barriocanal commented Apr 10, 2024

Dav1dde commented Apr 11, 2024

iambriccardo commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer commented Apr 11, 2024

Dav1dde commented Apr 11, 2024

jan-auer commented Apr 11, 2024

Dav1dde commented Apr 11, 2024

Choose a reason for hiding this comment

feat(clock-drift): Apply clock drift normalization only if `sent_at` is defined #3405

feat(clock-drift): Apply clock drift normalization only if `sent_at` is defined #3405

iambriccardo commented Apr 10, 2024 •

edited

Loading