feat(feedback): produce to ingest-feedback-events topic #3344
Conversation
|
You're on a pretty old base for this branch, I recommend updating. The Kafka Topic config recently changed and your change would not work in SaaS. |
…etsentry/relay into aliu/produce-to-feedback-topic
relay-server/src/services/store.rs
Outdated
| .options | ||
| .feedback_ingest_topic_rollout_rate; | ||
| let use_feedback_topic = | ||
| is_rolled_out(scoping.organization_id, feedback_ingest_topic_rollout_rate); |
There was a problem hiding this comment.
This will cause the rollout load volume to be unbalanced. Large orgs will send significantly more messages than small orgs. This might be okay. Be aware going from 10% -> 20% does not necessarily indicate a 2x increase in volume.
There was a problem hiding this comment.
10 -> 20 will equal a 2x increase in volume from any given org, right? But not overall.
| consumer = feedback_consumer(timeout=20) | ||
| else: | ||
| mini_sentry.set_global_config_option("feedback.ingest-topic.rollout-rate", 0.0) | ||
| consumer = events_consumer(timeout=20) |
There was a problem hiding this comment.
Might be worth also assigning other_consumer and calling .assert_empty() on it at the end of the test.
relay-server/src/services/store.rs
Outdated
| @@ -199,6 +210,10 @@ impl StoreService { | |||
| KafkaTopic::Attachments | |||
| } else if event_item.as_ref().map(|x| x.ty()) == Some(&ItemType::Transaction) { | |||
| KafkaTopic::Transactions | |||
| } else if use_feedback_topic | |||
There was a problem hiding this comment.
nit: is_rolled_out is called for every envelope, even if it does not contain a UserReportV2. So I would turn this condition around and evaluate is_rolled_out only when the event type matches.
| .current() | ||
| .options | ||
| .feedback_ingest_topic_rollout_rate; | ||
| if is_rolled_out(scoping.organization_id, feedback_ingest_topic_rollout_rate) { |
There was a problem hiding this comment.
This is checking the rollout rate with the org, not the event -- let's please roll out slowly.
There was a problem hiding this comment.
I think it's just used as a boolean on/off in practice, so should be fine 👍
There was a problem hiding this comment.
Not sure what the feedback event volume is, but suddenly going from 0 to N may cause a backlog until consumers scale up.
There was a problem hiding this comment.
Is there a function I can use to do it on the event level?
There was a problem hiding this comment.
We will probably keep it at the org level since feedback volumes aren't that high, and we'd like to test it on all feedback in s4s. Will comment here if that changes. Holding off merging until https://getsentry.atlassian.net/browse/OPS-5543 is done.
There was a problem hiding this comment.
We will probably keep it at the org level since feedback volumes aren't that high
That's totally fine, my comment was just a note to roll out slowly to avoid any surprises 😄
| .current() | ||
| .options | ||
| .feedback_ingest_topic_rollout_rate; | ||
| if is_rolled_out(scoping.organization_id, feedback_ingest_topic_rollout_rate) { |
There was a problem hiding this comment.
I think it's just used as a boolean on/off in practice, so should be fine 👍
| let feedback_ingest_topic_rollout_rate = self | ||
| .global_config | ||
| .current() | ||
| .options | ||
| .feedback_ingest_topic_rollout_rate; |
There was a problem hiding this comment.
tiny nit:
| let feedback_ingest_topic_rollout_rate = self | |
| .global_config | |
| .current() | |
| .options | |
| .feedback_ingest_topic_rollout_rate; | |
| let global_config = self.global_config.current(); | |
| let topic_rollout_rate = global_config.options.feedback_ingest_topic_rollout_rate; |
I like splitting this, so it takes up less lines and becomes a bit more readable. But need to check if rustfmt aggrees with me here ...
Option PR:
depends on getsentry/sentry#67839 (MERGED)
Feature flag PRs (outdated/unused):
https://github.com/getsentry/getsentry/pull/13426 (REVERTED)getsentry/sentry#67747 (REVERTED)https://github.com/getsentry/sentry-options-automator/pull/973 (CLOSED)Relates to getsentry/sentry#66100