-
-
Notifications
You must be signed in to change notification settings - Fork 61
ref(eap-outcomes): use sentry-options to change which timestamp to use #7838
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
Changes from all commits
f848782
0d67443
6c99601
dce3afc
8ec91ae
89cac49
a6dde70
f0c7713
77c3ef8
1200524
1e420a6
d5d361c
a991ba4
086982e
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 |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| [env] | ||
| SENTRY_OPTIONS_DIR = { value = "../sentry-options", relative = true } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| pub(crate) const SNUBA_SCHEMA: &str = | ||
| include_str!("../../sentry-options/schemas/snuba/schema.json"); | ||
|
Member
Author
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. the schemas are now baked in during compile time. this means during runtime, our only vulnerabilities now are |
||
|
|
||
| mod accepted_outcomes_consumer; | ||
| mod config; | ||
| mod consumer; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ use sentry_arroyo::types::{InnerMessage, Message, Partition}; | |
| use sentry_arroyo::utils::timing::Deadline; | ||
| use sentry_protos::snuba::v1::TraceItem; | ||
|
|
||
| use sentry_options::options; | ||
|
|
||
| use crate::types::{AggregatedOutcomesBatch, BucketKey}; | ||
|
|
||
| #[derive(Debug, Default)] | ||
|
|
@@ -57,8 +59,7 @@ pub struct OutcomesAggregator<TNext> { | |
| message_carried_over: Option<Message<AggregatedOutcomesBatch>>, | ||
| /// Commit request carried over from a poll where we had a message to retry. | ||
| commit_request_carried_over: Option<CommitRequest>, | ||
| /// Temporary option to change the timestamp source from | ||
| /// `received` to `timestamp` on the item event. | ||
| /// Cached value of the `consumer.use_item_timestamp` option, refreshed on each poll. | ||
| use_item_timestamp: bool, | ||
kenzoengineer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
|
|
@@ -68,7 +69,6 @@ impl<TNext> OutcomesAggregator<TNext> { | |
| max_batch_size: usize, | ||
| max_batch_time_ms: Duration, | ||
| bucket_interval: u64, | ||
| use_item_timestamp: bool, | ||
| ) -> Self { | ||
| Self { | ||
| next_step, | ||
|
|
@@ -80,7 +80,7 @@ impl<TNext> OutcomesAggregator<TNext> { | |
| latest_offsets: HashMap::new(), | ||
| message_carried_over: None, | ||
| commit_request_carried_over: None, | ||
| use_item_timestamp, | ||
| use_item_timestamp: false, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -129,6 +129,12 @@ impl<TNext: ProcessingStrategy<AggregatedOutcomesBatch>> ProcessingStrategy<Kafk | |
| for OutcomesAggregator<TNext> | ||
| { | ||
| fn poll(&mut self) -> Result<Option<CommitRequest>, StrategyError> { | ||
|
Member
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. @kenzoengineer so
Member
Author
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. for posterity: this has been benchmarked and the normal while theres a dramatic decrease, the numbers are still in the nanoseconds. We can keep it the naive way for now and in a future PR we can make this optimization if necessary |
||
| self.use_item_timestamp = options("snuba") | ||
| .ok() | ||
| .and_then(|o| o.get("consumer.use_item_timestamp").ok()) | ||
| .and_then(|v| v.as_bool()) | ||
| .unwrap_or(false); | ||
kenzoengineer marked this conversation as resolved.
Show resolved
Hide resolved
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. Option read on every poll causes unnecessary I/OMedium Severity
Member
Author
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. this is incorrect. we only check memory. |
||
|
|
||
| let commit_request = self.next_step.poll()?; | ||
| self.commit_request_carried_over = | ||
| merge_commit_request(self.commit_request_carried_over.take(), commit_request); | ||
|
|
@@ -208,6 +214,7 @@ impl<TNext: ProcessingStrategy<AggregatedOutcomesBatch>> ProcessingStrategy<Kafk | |
| .map(|t| t.seconds as u64) | ||
| .unwrap_or(0) | ||
| }; | ||
|
|
||
| let org_id = trace_item.organization_id; | ||
| let project_id = trace_item.project_id; | ||
|
|
||
|
|
@@ -263,7 +270,10 @@ mod tests { | |
| use prost::Message as ProstMessage; | ||
| use prost_types::Timestamp; | ||
| use sentry_arroyo::types::{Partition, Topic}; | ||
| use sentry_options::init_with_schemas; | ||
| use sentry_options::testing::override_options; | ||
| use sentry_protos::snuba::v1::{CategoryCount, Outcomes}; | ||
| use serde_json::json; | ||
|
|
||
| struct Noop { | ||
| last_message: Option<Message<AggregatedOutcomesBatch>>, | ||
|
|
@@ -323,7 +333,6 @@ mod tests { | |
| 500, | ||
| Duration::from_millis(5_000), | ||
| 60, | ||
| false, | ||
| ); | ||
|
|
||
| let topic = Topic::new("accepted-outcomes"); | ||
|
|
@@ -371,7 +380,6 @@ mod tests { | |
| 500, | ||
| Duration::from_millis(2_000), | ||
| 60, | ||
| false, | ||
| ); | ||
|
|
||
| let topic = Topic::new("snuba-items"); | ||
|
|
@@ -455,12 +463,12 @@ mod tests { | |
|
|
||
| #[test] | ||
| fn poll_flushes_when_max_batch_size_reached() { | ||
| init_with_schemas(&[("snuba", crate::SNUBA_SCHEMA)]).unwrap(); | ||
| let mut aggregator = OutcomesAggregator::new( | ||
| Noop { last_message: None }, | ||
| 1, | ||
| Duration::from_millis(30_000), | ||
| 60, | ||
| false, | ||
| ); | ||
|
|
||
| let partition = Partition::new(Topic::new("accepted-outcomes"), 0); | ||
|
|
@@ -481,6 +489,7 @@ mod tests { | |
|
|
||
| #[test] | ||
| fn submit_returns_backpressure_when_message_carried_over() { | ||
| init_with_schemas(&[("snuba", crate::SNUBA_SCHEMA)]).unwrap(); | ||
| struct RejectOnce { | ||
| rejected: bool, | ||
| } | ||
|
|
@@ -513,7 +522,6 @@ mod tests { | |
| 1, // flush after 1 bucket | ||
| Duration::from_millis(30_000), | ||
| 60, | ||
| false, | ||
| ); | ||
|
|
||
| let partition = Partition::new(Topic::new("test"), 0); | ||
|
|
@@ -549,6 +557,7 @@ mod tests { | |
|
|
||
| #[test] | ||
| fn join_honors_timeout_when_message_stays_carried_over() { | ||
| init_with_schemas(&[("snuba", crate::SNUBA_SCHEMA)]).unwrap(); | ||
| struct AlwaysReject; | ||
| impl ProcessingStrategy<AggregatedOutcomesBatch> for AlwaysReject { | ||
| fn poll(&mut self) -> Result<Option<CommitRequest>, StrategyError> { | ||
|
|
@@ -570,7 +579,7 @@ mod tests { | |
| } | ||
|
|
||
| let mut aggregator = | ||
| OutcomesAggregator::new(AlwaysReject, 1, Duration::from_millis(30_000), 60, false); | ||
| OutcomesAggregator::new(AlwaysReject, 1, Duration::from_millis(30_000), 60); | ||
| let partition = Partition::new(Topic::new("test"), 0); | ||
| let payload = make_payload(6_000, 1, 2, 3, &[(4, 1)]); | ||
|
|
||
|
|
@@ -590,12 +599,14 @@ mod tests { | |
|
|
||
| #[test] | ||
| fn submit_uses_item_timestamp_when_enabled() { | ||
| init_with_schemas(&[("snuba", crate::SNUBA_SCHEMA)]).unwrap(); | ||
| let _guard = | ||
| override_options(&[("snuba", "consumer.use_item_timestamp", json!(true))]).unwrap(); | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let mut aggregator = OutcomesAggregator::new( | ||
| Noop { last_message: None }, | ||
| 500, | ||
| Duration::from_millis(2_000), | ||
| 60, | ||
| true, | ||
| ); | ||
|
|
||
| let topic = Topic::new("snuba-items"); | ||
|
|
@@ -625,6 +636,9 @@ mod tests { | |
| trace_item.encode(&mut buf).unwrap(); | ||
| let payload = KafkaPayload::new(None, None, Some(buf)); | ||
|
|
||
| // we need to poll first in order to get the new value (true) | ||
| aggregator.poll().unwrap(); | ||
|
|
||
| aggregator | ||
| .submit(Message::new_broker_message( | ||
| payload, | ||
|
|
@@ -646,4 +660,82 @@ mod tests { | |
| Some(1) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn poll_updates_use_item_timestamp_dynamically() { | ||
kenzoengineer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| init_with_schemas(&[("snuba", crate::SNUBA_SCHEMA)]).unwrap(); | ||
| let mut aggregator = OutcomesAggregator::new( | ||
| Noop { last_message: None }, | ||
| 500, | ||
| Duration::from_millis(30_000), | ||
| 60, | ||
| ); | ||
|
|
||
| let partition = Partition::new(Topic::new("snuba-items"), 0); | ||
|
|
||
| let mut buf = Vec::new(); | ||
| TraceItem { | ||
| organization_id: 1, | ||
| project_id: 2, | ||
| received: Some(Timestamp { | ||
| seconds: 1_700_000_000, | ||
| nanos: 0, | ||
| }), | ||
| timestamp: Some(Timestamp { | ||
| seconds: 1_700_000_060, | ||
| nanos: 0, | ||
| }), | ||
| outcomes: Some(Outcomes { | ||
| key_id: 3, | ||
| category_count: vec![CategoryCount { | ||
| data_category: 4, | ||
| quantity: 1, | ||
| }], | ||
| }), | ||
| ..Default::default() | ||
| } | ||
| .encode(&mut buf) | ||
| .unwrap(); | ||
| let payload = KafkaPayload::new(None, None, Some(buf)); | ||
|
|
||
| let bucket_quantity = |aggregator: &OutcomesAggregator<Noop>, offset: u64| { | ||
| let key = BucketKey { | ||
| time_offset: offset, | ||
| org_id: 1, | ||
| project_id: 2, | ||
| key_id: 3, | ||
| category: 4, | ||
| }; | ||
| aggregator.batch.buckets.get(&key).map(|s| s.quantity) | ||
| }; | ||
|
|
||
| let mut offset = 0; | ||
| let mut do_submit = |aggregator: &mut OutcomesAggregator<Noop>| { | ||
| aggregator.poll().unwrap(); | ||
| aggregator | ||
| .submit(Message::new_broker_message( | ||
| payload.clone(), | ||
| partition, | ||
| offset, | ||
| Utc::now(), | ||
| )) | ||
| .unwrap(); | ||
| offset += 1; | ||
| }; | ||
|
|
||
| // Enable item timestamp | ||
| let guard = | ||
| override_options(&[("snuba", "consumer.use_item_timestamp", json!(true))]).unwrap(); | ||
| do_submit(&mut aggregator); | ||
| assert_eq!(bucket_quantity(&aggregator, 28_333_334), Some(1)); | ||
| assert_eq!(bucket_quantity(&aggregator, 28_333_333), None); | ||
|
|
||
| // Disable item timestamp | ||
| drop(guard); | ||
| let _guard = | ||
| override_options(&[("snuba", "consumer.use_item_timestamp", json!(false))]).unwrap(); | ||
| do_submit(&mut aggregator); | ||
| assert_eq!(bucket_quantity(&aggregator, 28_333_333), Some(1)); | ||
| assert_eq!(bucket_quantity(&aggregator, 28_333_334), Some(1)); // still present from first submit | ||
| } | ||
| } | ||


Uh oh!
There was an error while loading. Please reload this page.