ref(normalization): Move span validation to NormalizeProcessor #2679
Conversation
| let span = get_value!(event.spans).unwrap().first().unwrap(); | ||
| assert_eq!(get_value!(span.op).unwrap(), &"default".to_owned()); |
There was a problem hiding this comment.
This is modified from the original test. There used to be a snapshot over the whole span that now fails because transaction.received and transaction.contexts.trace.status exist. Instead of updating the snapshot, I've updated the assertion of what the test is validating.
There was a problem hiding this comment.
Could we simplify this test and just call validate_span_timestamps on a Span instead of creating an event?
There was a problem hiding this comment.
I'd like to minimize the changes on this PR, but happy to update it in a follow-up.
| assert_eq!( | ||
| process_value( | ||
| &mut event, | ||
| &mut NormalizeProcessor::default(), |
There was a problem hiding this comment.
These tests also use NormalizeProcessor instead of TransactionProcessor.
| @@ -188,6 +197,22 @@ impl<'a> Processor for NormalizeProcessor<'a> { | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| fn process_span( | |||
There was a problem hiding this comment.
Does this now also run for error events? I believe the TransactionProcessor prevented that.
There was a problem hiding this comment.
Yes, it does for errors. Errors aren't supposed to have spans so I thought it'd be convenient to skip that check over being a bit more defensive.
| let span = get_value!(event.spans).unwrap().first().unwrap(); | ||
| assert_eq!(get_value!(span.op).unwrap(), &"default".to_owned()); |
There was a problem hiding this comment.
Could we simplify this test and just call validate_span_timestamps on a Span instead of creating an event?
CHANGELOG.md
Outdated
| @@ -14,6 +14,7 @@ | |||
| - Disable resource link span ingestion. ([#2647](https://github.com/getsentry/relay/pull/2647)) | |||
| - Collect `http.decoded_response_content_length`. ([#2638](https://github.com/getsentry/relay/pull/2638)) | |||
| - Add TTID and TTFD tags to mobile spans. ([#2662](https://github.com/getsentry/relay/pull/2662)) | |||
| - Validate span timestamps and IDs in light normalization. ([#2679](https://github.com/getsentry/relay/pull/2679)) | |||
There was a problem hiding this comment.
Do we actually need a changelog for this? Span validation was already part of light normalization (through the TransactionProcessor), so that does not change from an outsider's point of view. Let me know if I'm missing something.
There was a problem hiding this comment.
Good catch! These checks didn't run when renormalize was enabled, which now do (first note on the PR description). I updated the changelog to be a bit more explicit.
There was a problem hiding this comment.
This new logic runs before light normalization's is_renormalize flag check. This means that this new code will run even if the flag is disabled, which previously wasn't the case. The purpose of is_renormalize is to make a decision on non-idempotent steps, and these checks are idempotent so this action should be safe.
Do we have enough test coverage to prove this?
TransactionsProcessorperforms some span attribute validation logic. This PR moves that logic toNormalizeProcessorin an attempt to move a step closer to having a single processor.Notes:
is_renormalizeflag check. This means that this new code will run even if the flag is disabled, which previously wasn't the case. The purpose ofis_renormalizeis to make a decision on non-idempotent steps, and these checks are idempotent so this action should be safe.NormalizeProcessorfrom top to bottom, but the end result will be the same. The span-related functionality at the bottom of the function is being updated often, so I'd rather to approach that at a later time to avoid conflicts.