From faad766a135c925acc27be7ee7442a2d448956fa Mon Sep 17 00:00:00 2001 From: David Herberth Date: Mon, 22 Jan 2024 14:45:17 +0100 Subject: [PATCH] feat(cardinality): Add cardinality limits config to project config (#2977) --- Cargo.lock | 3 +++ relay-cardinality/Cargo.toml | 2 ++ relay-cardinality/src/config.rs | 35 ++++++++++++++++++++++++++++ relay-cardinality/src/lib.rs | 2 ++ relay-cardinality/src/window.rs | 4 +++- relay-dynamic-config/Cargo.toml | 1 + relay-dynamic-config/src/metrics.rs | 27 ++++++++++++--------- relay-server/src/services/project.rs | 28 +++++++++++----------- 8 files changed, 76 insertions(+), 26 deletions(-) create mode 100644 relay-cardinality/src/config.rs diff --git a/Cargo.lock b/Cargo.lock index 01b2f506f2..544730cfb8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3491,9 +3491,11 @@ version = "24.1.0" dependencies = [ "criterion", "hashbrown 0.13.2", + "relay-base-schema", "relay-log", "relay-redis", "relay-statsd", + "serde", "thiserror", ] @@ -3569,6 +3571,7 @@ dependencies = [ "insta", "relay-auth", "relay-base-schema", + "relay-cardinality", "relay-common", "relay-event-normalization", "relay-filter", diff --git a/relay-cardinality/Cargo.toml b/relay-cardinality/Cargo.toml index 5f3cbbc26d..8d4c0b11e8 100644 --- a/relay-cardinality/Cargo.toml +++ b/relay-cardinality/Cargo.toml @@ -16,9 +16,11 @@ redis = ["relay-redis/impl"] [dependencies] hashbrown = { workspace = true } +relay-base-schema = { path = "../relay-base-schema" } relay-log = { path = "../relay-log" } relay-redis = { path = "../relay-redis" } relay-statsd = { path = "../relay-statsd" } +serde = { workspace = true } thiserror = { workspace = true } [dev-dependencies] diff --git a/relay-cardinality/src/config.rs b/relay-cardinality/src/config.rs new file mode 100644 index 0000000000..87dcbf30d3 --- /dev/null +++ b/relay-cardinality/src/config.rs @@ -0,0 +1,35 @@ +use relay_base_schema::metrics::MetricNamespace; +use serde::{Deserialize, Serialize}; + +use crate::SlidingWindow; + +/// A cardinality limit. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Deserialize, Serialize)] +pub struct CardinalityLimit { + /// Unique identifier of the cardinality limit. + pub id: String, + /// The sliding window to enforce the cardinality limits in. + pub window: SlidingWindow, + /// The cardinality limit. + pub limit: u64, + + /// Scope which the limit applies to. + pub scope: CardinalityScope, + /// Metric namespace the limit applies to. + /// + /// No namespace means this specific limit is enforced across all namespaces. + pub namespace: Option, +} + +/// A scope to restrict the [`CardinalityLimit`] to. +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Deserialize, Serialize)] +pub enum CardinalityScope { + /// The organization that this project belongs to. + /// + /// This is the top-level scope. + Organization, + + /// Any other scope that is not known by this Relay. + #[serde(other)] + Unknown, +} diff --git a/relay-cardinality/src/lib.rs b/relay-cardinality/src/lib.rs index cbbdfc4e05..972385f462 100644 --- a/relay-cardinality/src/lib.rs +++ b/relay-cardinality/src/lib.rs @@ -8,6 +8,7 @@ #[cfg(feature = "redis")] mod cache; +mod config; mod error; pub mod limiter; #[cfg(feature = "redis")] @@ -17,6 +18,7 @@ mod statsd; mod utils; mod window; +pub use self::config::*; pub use self::error::*; pub use self::limiter::{ CardinalityItem, CardinalityLimits, CardinalityScope, Config as CardinalityLimiterConfig, diff --git a/relay-cardinality/src/window.rs b/relay-cardinality/src/window.rs index 3d8265d897..753b8728dd 100644 --- a/relay-cardinality/src/window.rs +++ b/relay-cardinality/src/window.rs @@ -1,5 +1,7 @@ +use serde::{Deserialize, Serialize}; + /// A sliding window. -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Deserialize, Serialize)] pub struct SlidingWindow { /// The number of seconds to apply the limit to. pub window_seconds: u64, diff --git a/relay-dynamic-config/Cargo.toml b/relay-dynamic-config/Cargo.toml index 64e4d90add..53d4fe4816 100644 --- a/relay-dynamic-config/Cargo.toml +++ b/relay-dynamic-config/Cargo.toml @@ -17,6 +17,7 @@ anyhow = { workspace = true } assert-json-diff = "2.0.2" relay-auth = { path = "../relay-auth" } relay-base-schema = { path = "../relay-base-schema" } +relay-cardinality = { path = "../relay-cardinality" } relay-common = { path = "../relay-common" } relay-filter = { path = "../relay-filter" } relay-event-normalization = { path = "../relay-event-normalization" } diff --git a/relay-dynamic-config/src/metrics.rs b/relay-dynamic-config/src/metrics.rs index fd9927a7f3..acbfd2f155 100644 --- a/relay-dynamic-config/src/metrics.rs +++ b/relay-dynamic-config/src/metrics.rs @@ -3,6 +3,7 @@ use std::collections::BTreeSet; use relay_base_schema::data_category::DataCategory; +use relay_cardinality::CardinalityLimit; use relay_common::glob2::LazyGlob; use relay_common::glob3::GlobPatterns; use relay_protocol::RuleCondition; @@ -11,24 +12,21 @@ use serde::{Deserialize, Serialize}; use crate::project::ProjectConfig; /// Configuration for metrics filtering. -#[derive(Debug, Default, Clone, Serialize, Deserialize)] +#[derive(Debug, Default, Clone, Serialize, Deserialize, PartialEq)] #[serde(default, rename_all = "camelCase")] pub struct Metrics { + /// List of cardinality limits to enforce for this project. + #[serde(skip_serializing_if = "Vec::is_empty")] + pub cardinality_limits: Vec, /// List of patterns for blocking metrics based on their name. + #[serde(skip_serializing_if = "GlobPatterns::is_empty")] pub denied_names: GlobPatterns, } impl Metrics { - /// Returns a new instance of [`Metrics`]. - pub fn new(patterns: Vec) -> Self { - Self { - denied_names: GlobPatterns::new(patterns), - } - } - - /// Returns `true` if there are no blocked metrics. + /// Returns `true` if there are no changes to the metrics config. pub fn is_empty(&self) -> bool { - self.denied_names.is_empty() + self.cardinality_limits.is_empty() && self.denied_names.is_empty() } } @@ -53,7 +51,7 @@ const SESSION_EXTRACT_VERSION: u16 = 3; const EXTRACT_ABNORMAL_MECHANISM_VERSION: u16 = 2; /// Configuration for metric extraction from sessions. -#[derive(Debug, Clone, Copy, Default, serde::Deserialize, serde::Serialize)] +#[derive(Debug, Clone, Copy, Default, Deserialize, Serialize)] #[serde(default, rename_all = "camelCase")] pub struct SessionMetricsConfig { /// The revision of the extraction algorithm. @@ -505,6 +503,13 @@ mod tests { use super::*; use similar_asserts::assert_eq; + #[test] + fn test_empty_metrics_deserialize() { + let m: Metrics = serde_json::from_str("{}").unwrap(); + assert!(m.is_empty()); + assert_eq!(m, Metrics::default()); + } + #[test] fn parse_tag_spec_value() { let json = r#"{"key":"foo","value":"bar"}"#; diff --git a/relay-server/src/services/project.rs b/relay-server/src/services/project.rs index c976f39d3e..4514269846 100644 --- a/relay-server/src/services/project.rs +++ b/relay-server/src/services/project.rs @@ -1181,6 +1181,7 @@ impl Project { mod tests { use std::sync::{Arc, Mutex}; + use relay_common::glob3::GlobPatterns; use relay_common::time::UnixTimestamp; use relay_metrics::BucketValue; use relay_test::mock_service; @@ -1474,15 +1475,14 @@ mod tests { .into() } - fn apply_pattern_to_names<'a>( - names: Vec<&str>, - patterns: impl AsRef<[&'a str]>, - ) -> Vec { - let mut buckets = get_test_buckets(&names); - let patterns: Vec = patterns.as_ref().iter().map(|s| (*s).to_owned()).collect(); - let deny_list = Metrics::new(patterns); + fn apply_pattern_to_names(names: Vec<&str>, patterns: &[&str]) -> Vec { + let metrics = Metrics { + denied_names: GlobPatterns::new(patterns.iter().map(|&s| s.to_owned()).collect()), + ..Default::default() + }; - Project::apply_metrics_deny_list(&ErrorBoundary::Ok(deny_list), &mut buckets); + let mut buckets = get_test_buckets(&names); + Project::apply_metrics_deny_list(&ErrorBoundary::Ok(metrics), &mut buckets); buckets.into_iter().map(|bucket| bucket.name).collect() } @@ -1490,7 +1490,7 @@ mod tests { fn test_metric_deny_list_exact() { let names = get_test_bucket_names(); let input_qty = names.len(); - let remaining_names = apply_pattern_to_names(names, ["endpoint.parallel_requests"]); + let remaining_names = apply_pattern_to_names(names, &["endpoint.parallel_requests"]); // There's 1 bucket with that exact name. let buckets_to_remove = 1; @@ -1502,7 +1502,7 @@ mod tests { fn test_metric_deny_list_end_glob() { let names = get_test_bucket_names(); let input_qty = names.len(); - let remaining_names = apply_pattern_to_names(names, ["*foo"]); + let remaining_names = apply_pattern_to_names(names, &["*foo"]); // There's 1 bucket name with 'foo' in the end. let buckets_to_remove = 1; @@ -1514,7 +1514,7 @@ mod tests { fn test_metric_deny_list_middle_glob() { let names = get_test_bucket_names(); let input_qty = names.len(); - let remaining_names = apply_pattern_to_names(names, ["*foo*"]); + let remaining_names = apply_pattern_to_names(names, &["*foo*"]); // There's 4 bucket names with 'foo' in the middle, and one with foo in the end. let buckets_to_remove = 5; @@ -1526,7 +1526,7 @@ mod tests { fn test_metric_deny_list_beginning_glob() { let names = get_test_bucket_names(); let input_qty = names.len(); - let remaining_names = apply_pattern_to_names(names, ["endpoint*"]); + let remaining_names = apply_pattern_to_names(names, &["endpoint*"]); // There's 4 buckets starting with "endpoint". let buckets_to_remove = 4; @@ -1537,7 +1537,7 @@ mod tests { #[test] fn test_metric_deny_list_everything() { let names = get_test_bucket_names(); - let remaining_names = apply_pattern_to_names(names, ["*"]); + let remaining_names = apply_pattern_to_names(names, &["*"]); assert_eq!(remaining_names.len(), 0); } @@ -1546,7 +1546,7 @@ mod tests { fn test_metric_deny_list_multiple() { let names = get_test_bucket_names(); let input_qty = names.len(); - let remaining_names = apply_pattern_to_names(names, ["endpoint*", "*transactions*"]); + let remaining_names = apply_pattern_to_names(names, &["endpoint*", "*transactions*"]); let endpoint_buckets = 4; let transaction_buckets = 3;