diff --git a/CHANGELOG.md b/CHANGELOG.md index 9be818377bc..04a807bb3ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Parametrize transaction in dynamic sampling context. ([#3141](https://github.com/getsentry/relay/pull/3141)) - Adds ReplayVideo envelope-item type. ([#3105](https://github.com/getsentry/relay/pull/3105)) - Parse & scrub span description for supabase. ([#3153](https://github.com/getsentry/relay/pull/3153), [#3156](https://github.com/getsentry/relay/pull/3156)) +- Introduce generic filters in global configs. ([#3161](https://github.com/getsentry/relay/pull/3161)) **Bug Fixes**: diff --git a/Cargo.lock b/Cargo.lock index 55c82e490b5..e1b464d32fb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3741,6 +3741,7 @@ version = "24.2.0" dependencies = [ "anyhow", "assert-json-diff", + "indexmap 2.0.0", "insta", "relay-auth", "relay-base-schema", @@ -3846,6 +3847,7 @@ dependencies = [ name = "relay-filter" version = "24.2.0" dependencies = [ + "indexmap 2.0.0", "insta", "ipnetwork 0.20.0", "once_cell", diff --git a/Cargo.toml b/Cargo.toml index d372f054325..089be82147b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ futures = { version = "0.3", default-features = false, features = ["std"] } insta = { version = "1.31.0", features = ["json", "redactions", "ron"] } hash32 = "0.3.1" hashbrown = "0.14.3" +indexmap = "2.0.0" itertools = "0.10.5" once_cell = "1.13.1" parking_lot = "0.12.1" diff --git a/relay-dynamic-config/Cargo.toml b/relay-dynamic-config/Cargo.toml index 2113b7398f5..054a79b6ae9 100644 --- a/relay-dynamic-config/Cargo.toml +++ b/relay-dynamic-config/Cargo.toml @@ -23,7 +23,7 @@ relay-event-normalization = { path = "../relay-event-normalization" } relay-filter = { path = "../relay-filter" } relay-log = { path = "../relay-log" } relay-pii = { path = "../relay-pii" } -relay-protocol = { path = "../relay-protocol" } +relay-protocol = { path = "../relay-protocol" } relay-quotas = { path = "../relay-quotas" } relay-sampling = { path = "../relay-sampling" } serde = { workspace = true } @@ -33,3 +33,4 @@ smallvec = { workspace = true } [dev-dependencies] insta = { workspace = true } similar-asserts = { workspace = true } +indexmap = { workspace = true } diff --git a/relay-dynamic-config/src/global.rs b/relay-dynamic-config/src/global.rs index 13a2078bb5e..68e3af75052 100644 --- a/relay-dynamic-config/src/global.rs +++ b/relay-dynamic-config/src/global.rs @@ -5,10 +5,13 @@ use std::path::Path; use relay_base_schema::metrics::MetricNamespace; use relay_event_normalization::MeasurementsConfig; +use relay_filter::GenericFiltersConfig; use relay_quotas::Quota; use serde::{Deserialize, Serialize}; use serde_json::Value; +use crate::ErrorBoundary; + /// A dynamic configuration for all Relays passed down from Sentry. /// /// Values shared across all projects may also be included here, to keep @@ -23,6 +26,12 @@ pub struct GlobalConfig { /// Quotas that apply to all projects. #[serde(skip_serializing_if = "Vec::is_empty")] pub quotas: Vec, + /// Configuration for global inbound filters. + /// + /// These filters are merged with generic filters in project configs before + /// applying. + #[serde(skip_serializing_if = "is_err_or_empty")] + pub filters: ErrorBoundary, /// Sentry options passed down to Relay. #[serde( deserialize_with = "default_on_error", @@ -46,6 +55,21 @@ impl GlobalConfig { Ok(None) } } + + /// Returns the generic inbound filters. + pub fn filters(&self) -> Option<&GenericFiltersConfig> { + match &self.filters { + ErrorBoundary::Err(_) => None, + ErrorBoundary::Ok(f) => Some(f), + } + } +} + +fn is_err_or_empty(filters_config: &ErrorBoundary) -> bool { + match filters_config { + ErrorBoundary::Err(_) => true, + ErrorBoundary::Ok(config) => config.version == 0 && config.filters.is_empty(), + } } /// All options passed down from Sentry to Relay. @@ -246,6 +270,20 @@ mod tests { "namespace": null } ], + "filters": { + "version": 1, + "filters": [ + { + "id": "myError", + "isEnabled": true, + "condition": { + "op": "eq", + "name": "event.exceptions", + "value": "myError" + } + } + ] + }, "options": { "profiling.profile_metrics.unsampled_profiles.enabled": true } diff --git a/relay-dynamic-config/src/project.rs b/relay-dynamic-config/src/project.rs index 3912c2f896b..e921733fc53 100644 --- a/relay-dynamic-config/src/project.rs +++ b/relay-dynamic-config/src/project.rs @@ -3,7 +3,7 @@ use relay_event_normalization::{ BreakdownsConfig, MeasurementsConfig, PerformanceScoreConfig, SpanDescriptionRule, TransactionNameRule, }; -use relay_filter::FiltersConfig; +use relay_filter::ProjectFiltersConfig; use relay_pii::{DataScrubbingConfig, PiiConfig}; use relay_quotas::Quota; use relay_sampling::SamplingConfig; @@ -32,8 +32,8 @@ pub struct ProjectConfig { #[serde(skip_serializing_if = "Option::is_none")] pub grouping_config: Option, /// Configuration for filter rules. - #[serde(skip_serializing_if = "FiltersConfig::is_empty")] - pub filter_settings: FiltersConfig, + #[serde(skip_serializing_if = "ProjectFiltersConfig::is_empty")] + pub filter_settings: ProjectFiltersConfig, /// Configuration for data scrubbers. #[serde(skip_serializing_if = "DataScrubbingConfig::is_disabled")] pub datascrubbing_settings: DataScrubbingConfig, @@ -109,7 +109,7 @@ impl Default for ProjectConfig { trusted_relays: vec![], pii_config: None, grouping_config: None, - filter_settings: FiltersConfig::default(), + filter_settings: ProjectFiltersConfig::default(), datascrubbing_settings: DataScrubbingConfig::default(), event_retention: None, quotas: Vec::new(), @@ -154,8 +154,8 @@ pub struct LimitedProjectConfig { pub allowed_domains: Vec, pub trusted_relays: Vec, pub pii_config: Option, - #[serde(skip_serializing_if = "FiltersConfig::is_empty")] - pub filter_settings: FiltersConfig, + #[serde(skip_serializing_if = "ProjectFiltersConfig::is_empty")] + pub filter_settings: ProjectFiltersConfig, #[serde(skip_serializing_if = "DataScrubbingConfig::is_disabled")] pub datascrubbing_settings: DataScrubbingConfig, #[serde(skip_serializing_if = "Option::is_none")] diff --git a/relay-filter/Cargo.toml b/relay-filter/Cargo.toml index f81eaaa9dd2..d40037d5832 100644 --- a/relay-filter/Cargo.toml +++ b/relay-filter/Cargo.toml @@ -12,6 +12,7 @@ publish = false [dependencies] ipnetwork = "0.20.0" once_cell = { workspace = true } +indexmap = { workspace = true } regex = { workspace = true } relay-common = { path = "../relay-common" } relay-event-schema = { path = "../relay-event-schema" } diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index 336075ca86c..1ea40b44973 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -3,11 +3,15 @@ use std::borrow::Cow; use std::collections::BTreeSet; use std::convert::Infallible; +use std::fmt; +use std::ops::Deref; use std::str::FromStr; +use indexmap::IndexMap; use relay_common::glob3::GlobPatterns; use relay_protocol::RuleCondition; -use serde::{Deserialize, Serialize}; +use serde::ser::SerializeSeq; +use serde::{de, Deserialize, Serialize, Serializer}; /// Common configuration for event filters. #[derive(Clone, Debug, Default, Serialize, Deserialize)] @@ -230,9 +234,9 @@ impl LegacyBrowsersFilterConfig { } /// Configuration for a generic filter. -#[derive(Clone, Debug, Default, Serialize, Deserialize)] +#[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq)] #[serde(rename_all = "camelCase")] -pub(crate) struct GenericFilterConfig { +pub struct GenericFilterConfig { /// Unique identifier of the generic filter. pub id: String, /// Specifies whether this filter is enabled. @@ -249,12 +253,164 @@ impl GenericFilterConfig { } /// Configuration for generic filters. +/// +/// # Deserialization +/// +/// `filters` is expected to be a sequence of [`GenericFilterConfig`]. +/// Only the first occurrence of a filter is kept, and duplicates are removed. +/// Two filters are considered duplicates if they have the same ID, +/// independently of the condition. +/// +/// The list of filters is deserialized into an [`GenericFiltersMap`], where the +/// key is the filter's id and the value is the filter itself. The map is +/// converted back to a list when serializing it, without the filters that were +/// discarded as duplicates. See examples below. +/// +/// # Iterator +/// +/// Iterates in order through the generic filters in project configs and global +/// configs yielding the filters according to the principles below: +/// +/// - Filters from project configs are evaluated before filters from global +/// configs. +/// - No duplicates: once a filter is evaluated (yielded or skipped), no filter +/// with the same id is evaluated again. +/// - Filters in project configs override filters from global configs, but the +/// opposite is never the case. +/// - A filter in the project config can be a flag, where only `is_enabled` is +/// defined and `condition` is not. In that case: +/// - If `is_enabled` is true, the filter with a matching ID from global +/// configs is yielded without evaluating its `is_enabled`. Unless the filter +/// in the global config also has an empty condition, in which case the filter +/// is not yielded. +/// - If `is_enabled` is false, no filters with the same IDs are returned, +/// including matching filters from global configs. +/// +/// # Examples +/// +/// Deserialization: +/// +/// ``` +/// # use relay_filter::GenericFiltersConfig; +/// # use insta::assert_debug_snapshot; +/// +/// let json = r#"{ +/// "version": 1, +/// "filters": [ +/// { +/// "id": "filter1", +/// "isEnabled": false, +/// "condition": null +/// }, +/// { +/// "id": "filter1", +/// "isEnabled": true, +/// "condition": { +/// "op": "eq", +/// "name": "event.exceptions", +/// "value": "drop-error" +/// } +/// } +/// ] +/// }"#; +/// let deserialized = serde_json::from_str::(json).unwrap(); +/// assert_debug_snapshot!(deserialized, @r#" +/// GenericFiltersConfig { +/// version: 1, +/// filters: GenericFiltersMap( +/// { +/// "filter1": GenericFilterConfig { +/// id: "filter1", +/// is_enabled: false, +/// condition: None, +/// }, +/// }, +/// ), +/// } +/// "#); +/// ``` +/// +/// Deserialization of no filters defaults to an empty map: +/// +/// ``` +/// # use relay_filter::GenericFiltersConfig; +/// # use insta::assert_debug_snapshot; +/// +/// let json = r#"{ +/// "version": 1 +/// }"#; +/// let deserialized = serde_json::from_str::(json).unwrap(); +/// assert_debug_snapshot!(deserialized, @r#" +/// GenericFiltersConfig { +/// version: 1, +/// filters: GenericFiltersMap( +/// {}, +/// ), +/// } +/// "#); +/// ``` +/// +/// Serialization: +/// +/// ``` +/// # use relay_filter::{GenericFiltersConfig, GenericFilterConfig}; +/// # use relay_protocol::condition::RuleCondition; +/// # use insta::assert_display_snapshot; +/// +/// let filter = GenericFiltersConfig { +/// version: 1, +/// filters: vec![ +/// GenericFilterConfig { +/// id: "filter1".to_owned(), +/// is_enabled: true, +/// condition: Some(RuleCondition::eq("event.exceptions", "drop-error")), +/// }, +/// ].into(), +/// }; +/// let serialized = serde_json::to_string_pretty(&filter).unwrap(); +/// assert_display_snapshot!(serialized, @r#"{ +/// "version": 1, +/// "filters": [ +/// { +/// "id": "filter1", +/// "isEnabled": true, +/// "condition": { +/// "op": "eq", +/// "name": "event.exceptions", +/// "value": "drop-error" +/// } +/// } +/// ] +/// }"#); +/// ``` +/// +/// Serialization of filters is skipped if there aren't any: +/// +/// ``` +/// # use relay_filter::{GenericFiltersConfig, GenericFilterConfig, GenericFiltersMap}; +/// # use relay_protocol::condition::RuleCondition; +/// # use insta::assert_display_snapshot; +/// +/// let filter = GenericFiltersConfig { +/// version: 1, +/// filters: GenericFiltersMap::new(), +/// }; +/// let serialized = serde_json::to_string_pretty(&filter).unwrap(); +/// assert_display_snapshot!(serialized, @r#"{ +/// "version": 1 +/// }"#); +/// ``` #[derive(Clone, Debug, Default, Serialize, Deserialize)] -pub(crate) struct GenericFiltersConfig { +#[serde(rename_all = "camelCase")] +pub struct GenericFiltersConfig { /// Version of the filters configuration. pub version: u16, - /// List of generic filters. - pub filters: Vec, + /// Map of generic filters, sorted by the order in the payload from upstream. + /// + /// The map contains unique filters, meaning there are no two filters with + /// the same id. See struct docs for more details. + #[serde(default, skip_serializing_if = "GenericFiltersMap::is_empty")] + pub filters: GenericFiltersMap, } impl GenericFiltersConfig { @@ -264,10 +420,91 @@ impl GenericFiltersConfig { } } -/// Configuration for all event filters. +/// Map of generic filters, mapping from the id to the filter itself. +#[derive(Clone, Debug, Default)] +pub struct GenericFiltersMap(IndexMap); + +impl GenericFiltersMap { + /// Returns an empty map. + pub fn new() -> Self { + GenericFiltersMap(IndexMap::new()) + } + + /// Returns whether the map is empty. + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } +} + +impl From> for GenericFiltersMap { + fn from(filters: Vec) -> Self { + let mut map = IndexMap::with_capacity(filters.len()); + for filter in filters { + if !map.contains_key(&filter.id) { + map.insert(filter.id.clone(), filter); + } + } + GenericFiltersMap(map) + } +} + +impl Deref for GenericFiltersMap { + type Target = IndexMap; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl<'de> Deserialize<'de> for GenericFiltersMap { + fn deserialize(deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + struct GenericFiltersVisitor; + + impl<'de> serde::de::Visitor<'de> for GenericFiltersVisitor { + type Value = GenericFiltersMap; + + fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + formatter.write_str("a vector of filters: Vec") + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: de::SeqAccess<'de>, + { + let mut filters = IndexMap::with_capacity(seq.size_hint().unwrap_or(0)); + while let Some(filter) = seq.next_element::()? { + if !filters.contains_key(&filter.id) { + filters.insert(filter.id.clone(), filter); + } + } + Ok(GenericFiltersMap(filters)) + } + } + + deserializer.deserialize_seq(GenericFiltersVisitor) + } +} + +impl Serialize for GenericFiltersMap { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + let mut seq = serializer.serialize_seq(Some(self.0.len()))?; + for filter in self.0.values() { + seq.serialize_element(filter)?; + } + seq.end() + } +} + +/// Configuration for all event filters from project configs. #[derive(Clone, Debug, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct FiltersConfig { +pub struct ProjectFiltersConfig { /// Configuration for the Browser Extensions filter. #[serde(default, skip_serializing_if = "FilterConfig::is_empty")] pub browser_extensions: FilterConfig, @@ -307,12 +544,12 @@ pub struct FiltersConfig { )] pub ignore_transactions: IgnoreTransactionsFilterConfig, - /// Configuration for generic filters. + /// Configuration for generic filters from the project configs. #[serde(default, skip_serializing_if = "GenericFiltersConfig::is_empty")] - pub(crate) generic: GenericFiltersConfig, + pub generic: GenericFiltersConfig, } -impl FiltersConfig { +impl ProjectFiltersConfig { /// Returns true if there are no filter configurations declared. pub fn is_empty(&self) -> bool { self.browser_extensions.is_empty() @@ -334,9 +571,9 @@ mod tests { #[test] fn test_empty_config() -> Result<(), serde_json::Error> { - let filters_config = serde_json::from_str::("{}")?; + let filters_config = serde_json::from_str::("{}")?; insta::assert_debug_snapshot!(filters_config, @r###" - FiltersConfig { + ProjectFiltersConfig { browser_extensions: FilterConfig { is_enabled: false, }, @@ -368,7 +605,9 @@ mod tests { }, generic: GenericFiltersConfig { version: 0, - filters: [], + filters: GenericFiltersMap( + {}, + ), }, } "###); @@ -377,13 +616,13 @@ mod tests { #[test] fn test_serialize_empty() { - let filters_config = FiltersConfig::default(); + let filters_config = ProjectFiltersConfig::default(); insta::assert_json_snapshot!(filters_config, @"{}"); } #[test] fn test_serialize_full() { - let filters_config = FiltersConfig { + let filters_config = ProjectFiltersConfig { browser_extensions: FilterConfig { is_enabled: true }, client_ips: ClientIpsFilterConfig { blacklisted_ips: vec!["127.0.0.1".to_string()], @@ -416,7 +655,8 @@ mod tests { id: "hydrationError".to_string(), is_enabled: true, condition: Some(RuleCondition::eq("event.exceptions", "HydrationError")), - }], + }] + .into(), }, }; @@ -518,28 +758,30 @@ mod tests { insta::assert_debug_snapshot!(config, @r###" GenericFiltersConfig { version: 1, - filters: [ - GenericFilterConfig { - id: "hydrationError", - is_enabled: true, - condition: Some( - Eq( - EqCondition { - name: "event.exceptions", - value: String("HydrationError"), - options: EqCondOptions { - ignore_case: false, + filters: GenericFiltersMap( + { + "hydrationError": GenericFilterConfig { + id: "hydrationError", + is_enabled: true, + condition: Some( + Eq( + EqCondition { + name: "event.exceptions", + value: String("HydrationError"), + options: EqCondOptions { + ignore_case: false, + }, }, - }, + ), ), - ), + }, + "chunkLoadError": GenericFilterConfig { + id: "chunkLoadError", + is_enabled: false, + condition: None, + }, }, - GenericFilterConfig { - id: "chunkLoadError", - is_enabled: false, - condition: None, - }, - ], + ), } "###); } diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 24673de7157..15200f1c814 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -4,17 +4,24 @@ //! first one that matches, will result in the event being discarded with a [`FilterStatKey`] //! identifying the matching filter. -use crate::{FilterStatKey, GenericFiltersConfig}; +use std::iter::FusedIterator; + +use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig, GenericFiltersMap}; use relay_event_schema::protocol::Event; use relay_protocol::RuleCondition; /// Maximum supported version of the generic filters schema. /// /// If the version in the project config is higher, no generic filters are applied. -pub const VERSION: u16 = 1; +const MAX_SUPPORTED_VERSION: u16 = 1; -fn is_enabled(config: &GenericFiltersConfig) -> bool { - config.version > 0 && config.version <= VERSION +/// Returns whether the given generic config versions are supported. +pub fn are_generic_filters_supported( + global_filters_version: Option, + project_filters_version: u16, +) -> bool { + global_filters_version.map_or(true, |v| v <= MAX_SUPPORTED_VERSION) + && project_filters_version <= MAX_SUPPORTED_VERSION } /// Checks events by patterns in their error messages. @@ -27,32 +34,152 @@ fn matches(event: &Event, condition: Option<&RuleCondition>) -> bool { /// Filters events by patterns in their error messages. pub(crate) fn should_filter( event: &Event, - config: &GenericFiltersConfig, + project_filters: &GenericFiltersConfig, + global_filters: Option<&GenericFiltersConfig>, ) -> Result<(), FilterStatKey> { - // We check if the configuration is enabled, since we support only configuration with a version - // <= than the maximum one in this Relay instance. - if !is_enabled(config) { - return Ok(()); - } + let filters = merge_generic_filters( + project_filters, + global_filters, + #[cfg(test)] + MAX_SUPPORTED_VERSION, + ); - for filter_config in config.filters.iter() { - if !filter_config.is_empty() && matches(event, filter_config.condition.as_ref()) { - return Err(FilterStatKey::GenericFilter(filter_config.id.clone())); + for filter_config in filters { + if filter_config.is_enabled && matches(event, filter_config.condition) { + return Err(FilterStatKey::GenericFilter(filter_config.id.to_owned())); } } Ok(()) } +/// Returns an iterator that yields merged generic configs. +/// +/// Since filters of project and global configs are complementary and don't +/// provide much value in isolation, both versions must match +/// [`MAX_SUPPORTED_VERSION`] to be compatible. If filters aren't compatible, +/// an empty iterator is returned. +/// +/// If filters are compatible, an iterator over all filters is returned. This +/// iterator yields filters according to the principles below: +/// - Filters from project configs are evaluated before filters from global +/// configs. +/// - No duplicates: once a filter is evaluated (yielded or skipped), no filter +/// with the same id is evaluated again. +/// - If a filter with the same id exists in projects and global configs, both +/// are merged and the filter is yielded. Values from the filter in the project +/// config are prioritized. +fn merge_generic_filters<'a>( + project: &'a GenericFiltersConfig, + global: Option<&'a GenericFiltersConfig>, + #[cfg(test)] max_supported_version: u16, +) -> impl Iterator> { + #[cfg(not(test))] + let max_supported_version = MAX_SUPPORTED_VERSION; + + let is_supported = project.version <= max_supported_version + && global.map_or(true, |gf| gf.version <= max_supported_version); + + is_supported + .then(|| { + DynamicGenericFiltersConfigIter::new(&project.filters, global.map(|gc| &gc.filters)) + }) + .into_iter() + .flatten() +} + +/// Iterator over the generic filters of the project and global configs. +struct DynamicGenericFiltersConfigIter<'a> { + /// Generic project filters. + project: &'a GenericFiltersMap, + /// Index of the next filter in project configs to evaluate. + project_index: usize, + /// Generic global filters. + global: Option<&'a GenericFiltersMap>, + /// Index of the next filter in global configs to evaluate. + global_index: usize, +} + +impl<'a> DynamicGenericFiltersConfigIter<'a> { + pub fn new(project: &'a GenericFiltersMap, global: Option<&'a GenericFiltersMap>) -> Self { + DynamicGenericFiltersConfigIter { + project, + project_index: 0, + global, + global_index: 0, + } + } +} + +impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { + type Item = GenericFilterConfigRef<'a>; + + fn next(&mut self) -> Option { + if let Some((id, filter)) = self.project.get_index(self.project_index) { + self.project_index += 1; + let merged = merge_filters(filter, self.global.and_then(|gf| gf.get(id))); + return Some(merged); + } + + loop { + let (id, filter) = self.global?.get_index(self.global_index)?; + self.global_index += 1; + if !self.project.contains_key(id) { + return Some(filter.into()); + } + } + } +} + +impl<'a> FusedIterator for DynamicGenericFiltersConfigIter<'a> {} + +/// Merges the two filters with the same id, prioritizing values from the primary. +/// +/// It's assumed both filters share the same id. The returned filter will have +/// the primary filter's ID. +fn merge_filters<'a>( + primary: &'a GenericFilterConfig, + secondary: Option<&'a GenericFilterConfig>, +) -> GenericFilterConfigRef<'a> { + GenericFilterConfigRef { + id: primary.id.as_str(), + is_enabled: primary.is_enabled, + condition: primary + .condition + .as_ref() + .or(secondary.and_then(|filter| filter.condition.as_ref())), + } +} + +#[derive(Debug, Default, PartialEq)] +struct GenericFilterConfigRef<'a> { + id: &'a str, + is_enabled: bool, + condition: Option<&'a RuleCondition>, +} + +impl<'a> From<&'a GenericFilterConfig> for GenericFilterConfigRef<'a> { + fn from(value: &'a GenericFilterConfig) -> Self { + GenericFilterConfigRef { + id: value.id.as_str(), + is_enabled: value.is_enabled, + condition: value.condition.as_ref(), + } + } +} + #[cfg(test)] mod tests { - use crate::generic::{should_filter, VERSION}; + + use super::*; + + use crate::generic::{should_filter, MAX_SUPPORTED_VERSION}; use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig}; use relay_event_schema::protocol::{Event, LenientString}; use relay_protocol::Annotated; use relay_protocol::RuleCondition; - fn mock_filters() -> Vec { + fn mock_filters() -> GenericFiltersMap { vec![ GenericFilterConfig { id: "firstReleases".to_string(), @@ -65,6 +192,7 @@ mod tests { condition: Some(RuleCondition::eq("event.transaction", "/hello")), }, ] + .into() } #[test] @@ -80,7 +208,7 @@ mod tests { ..Default::default() }; assert_eq!( - should_filter(&event, &config), + should_filter(&event, &config, None), Err(FilterStatKey::GenericFilter("firstReleases".to_string())) ); @@ -90,7 +218,7 @@ mod tests { ..Default::default() }; assert_eq!( - should_filter(&event, &config), + should_filter(&event, &config, None), Err(FilterStatKey::GenericFilter( "helloTransactions".to_string() )) @@ -111,7 +239,7 @@ mod tests { ..Default::default() }; assert_eq!( - should_filter(&event, &config), + should_filter(&event, &config, None), Err(FilterStatKey::GenericFilter("firstReleases".to_string())) ); } @@ -128,14 +256,14 @@ mod tests { transaction: Annotated::new("/world".to_string()), ..Default::default() }; - assert_eq!(should_filter(&event, &config), Ok(())); + assert_eq!(should_filter(&event, &config, None), Ok(())); } #[test] fn test_should_filter_with_higher_config_version() { let config = GenericFiltersConfig { // We simulate receiving a higher configuration version, which we don't support. - version: VERSION + 1, + version: MAX_SUPPORTED_VERSION + 1, filters: mock_filters(), }; @@ -144,6 +272,497 @@ mod tests { transaction: Annotated::new("/hello".to_string()), ..Default::default() }; - assert_eq!(should_filter(&event, &config), Ok(())); + assert_eq!(should_filter(&event, &config, None), Ok(())); + } + + #[test] + fn test_should_filter_from_global_filters() { + let project = GenericFiltersConfig { + version: 1, + filters: vec![GenericFilterConfig { + id: "firstReleases".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.release", "1.0")), + }] + .into(), + }; + + let global = GenericFiltersConfig { + version: 1, + filters: vec![GenericFilterConfig { + id: "helloTransactions".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.transaction", "/hello")), + }] + .into(), + }; + + let event = Event { + transaction: Annotated::new("/hello".to_string()), + ..Default::default() + }; + + assert_eq!( + should_filter(&event, &project, Some(&global)), + Err(FilterStatKey::GenericFilter( + "helloTransactions".to_string() + )) + ); + } + + fn empty_filter() -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: GenericFiltersMap::new(), + } + } + + /// Returns a complete and enabled [`GenericFiltersConfig`]. + fn enabled_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: vec![GenericFilterConfig { + id: id.to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }] + .into(), + } + } + + /// Returns an enabled flag of a [`GenericFiltersConfig`]. + fn enabled_flag_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: vec![GenericFilterConfig { + id: id.to_owned(), + is_enabled: true, + condition: None, + }] + .into(), + } + } + + /// Returns a complete but disabled [`GenericFiltersConfig`]. + fn disabled_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: vec![GenericFilterConfig { + id: id.to_owned(), + is_enabled: false, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }] + .into(), + } + } + + /// Returns a disabled flag of a [`GenericFiltersConfig`]. + fn disabled_flag_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: vec![GenericFilterConfig { + id: id.to_owned(), + is_enabled: false, + condition: None, + }] + .into(), + } + } + + #[test] + fn test_no_combined_when_unsupported_project_version() { + let mut project = enabled_filter("unsupported-project"); + project.version = 2; + let global = enabled_filter("supported-global"); + assert!(merge_generic_filters(&project, Some(&global), 1).eq(None.into_iter())); + } + + #[test] + fn test_no_combined_when_unsupported_project_version_no_global() { + let mut project = enabled_filter("unsupported-project"); + project.version = 2; + assert!(merge_generic_filters(&project, None, 1).eq(None.into_iter())); + } + + #[test] + fn test_no_combined_when_unsupported_global_version() { + let project = enabled_filter("supported-project"); + let mut global = enabled_filter("unsupported-global"); + global.version = 2; + assert!(merge_generic_filters(&project, Some(&global), 1).eq(None.into_iter())); + } + + #[test] + fn test_no_combined_when_unsupported_project_and_global_version() { + let mut project = enabled_filter("unsupported-project"); + project.version = 2; + let mut global = enabled_filter("unsupported-global"); + global.version = 2; + assert!(merge_generic_filters(&project, Some(&global), 1).eq(None.into_iter())); + } + + #[test] + fn test_both_combined_when_supported_project_and_global_version() { + let project = enabled_filter("supported-project"); + let global = enabled_filter("supported-global"); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([ + project.filters.first().unwrap().1.into(), + global.filters.first().unwrap().1.into() + ] + .into_iter())); + } + + #[test] + fn test_project_combined_when_duplicated_filter_project_and_global() { + let project = enabled_filter("filter"); + let global = enabled_filter("filter"); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) + ); + } + + #[test] + fn test_no_combined_when_empty_project_and_global() { + let project = empty_filter(); + let global = empty_filter(); + assert!(merge_generic_filters(&project, Some(&global), 1).eq(None.into_iter())); + } + + #[test] + fn test_global_combined_when_empty_project_disabled_global_filter() { + let project = empty_filter(); + let global = disabled_filter("disabled-global"); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); + } + + #[test] + fn test_global_combined_when_empty_project_enabled_global_filters() { + let project = empty_filter(); + let global = enabled_filter("enabled-global"); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); + } + + #[test] + fn test_global_combined_when_empty_project_enabled_flag_global() { + let project = empty_filter(); + let global = enabled_flag_filter("skip"); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()])); + } + + #[test] + fn test_project_combined_when_disabled_project_empty_global() { + let project = disabled_filter("disabled-project"); + let global = empty_filter(); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) + ); + } + + #[test] + fn test_project_combined_when_disabled_project_missing_global() { + let project = disabled_filter("disabled-project"); + assert!(merge_generic_filters(&project, None, 1).eq([project + .filters + .first() + .unwrap() + .1 + .into(),] + .into_iter())); + } + + #[test] + fn test_both_combined_when_different_disabled_project_and_global() { + let project = disabled_filter("disabled-project"); + let global = disabled_filter("disabled-global"); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([ + project.filters.first().unwrap().1.into(), + global.filters.first().unwrap().1.into() + ])); + } + + #[test] + fn test_project_combined_duplicated_disabled_project_and_global() { + let project = disabled_filter("filter"); + let global = disabled_filter("filter"); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()]) + ); + } + + #[test] + fn test_merged_combined_when_disabled_project_enabled_global() { + let project = disabled_filter("filter"); + let global = enabled_filter("filter"); + let expected = &GenericFilterConfig { + id: "filter".to_owned(), + is_enabled: false, + condition: global.filters.first().unwrap().1.condition.clone(), + }; + assert!(merge_generic_filters(&project, Some(&global), 1).eq([expected.into()].into_iter())); + } + + #[test] + fn test_no_combined_when_enabled_flag_project_empty_global() { + let project = enabled_flag_filter("filter"); + let global = empty_filter(); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) + ); + } + + #[test] + fn test_project_combined_when_enabled_flag_project_missing_global() { + let project = enabled_flag_filter("filter"); + assert!(merge_generic_filters(&project, None, 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); + } + + #[test] + fn test_project_combined_when_disabled_flag_project_empty_global() { + let project = disabled_flag_filter("filter"); + let global = empty_filter(); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()]) + ); + } + + #[test] + fn test_project_combined_when_disabled_flag_project_missing_global() { + let project = disabled_flag_filter("filter"); + assert!(merge_generic_filters(&project, None, 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()])); + } + + #[test] + fn test_project_combined_when_enabled_project_empty_global() { + let project = enabled_filter("enabled-project"); + let global = empty_filter(); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) + ); + } + + #[test] + fn test_project_combined_when_enabled_project_missing_global() { + let project = enabled_filter("enabled-project"); + assert!(merge_generic_filters(&project, None, 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); + } + + #[test] + fn test_merged_combined_when_enabled_flag_project_disabled_global() { + let project = enabled_flag_filter("filter"); + let global = disabled_filter("filter"); + let expected = &GenericFilterConfig { + id: "filter".to_owned(), + is_enabled: true, + condition: global.filters.first().unwrap().1.condition.clone(), + }; + assert!(merge_generic_filters(&project, Some(&global), 1).eq([expected.into()].into_iter())); + } + + #[test] + fn test_global_combined_when_disabled_flag_project_disabled_global() { + let project = disabled_flag_filter("filter"); + let global = disabled_filter("filter"); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()])); + } + + #[test] + fn test_project_combined_when_enabled_project_disabled_global() { + let project = enabled_filter("filter"); + let global = disabled_filter("filter"); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) + ); + } + + #[test] + fn test_global_combined_when_enabled_flag_project_enabled_global() { + let project = enabled_flag_filter("filter"); + let global = enabled_filter("filter"); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); + } + + #[test] + fn test_merged_combined_when_disabled_flag_project_enabled_global() { + let project = disabled_flag_filter("filter"); + let global = enabled_filter("filter"); + let expected = &GenericFilterConfig { + id: "filter".to_owned(), + is_enabled: false, + condition: global.filters.first().unwrap().1.condition.clone(), + }; + assert!(merge_generic_filters(&project, Some(&global), 1).eq([expected.into()].into_iter())); + } + + #[test] + fn test_project_combined_when_enabled_project_enabled_flag_global() { + let project = enabled_filter("filter"); + let global = enabled_flag_filter("filter"); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) + ); + } + + #[test] + fn test_project_combined_when_enabled_flags_project_and_global() { + let project = enabled_flag_filter("filter"); + let global = enabled_flag_filter("filter"); + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) + ); + } + + #[test] + fn test_multiple_combined_filters() { + let project = GenericFiltersConfig { + version: 1, + filters: vec![ + GenericFilterConfig { + id: "0".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + GenericFilterConfig { + id: "1".to_owned(), + is_enabled: true, + condition: None, + }, + GenericFilterConfig { + id: "2".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + ] + .into(), + }; + let global = GenericFiltersConfig { + version: 1, + filters: vec![ + GenericFilterConfig { + id: "1".to_owned(), + is_enabled: false, + condition: Some(RuleCondition::eq("event.exceptions", "myOtherError")), + }, + GenericFilterConfig { + id: "3".to_owned(), + is_enabled: false, + condition: Some(RuleCondition::eq("event.exceptions", "myLastError")), + }, + ] + .into(), + }; + + let expected0 = &project.filters[0]; + let expected1 = &GenericFilterConfig { + id: "1".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myOtherError")), + }; + let expected2 = &project.filters[2]; + let expected3 = &global.filters[1]; + + assert!(merge_generic_filters(&project, Some(&global), 1).eq([ + expected0.into(), + expected1.into(), + expected2.into(), + expected3.into() + ] + .into_iter())); } } diff --git a/relay-filter/src/lib.rs b/relay-filter/src/lib.rs index 2185006f0cf..22cbcf5acc8 100644 --- a/relay-filter/src/lib.rs +++ b/relay-filter/src/lib.rs @@ -37,6 +37,8 @@ pub use crate::common::*; pub use crate::config::*; pub use crate::csp::matches_any_origin; +pub use crate::generic::are_generic_filters_supported; + /// Checks whether an event should be filtered for a particular configuration. /// /// If the event should be filtered, the `Err` returned contains a filter reason. @@ -44,12 +46,13 @@ pub use crate::csp::matches_any_origin; pub fn should_filter( event: &Event, client_ip: Option, - config: &FiltersConfig, + config: &ProjectFiltersConfig, + global_config: Option<&GenericFiltersConfig>, ) -> Result<(), FilterStatKey> { // In order to maintain backwards compatibility, we still want to run the old matching logic, // but we will try to match generic filters first, since the goal is to eventually fade out the // the normal filters except for the ones that have complex conditions. - generic::should_filter(event, &config.generic)?; + generic::should_filter(event, &config.generic, global_config)?; // The order of applying filters should not matter as they are additive. Still, be careful // when making changes to this order. diff --git a/relay-server/src/services/processor.rs b/relay-server/src/services/processor.rs index 476a3a2aa32..7cd2ba4b7f0 100644 --- a/relay-server/src/services/processor.rs +++ b/relay-server/src/services/processor.rs @@ -1240,7 +1240,7 @@ impl EnvelopeProcessorService { event::finalize(state, &self.inner.config)?; self.light_normalize_event(state)?; - event::filter(state)?; + event::filter(state, &self.inner.global_config.current())?; dynamic_sampling::tag_error_with_sampling_decision(state, &self.inner.config); if_processing!(self.inner.config, { @@ -1274,23 +1274,41 @@ impl EnvelopeProcessorService { event::finalize(state, &self.inner.config)?; self.light_normalize_event(state)?; dynamic_sampling::normalize(state); - event::filter(state)?; - dynamic_sampling::run(state, &self.inner.config); + event::filter(state, &self.inner.global_config.current())?; + // Don't extract metrics if relay can't apply generic inbound filters. + // An inbound filter applied in another up-to-date relay in chain may + // need to drop the event, and there should not be metrics from dropped + // events. + // In processing relays, always extract metrics to avoid losing them. + let supported_generic_filters = self.inner.global_config.current().filters.is_ok() + && relay_filter::are_generic_filters_supported( + self.inner + .global_config + .current() + .filters() + .map(|f| f.version), + state.project_state.config.filter_settings.generic.version, + ); - // Remember sampling decision, before it is reset in `dynamic_sampling::sample_envelope_items`. - let sampling_should_drop = state.sampling_result.should_drop(); + let mut sampling_should_drop = false; - // We avoid extracting metrics if we are not sampling the event while in non-processing - // relays, in order to synchronize rate limits on indexed and processed transactions. - if self.inner.config.processing_enabled() || sampling_should_drop { - self.extract_metrics(state)?; - } + if self.inner.config.processing_enabled() || supported_generic_filters { + dynamic_sampling::run(state, &self.inner.config); + // Remember sampling decision, before it is reset in `dynamic_sampling::sample_envelope_items`. + sampling_should_drop = state.sampling_result.should_drop(); - dynamic_sampling::sample_envelope_items( - state, - &self.inner.config, - &self.inner.global_config.current(), - ); + // We avoid extracting metrics if we are not sampling the event while in non-processing + // relays, in order to synchronize rate limits on indexed and processed transactions. + if self.inner.config.processing_enabled() || sampling_should_drop { + self.extract_metrics(state)?; + } + + dynamic_sampling::sample_envelope_items( + state, + &self.inner.config, + &self.inner.global_config.current(), + ); + } metric!( timer(RelayTimers::TransactionProcessingAfterDynamicSampling), @@ -2768,6 +2786,7 @@ mod tests { let global_config = GlobalConfig { measurements: None, quotas: vec![mock_quota("foo"), mock_quota("bar")], + filters: Default::default(), options: Options::default(), }; diff --git a/relay-server/src/services/processor/event.rs b/relay-server/src/services/processor/event.rs index a6523c807ec..699063f8074 100644 --- a/relay-server/src/services/processor/event.rs +++ b/relay-server/src/services/processor/event.rs @@ -7,7 +7,7 @@ use once_cell::sync::OnceCell; use relay_auth::RelayVersion; use relay_base_schema::events::EventType; use relay_config::Config; -use relay_dynamic_config::Feature; +use relay_dynamic_config::{Feature, GlobalConfig}; use relay_event_normalization::{nel, ClockDriftProcessor}; use relay_event_schema::processor::{self, ProcessingState}; use relay_event_schema::protocol::{ @@ -273,6 +273,7 @@ pub fn finalize( pub fn filter( state: &mut ProcessEnvelopeState, + global_config: &GlobalConfig, ) -> Result<(), ProcessingError> { let event = match state.event.value_mut() { Some(event) => event, @@ -285,12 +286,13 @@ pub fn filter( let filter_settings = &state.project_state.config.filter_settings; metric!(timer(RelayTimers::EventProcessingFiltering), { - relay_filter::should_filter(event, client_ip, filter_settings).map_err(|err| { - state - .managed_envelope - .reject(Outcome::Filtered(err.clone())); - ProcessingError::EventFiltered(err) - }) + relay_filter::should_filter(event, client_ip, filter_settings, global_config.filters()) + .map_err(|err| { + state + .managed_envelope + .reject(Outcome::Filtered(err.clone())); + ProcessingError::EventFiltered(err) + }) }) } diff --git a/tests/integration/fixtures/mini_sentry.py b/tests/integration/fixtures/mini_sentry.py index 4aa97ac05d0..2a2cb014da8 100644 --- a/tests/integration/fixtures/mini_sentry.py +++ b/tests/integration/fixtures/mini_sentry.py @@ -461,5 +461,6 @@ def reraise_test_failures(): ], "maxCustomMeasurements": 10, }, + "filters": {"version": 1, "filters": []}, "options": {}, } diff --git a/tests/integration/test_dynamic_sampling.py b/tests/integration/test_dynamic_sampling.py index 83aaa626333..05af8204a24 100644 --- a/tests/integration/test_dynamic_sampling.py +++ b/tests/integration/test_dynamic_sampling.py @@ -831,3 +831,25 @@ def get_profile_payload(transaction): }, "version": "1", } + + +def test_invalid_global_generic_filters_skip_dynamic_sampling(mini_sentry, relay): + mini_sentry.global_config["filters"] = { + "version": 65535, # u16::MAX + "filters": [], + } + + relay = relay(mini_sentry, _outcomes_enabled_config()) + + project_id = 42 + config = mini_sentry.add_basic_project_config(project_id) + config["config"]["transactionMetrics"] = {"version": 1} + public_key = config["publicKeys"][0]["publicKey"] + + # Reject all transactions with dynamic sampling + _add_sampling_config(config, sample_rate=0, rule_type="transaction") + + envelope, _, _ = _create_transaction_envelope(public_key, client_sample_rate=0) + + relay.send_envelope(project_id, envelope) + assert mini_sentry.captured_events.get(timeout=1) diff --git a/tests/integration/test_filters.py b/tests/integration/test_filters.py index 4bcafe20302..2f96ef72c0d 100644 --- a/tests/integration/test_filters.py +++ b/tests/integration/test_filters.py @@ -282,3 +282,37 @@ def test_ignore_transactions_filters_are_applied( else: event, _ = transactions_consumer.get_event() assert event["transaction"] == transaction_name + + +def test_global_filters_drop_events( + mini_sentry, relay_with_processing, events_consumer, outcomes_consumer +): + events_consumer = events_consumer() + outcomes_consumer = outcomes_consumer() + + mini_sentry.global_config["filters"] = { + "version": 1, + "filters": [ + { + "id": "premature-releases", + "isEnabled": True, + "condition": { + "op": "eq", + "name": "event.release", + "value": "0.0.0", + }, + } + ], + } + + project_id = 42 + mini_sentry.add_full_project_config(project_id) + relay = relay_with_processing() + + event = {"release": "0.0.0"} + relay.send_event(project_id, event) + + events_consumer.assert_empty() + outcomes = outcomes_consumer.get_outcomes() + assert len(outcomes) == 1 + assert outcomes[0]["reason"] == "premature-releases" diff --git a/tests/integration/test_metrics.py b/tests/integration/test_metrics.py index 0e78c3e37eb..0234d1e333c 100644 --- a/tests/integration/test_metrics.py +++ b/tests/integration/test_metrics.py @@ -1851,8 +1851,6 @@ def test_metric_bucket_encoding_dynamic_global_config_option( namespace: "array" } - print(mini_sentry.global_config) - metrics_consumer = metrics_consumer() relay = relay_with_processing(options=TEST_CONFIG) @@ -1880,3 +1878,178 @@ def test_metric_bucket_encoding_dynamic_global_config_option( else: assert metrics[dname]["value"] == [1337.0] assert metrics[sname]["value"] == [42.0] + + +@pytest.mark.parametrize("is_processing_relay", (False, True)) +@pytest.mark.parametrize( + "global_generic_filters", + [ + # Config is broken + { + "version": "Halp! I'm broken!", + "filters": [], + }, + # Config is valid, but filters aren't supported + { + "version": 65535, + "filters": [], + }, + ], +) +def test_relay_forwards_events_without_extracting_metrics_on_broken_global_filters( + mini_sentry, + relay, + relay_with_processing, + transactions_consumer, + metrics_consumer, + is_processing_relay, + global_generic_filters, +): + metrics_consumer = metrics_consumer() + tx_consumer = transactions_consumer() + + mini_sentry.global_config["filters"] = global_generic_filters + + project_id = 42 + mini_sentry.add_full_project_config(project_id) + config = mini_sentry.project_configs[project_id]["config"] + config["transactionMetrics"] = { + "version": 1, + } + + if is_processing_relay: + relay = relay_with_processing( + options={ + "aggregator": { + "bucket_interval": 1, + "initial_delay": 0, + "debounce_delay": 0, + "shift_key": "none", + } + } + ) + else: + relay = relay( + mini_sentry, + options={ + "aggregator": { + "bucket_interval": 1, + "initial_delay": 0, + "debounce_delay": 0, + "shift_key": "none", + } + }, + ) + + transaction = generate_transaction_item() + relay.send_transaction(project_id, transaction) + + if is_processing_relay: + tx, _ = tx_consumer.get_event() + assert tx is not None + # Processing Relays extract metrics even on broken global filters. + assert metrics_consumer.get_metrics(timeout=2) + else: + assert mini_sentry.captured_events.get(timeout=2) is not None + with pytest.raises(queue.Empty): + mini_sentry.captured_metrics.get(timeout=2) + + +@pytest.mark.parametrize("is_processing_relay", (False, True)) +def test_relay_forwards_events_without_extracting_metrics_on_unsupported_project_filters( + mini_sentry, + relay, + relay_with_processing, + transactions_consumer, + metrics_consumer, + is_processing_relay, +): + metrics_consumer = metrics_consumer() + tx_consumer = transactions_consumer() + + project_id = 42 + config = mini_sentry.add_full_project_config(project_id) + config = mini_sentry.project_configs[project_id]["config"] + config["filterSettings"] = { + "generic": { + "version": 65535, # u16::MAX + "filters": [], + } + } + config["transactionMetrics"] = { + "version": 1, + } + + if is_processing_relay: + relay = relay_with_processing( + options={ + "aggregator": { + "bucket_interval": 1, + "initial_delay": 0, + "debounce_delay": 0, + "shift_key": "none", + } + } + ) + else: + relay = relay( + mini_sentry, + options={ + "aggregator": { + "bucket_interval": 1, + "initial_delay": 0, + "debounce_delay": 0, + "shift_key": "none", + } + }, + ) + + transaction = generate_transaction_item() + relay.send_transaction(project_id, transaction) + + if is_processing_relay: + tx, _ = tx_consumer.get_event() + assert tx is not None + # Processing Relays extract metrics even on unsupported project filters. + assert metrics_consumer.get_metrics(timeout=2) + else: + assert mini_sentry.captured_events.get(timeout=2) + with pytest.raises(queue.Empty): + mini_sentry.captured_metrics.get(timeout=2) + + +def test_missing_global_filters_enables_metric_extraction( + mini_sentry, + relay_with_processing, + transactions_consumer, + metrics_consumer, +): + metrics_consumer = metrics_consumer() + tx_consumer = transactions_consumer() + + mini_sentry.global_config.pop("filters") + + project_id = 42 + mini_sentry.add_full_project_config(project_id) + config = mini_sentry.project_configs[project_id]["config"] + config["transactionMetrics"] = { + "version": 1, + } + + relay = relay_with_processing( + options={ + "aggregator": { + "bucket_interval": 1, + "initial_delay": 0, + "debounce_delay": 0, + "shift_key": "none", + } + } + ) + + transaction = generate_transaction_item() + relay.send_transaction(project_id, transaction) + + tx, _ = tx_consumer.get_event() + assert tx is not None + assert metrics_consumer.get_metrics()