-
Notifications
You must be signed in to change notification settings - Fork 94
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
feat(replay): separate feedback from attachments in the same envelope #3403
Changes from all commits
706dab1
dbfc1dc
d4a612a
e5f7461
03961b4
6fc34ed
c00d82c
117d674
f0a9f7a
5ab4197
af89ee4
e53603f
4dd7b1b
1c06509
0fa44f4
41f5597
870f623
b2ef5e7
e1f1597
ba5ac3b
9a81a1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -189,13 +189,19 @@ impl StoreService { | |
let retention = envelope.retention(); | ||
let event_id = envelope.event_id(); | ||
|
||
let feedback_ingest_same_envelope_attachments = self | ||
.global_config | ||
.current() | ||
.options | ||
.feedback_ingest_same_envelope_attachments; | ||
|
||
let event_item = envelope.as_mut().take_item_by(|item| { | ||
matches!( | ||
item.ty(), | ||
ItemType::Event | ||
| ItemType::Transaction | ||
| ItemType::Security | ||
| ItemType::UserReportV2 | ||
(item.ty(), feedback_ingest_same_envelope_attachments), | ||
(ItemType::Event, _) | ||
| (ItemType::Transaction, _) | ||
| (ItemType::Security, _) | ||
| (ItemType::UserReportV2, false) | ||
) | ||
}); | ||
let client = envelope.meta().client(); | ||
|
@@ -244,6 +250,17 @@ impl StoreService { | |
item, | ||
)?; | ||
} | ||
ItemType::UserReportV2 if feedback_ingest_same_envelope_attachments => { | ||
let remote_addr = envelope.meta().client_addr().map(|addr| addr.to_string()); | ||
self.produce_user_report_v2( | ||
event_id.ok_or(StoreError::NoEventId)?, | ||
scoping.project_id, | ||
scoping.organization_id, | ||
start_time, | ||
item, | ||
remote_addr, | ||
)?; | ||
} | ||
ItemType::Profile => self.produce_profile( | ||
scoping.organization_id, | ||
scoping.project_id, | ||
|
@@ -673,6 +690,36 @@ impl StoreService { | |
self.produce(KafkaTopic::Attachments, message) | ||
} | ||
|
||
fn produce_user_report_v2( | ||
&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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #3344 |
||
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 | ||
}; | ||
Comment on lines
+706
to
+710
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of scope for this PR, but should we hard-code this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this has to do with ops deployment of the feedback topic -- right now, its only in s4s and not other regions. This check and rollout rate will eventually be removed too |
||
|
||
let message = KafkaMessage::Event(EventKafkaMessage { | ||
project_id, | ||
event_id, | ||
payload: item.payload(), | ||
start_time: UnixTimestamp::from_instant(start_time).as_secs(), | ||
remote_addr, | ||
attachments: vec![], | ||
}); | ||
self.produce(topic, message) | ||
} | ||
|
||
fn send_metric_message( | ||
&self, | ||
namespace: MetricNamespace, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, remove this file. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Without context, this comment is not clear to me what the flag should do. What do you think about something like the following?