-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(replays): Add option which disables publishing to Snuba from Relay #98353
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #98353 +/- ##
========================================
Coverage 80.99% 80.99%
========================================
Files 8566 8570 +4
Lines 376863 377127 +264
Branches 24148 24148
========================================
+ Hits 305225 305462 +237
- Misses 71261 71288 +27
Partials 377 377 |
|
@sentry review |
| if recording.context["should_publish_replay_event"] and recording.replay_event: | ||
| publish_replay_event(json.dumps(recording.replay_event)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The feature to publish replay events to Snuba is non-functional due to a key mismatch. The code looks for relay_snuba_publish_disabled, but the option is named differently.
-
Description: There is a mismatch between the configuration option name
replay.relay-snuba-publishing-disabledand the key the consumer code attempts to retrieve from the recording payload,relay_snuba_publish_disabled. Becauserecording.get("relay_snuba_publish_disabled")insrc/sentry/replays/consumers/recording.pywill always returnNone, theshould_publish_replay_eventcontext value is alwaysNone. This causes the conditional check insrc/sentry/replays/usecases/ingest/__init__.pyto always evaluate to false. As a consequence, thepublish_replay_eventfunction is never called, rendering the feature to publish replay events to Snuba completely non-functional regardless of its configuration. -
Suggested fix: Correct the key used in
src/sentry/replays/consumers/recording.pyfromrelay_snuba_publish_disabledto match the key sent by Relay. Additionally, since the flag name implies disabling publishing, the conditional logic insrc/sentry/replays/usecases/ingest/__init__.pymay need to be inverted to correctly handle the flag's intent.
severity: 0.8, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
…nts (#5088) Checks the global config to see if it should or should not publish the replay-event to Snuba. For now the option should always be false until the recording consumer can be updated to handle publishing to Snuba. Related: getsentry/sentry-kafka-schemas#434 Related: getsentry/sentry#98353 --------- Co-authored-by: David Herberth <david.herberth@sentry.io>
Related: getsentry/sentry-kafka-schemas#434
Related: getsentry/relay#5088