From 6bd0907f1d79f53a7bec34bd2350b80677c8c37b Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Fri, 23 Feb 2024 21:08:36 +0100 Subject: [PATCH 01/18] feat(global-filters): Introduce global generic filters --- Cargo.lock | 2 + Cargo.toml | 1 + relay-dynamic-config/Cargo.toml | 3 +- relay-dynamic-config/src/global.rs | 44 ++ relay-dynamic-config/src/project.rs | 12 +- relay-filter/Cargo.toml | 1 + relay-filter/src/config.rs | 730 ++++++++++++++++++- relay-filter/src/generic.rs | 103 ++- relay-filter/src/lib.rs | 7 +- relay-server/src/services/processor.rs | 17 +- relay-server/src/services/processor/event.rs | 16 +- tests/integration/fixtures/mini_sentry.py | 1 + tests/integration/test_filters.py | 34 + tests/integration/test_metrics.py | 121 ++- 14 files changed, 1015 insertions(+), 77 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0418a046c7..eec36a3279 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 84381e4f63..4a004354eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,6 +26,7 @@ insta = { version = "1.31.0", features = ["json", "redactions", "ron"] } hash32 = "0.3.1" hashbrown = "0.14.3" itertools = "0.10.5" +indexmap = "2.0.0" once_cell = "1.13.1" parking_lot = "0.12.1" rand = "0.8.5" diff --git a/relay-dynamic-config/Cargo.toml b/relay-dynamic-config/Cargo.toml index 2113b7398f..054a79b6ae 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 13a2078bb5..e1635a27d6 100644 --- a/relay-dynamic-config/src/global.rs +++ b/relay-dynamic-config/src/global.rs @@ -3,8 +3,10 @@ use std::fs::File; use std::io::BufReader; use std::path::Path; +use crate::ErrorBoundary; 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; @@ -23,6 +25,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. + #[serde( + // deserialize_with = "default_on_error", + skip_serializing_if = "skip_default_error_boundary" + )] + pub filters: ErrorBoundary, /// Sentry options passed down to Relay. #[serde( deserialize_with = "default_on_error", @@ -31,6 +39,13 @@ pub struct GlobalConfig { pub options: Options, } +fn skip_default_error_boundary(t: &ErrorBoundary) -> bool { + match t { + ErrorBoundary::Err(_) => true, + ErrorBoundary::Ok(value) => value == &T::default(), + } +} + impl GlobalConfig { /// Loads the [`GlobalConfig`] from a file if it's provided. /// @@ -46,6 +61,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), + } + } + + /// Returns the version of generic inbound filters. + /// + /// If the filters failed to deserialize, [`u16::MAX`] is returned. + pub fn generic_filters_version(&self) -> u16 { + self.filters().map(|f| f.version).unwrap_or(u16::MAX) + } } /// All options passed down from Sentry to Relay. @@ -246,6 +276,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 3912c2f896..e921733fc5 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 f81eaaa9dd..d40037d583 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 336075ca86..663743167e 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -1,13 +1,16 @@ //! Config structs for all filters. use std::borrow::Cow; -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashSet}; use std::convert::Infallible; +use std::fmt; 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)] @@ -232,7 +235,7 @@ impl LegacyBrowsersFilterConfig { /// Configuration for a generic filter. #[derive(Clone, Debug, Default, Serialize, Deserialize)] #[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. @@ -248,13 +251,117 @@ impl GenericFilterConfig { } } +impl PartialEq for GenericFilterConfig { + fn eq(&self, other: &Self) -> bool { + self.id == other.id + } +} + /// Configuration for generic filters. -#[derive(Clone, Debug, Default, Serialize, Deserialize)] -pub(crate) struct GenericFiltersConfig { +/// +/// # Deserialization +/// +/// `filters` is expected to be a [`Vec`]. +/// 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 [`IndexMap`], 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. +/// +/// # 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: { +/// "filter1": GenericFilterConfig { +/// id: "filter1", +/// is_enabled: false, +/// condition: None, +/// }, +/// }, +/// } +/// "#); +/// ``` +/// +/// Serialization: +/// +/// ``` +/// # use relay_filter::{GenericFiltersConfig, GenericFilterConfig}; +/// # use relay_protocol::condition::RuleCondition; +/// # use insta::assert_display_snapshot; +/// # use indexmap::IndexMap; +/// +/// let filter = GenericFiltersConfig { +/// version: 1, +/// filters: IndexMap::from([( +/// "filter1".to_owned(), +/// GenericFilterConfig { +/// id: "filter1".to_owned(), +/// is_enabled: true, +/// condition: Some(RuleCondition::eq("event.exceptions", "drop-error")), +/// }, +/// )]), +/// }; +/// 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" +/// } +/// } +/// ] +/// }"#); +/// ``` +#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] +#[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( + deserialize_with = "deserialize_generic_filters", + serialize_with = "serialize_generic_filters" + )] + pub filters: IndexMap, } impl GenericFiltersConfig { @@ -264,10 +371,57 @@ impl GenericFiltersConfig { } } -/// Configuration for all event filters. +fn deserialize_generic_filters<'de, D>( + deserializer: D, +) -> Result, D::Error> +where + D: de::Deserializer<'de>, +{ + struct GenericFiltersVisitor; + + impl<'de> serde::de::Visitor<'de> for GenericFiltersVisitor { + type Value = IndexMap; + + 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::new(); + while let Some(filter) = seq.next_element::()? { + if filters.contains_key(&filter.id) { + continue; + } + filters.insert(filter.id.clone(), filter); + } + Ok(filters) + } + } + + deserializer.deserialize_seq(GenericFiltersVisitor) +} + +fn serialize_generic_filters( + index_map: &IndexMap, + serializer: S, +) -> Result +where + S: Serializer, +{ + let mut seq = serializer.serialize_seq(Some(index_map.len()))?; + for (_, filter) in index_map.iter() { + 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 +461,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() @@ -328,15 +482,209 @@ impl FiltersConfig { } } +/// Configuration of generic filters combined from project and global config. +/// +/// See [`CombinedFiltersConfigIter`] for details on how to iterate easily +/// through the applicable filters. +#[derive(Copy, Clone, Debug)] +pub(crate) struct CombinedFiltersConfig<'a> { + /// Configuration of generic filters from project configs. + project_filters: &'a GenericFiltersConfig, + /// Configuration of generic filters from global config. + global_filters: Option<&'a GenericFiltersConfig>, + /// Maximum supported version for generic filters. + /// + /// It applies to all filters, from both project and global configs. + max_version: u16, +} + +impl<'a> CombinedFiltersConfig<'a> { + /// Creates a [`CombinedFiltersConfig`] from the project and global configs. + pub fn new( + project_filters: &'a GenericFiltersConfig, + global_filters: Option<&'a GenericFiltersConfig>, + max_version: u16, + ) -> Self { + CombinedFiltersConfig { + project_filters, + global_filters, + max_version, + } + } +} + +impl<'a> IntoIterator for CombinedFiltersConfig<'a> { + type Item = &'a GenericFilterConfig; + type IntoIter = CombinedFiltersConfigIter<'a>; + + fn into_iter(self) -> Self::IntoIter { + CombinedFiltersConfigIter::new(self) + } +} + +/// Iterator over [`GenericFilterConfig`] of a project and global config. +/// +/// 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. +#[derive(Debug)] +pub(crate) struct CombinedFiltersConfigIter<'a> { + /// Configuration of project and global filters. + config: CombinedFiltersConfig<'a>, + // usize is easier to deal with apis + /// index of project config filters to return + project_index: usize, + /// index of global config filters to return + global_index: usize, + /// filters that have already been yielded + evaluated: HashSet<&'a String>, +} + +impl<'a> CombinedFiltersConfigIter<'a> { + /// Creates an iterator over the filters in [`CombinedFiltersConfig`]. + pub fn new(config: CombinedFiltersConfig<'a>) -> Self { + CombinedFiltersConfigIter { + config, + project_index: 0, + global_index: 0, + evaluated: HashSet::new(), + } + } +} + +impl<'a> CombinedFiltersConfigIter<'a> { + /// Returns whether the inbound filters support the maximum version. + /// + /// Filters are supported if the versions of filters of both project and + /// global configs are not greater than the given maximum version. + /// + /// Filters from the project and global configs are complementary, and in + /// isolation, they don't provide enough valuable information to perform the + /// filtering. Additionally, new versions may include features not supported + /// in the current Relay. + fn is_version_supported(&self) -> bool { + if self.config.max_version < self.config.project_filters.version { + return false; + } + if let Some(gc) = self.config.global_filters { + if self.config.max_version < gc.version { + return false; + } + } + true + } + + fn next_project_filter(&mut self) -> Option<&'a GenericFilterConfig> { + let Some((id, filter)) = self + .config + .project_filters + .filters + .get_index(self.project_index) + else { + return None; + }; + self.project_index += 1; + + // Mark the filter as evaluated first. If the filter is disabled, it + // should not be applied from global configs. + let newly_evaluated = self.evaluated.insert(id); + + if !filter.is_enabled || !newly_evaluated { + return None; + } + + if filter.condition.is_none() { + let Some(global_filters) = self.config.global_filters else { + return None; + }; + let Some(filter) = global_filters.filters.get(id) else { + return None; + }; + return Some(filter); + } + + Some(filter) + } + + fn next_global_filter(&mut self) -> Option<&'a GenericFilterConfig> { + let Some(global_filters) = self.config.global_filters else { + return None; + }; + let Some((id, filter)) = global_filters.filters.get_index(self.global_index) else { + return None; + }; + self.global_index += 1; + + if filter.is_empty() { + return None; + } + + let newly_added = self.evaluated.insert(id); + if !newly_added { + return None; + } + + Some(filter) + } +} + +impl<'a> Iterator for CombinedFiltersConfigIter<'a> { + type Item = &'a GenericFilterConfig; + + fn next(&mut self) -> Option { + if !self.is_version_supported() { + return None; + } + + loop { + if self.project_index < self.config.project_filters.filters.len() { + let filter = self.next_project_filter(); + if filter.is_some() { + return filter; + } + continue; + } + + if let Some(global_filters) = self.config.global_filters { + if self.global_index < global_filters.filters.len() { + let filter = self.next_global_filter(); + if filter.is_some() { + return filter; + } + continue; + } + } + + return None; + } + } +} + #[cfg(test)] mod tests { use super::*; + use std::iter; + #[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 +716,7 @@ mod tests { }, generic: GenericFiltersConfig { version: 0, - filters: [], + filters: {}, }, } "###); @@ -377,13 +725,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()], @@ -412,11 +760,14 @@ mod tests { }, generic: GenericFiltersConfig { version: 1, - filters: vec![GenericFilterConfig { - id: "hydrationError".to_string(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "HydrationError")), - }], + filters: IndexMap::from([( + "hydrationError".to_owned(), + GenericFilterConfig { + id: "hydrationError".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "HydrationError")), + }, + )]), }, }; @@ -518,8 +869,8 @@ mod tests { insta::assert_debug_snapshot!(config, @r###" GenericFiltersConfig { version: 1, - filters: [ - GenericFilterConfig { + filters: { + "hydrationError": GenericFilterConfig { id: "hydrationError", is_enabled: true, condition: Some( @@ -534,13 +885,340 @@ mod tests { ), ), }, - GenericFilterConfig { + "chunkLoadError": GenericFilterConfig { id: "chunkLoadError", is_enabled: false, condition: None, }, - ], + }, } "###); } + + fn empty_filter() -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::new(), + } + } + + /// Returns a complete and enabled [`GenericFiltersConfig`]. + fn enabled_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + id.to_owned(), + GenericFilterConfig { + id: id.to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + )]), + } + } + + /// Returns an enabled flag of a [`GenericFiltersConfig`]. + fn enabled_flag_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + id.to_owned(), + GenericFilterConfig { + id: id.to_owned(), + is_enabled: true, + condition: None, + }, + )]), + } + } + + /// Returns a complete but disabled [`GenericFiltersConfig`]. + fn disabled_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + id.to_owned(), + GenericFilterConfig { + id: id.to_owned(), + is_enabled: false, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + )]), + } + } + + /// Returns a disabled flag of a [`GenericFiltersConfig`]. + fn disabled_flag_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + id.to_owned(), + GenericFilterConfig { + id: id.to_owned(), + is_enabled: false, + condition: None, + }, + )]), + } + } + + macro_rules! assert_filters { + ($combined_filters:expr, $expected:expr) => { + let actual_ids = $combined_filters.into_iter(); + assert!(actual_ids.eq($expected)); + }; + } + + #[test] + fn test_no_combined_filters_on_unsupported_project_version() { + let mut project = enabled_filter("unsupported-project"); + project.version = 2; + let global = enabled_filter("supported-global"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_filters_on_unsupported_project_version_no_global() { + let mut project = enabled_filter("unsupported-project"); + project.version = 2; + assert_filters!( + CombinedFiltersConfig::new(&project, None, 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_filters_on_unsupported_global_version() { + let project = enabled_filter("supported-project"); + let mut global = enabled_filter("unsupported-global"); + global.version = 2; + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_filters_on_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_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_combined_filters_on_supported_project_and_global_version() { + let project = enabled_filter("supported-project"); + let global = enabled_filter("supported-global"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + [ + project.filters.first().unwrap().1, + global.filters.first().unwrap().1 + ] + .into_iter() + ); + } + + #[test] + fn test_no_combined_filters_when_no_filters() { + let project = empty_filter(); + let global = empty_filter(); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_global_filters() { + let project = empty_filter(); + let global = disabled_filter("disabled-global"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_global_combined_when_enabled_global_filters() { + let project = empty_filter(); + let global = enabled_filter("enabled-global"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + [global.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_no_combined_when_global_is_flag() { + let project = empty_filter(); + let global = enabled_flag_filter("skip"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_project_filters_empty_global() { + let project = disabled_filter("disabled-project"); + let global = empty_filter(); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + #[test] + fn test_no_combined_when_disabled_project_filters_missing_global() { + let project = disabled_filter("disabled-project"); + assert_filters!( + CombinedFiltersConfig::new(&project, None, 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_in_project_and_global() { + let project = disabled_filter("disabled-project"); + let global = disabled_filter("disabled-global"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_project_enabled_global() { + let project = disabled_filter("filter"); + let global = enabled_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_enabled_flag_project_empty_global() { + let project = enabled_flag_filter("filter"); + let global = empty_filter(); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_enabled_flag_project_missing_global() { + let project = enabled_flag_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, None, 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_flag_project_empty_global() { + let project = disabled_flag_filter("filter"); + let global = empty_filter(); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_flag_project_missing_global() { + let project = disabled_flag_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, None, 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_project_combined_when_only_project_enabled_empty_global() { + let project = enabled_filter("enabled-project"); + let global = empty_filter(); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_project_combined_when_only_project_enabled_missing_global() { + let project = enabled_filter("enabled-project"); + assert_filters!( + CombinedFiltersConfig::new(&project, None, 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_project_combined_when_enabled_flag_project_disabled_global() { + let project = enabled_filter("filter"); + let global = disabled_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_no_combined_when_disabled_flag_project_disabled_global() { + let project = disabled_flag_filter("filter"); + let global = disabled_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_project_combined_when_enabled_project_disabled_global() { + let project = enabled_filter("filter"); + let global = disabled_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_common_combined_when_enabled_flag_project_enabled_global() { + let project = enabled_flag_filter("filter"); + let global = enabled_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + [global.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_no_combined_when_disabled_flag_project_enabled_global() { + let project = disabled_flag_filter("filter"); + let global = enabled_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_common_combined_when_enabled_project_enabled_global() { + let project = enabled_filter("filter"); + let global = enabled_flag_filter("filter"); + assert_filters!( + CombinedFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1].into_iter() + ); + } } diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 24673de715..ec95be6c99 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -4,7 +4,7 @@ //! first one that matches, will result in the event being discarded with a [`FilterStatKey`] //! identifying the matching filter. -use crate::{FilterStatKey, GenericFiltersConfig}; +use crate::{CombinedFiltersConfig, FilterStatKey, GenericFiltersConfig}; use relay_event_schema::protocol::Event; use relay_protocol::RuleCondition; @@ -13,10 +13,6 @@ use relay_protocol::RuleCondition; /// If the version in the project config is higher, no generic filters are applied. pub const VERSION: u16 = 1; -fn is_enabled(config: &GenericFiltersConfig) -> bool { - config.version > 0 && config.version <= VERSION -} - /// Checks events by patterns in their error messages. fn matches(event: &Event, condition: Option<&RuleCondition>) -> bool { // TODO: the condition DSL needs to be extended to support more complex semantics, such as @@ -27,16 +23,15 @@ 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 generic_filters_config = + CombinedFiltersConfig::new(project_filters, global_filters, VERSION); + let filters = generic_filters_config.into_iter(); - for filter_config in config.filters.iter() { - if !filter_config.is_empty() && matches(event, filter_config.condition.as_ref()) { + for filter_config in filters { + if matches(event, filter_config.condition.as_ref()) { return Err(FilterStatKey::GenericFilter(filter_config.id.clone())); } } @@ -48,23 +43,30 @@ pub(crate) fn should_filter( mod tests { use crate::generic::{should_filter, VERSION}; use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig}; + use indexmap::IndexMap; use relay_event_schema::protocol::{Event, LenientString}; use relay_protocol::Annotated; use relay_protocol::RuleCondition; - fn mock_filters() -> Vec { - vec![ - GenericFilterConfig { - id: "firstReleases".to_string(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.release", "1.0")), - }, - GenericFilterConfig { - id: "helloTransactions".to_string(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.transaction", "/hello")), - }, - ] + fn mock_filters() -> IndexMap { + IndexMap::from([ + ( + "firstReleases".to_owned(), + GenericFilterConfig { + id: "firstReleases".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.release", "1.0")), + }, + ), + ( + "helloTransactions".to_owned(), + GenericFilterConfig { + id: "helloTransactions".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.transaction", "/hello")), + }, + ), + ]) } #[test] @@ -80,7 +82,7 @@ mod tests { ..Default::default() }; assert_eq!( - should_filter(&event, &config), + should_filter(&event, &config, None), Err(FilterStatKey::GenericFilter("firstReleases".to_string())) ); @@ -90,7 +92,7 @@ mod tests { ..Default::default() }; assert_eq!( - should_filter(&event, &config), + should_filter(&event, &config, None), Err(FilterStatKey::GenericFilter( "helloTransactions".to_string() )) @@ -111,7 +113,7 @@ mod tests { ..Default::default() }; assert_eq!( - should_filter(&event, &config), + should_filter(&event, &config, None), Err(FilterStatKey::GenericFilter("firstReleases".to_string())) ); } @@ -128,7 +130,7 @@ 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] @@ -144,6 +146,45 @@ 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: IndexMap::from([( + "firstReleases".to_owned(), + GenericFilterConfig { + id: "firstReleases".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.release", "1.0")), + }, + )]), + }; + + let global = GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + "helloTransactions".to_owned(), + GenericFilterConfig { + id: "helloTransactions".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.transaction", "/hello")), + }, + )]), + }; + + 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() + )) + ); } } diff --git a/relay-filter/src/lib.rs b/relay-filter/src/lib.rs index 2185006f0c..e8364895b1 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::VERSION as GENERIC_FILTERS_VERSION; + /// 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 d5c572f2d9..ac21d54949 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().filters())?; dynamic_sampling::tag_error_with_sampling_decision(state, &self.inner.config); if_processing!(self.inner.config, { @@ -1274,12 +1274,22 @@ impl EnvelopeProcessorService { event::finalize(state, &self.inner.config)?; self.light_normalize_event(state)?; dynamic_sampling::normalize(state); - event::filter(state)?; + let global_config = self.inner.global_config.current(); + event::filter(state, global_config.filters())?; dynamic_sampling::run(state, &self.inner.config); + // 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. + let supported_generic_filters = state.project_state.config.filter_settings.generic.version + <= relay_filter::GENERIC_FILTERS_VERSION + && global_config.generic_filters_version() <= relay_filter::GENERIC_FILTERS_VERSION; // 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() || state.sampling_result.should_drop() { + let extract_metrics_from_sampling = + self.inner.config.processing_enabled() || state.sampling_result.should_drop(); + if supported_generic_filters && extract_metrics_from_sampling { self.extract_metrics(state)?; } @@ -2757,6 +2767,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 b8eb12b377..648d810bdf 100644 --- a/relay-server/src/services/processor/event.rs +++ b/relay-server/src/services/processor/event.rs @@ -14,6 +14,7 @@ use relay_event_schema::protocol::{ Breadcrumb, Csp, Event, ExpectCt, ExpectStaple, Hpkp, LenientString, NetworkReportError, OtelContext, RelayInfo, SecurityReportType, Timestamp, Values, }; +use relay_filter::GenericFiltersConfig; use relay_pii::PiiProcessor; use relay_protocol::{Annotated, Array, FromValue, Object, Value}; use relay_quotas::DataCategory; @@ -273,6 +274,7 @@ pub fn finalize( pub fn filter( state: &mut ProcessEnvelopeState, + global_filters: Option<&GenericFiltersConfig>, ) -> Result<(), ProcessingError> { let event = match state.event.value_mut() { Some(event) => event, @@ -285,12 +287,14 @@ 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_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 4aa97ac05d..2a2cb014da 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_filters.py b/tests/integration/test_filters.py index 4bcafe2030..2f96ef72c0 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 0e78c3e37e..c121391c89 100644 --- a/tests/integration/test_metrics.py +++ b/tests/integration/test_metrics.py @@ -1,3 +1,4 @@ +from time import sleep import hashlib from collections import defaultdict from datetime import datetime, timedelta, timezone @@ -1851,8 +1852,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 +1879,121 @@ def test_metric_bucket_encoding_dynamic_global_config_option( else: assert metrics[dname]["value"] == [1337.0] assert metrics[sname]["value"] == [42.0] + + +def test_relay_forwards_events_without_extracting_metrics_on_broken_global_filters( + mini_sentry, + relay_with_processing, + transactions_consumer, + metrics_consumer, +): + metrics_consumer = metrics_consumer() + tx_consumer = transactions_consumer() + + mini_sentry.global_config["filters"] = { + "version": "Halp! I'm broken!", + "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 + sleep(1) + metrics_consumer.assert_empty() + + +def test_relay_forwards_events_without_extracting_metrics_on_unsupported_project_filters( + mini_sentry, + relay_with_processing, + transactions_consumer, + metrics_consumer, +): + 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, + } + + 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 + sleep(1) + metrics_consumer.assert_empty() + + +def test_missing_global_filters_enables_metric_extraction( + mini_sentry, + relay_with_processing, + transactions_consumer, + metrics_consumer, +): + metrics_consumer = metrics_consumer() + transactions_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) + + transaction, _ = transactions_consumer.get_event() + assert transaction is not None + metrics = metrics_consumer.get_metrics() + assert metrics is not None From bba2a7631db1c8a055a3339ffb3f3ec0f81d4f72 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Fri, 23 Feb 2024 22:09:02 +0100 Subject: [PATCH 02/18] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e5e42679b..ad94bf0810 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Extend GPU context with data for Unreal Engine crash reports. ([#3144](https://github.com/getsentry/relay/pull/3144)) - Parametrize transaction in dynamic sampling context. ([#3141](https://github.com/getsentry/relay/pull/3141)) - 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**: From 117bc038d2638f2b018e3fefc0edf02811e4808c Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Mon, 26 Feb 2024 08:47:37 +0100 Subject: [PATCH 03/18] rm development comments --- relay-dynamic-config/src/global.rs | 5 +---- relay-filter/src/config.rs | 7 +++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/relay-dynamic-config/src/global.rs b/relay-dynamic-config/src/global.rs index e1635a27d6..344be1dc1a 100644 --- a/relay-dynamic-config/src/global.rs +++ b/relay-dynamic-config/src/global.rs @@ -26,10 +26,7 @@ pub struct GlobalConfig { #[serde(skip_serializing_if = "Vec::is_empty")] pub quotas: Vec, /// Configuration for global inbound filters. - #[serde( - // deserialize_with = "default_on_error", - skip_serializing_if = "skip_default_error_boundary" - )] + #[serde(skip_serializing_if = "skip_default_error_boundary")] pub filters: ErrorBoundary, /// Sentry options passed down to Relay. #[serde( diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index 663743167e..708996573d 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -545,12 +545,11 @@ impl<'a> IntoIterator for CombinedFiltersConfig<'a> { pub(crate) struct CombinedFiltersConfigIter<'a> { /// Configuration of project and global filters. config: CombinedFiltersConfig<'a>, - // usize is easier to deal with apis - /// index of project config filters to return + /// Index of the next filter in project config to evaluate. project_index: usize, - /// index of global config filters to return + /// Index of the next filter in global config to evaluate. global_index: usize, - /// filters that have already been yielded + /// Filters that have been evaluated, either yielded or ignored. evaluated: HashSet<&'a String>, } From 950325dff6dc769f46dfcc6bb4122962dacd3f42 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Mon, 26 Feb 2024 14:07:22 +0100 Subject: [PATCH 04/18] address feedback Changes include: - Rename `CombinedFiltersConfig` -> `DynamicFiltersConfig` (same for the iterator) to align with `DynamicMeasurementsConfig`. - Add unit test covering multiple filters from project and global filters. - Add a brief comment in the iterator's loop to provide a bit more clarity into what the `loop` does. --- relay-filter/src/config.rs | 161 +++++++++++++++++++++++++++--------- relay-filter/src/generic.rs | 4 +- 2 files changed, 122 insertions(+), 43 deletions(-) diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index 708996573d..1f355c7b45 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -484,10 +484,10 @@ impl ProjectFiltersConfig { /// Configuration of generic filters combined from project and global config. /// -/// See [`CombinedFiltersConfigIter`] for details on how to iterate easily +/// See [`DynamicFiltersConfigIter`] for details on how to iterate easily /// through the applicable filters. #[derive(Copy, Clone, Debug)] -pub(crate) struct CombinedFiltersConfig<'a> { +pub(crate) struct DynamicFiltersConfig<'a> { /// Configuration of generic filters from project configs. project_filters: &'a GenericFiltersConfig, /// Configuration of generic filters from global config. @@ -498,14 +498,14 @@ pub(crate) struct CombinedFiltersConfig<'a> { max_version: u16, } -impl<'a> CombinedFiltersConfig<'a> { - /// Creates a [`CombinedFiltersConfig`] from the project and global configs. +impl<'a> DynamicFiltersConfig<'a> { + /// Creates a [`DynamicFiltersConfig`] from the project and global configs. pub fn new( project_filters: &'a GenericFiltersConfig, global_filters: Option<&'a GenericFiltersConfig>, max_version: u16, ) -> Self { - CombinedFiltersConfig { + DynamicFiltersConfig { project_filters, global_filters, max_version, @@ -513,12 +513,12 @@ impl<'a> CombinedFiltersConfig<'a> { } } -impl<'a> IntoIterator for CombinedFiltersConfig<'a> { +impl<'a> IntoIterator for DynamicFiltersConfig<'a> { type Item = &'a GenericFilterConfig; - type IntoIter = CombinedFiltersConfigIter<'a>; + type IntoIter = DynamicFiltersConfigIter<'a>; fn into_iter(self) -> Self::IntoIter { - CombinedFiltersConfigIter::new(self) + DynamicFiltersConfigIter::new(self) } } @@ -542,9 +542,9 @@ impl<'a> IntoIterator for CombinedFiltersConfig<'a> { /// - If `is_enabled` is false, no filters with the same IDs are returned, /// including matching filters from global configs. #[derive(Debug)] -pub(crate) struct CombinedFiltersConfigIter<'a> { +pub(crate) struct DynamicFiltersConfigIter<'a> { /// Configuration of project and global filters. - config: CombinedFiltersConfig<'a>, + config: DynamicFiltersConfig<'a>, /// Index of the next filter in project config to evaluate. project_index: usize, /// Index of the next filter in global config to evaluate. @@ -553,10 +553,10 @@ pub(crate) struct CombinedFiltersConfigIter<'a> { evaluated: HashSet<&'a String>, } -impl<'a> CombinedFiltersConfigIter<'a> { - /// Creates an iterator over the filters in [`CombinedFiltersConfig`]. - pub fn new(config: CombinedFiltersConfig<'a>) -> Self { - CombinedFiltersConfigIter { +impl<'a> DynamicFiltersConfigIter<'a> { + /// Creates an iterator over the filters in [`DynamicFiltersConfig`]. + pub fn new(config: DynamicFiltersConfig<'a>) -> Self { + DynamicFiltersConfigIter { config, project_index: 0, global_index: 0, @@ -565,7 +565,7 @@ impl<'a> CombinedFiltersConfigIter<'a> { } } -impl<'a> CombinedFiltersConfigIter<'a> { +impl<'a> DynamicFiltersConfigIter<'a> { /// Returns whether the inbound filters support the maximum version. /// /// Filters are supported if the versions of filters of both project and @@ -641,7 +641,7 @@ impl<'a> CombinedFiltersConfigIter<'a> { } } -impl<'a> Iterator for CombinedFiltersConfigIter<'a> { +impl<'a> Iterator for DynamicFiltersConfigIter<'a> { type Item = &'a GenericFilterConfig; fn next(&mut self) -> Option { @@ -649,6 +649,13 @@ impl<'a> Iterator for CombinedFiltersConfigIter<'a> { return None; } + // This loop starts by iterating through all project filters: increments + // `project_index` on every iteration until it grows too much (i.e. all + // project filters are evaluated), yielding the evaluated filter if + // necessary. + // Then, the same approach is applied to global filters. + // `continue` restarts the loop, so global filters aren't evaluated + // until all project filters are. loop { if self.project_index < self.config.project_filters.filters.len() { let filter = self.next_project_filter(); @@ -974,7 +981,7 @@ mod tests { project.version = 2; let global = enabled_filter("supported-global"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -984,7 +991,7 @@ mod tests { let mut project = enabled_filter("unsupported-project"); project.version = 2; assert_filters!( - CombinedFiltersConfig::new(&project, None, 1), + DynamicFiltersConfig::new(&project, None, 1), iter::empty::<&GenericFilterConfig>() ); } @@ -995,7 +1002,7 @@ mod tests { let mut global = enabled_filter("unsupported-global"); global.version = 2; assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1007,7 +1014,7 @@ mod tests { let mut global = enabled_filter("unsupported-global"); global.version = 2; assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1017,7 +1024,7 @@ mod tests { let project = enabled_filter("supported-project"); let global = enabled_filter("supported-global"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), [ project.filters.first().unwrap().1, global.filters.first().unwrap().1 @@ -1031,7 +1038,7 @@ mod tests { let project = empty_filter(); let global = empty_filter(); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1041,7 +1048,7 @@ mod tests { let project = empty_filter(); let global = disabled_filter("disabled-global"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1051,7 +1058,7 @@ mod tests { let project = empty_filter(); let global = enabled_filter("enabled-global"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), [global.filters.first().unwrap().1].into_iter() ); } @@ -1061,7 +1068,7 @@ mod tests { let project = empty_filter(); let global = enabled_flag_filter("skip"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1071,7 +1078,7 @@ mod tests { let project = disabled_filter("disabled-project"); let global = empty_filter(); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1079,7 +1086,7 @@ mod tests { fn test_no_combined_when_disabled_project_filters_missing_global() { let project = disabled_filter("disabled-project"); assert_filters!( - CombinedFiltersConfig::new(&project, None, 1), + DynamicFiltersConfig::new(&project, None, 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1089,7 +1096,7 @@ mod tests { let project = disabled_filter("disabled-project"); let global = disabled_filter("disabled-global"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1099,7 +1106,7 @@ mod tests { let project = disabled_filter("filter"); let global = enabled_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1109,7 +1116,7 @@ mod tests { let project = enabled_flag_filter("filter"); let global = empty_filter(); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1118,7 +1125,7 @@ mod tests { fn test_no_combined_when_enabled_flag_project_missing_global() { let project = enabled_flag_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, None, 1), + DynamicFiltersConfig::new(&project, None, 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1128,7 +1135,7 @@ mod tests { let project = disabled_flag_filter("filter"); let global = empty_filter(); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1137,7 +1144,7 @@ mod tests { fn test_no_combined_when_disabled_flag_project_missing_global() { let project = disabled_flag_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, None, 1), + DynamicFiltersConfig::new(&project, None, 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1147,7 +1154,7 @@ mod tests { let project = enabled_filter("enabled-project"); let global = empty_filter(); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), [project.filters.first().unwrap().1].into_iter() ); } @@ -1156,7 +1163,7 @@ mod tests { fn test_project_combined_when_only_project_enabled_missing_global() { let project = enabled_filter("enabled-project"); assert_filters!( - CombinedFiltersConfig::new(&project, None, 1), + DynamicFiltersConfig::new(&project, None, 1), [project.filters.first().unwrap().1].into_iter() ); } @@ -1166,7 +1173,7 @@ mod tests { let project = enabled_filter("filter"); let global = disabled_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), [project.filters.first().unwrap().1].into_iter() ); } @@ -1176,7 +1183,7 @@ mod tests { let project = disabled_flag_filter("filter"); let global = disabled_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1186,7 +1193,7 @@ mod tests { let project = enabled_filter("filter"); let global = disabled_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), [project.filters.first().unwrap().1].into_iter() ); } @@ -1196,7 +1203,7 @@ mod tests { let project = enabled_flag_filter("filter"); let global = enabled_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), [global.filters.first().unwrap().1].into_iter() ); } @@ -1206,7 +1213,7 @@ mod tests { let project = disabled_flag_filter("filter"); let global = enabled_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1216,8 +1223,80 @@ mod tests { let project = enabled_filter("filter"); let global = enabled_flag_filter("filter"); assert_filters!( - CombinedFiltersConfig::new(&project, Some(&global), 1), + DynamicFiltersConfig::new(&project, Some(&global), 1), [project.filters.first().unwrap().1].into_iter() ); } + + #[test] + fn test_multiple_combined_filters() { + let project = GenericFiltersConfig { + version: 1, + filters: IndexMap::from([ + ( + "0".to_owned(), + GenericFilterConfig { + id: "0".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + ), + ( + "1".to_owned(), + GenericFilterConfig { + id: "1".to_owned(), + is_enabled: true, + condition: None, + }, + ), + ( + "2".to_owned(), + GenericFilterConfig { + id: "2".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + ), + ( + "not-picked".to_owned(), + GenericFilterConfig { + id: "not-picked".to_owned(), + is_enabled: true, + condition: None, + }, + ), + ]), + }; + let global = GenericFiltersConfig { + version: 1, + filters: IndexMap::from([ + ( + "1".to_owned(), + GenericFilterConfig { + id: "1".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myOtherError")), + }, + ), + ( + "3".to_owned(), + GenericFilterConfig { + id: "3".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myLastError")), + }, + ), + ]), + }; + assert_filters!( + DynamicFiltersConfig::new(&project, Some(&global), 1), + [ + &project.filters[0], // id=0, picked from project + &global.filters[0], // id=1, picked from global through the project's flag + &project.filters[2], // id=2, picked from project + &global.filters[1] // id=3, picked from global + ] + .into_iter() + ); + } } diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index ec95be6c99..7a04ca8f37 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -4,7 +4,7 @@ //! first one that matches, will result in the event being discarded with a [`FilterStatKey`] //! identifying the matching filter. -use crate::{CombinedFiltersConfig, FilterStatKey, GenericFiltersConfig}; +use crate::{DynamicFiltersConfig, FilterStatKey, GenericFiltersConfig}; use relay_event_schema::protocol::Event; use relay_protocol::RuleCondition; @@ -27,7 +27,7 @@ pub(crate) fn should_filter( global_filters: Option<&GenericFiltersConfig>, ) -> Result<(), FilterStatKey> { let generic_filters_config = - CombinedFiltersConfig::new(project_filters, global_filters, VERSION); + DynamicFiltersConfig::new(project_filters, global_filters, VERSION); let filters = generic_filters_config.into_iter(); for filter_config in filters { From 45e2f94c76d625acc63521cd0f31e3308ffa365f Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Tue, 27 Feb 2024 16:21:14 +0100 Subject: [PATCH 05/18] feedback --- relay-dynamic-config/src/global.rs | 11 +- relay-filter/src/config.rs | 286 +++++++++++++++---------- relay-filter/src/generic.rs | 14 +- relay-filter/src/lib.rs | 2 +- relay-server/src/services/processor.rs | 7 +- 5 files changed, 197 insertions(+), 123 deletions(-) diff --git a/relay-dynamic-config/src/global.rs b/relay-dynamic-config/src/global.rs index 344be1dc1a..f5cf71d4e7 100644 --- a/relay-dynamic-config/src/global.rs +++ b/relay-dynamic-config/src/global.rs @@ -26,7 +26,10 @@ pub struct GlobalConfig { #[serde(skip_serializing_if = "Vec::is_empty")] pub quotas: Vec, /// Configuration for global inbound filters. - #[serde(skip_serializing_if = "skip_default_error_boundary")] + /// + /// These filters are merged with generic filters in project configs before + /// applying. + #[serde(skip_serializing_if = "skip_generic_filters")] pub filters: ErrorBoundary, /// Sentry options passed down to Relay. #[serde( @@ -36,10 +39,10 @@ pub struct GlobalConfig { pub options: Options, } -fn skip_default_error_boundary(t: &ErrorBoundary) -> bool { - match t { +fn skip_generic_filters(filters_config: &ErrorBoundary) -> bool { + match filters_config { ErrorBoundary::Err(_) => true, - ErrorBoundary::Ok(value) => value == &T::default(), + ErrorBoundary::Ok(config) => config.version == 0 && config.filters.is_empty(), } } diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index 1f355c7b45..3c890ca15e 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -4,6 +4,7 @@ use std::borrow::Cow; use std::collections::{BTreeSet, HashSet}; use std::convert::Infallible; use std::fmt; +use std::iter::FusedIterator; use std::str::FromStr; use indexmap::IndexMap; @@ -251,9 +252,12 @@ impl GenericFilterConfig { } } +#[cfg(test)] impl PartialEq for GenericFilterConfig { fn eq(&self, other: &Self) -> bool { self.id == other.id + && self.is_enabled == other.is_enabled + && self.condition == other.condition } } @@ -271,6 +275,26 @@ impl PartialEq for GenericFilterConfig { /// 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: @@ -348,7 +372,25 @@ impl PartialEq for GenericFilterConfig { /// ] /// }"#); /// ``` -#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] +/// +/// Serialization of filters is skipped if there aren't any: +/// +/// ``` +/// # use relay_filter::{GenericFiltersConfig, GenericFilterConfig}; +/// # use relay_protocol::condition::RuleCondition; +/// # use insta::assert_display_snapshot; +/// # use indexmap::IndexMap; +/// +/// let filter = GenericFiltersConfig { +/// version: 1, +/// filters: IndexMap::new(), +/// }; +/// let serialized = serde_json::to_string_pretty(&filter).unwrap(); +/// assert_display_snapshot!(serialized, @r#"{ +/// "version": 1 +/// }"#); +/// ``` +#[derive(Clone, Debug, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct GenericFiltersConfig { /// Version of the filters configuration. @@ -358,8 +400,8 @@ pub struct GenericFiltersConfig { /// The map contains unique filters, meaning there are no two filters with /// the same id. See struct docs for more details. #[serde( - deserialize_with = "deserialize_generic_filters", - serialize_with = "serialize_generic_filters" + skip_serializing_if = "IndexMap::is_empty", + with = "generic_filters_custom_serialization" )] pub filters: IndexMap, } @@ -371,51 +413,54 @@ impl GenericFiltersConfig { } } -fn deserialize_generic_filters<'de, D>( - deserializer: D, -) -> Result, D::Error> -where - D: de::Deserializer<'de>, -{ - struct GenericFiltersVisitor; +mod generic_filters_custom_serialization { + use super::*; - impl<'de> serde::de::Visitor<'de> for GenericFiltersVisitor { - type Value = IndexMap; + pub fn deserialize<'de, D>( + deserializer: D, + ) -> Result, D::Error> + where + D: de::Deserializer<'de>, + { + struct GenericFiltersVisitor; - fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - formatter.write_str("a vector of filters: Vec") - } + impl<'de> serde::de::Visitor<'de> for GenericFiltersVisitor { + type Value = IndexMap; - fn visit_seq(self, mut seq: A) -> Result - where - A: de::SeqAccess<'de>, - { - let mut filters = IndexMap::new(); - while let Some(filter) = seq.next_element::()? { - if filters.contains_key(&filter.id) { - continue; + 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); + } } - filters.insert(filter.id.clone(), filter); + Ok(filters) } - Ok(filters) } - } - deserializer.deserialize_seq(GenericFiltersVisitor) -} + deserializer.deserialize_seq(GenericFiltersVisitor) + } -fn serialize_generic_filters( - index_map: &IndexMap, - serializer: S, -) -> Result -where - S: Serializer, -{ - let mut seq = serializer.serialize_seq(Some(index_map.len()))?; - for (_, filter) in index_map.iter() { - seq.serialize_element(filter)?; + pub fn serialize( + index_map: &IndexMap, + serializer: S, + ) -> Result + where + S: Serializer, + { + let mut seq = serializer.serialize_seq(Some(index_map.len()))?; + for filter in index_map.values() { + seq.serialize_element(filter)?; + } + seq.end() } - seq.end() } /// Configuration for all event filters from project configs. @@ -487,7 +532,7 @@ impl ProjectFiltersConfig { /// See [`DynamicFiltersConfigIter`] for details on how to iterate easily /// through the applicable filters. #[derive(Copy, Clone, Debug)] -pub(crate) struct DynamicFiltersConfig<'a> { +pub(crate) struct DynamicGenericFiltersConfig<'a> { /// Configuration of generic filters from project configs. project_filters: &'a GenericFiltersConfig, /// Configuration of generic filters from global config. @@ -495,30 +540,30 @@ pub(crate) struct DynamicFiltersConfig<'a> { /// Maximum supported version for generic filters. /// /// It applies to all filters, from both project and global configs. - max_version: u16, + supported_version: u16, } -impl<'a> DynamicFiltersConfig<'a> { +impl<'a> DynamicGenericFiltersConfig<'a> { /// Creates a [`DynamicFiltersConfig`] from the project and global configs. pub fn new( project_filters: &'a GenericFiltersConfig, global_filters: Option<&'a GenericFiltersConfig>, - max_version: u16, + supported_version: u16, ) -> Self { - DynamicFiltersConfig { + DynamicGenericFiltersConfig { project_filters, global_filters, - max_version, + supported_version, } } } -impl<'a> IntoIterator for DynamicFiltersConfig<'a> { +impl<'a> IntoIterator for DynamicGenericFiltersConfig<'a> { type Item = &'a GenericFilterConfig; - type IntoIter = DynamicFiltersConfigIter<'a>; + type IntoIter = DynamicGenericFiltersConfigIter<'a>; fn into_iter(self) -> Self::IntoIter { - DynamicFiltersConfigIter::new(self) + DynamicGenericFiltersConfigIter::new(self) } } @@ -542,21 +587,21 @@ impl<'a> IntoIterator for DynamicFiltersConfig<'a> { /// - If `is_enabled` is false, no filters with the same IDs are returned, /// including matching filters from global configs. #[derive(Debug)] -pub(crate) struct DynamicFiltersConfigIter<'a> { +pub(crate) struct DynamicGenericFiltersConfigIter<'a> { /// Configuration of project and global filters. - config: DynamicFiltersConfig<'a>, + config: DynamicGenericFiltersConfig<'a>, /// Index of the next filter in project config to evaluate. project_index: usize, /// Index of the next filter in global config to evaluate. global_index: usize, /// Filters that have been evaluated, either yielded or ignored. - evaluated: HashSet<&'a String>, + evaluated: HashSet<&'a str>, } -impl<'a> DynamicFiltersConfigIter<'a> { +impl<'a> DynamicGenericFiltersConfigIter<'a> { /// Creates an iterator over the filters in [`DynamicFiltersConfig`]. - pub fn new(config: DynamicFiltersConfig<'a>) -> Self { - DynamicFiltersConfigIter { + pub fn new(config: DynamicGenericFiltersConfig<'a>) -> Self { + DynamicGenericFiltersConfigIter { config, project_index: 0, global_index: 0, @@ -565,7 +610,7 @@ impl<'a> DynamicFiltersConfigIter<'a> { } } -impl<'a> DynamicFiltersConfigIter<'a> { +impl<'a> DynamicGenericFiltersConfigIter<'a> { /// Returns whether the inbound filters support the maximum version. /// /// Filters are supported if the versions of filters of both project and @@ -576,15 +621,11 @@ impl<'a> DynamicFiltersConfigIter<'a> { /// filtering. Additionally, new versions may include features not supported /// in the current Relay. fn is_version_supported(&self) -> bool { - if self.config.max_version < self.config.project_filters.version { - return false; - } - if let Some(gc) = self.config.global_filters { - if self.config.max_version < gc.version { - return false; - } - } - true + self.config.project_filters.version <= self.config.supported_version + && self + .config + .global_filters + .map_or(true, |gf| gf.version <= self.config.supported_version) } fn next_project_filter(&mut self) -> Option<&'a GenericFilterConfig> { @@ -597,26 +638,20 @@ impl<'a> DynamicFiltersConfigIter<'a> { return None; }; self.project_index += 1; + self.evaluated.insert(id); - // Mark the filter as evaluated first. If the filter is disabled, it - // should not be applied from global configs. - let newly_evaluated = self.evaluated.insert(id); - - if !filter.is_enabled || !newly_evaluated { + if !filter.is_enabled { return None; } - if filter.condition.is_none() { - let Some(global_filters) = self.config.global_filters else { - return None; - }; - let Some(filter) = global_filters.filters.get(id) else { - return None; - }; - return Some(filter); + if filter.condition.is_some() { + Some(filter) + } else { + self.config + .global_filters + .and_then(|config| config.filters.get(id)) + .filter(|filter| filter.condition.is_some()) } - - Some(filter) } fn next_global_filter(&mut self) -> Option<&'a GenericFilterConfig> { @@ -632,16 +667,11 @@ impl<'a> DynamicFiltersConfigIter<'a> { return None; } - let newly_added = self.evaluated.insert(id); - if !newly_added { - return None; - } - - Some(filter) + self.evaluated.insert(id).then_some(filter) } } -impl<'a> Iterator for DynamicFiltersConfigIter<'a> { +impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { type Item = &'a GenericFilterConfig; fn next(&mut self) -> Option { @@ -680,6 +710,8 @@ impl<'a> Iterator for DynamicFiltersConfigIter<'a> { } } +impl<'a> FusedIterator for DynamicGenericFiltersConfigIter<'a> {} + #[cfg(test)] mod tests { use super::*; @@ -981,7 +1013,7 @@ mod tests { project.version = 2; let global = enabled_filter("supported-global"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -991,7 +1023,7 @@ mod tests { let mut project = enabled_filter("unsupported-project"); project.version = 2; assert_filters!( - DynamicFiltersConfig::new(&project, None, 1), + DynamicGenericFiltersConfig::new(&project, None, 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1002,7 +1034,7 @@ mod tests { let mut global = enabled_filter("unsupported-global"); global.version = 2; assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1014,7 +1046,7 @@ mod tests { let mut global = enabled_filter("unsupported-global"); global.version = 2; assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1024,7 +1056,7 @@ mod tests { let project = enabled_filter("supported-project"); let global = enabled_filter("supported-global"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), [ project.filters.first().unwrap().1, global.filters.first().unwrap().1 @@ -1033,12 +1065,22 @@ mod tests { ); } + #[test] + fn test_no_combined_duplicates_when_both_enabled() { + let project = enabled_filter("filter"); + let global = enabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1,].into_iter() + ); + } + #[test] fn test_no_combined_filters_when_no_filters() { let project = empty_filter(); let global = empty_filter(); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1048,7 +1090,7 @@ mod tests { let project = empty_filter(); let global = disabled_filter("disabled-global"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1058,7 +1100,7 @@ mod tests { let project = empty_filter(); let global = enabled_filter("enabled-global"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), [global.filters.first().unwrap().1].into_iter() ); } @@ -1068,7 +1110,7 @@ mod tests { let project = empty_filter(); let global = enabled_flag_filter("skip"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1078,7 +1120,7 @@ mod tests { let project = disabled_filter("disabled-project"); let global = empty_filter(); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1086,7 +1128,7 @@ mod tests { fn test_no_combined_when_disabled_project_filters_missing_global() { let project = disabled_filter("disabled-project"); assert_filters!( - DynamicFiltersConfig::new(&project, None, 1), + DynamicGenericFiltersConfig::new(&project, None, 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1096,7 +1138,17 @@ mod tests { let project = disabled_filter("disabled-project"); let global = disabled_filter("disabled-global"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_duplicates_when_both_disabled() { + let project = disabled_filter("filter"); + let global = disabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1106,7 +1158,7 @@ mod tests { let project = disabled_filter("filter"); let global = enabled_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1116,7 +1168,7 @@ mod tests { let project = enabled_flag_filter("filter"); let global = empty_filter(); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1125,7 +1177,7 @@ mod tests { fn test_no_combined_when_enabled_flag_project_missing_global() { let project = enabled_flag_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, None, 1), + DynamicGenericFiltersConfig::new(&project, None, 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1135,7 +1187,7 @@ mod tests { let project = disabled_flag_filter("filter"); let global = empty_filter(); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1144,7 +1196,7 @@ mod tests { fn test_no_combined_when_disabled_flag_project_missing_global() { let project = disabled_flag_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, None, 1), + DynamicGenericFiltersConfig::new(&project, None, 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1154,7 +1206,7 @@ mod tests { let project = enabled_filter("enabled-project"); let global = empty_filter(); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), [project.filters.first().unwrap().1].into_iter() ); } @@ -1163,7 +1215,7 @@ mod tests { fn test_project_combined_when_only_project_enabled_missing_global() { let project = enabled_filter("enabled-project"); assert_filters!( - DynamicFiltersConfig::new(&project, None, 1), + DynamicGenericFiltersConfig::new(&project, None, 1), [project.filters.first().unwrap().1].into_iter() ); } @@ -1173,7 +1225,7 @@ mod tests { let project = enabled_filter("filter"); let global = disabled_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), [project.filters.first().unwrap().1].into_iter() ); } @@ -1183,7 +1235,7 @@ mod tests { let project = disabled_flag_filter("filter"); let global = disabled_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1193,7 +1245,7 @@ mod tests { let project = enabled_filter("filter"); let global = disabled_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), [project.filters.first().unwrap().1].into_iter() ); } @@ -1203,7 +1255,7 @@ mod tests { let project = enabled_flag_filter("filter"); let global = enabled_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), [global.filters.first().unwrap().1].into_iter() ); } @@ -1213,7 +1265,7 @@ mod tests { let project = disabled_flag_filter("filter"); let global = enabled_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), iter::empty::<&GenericFilterConfig>() ); } @@ -1223,11 +1275,21 @@ mod tests { let project = enabled_filter("filter"); let global = enabled_flag_filter("filter"); assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), [project.filters.first().unwrap().1].into_iter() ); } + #[test] + fn test_no_combined_when_enabled_flags_project_and_global() { + let project = enabled_flag_filter("filter"); + let global = enabled_flag_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + #[test] fn test_multiple_combined_filters() { let project = GenericFiltersConfig { @@ -1289,7 +1351,7 @@ mod tests { ]), }; assert_filters!( - DynamicFiltersConfig::new(&project, Some(&global), 1), + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), [ &project.filters[0], // id=0, picked from project &global.filters[0], // id=1, picked from global through the project's flag diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 7a04ca8f37..b80a665a04 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -4,14 +4,22 @@ //! first one that matches, will result in the event being discarded with a [`FilterStatKey`] //! identifying the matching filter. -use crate::{DynamicFiltersConfig, FilterStatKey, GenericFiltersConfig}; +use crate::{DynamicGenericFiltersConfig, FilterStatKey, GenericFiltersConfig}; 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 VERSION: u16 = 1; + +/// Returns whether the given generic config versions are supported. +pub fn are_generic_filters_supported( + global_filters_version: u16, + project_filters_version: u16, +) -> bool { + global_filters_version <= VERSION && project_filters_version <= VERSION +} /// Checks events by patterns in their error messages. fn matches(event: &Event, condition: Option<&RuleCondition>) -> bool { @@ -27,7 +35,7 @@ pub(crate) fn should_filter( global_filters: Option<&GenericFiltersConfig>, ) -> Result<(), FilterStatKey> { let generic_filters_config = - DynamicFiltersConfig::new(project_filters, global_filters, VERSION); + DynamicGenericFiltersConfig::new(project_filters, global_filters, VERSION); let filters = generic_filters_config.into_iter(); for filter_config in filters { diff --git a/relay-filter/src/lib.rs b/relay-filter/src/lib.rs index e8364895b1..22cbcf5acc 100644 --- a/relay-filter/src/lib.rs +++ b/relay-filter/src/lib.rs @@ -37,7 +37,7 @@ pub use crate::common::*; pub use crate::config::*; pub use crate::csp::matches_any_origin; -pub use crate::generic::VERSION as GENERIC_FILTERS_VERSION; +pub use crate::generic::are_generic_filters_supported; /// Checks whether an event should be filtered for a particular configuration. /// diff --git a/relay-server/src/services/processor.rs b/relay-server/src/services/processor.rs index ac21d54949..81c923858a 100644 --- a/relay-server/src/services/processor.rs +++ b/relay-server/src/services/processor.rs @@ -1282,9 +1282,10 @@ impl EnvelopeProcessorService { // 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. - let supported_generic_filters = state.project_state.config.filter_settings.generic.version - <= relay_filter::GENERIC_FILTERS_VERSION - && global_config.generic_filters_version() <= relay_filter::GENERIC_FILTERS_VERSION; + let supported_generic_filters = relay_filter::are_generic_filters_supported( + global_config.generic_filters_version(), + state.project_state.config.filter_settings.generic.version, + ); // 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. let extract_metrics_from_sampling = From b57c12245eaaafd914ab1719e97e42dfd65356b8 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Tue, 27 Feb 2024 16:51:12 +0100 Subject: [PATCH 06/18] more feedback --- relay-dynamic-config/src/global.rs | 7 - relay-filter/src/config.rs | 619 +----------------------- relay-filter/src/generic.rs | 627 ++++++++++++++++++++++++- relay-server/src/services/processor.rs | 2 +- 4 files changed, 626 insertions(+), 629 deletions(-) diff --git a/relay-dynamic-config/src/global.rs b/relay-dynamic-config/src/global.rs index f5cf71d4e7..55b87eb42a 100644 --- a/relay-dynamic-config/src/global.rs +++ b/relay-dynamic-config/src/global.rs @@ -69,13 +69,6 @@ impl GlobalConfig { ErrorBoundary::Ok(f) => Some(f), } } - - /// Returns the version of generic inbound filters. - /// - /// If the filters failed to deserialize, [`u16::MAX`] is returned. - pub fn generic_filters_version(&self) -> u16 { - self.filters().map(|f| f.version).unwrap_or(u16::MAX) - } } /// All options passed down from Sentry to Relay. diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index 3c890ca15e..11f990e460 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -1,10 +1,9 @@ //! Config structs for all filters. use std::borrow::Cow; -use std::collections::{BTreeSet, HashSet}; +use std::collections::BTreeSet; use std::convert::Infallible; use std::fmt; -use std::iter::FusedIterator; use std::str::FromStr; use indexmap::IndexMap; @@ -527,197 +526,10 @@ impl ProjectFiltersConfig { } } -/// Configuration of generic filters combined from project and global config. -/// -/// See [`DynamicFiltersConfigIter`] for details on how to iterate easily -/// through the applicable filters. -#[derive(Copy, Clone, Debug)] -pub(crate) struct DynamicGenericFiltersConfig<'a> { - /// Configuration of generic filters from project configs. - project_filters: &'a GenericFiltersConfig, - /// Configuration of generic filters from global config. - global_filters: Option<&'a GenericFiltersConfig>, - /// Maximum supported version for generic filters. - /// - /// It applies to all filters, from both project and global configs. - supported_version: u16, -} - -impl<'a> DynamicGenericFiltersConfig<'a> { - /// Creates a [`DynamicFiltersConfig`] from the project and global configs. - pub fn new( - project_filters: &'a GenericFiltersConfig, - global_filters: Option<&'a GenericFiltersConfig>, - supported_version: u16, - ) -> Self { - DynamicGenericFiltersConfig { - project_filters, - global_filters, - supported_version, - } - } -} - -impl<'a> IntoIterator for DynamicGenericFiltersConfig<'a> { - type Item = &'a GenericFilterConfig; - type IntoIter = DynamicGenericFiltersConfigIter<'a>; - - fn into_iter(self) -> Self::IntoIter { - DynamicGenericFiltersConfigIter::new(self) - } -} - -/// Iterator over [`GenericFilterConfig`] of a project and global config. -/// -/// 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. -#[derive(Debug)] -pub(crate) struct DynamicGenericFiltersConfigIter<'a> { - /// Configuration of project and global filters. - config: DynamicGenericFiltersConfig<'a>, - /// Index of the next filter in project config to evaluate. - project_index: usize, - /// Index of the next filter in global config to evaluate. - global_index: usize, - /// Filters that have been evaluated, either yielded or ignored. - evaluated: HashSet<&'a str>, -} - -impl<'a> DynamicGenericFiltersConfigIter<'a> { - /// Creates an iterator over the filters in [`DynamicFiltersConfig`]. - pub fn new(config: DynamicGenericFiltersConfig<'a>) -> Self { - DynamicGenericFiltersConfigIter { - config, - project_index: 0, - global_index: 0, - evaluated: HashSet::new(), - } - } -} - -impl<'a> DynamicGenericFiltersConfigIter<'a> { - /// Returns whether the inbound filters support the maximum version. - /// - /// Filters are supported if the versions of filters of both project and - /// global configs are not greater than the given maximum version. - /// - /// Filters from the project and global configs are complementary, and in - /// isolation, they don't provide enough valuable information to perform the - /// filtering. Additionally, new versions may include features not supported - /// in the current Relay. - fn is_version_supported(&self) -> bool { - self.config.project_filters.version <= self.config.supported_version - && self - .config - .global_filters - .map_or(true, |gf| gf.version <= self.config.supported_version) - } - - fn next_project_filter(&mut self) -> Option<&'a GenericFilterConfig> { - let Some((id, filter)) = self - .config - .project_filters - .filters - .get_index(self.project_index) - else { - return None; - }; - self.project_index += 1; - self.evaluated.insert(id); - - if !filter.is_enabled { - return None; - } - - if filter.condition.is_some() { - Some(filter) - } else { - self.config - .global_filters - .and_then(|config| config.filters.get(id)) - .filter(|filter| filter.condition.is_some()) - } - } - - fn next_global_filter(&mut self) -> Option<&'a GenericFilterConfig> { - let Some(global_filters) = self.config.global_filters else { - return None; - }; - let Some((id, filter)) = global_filters.filters.get_index(self.global_index) else { - return None; - }; - self.global_index += 1; - - if filter.is_empty() { - return None; - } - - self.evaluated.insert(id).then_some(filter) - } -} - -impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { - type Item = &'a GenericFilterConfig; - - fn next(&mut self) -> Option { - if !self.is_version_supported() { - return None; - } - - // This loop starts by iterating through all project filters: increments - // `project_index` on every iteration until it grows too much (i.e. all - // project filters are evaluated), yielding the evaluated filter if - // necessary. - // Then, the same approach is applied to global filters. - // `continue` restarts the loop, so global filters aren't evaluated - // until all project filters are. - loop { - if self.project_index < self.config.project_filters.filters.len() { - let filter = self.next_project_filter(); - if filter.is_some() { - return filter; - } - continue; - } - - if let Some(global_filters) = self.config.global_filters { - if self.global_index < global_filters.filters.len() { - let filter = self.next_global_filter(); - if filter.is_some() { - return filter; - } - continue; - } - } - - return None; - } - } -} - -impl<'a> FusedIterator for DynamicGenericFiltersConfigIter<'a> {} - #[cfg(test)] mod tests { use super::*; - use std::iter; - #[test] fn test_empty_config() -> Result<(), serde_json::Error> { let filters_config = serde_json::from_str::("{}")?; @@ -932,433 +744,4 @@ mod tests { } "###); } - - fn empty_filter() -> GenericFiltersConfig { - GenericFiltersConfig { - version: 1, - filters: IndexMap::new(), - } - } - - /// Returns a complete and enabled [`GenericFiltersConfig`]. - fn enabled_filter(id: &str) -> GenericFiltersConfig { - GenericFiltersConfig { - version: 1, - filters: IndexMap::from([( - id.to_owned(), - GenericFilterConfig { - id: id.to_owned(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "myError")), - }, - )]), - } - } - - /// Returns an enabled flag of a [`GenericFiltersConfig`]. - fn enabled_flag_filter(id: &str) -> GenericFiltersConfig { - GenericFiltersConfig { - version: 1, - filters: IndexMap::from([( - id.to_owned(), - GenericFilterConfig { - id: id.to_owned(), - is_enabled: true, - condition: None, - }, - )]), - } - } - - /// Returns a complete but disabled [`GenericFiltersConfig`]. - fn disabled_filter(id: &str) -> GenericFiltersConfig { - GenericFiltersConfig { - version: 1, - filters: IndexMap::from([( - id.to_owned(), - GenericFilterConfig { - id: id.to_owned(), - is_enabled: false, - condition: Some(RuleCondition::eq("event.exceptions", "myError")), - }, - )]), - } - } - - /// Returns a disabled flag of a [`GenericFiltersConfig`]. - fn disabled_flag_filter(id: &str) -> GenericFiltersConfig { - GenericFiltersConfig { - version: 1, - filters: IndexMap::from([( - id.to_owned(), - GenericFilterConfig { - id: id.to_owned(), - is_enabled: false, - condition: None, - }, - )]), - } - } - - macro_rules! assert_filters { - ($combined_filters:expr, $expected:expr) => { - let actual_ids = $combined_filters.into_iter(); - assert!(actual_ids.eq($expected)); - }; - } - - #[test] - fn test_no_combined_filters_on_unsupported_project_version() { - let mut project = enabled_filter("unsupported-project"); - project.version = 2; - let global = enabled_filter("supported-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_filters_on_unsupported_project_version_no_global() { - let mut project = enabled_filter("unsupported-project"); - project.version = 2; - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_filters_on_unsupported_global_version() { - let project = enabled_filter("supported-project"); - let mut global = enabled_filter("unsupported-global"); - global.version = 2; - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_filters_on_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_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_combined_filters_on_supported_project_and_global_version() { - let project = enabled_filter("supported-project"); - let global = enabled_filter("supported-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [ - project.filters.first().unwrap().1, - global.filters.first().unwrap().1 - ] - .into_iter() - ); - } - - #[test] - fn test_no_combined_duplicates_when_both_enabled() { - let project = enabled_filter("filter"); - let global = enabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1,].into_iter() - ); - } - - #[test] - fn test_no_combined_filters_when_no_filters() { - let project = empty_filter(); - let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_when_disabled_global_filters() { - let project = empty_filter(); - let global = disabled_filter("disabled-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_global_combined_when_enabled_global_filters() { - let project = empty_filter(); - let global = enabled_filter("enabled-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [global.filters.first().unwrap().1].into_iter() - ); - } - - #[test] - fn test_no_combined_when_global_is_flag() { - let project = empty_filter(); - let global = enabled_flag_filter("skip"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_when_disabled_project_filters_empty_global() { - let project = disabled_filter("disabled-project"); - let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - #[test] - fn test_no_combined_when_disabled_project_filters_missing_global() { - let project = disabled_filter("disabled-project"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_when_disabled_in_project_and_global() { - let project = disabled_filter("disabled-project"); - let global = disabled_filter("disabled-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_duplicates_when_both_disabled() { - let project = disabled_filter("filter"); - let global = disabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_when_disabled_project_enabled_global() { - let project = disabled_filter("filter"); - let global = enabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_when_enabled_flag_project_empty_global() { - let project = enabled_flag_filter("filter"); - let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_when_enabled_flag_project_missing_global() { - let project = enabled_flag_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_when_disabled_flag_project_empty_global() { - let project = disabled_flag_filter("filter"); - let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_no_combined_when_disabled_flag_project_missing_global() { - let project = disabled_flag_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_project_combined_when_only_project_enabled_empty_global() { - let project = enabled_filter("enabled-project"); - let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1].into_iter() - ); - } - - #[test] - fn test_project_combined_when_only_project_enabled_missing_global() { - let project = enabled_filter("enabled-project"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - [project.filters.first().unwrap().1].into_iter() - ); - } - - #[test] - fn test_project_combined_when_enabled_flag_project_disabled_global() { - let project = enabled_filter("filter"); - let global = disabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1].into_iter() - ); - } - - #[test] - fn test_no_combined_when_disabled_flag_project_disabled_global() { - let project = disabled_flag_filter("filter"); - let global = disabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_project_combined_when_enabled_project_disabled_global() { - let project = enabled_filter("filter"); - let global = disabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1].into_iter() - ); - } - - #[test] - fn test_common_combined_when_enabled_flag_project_enabled_global() { - let project = enabled_flag_filter("filter"); - let global = enabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [global.filters.first().unwrap().1].into_iter() - ); - } - - #[test] - fn test_no_combined_when_disabled_flag_project_enabled_global() { - let project = disabled_flag_filter("filter"); - let global = enabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_common_combined_when_enabled_project_enabled_global() { - let project = enabled_filter("filter"); - let global = enabled_flag_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1].into_iter() - ); - } - - #[test] - fn test_no_combined_when_enabled_flags_project_and_global() { - let project = enabled_flag_filter("filter"); - let global = enabled_flag_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); - } - - #[test] - fn test_multiple_combined_filters() { - let project = GenericFiltersConfig { - version: 1, - filters: IndexMap::from([ - ( - "0".to_owned(), - GenericFilterConfig { - id: "0".to_owned(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "myError")), - }, - ), - ( - "1".to_owned(), - GenericFilterConfig { - id: "1".to_owned(), - is_enabled: true, - condition: None, - }, - ), - ( - "2".to_owned(), - GenericFilterConfig { - id: "2".to_owned(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "myError")), - }, - ), - ( - "not-picked".to_owned(), - GenericFilterConfig { - id: "not-picked".to_owned(), - is_enabled: true, - condition: None, - }, - ), - ]), - }; - let global = GenericFiltersConfig { - version: 1, - filters: IndexMap::from([ - ( - "1".to_owned(), - GenericFilterConfig { - id: "1".to_owned(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "myOtherError")), - }, - ), - ( - "3".to_owned(), - GenericFilterConfig { - id: "3".to_owned(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "myLastError")), - }, - ), - ]), - }; - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [ - &project.filters[0], // id=0, picked from project - &global.filters[0], // id=1, picked from global through the project's flag - &project.filters[2], // id=2, picked from project - &global.filters[1] // id=3, picked from global - ] - .into_iter() - ); - } } diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index b80a665a04..363cb8e967 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -4,7 +4,10 @@ //! first one that matches, will result in the event being discarded with a [`FilterStatKey`] //! identifying the matching filter. -use crate::{DynamicGenericFiltersConfig, FilterStatKey, GenericFiltersConfig}; +use std::collections::HashSet; +use std::iter::FusedIterator; + +use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig}; use relay_event_schema::protocol::Event; use relay_protocol::RuleCondition; @@ -15,10 +18,10 @@ const VERSION: u16 = 1; /// Returns whether the given generic config versions are supported. pub fn are_generic_filters_supported( - global_filters_version: u16, + global_filters_version: Option, project_filters_version: u16, ) -> bool { - global_filters_version <= VERSION && project_filters_version <= VERSION + global_filters_version.map_or(false, |v| v <= VERSION) && project_filters_version <= VERSION } /// Checks events by patterns in their error messages. @@ -47,8 +50,197 @@ pub(crate) fn should_filter( Ok(()) } +/// Configuration of generic filters combined from project and global config. +/// +/// See [`DynamicFiltersConfigIter`] for details on how to iterate easily +/// through the applicable filters. +#[derive(Copy, Clone, Debug)] +struct DynamicGenericFiltersConfig<'a> { + /// Configuration of generic filters from project configs. + project_filters: &'a GenericFiltersConfig, + /// Configuration of generic filters from global config. + global_filters: Option<&'a GenericFiltersConfig>, + /// Maximum supported version for generic filters. + /// + /// It applies to all filters, from both project and global configs. + supported_version: u16, +} + +impl<'a> DynamicGenericFiltersConfig<'a> { + /// Creates a [`DynamicFiltersConfig`] from the project and global configs. + pub fn new( + project_filters: &'a GenericFiltersConfig, + global_filters: Option<&'a GenericFiltersConfig>, + supported_version: u16, + ) -> Self { + DynamicGenericFiltersConfig { + project_filters, + global_filters, + supported_version, + } + } +} + +impl<'a> IntoIterator for DynamicGenericFiltersConfig<'a> { + type Item = &'a GenericFilterConfig; + type IntoIter = DynamicGenericFiltersConfigIter<'a>; + + fn into_iter(self) -> Self::IntoIter { + DynamicGenericFiltersConfigIter::new(self) + } +} + +/// Iterator over [`GenericFilterConfig`] of a project and global config. +/// +/// 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. +#[derive(Debug)] +struct DynamicGenericFiltersConfigIter<'a> { + /// Configuration of project and global filters. + config: DynamicGenericFiltersConfig<'a>, + /// Index of the next filter in project config to evaluate. + project_index: usize, + /// Index of the next filter in global config to evaluate. + global_index: usize, + /// Filters that have been evaluated, either yielded or ignored. + evaluated: HashSet<&'a str>, +} + +impl<'a> DynamicGenericFiltersConfigIter<'a> { + /// Creates an iterator over the filters in [`DynamicFiltersConfig`]. + pub fn new(config: DynamicGenericFiltersConfig<'a>) -> Self { + DynamicGenericFiltersConfigIter { + config, + project_index: 0, + global_index: 0, + evaluated: HashSet::new(), + } + } +} + +impl<'a> DynamicGenericFiltersConfigIter<'a> { + /// Returns whether the inbound filters support the maximum version. + /// + /// Filters are supported if the versions of filters of both project and + /// global configs are not greater than the given maximum version. + /// + /// Filters from the project and global configs are complementary, and in + /// isolation, they don't provide enough valuable information to perform the + /// filtering. Additionally, new versions may include features not supported + /// in the current Relay. + fn is_version_supported(&self) -> bool { + self.config.project_filters.version <= self.config.supported_version + && self + .config + .global_filters + .map_or(true, |gf| gf.version <= self.config.supported_version) + } + + fn next_project_filter(&mut self) -> Option<&'a GenericFilterConfig> { + let Some((id, filter)) = self + .config + .project_filters + .filters + .get_index(self.project_index) + else { + return None; + }; + self.project_index += 1; + self.evaluated.insert(id); + + if !filter.is_enabled { + return None; + } + + if filter.condition.is_some() { + Some(filter) + } else { + self.config + .global_filters + .and_then(|config| config.filters.get(id)) + .filter(|filter| filter.condition.is_some()) + } + } + + fn next_global_filter(&mut self) -> Option<&'a GenericFilterConfig> { + let Some(global_filters) = self.config.global_filters else { + return None; + }; + let Some((id, filter)) = global_filters.filters.get_index(self.global_index) else { + return None; + }; + self.global_index += 1; + + if filter.is_empty() { + return None; + } + + self.evaluated.insert(id).then_some(filter) + } +} + +impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { + type Item = &'a GenericFilterConfig; + + fn next(&mut self) -> Option { + if !self.is_version_supported() { + return None; + } + + // This loop starts by iterating through all project filters: increments + // `project_index` on every iteration until it grows too much (i.e. all + // project filters are evaluated), yielding the evaluated filter if + // necessary. + // Then, the same approach is applied to global filters. + // `continue` restarts the loop, so global filters aren't evaluated + // until all project filters are. + loop { + if self.project_index < self.config.project_filters.filters.len() { + let filter = self.next_project_filter(); + if filter.is_some() { + return filter; + } + continue; + } + + if let Some(global_filters) = self.config.global_filters { + if self.global_index < global_filters.filters.len() { + let filter = self.next_global_filter(); + if filter.is_some() { + return filter; + } + continue; + } + } + + return None; + } + } +} + +impl<'a> FusedIterator for DynamicGenericFiltersConfigIter<'a> {} + #[cfg(test)] mod tests { + use std::iter; + + use super::*; + use crate::generic::{should_filter, VERSION}; use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig}; use indexmap::IndexMap; @@ -195,4 +387,433 @@ mod tests { )) ); } + + fn empty_filter() -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::new(), + } + } + + /// Returns a complete and enabled [`GenericFiltersConfig`]. + fn enabled_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + id.to_owned(), + GenericFilterConfig { + id: id.to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + )]), + } + } + + /// Returns an enabled flag of a [`GenericFiltersConfig`]. + fn enabled_flag_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + id.to_owned(), + GenericFilterConfig { + id: id.to_owned(), + is_enabled: true, + condition: None, + }, + )]), + } + } + + /// Returns a complete but disabled [`GenericFiltersConfig`]. + fn disabled_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + id.to_owned(), + GenericFilterConfig { + id: id.to_owned(), + is_enabled: false, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + )]), + } + } + + /// Returns a disabled flag of a [`GenericFiltersConfig`]. + fn disabled_flag_filter(id: &str) -> GenericFiltersConfig { + GenericFiltersConfig { + version: 1, + filters: IndexMap::from([( + id.to_owned(), + GenericFilterConfig { + id: id.to_owned(), + is_enabled: false, + condition: None, + }, + )]), + } + } + + macro_rules! assert_filters { + ($combined_filters:expr, $expected:expr) => { + let actual_ids = $combined_filters.into_iter(); + assert!(actual_ids.eq($expected)); + }; + } + + #[test] + fn test_no_combined_filters_on_unsupported_project_version() { + let mut project = enabled_filter("unsupported-project"); + project.version = 2; + let global = enabled_filter("supported-global"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_filters_on_unsupported_project_version_no_global() { + let mut project = enabled_filter("unsupported-project"); + project.version = 2; + assert_filters!( + DynamicGenericFiltersConfig::new(&project, None, 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_filters_on_unsupported_global_version() { + let project = enabled_filter("supported-project"); + let mut global = enabled_filter("unsupported-global"); + global.version = 2; + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_filters_on_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_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_combined_filters_on_supported_project_and_global_version() { + let project = enabled_filter("supported-project"); + let global = enabled_filter("supported-global"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [ + project.filters.first().unwrap().1, + global.filters.first().unwrap().1 + ] + .into_iter() + ); + } + + #[test] + fn test_no_combined_duplicates_when_both_enabled() { + let project = enabled_filter("filter"); + let global = enabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1,].into_iter() + ); + } + + #[test] + fn test_no_combined_filters_when_no_filters() { + let project = empty_filter(); + let global = empty_filter(); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_global_filters() { + let project = empty_filter(); + let global = disabled_filter("disabled-global"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_global_combined_when_enabled_global_filters() { + let project = empty_filter(); + let global = enabled_filter("enabled-global"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [global.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_no_combined_when_global_is_flag() { + let project = empty_filter(); + let global = enabled_flag_filter("skip"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_project_filters_empty_global() { + let project = disabled_filter("disabled-project"); + let global = empty_filter(); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + #[test] + fn test_no_combined_when_disabled_project_filters_missing_global() { + let project = disabled_filter("disabled-project"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, None, 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_in_project_and_global() { + let project = disabled_filter("disabled-project"); + let global = disabled_filter("disabled-global"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_duplicates_when_both_disabled() { + let project = disabled_filter("filter"); + let global = disabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_project_enabled_global() { + let project = disabled_filter("filter"); + let global = enabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_enabled_flag_project_empty_global() { + let project = enabled_flag_filter("filter"); + let global = empty_filter(); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_enabled_flag_project_missing_global() { + let project = enabled_flag_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, None, 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_flag_project_empty_global() { + let project = disabled_flag_filter("filter"); + let global = empty_filter(); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_no_combined_when_disabled_flag_project_missing_global() { + let project = disabled_flag_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, None, 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_project_combined_when_only_project_enabled_empty_global() { + let project = enabled_filter("enabled-project"); + let global = empty_filter(); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_project_combined_when_only_project_enabled_missing_global() { + let project = enabled_filter("enabled-project"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, None, 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_project_combined_when_enabled_flag_project_disabled_global() { + let project = enabled_filter("filter"); + let global = disabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_no_combined_when_disabled_flag_project_disabled_global() { + let project = disabled_flag_filter("filter"); + let global = disabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_project_combined_when_enabled_project_disabled_global() { + let project = enabled_filter("filter"); + let global = disabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_common_combined_when_enabled_flag_project_enabled_global() { + let project = enabled_flag_filter("filter"); + let global = enabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [global.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_no_combined_when_disabled_flag_project_enabled_global() { + let project = disabled_flag_filter("filter"); + let global = enabled_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_common_combined_when_enabled_project_enabled_global() { + let project = enabled_filter("filter"); + let global = enabled_flag_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [project.filters.first().unwrap().1].into_iter() + ); + } + + #[test] + fn test_no_combined_when_enabled_flags_project_and_global() { + let project = enabled_flag_filter("filter"); + let global = enabled_flag_filter("filter"); + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + iter::empty::<&GenericFilterConfig>() + ); + } + + #[test] + fn test_multiple_combined_filters() { + let project = GenericFiltersConfig { + version: 1, + filters: IndexMap::from([ + ( + "0".to_owned(), + GenericFilterConfig { + id: "0".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + ), + ( + "1".to_owned(), + GenericFilterConfig { + id: "1".to_owned(), + is_enabled: true, + condition: None, + }, + ), + ( + "2".to_owned(), + GenericFilterConfig { + id: "2".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }, + ), + ( + "not-picked".to_owned(), + GenericFilterConfig { + id: "not-picked".to_owned(), + is_enabled: true, + condition: None, + }, + ), + ]), + }; + let global = GenericFiltersConfig { + version: 1, + filters: IndexMap::from([ + ( + "1".to_owned(), + GenericFilterConfig { + id: "1".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myOtherError")), + }, + ), + ( + "3".to_owned(), + GenericFilterConfig { + id: "3".to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myLastError")), + }, + ), + ]), + }; + assert_filters!( + DynamicGenericFiltersConfig::new(&project, Some(&global), 1), + [ + &project.filters[0], // id=0, picked from project + &global.filters[0], // id=1, picked from global through the project's flag + &project.filters[2], // id=2, picked from project + &global.filters[1] // id=3, picked from global + ] + .into_iter() + ); + } } diff --git a/relay-server/src/services/processor.rs b/relay-server/src/services/processor.rs index 81c923858a..a70dff2c33 100644 --- a/relay-server/src/services/processor.rs +++ b/relay-server/src/services/processor.rs @@ -1283,7 +1283,7 @@ impl EnvelopeProcessorService { // need to drop the event, and there should not be metrics from dropped // events. let supported_generic_filters = relay_filter::are_generic_filters_supported( - global_config.generic_filters_version(), + global_config.filters().map(|f| f.version), state.project_state.config.filter_settings.generic.version, ); // We avoid extracting metrics if we are not sampling the event while in non-processing From 6fafec00db4803bd6fd93ac0a21962053a972f96 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Tue, 27 Feb 2024 17:00:51 +0100 Subject: [PATCH 07/18] fix doc links --- relay-filter/src/generic.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 363cb8e967..371b82fb56 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -52,7 +52,7 @@ pub(crate) fn should_filter( /// Configuration of generic filters combined from project and global config. /// -/// See [`DynamicFiltersConfigIter`] for details on how to iterate easily +/// See [`DynamicGenericFiltersConfigIter`] for details on how to iterate easily /// through the applicable filters. #[derive(Copy, Clone, Debug)] struct DynamicGenericFiltersConfig<'a> { @@ -67,7 +67,7 @@ struct DynamicGenericFiltersConfig<'a> { } impl<'a> DynamicGenericFiltersConfig<'a> { - /// Creates a [`DynamicFiltersConfig`] from the project and global configs. + /// Creates a [`DynamicGenericFiltersConfig`] from the project and global configs. pub fn new( project_filters: &'a GenericFiltersConfig, global_filters: Option<&'a GenericFiltersConfig>, @@ -122,7 +122,7 @@ struct DynamicGenericFiltersConfigIter<'a> { } impl<'a> DynamicGenericFiltersConfigIter<'a> { - /// Creates an iterator over the filters in [`DynamicFiltersConfig`]. + /// Creates an iterator over the filters in [`DynamicGenericFiltersConfig`]. pub fn new(config: DynamicGenericFiltersConfig<'a>) -> Self { DynamicGenericFiltersConfigIter { config, From 83848978c571ce5ad9a82cb47064a3bebc318a2a Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Tue, 27 Feb 2024 21:24:05 +0100 Subject: [PATCH 08/18] some more feedback --- relay-filter/src/config.rs | 19 +++++++++++++++++++ relay-server/src/services/processor.rs | 11 +++++++---- relay-server/src/services/processor/event.rs | 13 +++++++------ 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index 11f990e460..e9c1d6499a 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -336,6 +336,24 @@ impl PartialEq for GenericFilterConfig { /// "#); /// ``` /// +/// 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: {}, +/// } +/// "#); +/// ``` +/// /// Serialization: /// /// ``` @@ -399,6 +417,7 @@ pub struct GenericFiltersConfig { /// 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 = "IndexMap::is_empty", with = "generic_filters_custom_serialization" )] diff --git a/relay-server/src/services/processor.rs b/relay-server/src/services/processor.rs index a70dff2c33..8838e06fb2 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, self.inner.global_config.current().filters())?; + 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,8 +1274,7 @@ impl EnvelopeProcessorService { event::finalize(state, &self.inner.config)?; self.light_normalize_event(state)?; dynamic_sampling::normalize(state); - let global_config = self.inner.global_config.current(); - event::filter(state, global_config.filters())?; + event::filter(state, self.inner.global_config.current())?; dynamic_sampling::run(state, &self.inner.config); // Don't extract metrics if relay can't apply generic inbound filters. @@ -1283,7 +1282,11 @@ impl EnvelopeProcessorService { // need to drop the event, and there should not be metrics from dropped // events. let supported_generic_filters = relay_filter::are_generic_filters_supported( - global_config.filters().map(|f| f.version), + self.inner + .global_config + .current() + .filters() + .map(|f| f.version), state.project_state.config.filter_settings.generic.version, ); // We avoid extracting metrics if we are not sampling the event while in non-processing diff --git a/relay-server/src/services/processor/event.rs b/relay-server/src/services/processor/event.rs index 648d810bdf..2fa2351ea7 100644 --- a/relay-server/src/services/processor/event.rs +++ b/relay-server/src/services/processor/event.rs @@ -1,13 +1,14 @@ //! Event processor related code. use std::error::Error; +use std::sync::Arc; use chrono::Duration as SignedDuration; 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::{ @@ -29,6 +30,7 @@ use { use crate::envelope::{AttachmentType, ContentType, Item, ItemType}; use crate::extractors::RequestMeta; +use crate::services::global_config::GlobalConfigHandle; use crate::services::outcome::Outcome; use crate::services::processor::{ EventProcessing, ExtractedEvent, ProcessEnvelopeState, ProcessingError, MINIMUM_CLOCK_DRIFT, @@ -274,7 +276,7 @@ pub fn finalize( pub fn filter( state: &mut ProcessEnvelopeState, - global_filters: Option<&GenericFiltersConfig>, + global_config: Arc, ) -> Result<(), ProcessingError> { let event = match state.event.value_mut() { Some(event) => event, @@ -287,14 +289,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, global_filters).map_err( - |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) - }, - ) + }) }) } From 8aee088ed760c654a4eac09493d7bd19c52878de Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Tue, 27 Feb 2024 21:29:53 +0100 Subject: [PATCH 09/18] fix lint :) --- relay-server/src/services/processor/event.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/relay-server/src/services/processor/event.rs b/relay-server/src/services/processor/event.rs index 2fa2351ea7..2bfe039b7d 100644 --- a/relay-server/src/services/processor/event.rs +++ b/relay-server/src/services/processor/event.rs @@ -15,7 +15,6 @@ use relay_event_schema::protocol::{ Breadcrumb, Csp, Event, ExpectCt, ExpectStaple, Hpkp, LenientString, NetworkReportError, OtelContext, RelayInfo, SecurityReportType, Timestamp, Values, }; -use relay_filter::GenericFiltersConfig; use relay_pii::PiiProcessor; use relay_protocol::{Annotated, Array, FromValue, Object, Value}; use relay_quotas::DataCategory; @@ -30,7 +29,6 @@ use { use crate::envelope::{AttachmentType, ContentType, Item, ItemType}; use crate::extractors::RequestMeta; -use crate::services::global_config::GlobalConfigHandle; use crate::services::outcome::Outcome; use crate::services::processor::{ EventProcessing, ExtractedEvent, ProcessEnvelopeState, ProcessingError, MINIMUM_CLOCK_DRIFT, From a6db146fd2cbf1c653f989bbb05790e425e8023d Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 29 Feb 2024 09:35:29 +0100 Subject: [PATCH 10/18] simplify iterator --- relay-dynamic-config/src/global.rs | 23 +- relay-filter/src/generic.rs | 633 +++++++++---------- relay-filter/src/lib.rs | 2 - relay-server/src/services/processor.rs | 16 +- relay-server/src/services/processor/event.rs | 19 +- 5 files changed, 324 insertions(+), 369 deletions(-) diff --git a/relay-dynamic-config/src/global.rs b/relay-dynamic-config/src/global.rs index 55b87eb42a..af0aea2179 100644 --- a/relay-dynamic-config/src/global.rs +++ b/relay-dynamic-config/src/global.rs @@ -3,7 +3,6 @@ use std::fs::File; use std::io::BufReader; use std::path::Path; -use crate::ErrorBoundary; use relay_base_schema::metrics::MetricNamespace; use relay_event_normalization::MeasurementsConfig; use relay_filter::GenericFiltersConfig; @@ -29,8 +28,11 @@ pub struct GlobalConfig { /// /// These filters are merged with generic filters in project configs before /// applying. - #[serde(skip_serializing_if = "skip_generic_filters")] - pub filters: ErrorBoundary, + #[serde( + deserialize_with = "default_on_error", + skip_serializing_if = "Option::is_none" + )] + pub filters: Option, /// Sentry options passed down to Relay. #[serde( deserialize_with = "default_on_error", @@ -39,13 +41,6 @@ pub struct GlobalConfig { pub options: Options, } -fn skip_generic_filters(filters_config: &ErrorBoundary) -> bool { - match filters_config { - ErrorBoundary::Err(_) => true, - ErrorBoundary::Ok(config) => config.version == 0 && config.filters.is_empty(), - } -} - impl GlobalConfig { /// Loads the [`GlobalConfig`] from a file if it's provided. /// @@ -61,14 +56,6 @@ 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), - } - } } /// All options passed down from Sentry to Relay. diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 371b82fb56..16f2ce226e 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -4,10 +4,10 @@ //! first one that matches, will result in the event being discarded with a [`FilterStatKey`] //! identifying the matching filter. -use std::collections::HashSet; use std::iter::FusedIterator; use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig}; +use indexmap::IndexMap; use relay_event_schema::protocol::Event; use relay_protocol::RuleCondition; @@ -16,14 +16,6 @@ use relay_protocol::RuleCondition; /// If the version in the project config is higher, no generic filters are applied. const VERSION: u16 = 1; -/// 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(false, |v| v <= VERSION) && project_filters_version <= VERSION -} - /// Checks events by patterns in their error messages. fn matches(event: &Event, condition: Option<&RuleCondition>) -> bool { // TODO: the condition DSL needs to be extended to support more complex semantics, such as @@ -37,207 +29,146 @@ pub(crate) fn should_filter( project_filters: &GenericFiltersConfig, global_filters: Option<&GenericFiltersConfig>, ) -> Result<(), FilterStatKey> { - let generic_filters_config = - DynamicGenericFiltersConfig::new(project_filters, global_filters, VERSION); - let filters = generic_filters_config.into_iter(); + let filters = merge_generic_filters( + project_filters, + global_filters, + #[cfg(test)] + VERSION, + ); for filter_config in filters { - if matches(event, filter_config.condition.as_ref()) { - return Err(FilterStatKey::GenericFilter(filter_config.id.clone())); + if filter_config.is_enabled && matches(event, filter_config.condition) { + return Err(FilterStatKey::GenericFilter(filter_config.id.to_owned())); } } Ok(()) } -/// Configuration of generic filters combined from project and global config. -/// -/// See [`DynamicGenericFiltersConfigIter`] for details on how to iterate easily -/// through the applicable filters. -#[derive(Copy, Clone, Debug)] -struct DynamicGenericFiltersConfig<'a> { - /// Configuration of generic filters from project configs. - project_filters: &'a GenericFiltersConfig, - /// Configuration of generic filters from global config. - global_filters: Option<&'a GenericFiltersConfig>, - /// Maximum supported version for generic filters. - /// - /// It applies to all filters, from both project and global configs. - supported_version: u16, -} - -impl<'a> DynamicGenericFiltersConfig<'a> { - /// Creates a [`DynamicGenericFiltersConfig`] from the project and global configs. - pub fn new( - project_filters: &'a GenericFiltersConfig, - global_filters: Option<&'a GenericFiltersConfig>, - supported_version: u16, - ) -> Self { - DynamicGenericFiltersConfig { - project_filters, - global_filters, - supported_version, - } - } -} - -impl<'a> IntoIterator for DynamicGenericFiltersConfig<'a> { - type Item = &'a GenericFilterConfig; - type IntoIter = DynamicGenericFiltersConfigIter<'a>; - - fn into_iter(self) -> Self::IntoIter { - DynamicGenericFiltersConfigIter::new(self) - } -} - -/// Iterator over [`GenericFilterConfig`] of a project and global config. +/// Returns an iterator that yields merged generic configs. /// -/// Iterates in order through the generic filters in project configs and global -/// configs yielding the filters according to the principles below: +/// Since filters of project and global configs are complementary and don't +/// provide much value in isolation, both versions must match [`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. -/// - 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. -#[derive(Debug)] +/// - 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)] version: u16, +) -> impl Iterator> { + #[cfg(not(test))] + let version = VERSION; + + let is_supported = + project.version <= version && global.map_or(true, |gf| gf.version <= 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> { - /// Configuration of project and global filters. - config: DynamicGenericFiltersConfig<'a>, - /// Index of the next filter in project config to evaluate. + /// Generic project filters. + project: &'a IndexMap, + /// Index of the next filter in project configs to evaluate. project_index: usize, - /// Index of the next filter in global config to evaluate. + /// Generic global filters. + global: Option<&'a IndexMap>, + /// Index of the next filter in global configs to evaluate. global_index: usize, - /// Filters that have been evaluated, either yielded or ignored. - evaluated: HashSet<&'a str>, } impl<'a> DynamicGenericFiltersConfigIter<'a> { - /// Creates an iterator over the filters in [`DynamicGenericFiltersConfig`]. - pub fn new(config: DynamicGenericFiltersConfig<'a>) -> Self { + pub fn new( + project: &'a IndexMap, + global: Option<&'a IndexMap>, + ) -> Self { DynamicGenericFiltersConfigIter { - config, + project, project_index: 0, + global, global_index: 0, - evaluated: HashSet::new(), } } } -impl<'a> DynamicGenericFiltersConfigIter<'a> { - /// Returns whether the inbound filters support the maximum version. - /// - /// Filters are supported if the versions of filters of both project and - /// global configs are not greater than the given maximum version. - /// - /// Filters from the project and global configs are complementary, and in - /// isolation, they don't provide enough valuable information to perform the - /// filtering. Additionally, new versions may include features not supported - /// in the current Relay. - fn is_version_supported(&self) -> bool { - self.config.project_filters.version <= self.config.supported_version - && self - .config - .global_filters - .map_or(true, |gf| gf.version <= self.config.supported_version) - } - - fn next_project_filter(&mut self) -> Option<&'a GenericFilterConfig> { - let Some((id, filter)) = self - .config - .project_filters - .filters - .get_index(self.project_index) - else { - return None; - }; - self.project_index += 1; - self.evaluated.insert(id); +impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { + type Item = MergedFilter<'a>; - if !filter.is_enabled { - return None; + 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); } - if filter.condition.is_some() { - Some(filter) - } else { - self.config - .global_filters - .and_then(|config| config.filters.get(id)) - .filter(|filter| filter.condition.is_some()) + loop { + let Some((id, filter)) = self + .global + .and_then(|filters| filters.get_index(self.global_index)) + else { + return None; + }; + self.global_index += 1; + if !self.project.contains_key(id) { + return Some(filter.into()); + } } } +} - fn next_global_filter(&mut self) -> Option<&'a GenericFilterConfig> { - let Some(global_filters) = self.config.global_filters else { - return None; - }; - let Some((id, filter)) = global_filters.filters.get_index(self.global_index) else { - return None; - }; - self.global_index += 1; - - if filter.is_empty() { - return None; - } +impl<'a> FusedIterator for DynamicGenericFiltersConfigIter<'a> {} - self.evaluated.insert(id).then_some(filter) +/// 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>, +) -> MergedFilter<'a> { + MergedFilter { + id: primary.id.as_str(), + is_enabled: primary.is_enabled, + condition: primary + .condition + .as_ref() + .or(secondary.and_then(|filter| filter.condition.as_ref())), } } -impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { - type Item = &'a GenericFilterConfig; - - fn next(&mut self) -> Option { - if !self.is_version_supported() { - return None; - } - - // This loop starts by iterating through all project filters: increments - // `project_index` on every iteration until it grows too much (i.e. all - // project filters are evaluated), yielding the evaluated filter if - // necessary. - // Then, the same approach is applied to global filters. - // `continue` restarts the loop, so global filters aren't evaluated - // until all project filters are. - loop { - if self.project_index < self.config.project_filters.filters.len() { - let filter = self.next_project_filter(); - if filter.is_some() { - return filter; - } - continue; - } - - if let Some(global_filters) = self.config.global_filters { - if self.global_index < global_filters.filters.len() { - let filter = self.next_global_filter(); - if filter.is_some() { - return filter; - } - continue; - } - } +#[derive(Debug, Default, PartialEq)] +struct MergedFilter<'a> { + id: &'a str, + is_enabled: bool, + condition: Option<&'a RuleCondition>, +} - return None; +impl<'a> From<&'a GenericFilterConfig> for MergedFilter<'a> { + fn from(value: &'a GenericFilterConfig) -> Self { + MergedFilter { + id: value.id.as_str(), + is_enabled: value.is_enabled, + condition: value.condition.as_ref(), } } } -impl<'a> FusedIterator for DynamicGenericFiltersConfigIter<'a> {} - #[cfg(test)] mod tests { - use std::iter; use super::*; @@ -455,293 +386,342 @@ mod tests { } } - macro_rules! assert_filters { - ($combined_filters:expr, $expected:expr) => { - let actual_ids = $combined_filters.into_iter(); - assert!(actual_ids.eq($expected)); - }; - } - #[test] - fn test_no_combined_filters_on_unsupported_project_version() { + 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_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq(None.into_iter())); } #[test] - fn test_no_combined_filters_on_unsupported_project_version_no_global() { + fn test_no_combined_when_unsupported_project_version_no_global() { let mut project = enabled_filter("unsupported-project"); project.version = 2; - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, None, 1).eq(None.into_iter())); } #[test] - fn test_no_combined_filters_on_unsupported_global_version() { + 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_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq(None.into_iter())); } #[test] - fn test_no_combined_filters_on_unsupported_project_and_global_version() { + 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_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq(None.into_iter())); } #[test] - fn test_combined_filters_on_supported_project_and_global_version() { + fn test_both_combined_when_supported_project_and_global_version() { let project = enabled_filter("supported-project"); let global = enabled_filter("supported-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [ - project.filters.first().unwrap().1, - global.filters.first().unwrap().1 - ] - .into_iter() - ); + 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_no_combined_duplicates_when_both_enabled() { + fn test_project_combined_when_duplicated_filter_project_and_global() { let project = enabled_filter("filter"); let global = enabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1,].into_iter() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) ); } #[test] - fn test_no_combined_filters_when_no_filters() { + fn test_no_combined_when_empty_project_and_global() { let project = empty_filter(); let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq(None.into_iter())); } #[test] - fn test_no_combined_when_disabled_global_filters() { + fn test_global_combined_when_empty_project_disabled_global_filter() { let project = empty_filter(); let global = disabled_filter("disabled-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); } #[test] - fn test_global_combined_when_enabled_global_filters() { + fn test_global_combined_when_empty_project_enabled_global_filters() { let project = empty_filter(); let global = enabled_filter("enabled-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [global.filters.first().unwrap().1].into_iter() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); } #[test] - fn test_no_combined_when_global_is_flag() { + fn test_global_combined_when_empty_project_enabled_flag_global() { let project = empty_filter(); let global = enabled_flag_filter("skip"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()])); } #[test] - fn test_no_combined_when_disabled_project_filters_empty_global() { + fn test_project_combined_when_disabled_project_empty_global() { let project = disabled_filter("disabled-project"); let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) ); } + #[test] - fn test_no_combined_when_disabled_project_filters_missing_global() { + fn test_project_combined_when_disabled_project_missing_global() { let project = disabled_filter("disabled-project"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, None, 1).eq([project + .filters + .first() + .unwrap() + .1 + .into(),] + .into_iter())); } #[test] - fn test_no_combined_when_disabled_in_project_and_global() { + fn test_both_combined_when_different_disabled_project_and_global() { let project = disabled_filter("disabled-project"); let global = disabled_filter("disabled-global"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([ + project.filters.first().unwrap().1.into(), + global.filters.first().unwrap().1.into() + ])); } #[test] - fn test_no_combined_duplicates_when_both_disabled() { + fn test_project_combined_duplicated_disabled_project_and_global() { let project = disabled_filter("filter"); let global = disabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()]) ); } #[test] - fn test_no_combined_when_disabled_project_enabled_global() { + fn test_merged_combined_when_disabled_project_enabled_global() { let project = disabled_filter("filter"); let global = enabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + 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_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) ); } #[test] - fn test_no_combined_when_enabled_flag_project_missing_global() { + fn test_project_combined_when_enabled_flag_project_missing_global() { let project = enabled_flag_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, None, 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); } #[test] - fn test_no_combined_when_disabled_flag_project_empty_global() { + fn test_project_combined_when_disabled_flag_project_empty_global() { let project = disabled_flag_filter("filter"); let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()]) ); } #[test] - fn test_no_combined_when_disabled_flag_project_missing_global() { + fn test_project_combined_when_disabled_flag_project_missing_global() { let project = disabled_flag_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - iter::empty::<&GenericFilterConfig>() - ); + assert!(merge_generic_filters(&project, None, 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()])); } #[test] - fn test_project_combined_when_only_project_enabled_empty_global() { + fn test_project_combined_when_enabled_project_empty_global() { let project = enabled_filter("enabled-project"); let global = empty_filter(); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1].into_iter() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) ); } #[test] - fn test_project_combined_when_only_project_enabled_missing_global() { + fn test_project_combined_when_enabled_project_missing_global() { let project = enabled_filter("enabled-project"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, None, 1), - [project.filters.first().unwrap().1].into_iter() - ); + assert!(merge_generic_filters(&project, None, 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); } #[test] - fn test_project_combined_when_enabled_flag_project_disabled_global() { - let project = enabled_filter("filter"); + fn test_merged_combined_when_enabled_flag_project_disabled_global() { + let project = enabled_flag_filter("filter"); let global = disabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1].into_iter() - ); + 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_no_combined_when_disabled_flag_project_disabled_global() { + fn test_global_combined_when_disabled_flag_project_disabled_global() { let project = disabled_flag_filter("filter"); let global = disabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + 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_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1].into_iter() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) ); } #[test] - fn test_common_combined_when_enabled_flag_project_enabled_global() { + fn test_global_combined_when_enabled_flag_project_enabled_global() { let project = enabled_flag_filter("filter"); let global = enabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [global.filters.first().unwrap().1].into_iter() - ); + assert!(merge_generic_filters(&project, Some(&global), 1).eq([global + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter())); } #[test] - fn test_no_combined_when_disabled_flag_project_enabled_global() { + fn test_merged_combined_when_disabled_flag_project_enabled_global() { let project = disabled_flag_filter("filter"); let global = enabled_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() - ); + 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_common_combined_when_enabled_project_enabled_global() { + fn test_project_combined_when_enabled_project_enabled_flag_global() { let project = enabled_filter("filter"); let global = enabled_flag_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [project.filters.first().unwrap().1].into_iter() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) ); } #[test] - fn test_no_combined_when_enabled_flags_project_and_global() { + fn test_project_combined_when_enabled_flags_project_and_global() { let project = enabled_flag_filter("filter"); let global = enabled_flag_filter("filter"); - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - iter::empty::<&GenericFilterConfig>() + assert!( + merge_generic_filters(&project, Some(&global), 1).eq([project + .filters + .first() + .unwrap() + .1 + .into()] + .into_iter()) ); } @@ -774,14 +754,6 @@ mod tests { condition: Some(RuleCondition::eq("event.exceptions", "myError")), }, ), - ( - "not-picked".to_owned(), - GenericFilterConfig { - id: "not-picked".to_owned(), - is_enabled: true, - condition: None, - }, - ), ]), }; let global = GenericFiltersConfig { @@ -791,7 +763,7 @@ mod tests { "1".to_owned(), GenericFilterConfig { id: "1".to_owned(), - is_enabled: true, + is_enabled: false, condition: Some(RuleCondition::eq("event.exceptions", "myOtherError")), }, ), @@ -799,21 +771,28 @@ mod tests { "3".to_owned(), GenericFilterConfig { id: "3".to_owned(), - is_enabled: true, + is_enabled: false, condition: Some(RuleCondition::eq("event.exceptions", "myLastError")), }, ), ]), }; - assert_filters!( - DynamicGenericFiltersConfig::new(&project, Some(&global), 1), - [ - &project.filters[0], // id=0, picked from project - &global.filters[0], // id=1, picked from global through the project's flag - &project.filters[2], // id=2, picked from project - &global.filters[1] // id=3, picked from global - ] - .into_iter() - ); + + 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 22cbcf5acc..9f092d609a 100644 --- a/relay-filter/src/lib.rs +++ b/relay-filter/src/lib.rs @@ -37,8 +37,6 @@ 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. diff --git a/relay-server/src/services/processor.rs b/relay-server/src/services/processor.rs index 8838e06fb2..674d854641 100644 --- a/relay-server/src/services/processor.rs +++ b/relay-server/src/services/processor.rs @@ -1277,23 +1277,9 @@ impl EnvelopeProcessorService { event::filter(state, self.inner.global_config.current())?; dynamic_sampling::run(state, &self.inner.config); - // 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. - let supported_generic_filters = relay_filter::are_generic_filters_supported( - self.inner - .global_config - .current() - .filters() - .map(|f| f.version), - state.project_state.config.filter_settings.generic.version, - ); // 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. - let extract_metrics_from_sampling = - self.inner.config.processing_enabled() || state.sampling_result.should_drop(); - if supported_generic_filters && extract_metrics_from_sampling { + if self.inner.config.processing_enabled() || state.sampling_result.should_drop() { self.extract_metrics(state)?; } diff --git a/relay-server/src/services/processor/event.rs b/relay-server/src/services/processor/event.rs index 2bfe039b7d..a871a03496 100644 --- a/relay-server/src/services/processor/event.rs +++ b/relay-server/src/services/processor/event.rs @@ -287,13 +287,18 @@ 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, global_config.filters()) - .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.as_ref(), + ) + .map_err(|err| { + state + .managed_envelope + .reject(Outcome::Filtered(err.clone())); + ProcessingError::EventFiltered(err) + }) }) } From d7790eab2cb3ba91d9269a1bddb685314db7b632 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 29 Feb 2024 11:51:17 +0100 Subject: [PATCH 11/18] some more feedback --- relay-dynamic-config/src/global.rs | 24 +- relay-filter/src/config.rs | 172 +++++++------- relay-filter/src/generic.rs | 226 ++++++++----------- relay-server/src/services/processor/event.rs | 19 +- tests/integration/test_metrics.py | 119 ---------- 5 files changed, 216 insertions(+), 344 deletions(-) diff --git a/relay-dynamic-config/src/global.rs b/relay-dynamic-config/src/global.rs index af0aea2179..8e98de5b04 100644 --- a/relay-dynamic-config/src/global.rs +++ b/relay-dynamic-config/src/global.rs @@ -10,6 +10,8 @@ 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 @@ -28,11 +30,8 @@ pub struct GlobalConfig { /// /// These filters are merged with generic filters in project configs before /// applying. - #[serde( - deserialize_with = "default_on_error", - skip_serializing_if = "Option::is_none" - )] - pub filters: Option, + #[serde(skip_serializing_if = "skip_generic_filters")] + pub filters: ErrorBoundary, /// Sentry options passed down to Relay. #[serde( deserialize_with = "default_on_error", @@ -56,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 skip_generic_filters(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. diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index e9c1d6499a..e575322509 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -4,6 +4,7 @@ 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; @@ -233,7 +234,7 @@ 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 struct GenericFilterConfig { /// Unique identifier of the generic filter. @@ -251,15 +252,6 @@ impl GenericFilterConfig { } } -#[cfg(test)] -impl PartialEq for GenericFilterConfig { - fn eq(&self, other: &Self) -> bool { - self.id == other.id - && self.is_enabled == other.is_enabled - && self.condition == other.condition - } -} - /// Configuration for generic filters. /// /// # Deserialization @@ -269,10 +261,10 @@ impl PartialEq for GenericFilterConfig { /// Two filters are considered duplicates if they have the same ID, /// independently of the condition. /// -/// The list of filters is deserialized into an [`IndexMap`], 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. +/// 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 /// @@ -325,13 +317,15 @@ impl PartialEq for GenericFilterConfig { /// assert_debug_snapshot!(deserialized, @r#" /// GenericFiltersConfig { /// version: 1, -/// filters: { -/// "filter1": GenericFilterConfig { -/// id: "filter1", -/// is_enabled: false, -/// condition: None, +/// filters: GenericFiltersMap( +/// { +/// "filter1": GenericFilterConfig { +/// id: "filter1", +/// is_enabled: false, +/// condition: None, +/// }, /// }, -/// }, +/// ), /// } /// "#); /// ``` @@ -349,7 +343,9 @@ impl PartialEq for GenericFilterConfig { /// assert_debug_snapshot!(deserialized, @r#" /// GenericFiltersConfig { /// version: 1, -/// filters: {}, +/// filters: GenericFiltersMap( +/// {}, +/// ), /// } /// "#); /// ``` @@ -360,18 +356,16 @@ impl PartialEq for GenericFilterConfig { /// # use relay_filter::{GenericFiltersConfig, GenericFilterConfig}; /// # use relay_protocol::condition::RuleCondition; /// # use insta::assert_display_snapshot; -/// # use indexmap::IndexMap; /// /// let filter = GenericFiltersConfig { /// version: 1, -/// filters: IndexMap::from([( -/// "filter1".to_owned(), +/// 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#"{ @@ -393,14 +387,13 @@ impl PartialEq for GenericFilterConfig { /// Serialization of filters is skipped if there aren't any: /// /// ``` -/// # use relay_filter::{GenericFiltersConfig, GenericFilterConfig}; +/// # use relay_filter::{GenericFiltersConfig, GenericFilterConfig, GenericFiltersMap}; /// # use relay_protocol::condition::RuleCondition; /// # use insta::assert_display_snapshot; -/// # use indexmap::IndexMap; /// /// let filter = GenericFiltersConfig { /// version: 1, -/// filters: IndexMap::new(), +/// filters: GenericFiltersMap::new(), /// }; /// let serialized = serde_json::to_string_pretty(&filter).unwrap(); /// assert_display_snapshot!(serialized, @r#"{ @@ -416,34 +409,60 @@ pub struct GenericFiltersConfig { /// /// 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 = "IndexMap::is_empty", - with = "generic_filters_custom_serialization" - )] - pub filters: IndexMap, + #[serde(default, skip_serializing_if = "GenericFiltersMap::is_empty")] + pub filters: GenericFiltersMap, } impl GenericFiltersConfig { /// Returns true if the filters are not declared. pub fn is_empty(&self) -> bool { - self.filters.is_empty() + self.filters.0.is_empty() } } -mod generic_filters_custom_serialization { - use super::*; +/// 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 { + map.insert(filter.id.clone(), filter.clone()); + } + GenericFiltersMap(map) + } +} + +impl Deref for GenericFiltersMap { + type Target = IndexMap; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} - pub fn deserialize<'de, D>( - deserializer: D, - ) -> Result, D::Error> +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 = IndexMap; + type Value = GenericFiltersMap; fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { formatter.write_str("a vector of filters: Vec") @@ -459,22 +478,21 @@ mod generic_filters_custom_serialization { filters.insert(filter.id.clone(), filter); } } - Ok(filters) + Ok(GenericFiltersMap(filters)) } } deserializer.deserialize_seq(GenericFiltersVisitor) } +} - pub fn serialize( - index_map: &IndexMap, - serializer: S, - ) -> Result +impl Serialize for GenericFiltersMap { + fn serialize(&self, serializer: S) -> Result where S: Serializer, { - let mut seq = serializer.serialize_seq(Some(index_map.len()))?; - for filter in index_map.values() { + let mut seq = serializer.serialize_seq(Some(self.0.len()))?; + for filter in self.0.values() { seq.serialize_element(filter)?; } seq.end() @@ -585,7 +603,9 @@ mod tests { }, generic: GenericFiltersConfig { version: 0, - filters: {}, + filters: GenericFiltersMap( + {}, + ), }, } "###); @@ -629,14 +649,12 @@ mod tests { }, generic: GenericFiltersConfig { version: 1, - filters: IndexMap::from([( - "hydrationError".to_owned(), - GenericFilterConfig { - id: "hydrationError".to_string(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "HydrationError")), - }, - )]), + filters: vec![GenericFilterConfig { + id: "hydrationError".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "HydrationError")), + }] + .into(), }, }; @@ -738,28 +756,30 @@ mod tests { insta::assert_debug_snapshot!(config, @r###" GenericFiltersConfig { version: 1, - filters: { - "hydrationError": 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, + }, + "chunkLoadError": GenericFilterConfig { + id: "chunkLoadError", + is_enabled: false, + condition: None, + }, }, - }, + ), } "###); } diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 16f2ce226e..34ffab8339 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -6,8 +6,7 @@ use std::iter::FusedIterator; -use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig}; -use indexmap::IndexMap; +use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig, GenericFiltersMap}; use relay_event_schema::protocol::Event; use relay_protocol::RuleCondition; @@ -64,7 +63,7 @@ fn merge_generic_filters<'a>( project: &'a GenericFiltersConfig, global: Option<&'a GenericFiltersConfig>, #[cfg(test)] version: u16, -) -> impl Iterator> { +) -> impl Iterator> { #[cfg(not(test))] let version = VERSION; @@ -82,20 +81,17 @@ fn merge_generic_filters<'a>( /// Iterator over the generic filters of the project and global configs. struct DynamicGenericFiltersConfigIter<'a> { /// Generic project filters. - project: &'a IndexMap, + project: &'a GenericFiltersMap, /// Index of the next filter in project configs to evaluate. project_index: usize, /// Generic global filters. - global: Option<&'a IndexMap>, + 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 IndexMap, - global: Option<&'a IndexMap>, - ) -> Self { + pub fn new(project: &'a GenericFiltersMap, global: Option<&'a GenericFiltersMap>) -> Self { DynamicGenericFiltersConfigIter { project, project_index: 0, @@ -106,7 +102,7 @@ impl<'a> DynamicGenericFiltersConfigIter<'a> { } impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { - type Item = MergedFilter<'a>; + type Item = GenericFilterConfigRef<'a>; fn next(&mut self) -> Option { if let Some((id, filter)) = self.project.get_index(self.project_index) { @@ -116,10 +112,7 @@ impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { } loop { - let Some((id, filter)) = self - .global - .and_then(|filters| filters.get_index(self.global_index)) - else { + let Some((id, filter)) = self.global?.get_index(self.global_index) else { return None; }; self.global_index += 1; @@ -139,8 +132,8 @@ impl<'a> FusedIterator for DynamicGenericFiltersConfigIter<'a> {} fn merge_filters<'a>( primary: &'a GenericFilterConfig, secondary: Option<&'a GenericFilterConfig>, -) -> MergedFilter<'a> { - MergedFilter { +) -> GenericFilterConfigRef<'a> { + GenericFilterConfigRef { id: primary.id.as_str(), is_enabled: primary.is_enabled, condition: primary @@ -151,15 +144,15 @@ fn merge_filters<'a>( } #[derive(Debug, Default, PartialEq)] -struct MergedFilter<'a> { +struct GenericFilterConfigRef<'a> { id: &'a str, is_enabled: bool, condition: Option<&'a RuleCondition>, } -impl<'a> From<&'a GenericFilterConfig> for MergedFilter<'a> { +impl<'a> From<&'a GenericFilterConfig> for GenericFilterConfigRef<'a> { fn from(value: &'a GenericFilterConfig) -> Self { - MergedFilter { + GenericFilterConfigRef { id: value.id.as_str(), is_enabled: value.is_enabled, condition: value.condition.as_ref(), @@ -174,30 +167,24 @@ mod tests { use crate::generic::{should_filter, VERSION}; use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig}; - use indexmap::IndexMap; use relay_event_schema::protocol::{Event, LenientString}; use relay_protocol::Annotated; use relay_protocol::RuleCondition; - fn mock_filters() -> IndexMap { - IndexMap::from([ - ( - "firstReleases".to_owned(), - GenericFilterConfig { - id: "firstReleases".to_string(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.release", "1.0")), - }, - ), - ( - "helloTransactions".to_owned(), - GenericFilterConfig { - id: "helloTransactions".to_string(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.transaction", "/hello")), - }, - ), - ]) + fn mock_filters() -> GenericFiltersMap { + vec![ + GenericFilterConfig { + id: "firstReleases".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.release", "1.0")), + }, + GenericFilterConfig { + id: "helloTransactions".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.transaction", "/hello")), + }, + ] + .into() } #[test] @@ -284,26 +271,22 @@ mod tests { fn test_should_filter_from_global_filters() { let project = GenericFiltersConfig { version: 1, - filters: IndexMap::from([( - "firstReleases".to_owned(), - GenericFilterConfig { - id: "firstReleases".to_string(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.release", "1.0")), - }, - )]), + 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: IndexMap::from([( - "helloTransactions".to_owned(), - GenericFilterConfig { - id: "helloTransactions".to_string(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.transaction", "/hello")), - }, - )]), + filters: vec![GenericFilterConfig { + id: "helloTransactions".to_string(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.transaction", "/hello")), + }] + .into(), }; let event = Event { @@ -322,7 +305,7 @@ mod tests { fn empty_filter() -> GenericFiltersConfig { GenericFiltersConfig { version: 1, - filters: IndexMap::new(), + filters: GenericFiltersMap::new(), } } @@ -330,14 +313,12 @@ mod tests { fn enabled_filter(id: &str) -> GenericFiltersConfig { GenericFiltersConfig { version: 1, - filters: IndexMap::from([( - id.to_owned(), - GenericFilterConfig { - id: id.to_owned(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "myError")), - }, - )]), + filters: vec![GenericFilterConfig { + id: id.to_owned(), + is_enabled: true, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }] + .into(), } } @@ -345,14 +326,12 @@ mod tests { fn enabled_flag_filter(id: &str) -> GenericFiltersConfig { GenericFiltersConfig { version: 1, - filters: IndexMap::from([( - id.to_owned(), - GenericFilterConfig { - id: id.to_owned(), - is_enabled: true, - condition: None, - }, - )]), + filters: vec![GenericFilterConfig { + id: id.to_owned(), + is_enabled: true, + condition: None, + }] + .into(), } } @@ -360,14 +339,12 @@ mod tests { fn disabled_filter(id: &str) -> GenericFiltersConfig { GenericFiltersConfig { version: 1, - filters: IndexMap::from([( - id.to_owned(), - GenericFilterConfig { - id: id.to_owned(), - is_enabled: false, - condition: Some(RuleCondition::eq("event.exceptions", "myError")), - }, - )]), + filters: vec![GenericFilterConfig { + id: id.to_owned(), + is_enabled: false, + condition: Some(RuleCondition::eq("event.exceptions", "myError")), + }] + .into(), } } @@ -375,14 +352,12 @@ mod tests { fn disabled_flag_filter(id: &str) -> GenericFiltersConfig { GenericFiltersConfig { version: 1, - filters: IndexMap::from([( - id.to_owned(), - GenericFilterConfig { - id: id.to_owned(), - is_enabled: false, - condition: None, - }, - )]), + filters: vec![GenericFilterConfig { + id: id.to_owned(), + is_enabled: false, + condition: None, + }] + .into(), } } @@ -729,53 +704,40 @@ mod tests { fn test_multiple_combined_filters() { let project = GenericFiltersConfig { version: 1, - filters: IndexMap::from([ - ( - "0".to_owned(), - GenericFilterConfig { - id: "0".to_owned(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "myError")), - }, - ), - ( - "1".to_owned(), - GenericFilterConfig { - id: "1".to_owned(), - is_enabled: true, - condition: None, - }, - ), - ( - "2".to_owned(), - GenericFilterConfig { - id: "2".to_owned(), - is_enabled: true, - condition: Some(RuleCondition::eq("event.exceptions", "myError")), - }, - ), - ]), + 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: IndexMap::from([ - ( - "1".to_owned(), - GenericFilterConfig { - id: "1".to_owned(), - is_enabled: false, - condition: Some(RuleCondition::eq("event.exceptions", "myOtherError")), - }, - ), - ( - "3".to_owned(), - GenericFilterConfig { - id: "3".to_owned(), - is_enabled: false, - condition: Some(RuleCondition::eq("event.exceptions", "myLastError")), - }, - ), - ]), + 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]; diff --git a/relay-server/src/services/processor/event.rs b/relay-server/src/services/processor/event.rs index 7677b5cbc6..13ce47a8d5 100644 --- a/relay-server/src/services/processor/event.rs +++ b/relay-server/src/services/processor/event.rs @@ -287,18 +287,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, - global_config.filters.as_ref(), - ) - .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/test_metrics.py b/tests/integration/test_metrics.py index c121391c89..4d07b47201 100644 --- a/tests/integration/test_metrics.py +++ b/tests/integration/test_metrics.py @@ -1,4 +1,3 @@ -from time import sleep import hashlib from collections import defaultdict from datetime import datetime, timedelta, timezone @@ -1879,121 +1878,3 @@ def test_metric_bucket_encoding_dynamic_global_config_option( else: assert metrics[dname]["value"] == [1337.0] assert metrics[sname]["value"] == [42.0] - - -def test_relay_forwards_events_without_extracting_metrics_on_broken_global_filters( - mini_sentry, - relay_with_processing, - transactions_consumer, - metrics_consumer, -): - metrics_consumer = metrics_consumer() - tx_consumer = transactions_consumer() - - mini_sentry.global_config["filters"] = { - "version": "Halp! I'm broken!", - "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 - sleep(1) - metrics_consumer.assert_empty() - - -def test_relay_forwards_events_without_extracting_metrics_on_unsupported_project_filters( - mini_sentry, - relay_with_processing, - transactions_consumer, - metrics_consumer, -): - 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, - } - - 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 - sleep(1) - metrics_consumer.assert_empty() - - -def test_missing_global_filters_enables_metric_extraction( - mini_sentry, - relay_with_processing, - transactions_consumer, - metrics_consumer, -): - metrics_consumer = metrics_consumer() - transactions_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) - - transaction, _ = transactions_consumer.get_event() - assert transaction is not None - metrics = metrics_consumer.get_metrics() - assert metrics is not None From 791ac3ac00bcf799a31bd0a073191bf0e1f279ed Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 29 Feb 2024 12:00:31 +0100 Subject: [PATCH 12/18] missing checks on From --- relay-filter/src/config.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index e575322509..00628fb65a 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -440,7 +440,9 @@ impl From> for GenericFiltersMap { fn from(filters: Vec) -> Self { let mut map = IndexMap::with_capacity(filters.len()); for filter in filters { - map.insert(filter.id.clone(), filter.clone()); + if !map.contains_key(&filter.id) { + map.insert(filter.id.clone(), filter); + } } GenericFiltersMap(map) } From bfb8657364cfc5fa9739b625bf72f78aec987d20 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 29 Feb 2024 15:03:24 +0100 Subject: [PATCH 13/18] disable metrics on unsupported/broken generic global filters --- Cargo.toml | 2 +- relay-filter/src/config.rs | 2 +- relay-filter/src/generic.rs | 12 +- relay-filter/src/lib.rs | 2 + relay-server/src/services/processor.rs | 43 +++-- relay-server/src/services/processor/event.rs | 3 +- tests/integration/test_metrics.py | 156 +++++++++++++++++++ 7 files changed, 200 insertions(+), 20 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c55acfe4c9..089be82147 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,8 +25,8 @@ 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" -itertools = "0.10.5" indexmap = "2.0.0" +itertools = "0.10.5" once_cell = "1.13.1" parking_lot = "0.12.1" rand = "0.8.5" diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index 00628fb65a..a03ae78471 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -416,7 +416,7 @@ pub struct GenericFiltersConfig { impl GenericFiltersConfig { /// Returns true if the filters are not declared. pub fn is_empty(&self) -> bool { - self.filters.0.is_empty() + self.filters.is_empty() } } diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 34ffab8339..9bb38a78cb 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -15,6 +15,14 @@ use relay_protocol::RuleCondition; /// If the version in the project config is higher, no generic filters are applied. const VERSION: u16 = 1; +/// 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 <= VERSION) && project_filters_version <= VERSION +} + /// Checks events by patterns in their error messages. fn matches(event: &Event, condition: Option<&RuleCondition>) -> bool { // TODO: the condition DSL needs to be extended to support more complex semantics, such as @@ -112,9 +120,7 @@ impl<'a> Iterator for DynamicGenericFiltersConfigIter<'a> { } loop { - let Some((id, filter)) = self.global?.get_index(self.global_index) else { - return None; - }; + let (id, filter) = self.global?.get_index(self.global_index)?; self.global_index += 1; if !self.project.contains_key(id) { return Some(filter.into()); diff --git a/relay-filter/src/lib.rs b/relay-filter/src/lib.rs index 9f092d609a..22cbcf5acc 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. diff --git a/relay-server/src/services/processor.rs b/relay-server/src/services/processor.rs index 3050c377da..13e5f1a420 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, self.inner.global_config.current())?; + 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,20 +1274,37 @@ impl EnvelopeProcessorService { event::finalize(state, &self.inner.config)?; self.light_normalize_event(state)?; dynamic_sampling::normalize(state); - event::filter(state, self.inner.global_config.current())?; - 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, + ); - // 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() || state.sampling_result.should_drop() { - self.extract_metrics(state)?; - } + if self.inner.config.processing_enabled() || supported_generic_filters { + dynamic_sampling::run(state, &self.inner.config); - 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() || state.sampling_result.should_drop() { + self.extract_metrics(state)?; + } + + dynamic_sampling::sample_envelope_items( + state, + &self.inner.config, + &self.inner.global_config.current(), + ); + } metric!( timer(RelayTimers::TransactionProcessingAfterDynamicSampling), diff --git a/relay-server/src/services/processor/event.rs b/relay-server/src/services/processor/event.rs index 13ce47a8d5..699063f807 100644 --- a/relay-server/src/services/processor/event.rs +++ b/relay-server/src/services/processor/event.rs @@ -1,7 +1,6 @@ //! Event processor related code. use std::error::Error; -use std::sync::Arc; use chrono::Duration as SignedDuration; use once_cell::sync::OnceCell; @@ -274,7 +273,7 @@ pub fn finalize( pub fn filter( state: &mut ProcessEnvelopeState, - global_config: Arc, + global_config: &GlobalConfig, ) -> Result<(), ProcessingError> { let event = match state.event.value_mut() { Some(event) => event, diff --git a/tests/integration/test_metrics.py b/tests/integration/test_metrics.py index 4d07b47201..362cf345f1 100644 --- a/tests/integration/test_metrics.py +++ b/tests/integration/test_metrics.py @@ -1878,3 +1878,159 @@ 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)) +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, +): + metrics_consumer = metrics_consumer() + tx_consumer = transactions_consumer() + + mini_sentry.global_config["filters"] = { + "version": "Halp! I'm broken!", + "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 + else: + assert mini_sentry.captured_events.get() is not None + metrics_consumer.assert_empty() + + +@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 + else: + assert mini_sentry.captured_events.get() is not None + metrics_consumer.assert_empty() + + +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() From e47c90074c5f6dadebb5839c79e0e5e6a9ad0208 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 29 Feb 2024 15:18:46 +0100 Subject: [PATCH 14/18] add more tests --- tests/integration/test_dynamic_sampling.py | 23 ++++++++++++++++++++++ tests/integration/test_metrics.py | 21 ++++++++++++++++---- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_dynamic_sampling.py b/tests/integration/test_dynamic_sampling.py index 83aaa62633..aef44a6e62 100644 --- a/tests/integration/test_dynamic_sampling.py +++ b/tests/integration/test_dynamic_sampling.py @@ -831,3 +831,26 @@ def get_profile_payload(transaction): }, "version": "1", } + + +def test_invalid_global_generic_filters_skip_dynamic_sampling(mini_sentry, relay): + project_id = 42 + relay = relay(mini_sentry, _outcomes_enabled_config()) + + mini_sentry.global_config["filters"] = { + "generic": { + "version": 65535, # u16::MAX + "filters": [], + } + } + + config = mini_sentry.add_basic_project_config(project_id) + public_key = config["publicKeys"][0]["publicKey"] + + # Reject all transactions with dynamic sampling + _add_sampling_config(config, sample_rate=0.0, rule_type="transaction") + + envelope, _, _ = _create_transaction_envelope(public_key) + + relay.send_envelope(project_id, envelope) + assert mini_sentry.captured_events.get(timeout=1) diff --git a/tests/integration/test_metrics.py b/tests/integration/test_metrics.py index 362cf345f1..fbd8d7e644 100644 --- a/tests/integration/test_metrics.py +++ b/tests/integration/test_metrics.py @@ -1881,6 +1881,21 @@ def test_metric_bucket_encoding_dynamic_global_config_option( @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, @@ -1888,14 +1903,12 @@ def test_relay_forwards_events_without_extracting_metrics_on_broken_global_filte transactions_consumer, metrics_consumer, is_processing_relay, + global_generic_filters, ): metrics_consumer = metrics_consumer() tx_consumer = transactions_consumer() - mini_sentry.global_config["filters"] = { - "version": "Halp! I'm broken!", - "filters": [], - } + mini_sentry.global_config["filters"] = global_generic_filters project_id = 42 mini_sentry.add_full_project_config(project_id) From eb784c792214bde6a4c093393a393763f4804979 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 29 Feb 2024 15:42:13 +0100 Subject: [PATCH 15/18] fix test --- tests/integration/test_dynamic_sampling.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/integration/test_dynamic_sampling.py b/tests/integration/test_dynamic_sampling.py index aef44a6e62..2e9ea1193f 100644 --- a/tests/integration/test_dynamic_sampling.py +++ b/tests/integration/test_dynamic_sampling.py @@ -834,23 +834,24 @@ def get_profile_payload(transaction): def test_invalid_global_generic_filters_skip_dynamic_sampling(mini_sentry, relay): - project_id = 42 - relay = relay(mini_sentry, _outcomes_enabled_config()) - mini_sentry.global_config["filters"] = { - "generic": { - "version": 65535, # u16::MAX - "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.0, rule_type="transaction") + _add_sampling_config(config, sample_rate=0.000001, rule_type="transaction") - envelope, _, _ = _create_transaction_envelope(public_key) + envelope, _, _ = _create_transaction_envelope( + public_key, client_sample_rate=0.000001 + ) relay.send_envelope(project_id, envelope) assert mini_sentry.captured_events.get(timeout=1) From 7454814b496e0986a9c777a5b52b980ba9a4265d Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 29 Feb 2024 15:44:04 +0100 Subject: [PATCH 16/18] sample-rate-test 0 --- tests/integration/test_dynamic_sampling.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_dynamic_sampling.py b/tests/integration/test_dynamic_sampling.py index 2e9ea1193f..05af8204a2 100644 --- a/tests/integration/test_dynamic_sampling.py +++ b/tests/integration/test_dynamic_sampling.py @@ -847,11 +847,9 @@ def test_invalid_global_generic_filters_skip_dynamic_sampling(mini_sentry, relay public_key = config["publicKeys"][0]["publicKey"] # Reject all transactions with dynamic sampling - _add_sampling_config(config, sample_rate=0.000001, rule_type="transaction") + _add_sampling_config(config, sample_rate=0, rule_type="transaction") - envelope, _, _ = _create_transaction_envelope( - public_key, client_sample_rate=0.000001 - ) + envelope, _, _ = _create_transaction_envelope(public_key, client_sample_rate=0) relay.send_envelope(project_id, envelope) assert mini_sentry.captured_events.get(timeout=1) From 28ed597458dcf483fb76e8baa287b00b7a5c682b Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Fri, 1 Mar 2024 12:19:33 +0100 Subject: [PATCH 17/18] fix tests --- relay-dynamic-config/src/global.rs | 4 ++-- relay-filter/src/config.rs | 2 +- relay-filter/src/generic.rs | 19 ++++++++++--------- tests/integration/test_metrics.py | 16 +++++++++++----- 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/relay-dynamic-config/src/global.rs b/relay-dynamic-config/src/global.rs index 8e98de5b04..68e3af7505 100644 --- a/relay-dynamic-config/src/global.rs +++ b/relay-dynamic-config/src/global.rs @@ -30,7 +30,7 @@ pub struct GlobalConfig { /// /// These filters are merged with generic filters in project configs before /// applying. - #[serde(skip_serializing_if = "skip_generic_filters")] + #[serde(skip_serializing_if = "is_err_or_empty")] pub filters: ErrorBoundary, /// Sentry options passed down to Relay. #[serde( @@ -65,7 +65,7 @@ impl GlobalConfig { } } -fn skip_generic_filters(filters_config: &ErrorBoundary) -> bool { +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(), diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index a03ae78471..219f54ae3a 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -256,7 +256,7 @@ impl GenericFilterConfig { /// /// # Deserialization /// -/// `filters` is expected to be a [`Vec`]. +/// `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. diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 9bb38a78cb..7f4077cfb7 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -13,14 +13,15 @@ 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. -const VERSION: u16 = 1; +const MAX_SUPPORTED_VERSION: u16 = 1; /// 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 <= VERSION) && project_filters_version <= VERSION + 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. @@ -40,7 +41,7 @@ pub(crate) fn should_filter( project_filters, global_filters, #[cfg(test)] - VERSION, + MAX_SUPPORTED_VERSION, ); for filter_config in filters { @@ -70,13 +71,13 @@ pub(crate) fn should_filter( fn merge_generic_filters<'a>( project: &'a GenericFiltersConfig, global: Option<&'a GenericFiltersConfig>, - #[cfg(test)] version: u16, + #[cfg(test)] max_supported_version: u16, ) -> impl Iterator> { #[cfg(not(test))] - let version = VERSION; + let max_supported_version = MAX_SUPPORTED_VERSION; - let is_supported = - project.version <= version && global.map_or(true, |gf| gf.version <= version); + let is_supported = project.version <= max_supported_version + && global.map_or(true, |gf| gf.version <= max_supported_version); is_supported .then(|| { @@ -171,7 +172,7 @@ mod tests { use super::*; - use crate::generic::{should_filter, VERSION}; + use crate::generic::{should_filter, MAX_SUPPORTED_VERSION}; use crate::{FilterStatKey, GenericFilterConfig, GenericFiltersConfig}; use relay_event_schema::protocol::{Event, LenientString}; use relay_protocol::Annotated; @@ -261,7 +262,7 @@ mod tests { 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(), }; diff --git a/tests/integration/test_metrics.py b/tests/integration/test_metrics.py index fbd8d7e644..0234d1e333 100644 --- a/tests/integration/test_metrics.py +++ b/tests/integration/test_metrics.py @@ -1891,7 +1891,7 @@ def test_metric_bucket_encoding_dynamic_global_config_option( }, # Config is valid, but filters aren't supported { - "version": "65535", + "version": 65535, "filters": [], }, ], @@ -1947,9 +1947,12 @@ def test_relay_forwards_events_without_extracting_metrics_on_broken_global_filte 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() is not None - metrics_consumer.assert_empty() + 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)) @@ -2007,9 +2010,12 @@ def test_relay_forwards_events_without_extracting_metrics_on_unsupported_project 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() is not None - metrics_consumer.assert_empty() + 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( From 93dfac923c9de8aed20a56a3425a49a9fbd5bc94 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Fri, 1 Mar 2024 12:25:51 +0100 Subject: [PATCH 18/18] fix docs --- relay-filter/src/config.rs | 2 +- relay-filter/src/generic.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/relay-filter/src/config.rs b/relay-filter/src/config.rs index 219f54ae3a..1ea40b4497 100644 --- a/relay-filter/src/config.rs +++ b/relay-filter/src/config.rs @@ -256,7 +256,7 @@ impl GenericFilterConfig { /// /// # Deserialization /// -/// `filters` is expected to be a sequence of [`GenericfilterConfig`]. +/// `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. diff --git a/relay-filter/src/generic.rs b/relay-filter/src/generic.rs index 7f4077cfb7..15200f1c81 100644 --- a/relay-filter/src/generic.rs +++ b/relay-filter/src/generic.rs @@ -56,8 +56,9 @@ pub(crate) fn should_filter( /// 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 [`VERSION`] to be -/// compatible. If filters aren't compatible, an empty iterator is returned. +/// 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: