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

ref(normalization): Emit sentry errors on normalization decisions and make feature flags and configs NOOP #3571

Closed
wants to merge 2 commits into from

Conversation

iker-barriocanal
Copy link
Member

This PR does the following:

  • Run normalization unconditionally on all relays. Full normalization is only run in processing relays.
  • Remove integration tests on feature flags and static config. I'll reintroduce these once the behavior is reintroduced.
  • Computes normalization decisions only for observability purposes (see point below).
  • Emits a sentry error on processing error/default events in both PoPs and Relays. A global config controls the sampling rate at which the errors are emitted. These errors contain tags and context that will be used to determine the correctness of decisions, and these are added to the Sentry SDK scope throughout the envelope processing.

Context: there's some unexpected behavior, that may be caused by an inaccurate normalization run decision, and results in error events with type=default. Source is unknown, and adding observability should provide enough data to confirm and scope down the issues.

Note: this PR should land master before the next Relay release on May 15. Currently, normalization is controlled by feature flags that get propagated from internal relays and force them to run full event normalization, including steps that break backward compatibility. As a result, updates to the protocol or normalization may not be effective. Merging this PR solves this issue.

#skip-changelog

@iker-barriocanal iker-barriocanal requested a review from a team as a code owner May 8, 2024 21:54
}
});

let full_normalization = self.inner.config.processing_enabled();
Copy link
Member

Choose a reason for hiding this comment

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

This used to be:

self.inner.config.processing_enabled()
            && options.processing_disable_normalization
            && (state.envelope().meta().is_from_internal_relay())

Just to make sure, this is the intended behaviour?

@Dav1dde
Copy link
Member

Dav1dde commented May 29, 2024

I think we should consider attaching the normalization status to the item instead (what Joris proposed). This would improve consistency and is easier to debug.

@iker-barriocanal
Copy link
Member Author

I'll update the implementation to use item headers instead. I'll follow up with a new PR.

@iker-barriocanal iker-barriocanal deleted the iker/ref/norm-decision-metrics branch May 31, 2024 07:29
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.

None yet

2 participants