feat(normalization): Remove stale spans from transactions #2627
Conversation
There are some cases where spans with stale timestamps reach Relay. Relay doesn't perform full normalization on these and forwards them, causing issues in later parts of the pipeline. This PR extends the `TimestampProcessor` to remove stale spans from events.
| Ok(()) | ||
| } | ||
|
|
||
| fn process_span( |
There was a problem hiding this comment.
Docs for the processor are up-to-date, I accidentally updated them in the previous PR.
| if let Some(start_timestamp) = span.start_timestamp.value() { | ||
| if start_timestamp.into_inner().timestamp_millis() < 0 { | ||
| meta.add_error(Error::invalid(format!( | ||
| "timestamp is too stale: {}", | ||
| start_timestamp | ||
| ))); | ||
| return Err(ProcessingAction::DeleteValueHard); | ||
| } | ||
| } | ||
| if let Some(end_timestamp) = span.timestamp.value() { | ||
| if end_timestamp.into_inner().timestamp_millis() < 0 { | ||
| meta.add_error(Error::invalid(format!( | ||
| "timestamp is too stale: {}", | ||
| end_timestamp | ||
| ))); | ||
| return Err(ProcessingAction::DeleteValueHard); | ||
| } | ||
| } |
There was a problem hiding this comment.
What if we put this logic in process_timestamp instead of process_span? That should automatically cover all timestamps, plus it would attach the meta error to the correct field.
There was a problem hiding this comment.
That will deal with only the timestamp itself, but we want to take action on the container of the timestamp. For spans, this means dropping the span; for transactions, this means dropping the entire event. We can't have this level of control in process_timestamp unfortunately.
There was a problem hiding this comment.
I explored introducing a new type of error to remove the parent container. This approach can work for calls on top-level containers like events, but it requires major refactors and decreases development experience a lot for child components like distributions. For that reason, I'm going to stick to the current approach.
There are some cases where spans with stale timestamps reach Relay. Relay doesn't perform full normalization on these and forwards them, causing issues in later parts of the pipeline. This PR extends the
TimestampProcessorto remove stale spans from events.