fix(spans): Reject spans with invalid timestamps #3013
Conversation
|
|
||
| **Internal**: | ||
|
|
||
| - Drop spans ending outside the valid timestamp range. ([#3013](https://github.com/getsentry/relay/pull/3013)) |
There was a problem hiding this comment.
The change only applies to Relays and not librelay, so not updating py/Changelog.
| // Transactions with spans with invalid timestamps are not expected to be | ||
| // rejected. |
There was a problem hiding this comment.
This statement is not really true right now, since in the validate_span function we can return an error if either start or end timestamps are absent (which is kind of invalid too), so we will return the error from validate_transaction as well and reject the transaction as well.
It would be great to change this comment here to reflect what condition wouldn't actually reject the transaction.
There was a problem hiding this comment.
Thanks for pointing it out! I reworded it in b9b8b70, let me know if this is still unclear.
| @@ -1881,3 +1881,74 @@ def test_span_extraction_with_ddm_missing_values( | |||
| } | |||
|
|
|||
| spans_consumer.assert_empty() | |||
|
|
|||
|
|
|||
| def test_span_reject_invalid_timestamps( | |||
There was a problem hiding this comment.
Do we need an integration test for this? Could it be a unit test instead?
There was a problem hiding this comment.
A unit test is not as solid. The timestamp range on spans is only enforced in stand-alone spans and not on spans processed as part of transactions, meaning testing a single component is not enough and we should test the span processing pipeline instead.
Co-authored-by: Oleksandr <1931331+olksdr@users.noreply.github.com>
90fa749 removed the
timestamp_rangefrom span normalization as it was not used. This PR introduces it back and validates a span's timestamps are valid. A span has valid timestamps when the following conditions are met:startandendtimestamps exist.start<=endtimestamps.endtimestamp is within the valid timestamp range. This range is defined in relay's config, in the secondary aggregator.