ref(normalization): Remove TransactionsProcessor #2714
Conversation
| if let Some(trace_context) = event.context_mut::<TraceContext>() { | ||
| trace_context.op.get_or_insert_with(|| "default".to_owned()); | ||
| } |
There was a problem hiding this comment.
This was part of validate_transaction, but moving it here allows the validation function to accept a &event instead of &mut event.
| // TODO: Parts of this processor should probably be a filter so we | ||
| // can revert some changes to ProcessingAction) | ||
|
|
||
| validate_transaction(event, self.config.transaction_range.as_ref())?; |
There was a problem hiding this comment.
The validation happened after normalizing the transaction name, a few lines below. Since validation does not depend on the transaction name, moving it to the top here allows Relay to reject invalid transactions a bit earlier and save some computing. Note the outcome does not change. Happy to move this to another PR if this feels out of scope.
| @@ -1362,6 +1326,11 @@ mod tests { | |||
| }, | |||
| ]"#); | |||
|
|
|||
| assert_eq!( | |||
There was a problem hiding this comment.
These assertions on the transaction source were lost in a previous PR, adding them back in this one.
| let processor_config = NormalizeProcessorConfig::default(); | ||
| let mut processor = NormalizeProcessor::new(processor_config.clone()); | ||
| process_value(&mut processed, &mut processor, ProcessingState::root()).unwrap(); | ||
| remove_event_received(&mut processed); |
There was a problem hiding this comment.
event.received is always updated with the latest timestamp, so removing it from the events makes us compare them easily. This is not new functionality and is already the case, see:
We can probably get rid of some tests when we unify more functionality.
| @@ -510,7 +478,7 @@ mod tests { | |||
| fn test_replace_missing_timestamp() { | |||
| let span = Span { | |||
| start_timestamp: Annotated::new( | |||
| Utc.with_ymd_and_hms(1968, 1, 1, 0, 0, 1).unwrap().into(), | |||
| Utc.with_ymd_and_hms(1970, 1, 1, 0, 0, 1).unwrap().into(), | |||
There was a problem hiding this comment.
Since the normalize processor now runs more logic, it'll drop spans happening before 1970/1/1. This fixes the test while keeping it valid.
There was a problem hiding this comment.
Can this file actually be remove/renamed now? It doesn't contain processor anymore and it's more like utils for validation and looks like only 1 function for normalization.
There was a problem hiding this comment.
Good suggestion! I'll get to this in a follow-up PR, this one is already big enough.
* master: feat(protocol): Add access to `device.model` on contexts (#2728) Revert "feat(protocol): Support comparisons in rule conditions on strings" (#2727) ref(normalization): Remove TransactionsProcessor (#2714) feat(protocol): Support comparisons in rule conditions on strings (#2721) test(normalization): Reduce span default op test scope (#2726) release: 23.11.0 fix(normalization): Restore performance score normalization (#2725)
This PR removes the transaction processor and moves its normalization to
NormalizeProcessor, bringing us a step closer to having a single processor. There are no new normalization steps in this PR.#skip-changelog