Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(normalization): Skip validation on renormalization #3214

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

iker-barriocanal
Copy link
Contributor

Previous normalization PRs run validation from librelay calls. However, some sentry tests run full normalization in librelay on partial (i.e. invalid) payloads, and running validation makes plenty of tests fail (see getsentry/sentry#66134).

Similarly, many sentry tests expect span normalization to never happen. After spending significant time "fixing"* some tests, it'd require me even more time to "fix" the remaining ones as it requires a deeper understanding of other features/codebases. Thus, I'm reintroducing normalize_spans to disable span normalization (mostly exclusive time computation) from librelay calls and unblock the librelay release.

*Note: "fixing" a test means using data relay outputs from normalization. This doesn't align with other assumptions made in the tests, nor modifications to the event payload in the rest of the event processing pipeline.

Future work

  • Consider span normalization a regular step in event normalization behind the is_renormalize flag, removing the normalize_spans flag.

@iker-barriocanal iker-barriocanal self-assigned this Mar 5, 2024
@iker-barriocanal iker-barriocanal marked this pull request as ready for review March 5, 2024 09:57
@iker-barriocanal iker-barriocanal requested a review from a team as a code owner March 5, 2024 09:57
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock. It would be nice to get rid of the config flag eventually.

Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand the change and it seems to me we're working around bad tests in Sentry, but we are definitely blocked on this.

Have you considered exposing the flag from librelay via Keyword arg, so we have at least a chance of fixing the Sentry tests on a step by step basis?

@iker-barriocanal
Copy link
Contributor Author

@Dav1dde great idea! Exposed the flag in 380895a. It can be enabled by initing the StoreNormalizer with normalize_spans=True, is_renormalize=False.

@iker-barriocanal iker-barriocanal merged commit abe779e into master Mar 5, 2024
20 checks passed
@iker-barriocanal iker-barriocanal deleted the iker/fix/librelay-renormalization branch March 5, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants