ref(processor): Remove event_fully_normalized from state #4371
Conversation
|
|
||
| if_processing!(self.inner.config, { | ||
| unreal::process(state)?; | ||
| attachment::create_placeholders(state); | ||
| if let Some(inner_event_fully_normalized) = unreal::process(state)? { |
There was a problem hiding this comment.
Very ugly, but we have to have a tristate, since we need to represent the fact of not updating the event normalization boolean.
There was a problem hiding this comment.
This would be less awkward with an enum that encodes the tristate. Then you could do something like event_fully_normalized.update(unreal::process(state)?);
relay-server/src/envelope.rs
Outdated
| @@ -1174,6 +1174,14 @@ impl Envelope { | |||
| } | |||
| } | |||
|
|
|||
| /// Returns `true` whether the event is fully normalized, `false`. | |||
There was a problem hiding this comment.
| /// Returns `true` whether the event is fully normalized, `false`. | |
| /// Returns `true` if the event is fully normalized. |
| @@ -1461,20 +1448,21 @@ impl EnvelopeProcessorService { | |||
| fn normalize_event<G: EventProcessing>( | |||
| &self, | |||
| state: &mut ProcessEnvelopeState<G>, | |||
| ) -> Result<(), ProcessingError> { | |||
| mut event_fully_normalized: bool, | |||
| ) -> Result<Option<bool>, ProcessingError> { | |||
There was a problem hiding this comment.
We should not pass this around as a boolean, it's too easy to have multiple bools mix. Let's make a newtype, it can be as simple as struct InsertNameHere(pub bool).
There was a problem hiding this comment.
I did not opt for the enum since I want the type to represent the two possible states and then the Option refers to whether the calculation should change the value.
I could create a small macro to set the variables automatically, it could make the code nicer but it feels a bit overkill for now.
relay-server/src/envelope.rs
Outdated
| @@ -1174,6 +1174,14 @@ impl Envelope { | |||
| } | |||
| } | |||
|
|
|||
| /// Returns `true` whether the event is fully normalized, `false`. | |||
| pub fn event_fully_normalized(&self) -> bool { | |||
There was a problem hiding this comment.
I would not put normalization concerns and business logic on the envelope.
If we have a newtype, we also have a place to put it.
There was a problem hiding this comment.
The fully_normalized flag is already on the envelope, so IMO it's fine to have this getter.
Yes, this is specified in the PR description. |
This PR removes the
event_fully_normalizedfrom the state of the processor. It's part of a series of PRs that aim to get rid of the shared state, for improving the code quality of the processor.#skip-changelog