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(store): remove references to feedback topic option after rollout #3604

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions relay-dynamic-config/src/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,17 +187,6 @@ pub struct Options {
)]
pub metric_stats_rollout_rate: f32,

/// Rollout rate for producing to the ingest-feedback-events topic.
///
/// Rate needs to be between `0.0` and `1.0`.
/// If set to `1.0` all organizations will ingest to the feedback topic.
#[serde(
rename = "feedback.ingest-topic.rollout-rate",
deserialize_with = "default_on_error",
skip_serializing_if = "is_default"
)]
pub feedback_ingest_topic_rollout_rate: f32,

/// Flag for handling feedback and attachments in the same envelope. This is for the SDK team to send less requests
/// for the user feedback screenshots feature. Prior to this change, feedback sent w/attachments would be produced
/// to the attachments topic, rather than its own topic. The items are now split up accordingly.
Expand Down
29 changes: 3 additions & 26 deletions relay-server/src/services/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ use crate::services::global_config::GlobalConfigHandle;
use crate::services::outcome::{DiscardReason, Outcome, TrackOutcome};
use crate::services::processor::Processed;
use crate::statsd::RelayCounters;
use crate::utils::{
is_rolled_out, ArrayEncoding, BucketEncoder, ExtractionMode, FormDataIter, TypedEnvelope,
};
use crate::utils::{ArrayEncoding, BucketEncoder, ExtractionMode, FormDataIter, TypedEnvelope};

/// Fallback name used for attachment items without a `filename` header.
const UNNAMED_ATTACHMENT: &str = "Unnamed Attachment";
Expand Down Expand Up @@ -212,16 +210,7 @@ impl StoreService {
} else if event_item.as_ref().map(|x| x.ty()) == Some(&ItemType::Transaction) {
KafkaTopic::Transactions
} else if event_item.as_ref().map(|x| x.ty()) == Some(&ItemType::UserReportV2) {
let feedback_ingest_topic_rollout_rate = self
.global_config
.current()
.options
.feedback_ingest_topic_rollout_rate;
if is_rolled_out(scoping.organization_id, feedback_ingest_topic_rollout_rate) {
KafkaTopic::Feedback
} else {
KafkaTopic::Events
}
KafkaTopic::Feedback
} else {
KafkaTopic::Events
};
Expand Down Expand Up @@ -256,7 +245,6 @@ impl StoreService {
self.produce_user_report_v2(
event_id.ok_or(StoreError::NoEventId)?,
scoping.project_id,
scoping.organization_id,
start_time,
item,
remote_addr,
Expand Down Expand Up @@ -667,21 +655,10 @@ impl StoreService {
&self,
event_id: EventId,
project_id: ProjectId,
organization_id: u64,
start_time: Instant,
item: &Item,
remote_addr: Option<String>,
) -> Result<(), StoreError> {
// check rollout rate option (effectively a FF) to determine whether to produce to new infra
let global_config = self.global_config.current();
let feedback_ingest_topic_rollout_rate =
global_config.options.feedback_ingest_topic_rollout_rate;
let topic = if is_rolled_out(organization_id, feedback_ingest_topic_rollout_rate) {
KafkaTopic::Feedback
} else {
KafkaTopic::Events
};

let message = KafkaMessage::Event(EventKafkaMessage {
project_id,
event_id,
Expand All @@ -690,7 +667,7 @@ impl StoreService {
remote_addr,
attachments: vec![],
});
self.produce(topic, message)
self.produce(KafkaTopic::Feedback, message)
}

fn send_metric_message(
Expand Down
40 changes: 10 additions & 30 deletions tests/integration/test_feedback.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,11 @@ def assert_expected_feedback(parsed_feedback, sent_feedback):


@pytest.mark.parametrize("use_feedback_ingest_v2", (False, True))
@pytest.mark.parametrize("use_feedback_topic", (False, True))
def test_feedback_event_with_processing(
mini_sentry,
relay_with_processing,
events_consumer,
feedback_consumer,
use_feedback_topic,
use_feedback_ingest_v2,
):
mini_sentry.add_basic_project_config(
Expand All @@ -105,14 +103,8 @@ def test_feedback_event_with_processing(
"feedback.ingest-inline-attachments", use_feedback_ingest_v2
)

if use_feedback_topic:
mini_sentry.set_global_config_option("feedback.ingest-topic.rollout-rate", 1.0)
consumer = feedback_consumer(timeout=20)
other_consumer = events_consumer(timeout=20)
else:
mini_sentry.set_global_config_option("feedback.ingest-topic.rollout-rate", 0.0)
consumer = events_consumer(timeout=20)
other_consumer = feedback_consumer(timeout=20)
consumer = feedback_consumer(timeout=20)
events_consumer = events_consumer(timeout=20)

feedback = generate_feedback_sdk_event()
relay = relay_with_processing()
Expand All @@ -125,14 +117,13 @@ def test_feedback_event_with_processing(
# Assert required fields were returned
assert_expected_feedback(parsed_feedback, feedback)

# test message wasn't dup'd to the wrong topic
other_consumer.assert_empty()
# test message wasn't dup'd to the event topic
events_consumer.assert_empty()


@pytest.mark.parametrize("use_feedback_ingest_v2", (False, True))
@pytest.mark.parametrize("use_feedback_topic", (False, True))
def test_feedback_events_without_processing(
mini_sentry, relay_chain, use_feedback_topic, use_feedback_ingest_v2
mini_sentry, relay_chain, use_feedback_ingest_v2
):
project_id = 42
mini_sentry.add_basic_project_config(
Expand All @@ -142,9 +133,6 @@ def test_feedback_events_without_processing(
mini_sentry.set_global_config_option(
"feedback.ingest-inline-attachments", use_feedback_ingest_v2
)
mini_sentry.set_global_config_option(
"feedback.ingest-topic.rollout-rate", 1.0 if use_feedback_topic else 0.0
)

replay_item = generate_feedback_sdk_event()
relay = relay_chain(min_relay_version="latest")
Expand All @@ -157,30 +145,22 @@ def test_feedback_events_without_processing(
assert userfeedback.type == "feedback"


@pytest.mark.parametrize("use_feedback_topic", (False, True))
def test_feedback_with_attachment_in_same_envelope(
mini_sentry,
relay_with_processing,
feedback_consumer,
events_consumer,
attachments_consumer,
use_feedback_topic,
):
mini_sentry.add_basic_project_config(
42, extra={"config": {"features": ["organizations:user-feedback-ingest"]}}
)
# Test will only pass with this option set
mini_sentry.set_global_config_option("feedback.ingest-inline-attachments", True)

if use_feedback_topic:
mini_sentry.set_global_config_option("feedback.ingest-topic.rollout-rate", 1.0)
other_consumer = events_consumer(timeout=20)
feedback_consumer = feedback_consumer(timeout=20)
else:
mini_sentry.set_global_config_option("feedback.ingest-topic.rollout-rate", 0.0)
other_consumer = feedback_consumer(timeout=20)
feedback_consumer = events_consumer(timeout=20)
feedback_consumer = feedback_consumer(timeout=20)
attachments_consumer = attachments_consumer(timeout=20)
events_consumer = events_consumer(timeout=20)

feedback = generate_feedback_sdk_event()
event_id = feedback["event_id"]
Expand Down Expand Up @@ -233,8 +213,8 @@ def test_feedback_with_attachment_in_same_envelope(
# Assert required fields were returned
assert_expected_feedback(parsed_feedback, feedback)

# test message wasn't dup'd to the wrong topic
other_consumer.assert_empty()
# test message wasn't dup'd to the events topic
events_consumer.assert_empty()

# test message wasn't sent to attachments topic
# test no extra messages (e.g. the feedback) were sent to the attachments topic
attachments_consumer.assert_empty()
Loading