ref(normalization): Only run normalization once, with item headers #3730
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iker-barriocanal
force-pushed
the
iker/feat/norm-item-header
branch
2 times, most recently
from
c0b2d5d
to
823ebe4
Compare
iker-barriocanal
force-pushed
the
iker/feat/norm-item-header
branch
from
868fdff
to
3508fdc
Compare
There was a problem hiding this comment.
There have been no protocol/normalization changes, so it's safe to reuse the same feature flags.
| @@ -161,5 +161,5 @@ | |||
| "username": "username" | |||
| }, | |||
| "version": "7", | |||
| "location": null | |||
| "location": "not-empty-so-that-it-isnt-dropped" | |||
There was a problem hiding this comment.
Extra fields with null values are not serialized. Setting this value to anything else ensures we test future-compatibility.
tests/integration/test_store.py
Outdated
There was a problem hiding this comment.
These use cases are covered in new tests in tests/integration/test_normalization.py.
iker-barriocanal
changed the title
jjbayer
reviewed
Jun 18, 2024
jjbayer
approved these changes
Jun 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the approach to decide whether to run normalization on an event.
Relay looks at the headers of items in the envelope to decide whether to run normalization, instead of relying on assumptions on whether the previous relay run normalization. Since external systems can rewrite the header, only envelopes coming from relays in the internal infrastructure are trusted.
Relay resets the
fully_normalizedflag after updating the event when processing envelopes, in order to ensure it runs normalization. It follows the same approach for events created in the processing pipeline.Events from attachments
Relay disables the normalization flag for events created from attachments, and it will later normalize them.
This step is required as only processing relays deal with attachments. It could be simpler if we moved the attachment processing out of processing relays, but this is out of scope of this PR.
Background
Currently, PoP relays run some normalization, and processing relays run full normalization. Since both layers are internal and their deployments are controlled by Sentry, running full normalization in PoP relays saves the CPU in processing relays (estimated around 10%) while ensuring it always runs once.
Previous attempt
The previous attempt forced full normalization in PoPs and assumed processing relays would always receive the fully normalized events. However, it didn't consider events created in processing relays (from minidumps and various crash reporting mechanisms), resulting in ingesting events without running normalization.
Rollout and rollback
Feature flags will first enable full normalization in PoPs and then disable normalization in processing. The integration test
test_relay_chain_normalizes_eventsvalidates this flow. Disabling feature flags will roll back this behavior.