diff --git a/CHANGELOG.md b/CHANGELOG.md index 5468965ec92..46ba4c0ae2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Internal**: +- Enforce span limits for transactions and vice versa. ([#4963](https://github.com/getsentry/relay/pull/4963)) - Emit outcomes for skipped large attachments on playstation crashes. ([#4862](https://github.com/getsentry/relay/pull/4862)) - Disable span metrics. ([#4931](https://github.com/getsentry/relay/pull/4931),[#4955](https://github.com/getsentry/relay/pull/4955)) diff --git a/relay-quotas/src/rate_limit.rs b/relay-quotas/src/rate_limit.rs index f19a70812d2..1ab286f4654 100644 --- a/relay-quotas/src/rate_limit.rs +++ b/relay-quotas/src/rate_limit.rs @@ -3,6 +3,7 @@ use std::str::FromStr; use std::sync::{Arc, Mutex, PoisonError}; use std::time::{Duration, Instant}; +use relay_base_schema::data_category::DataCategory; use relay_base_schema::metrics::MetricNamespace; use relay_base_schema::organization::OrganizationId; use relay_base_schema::project::{ProjectId, ProjectKey}; @@ -482,8 +483,31 @@ impl CachedRateLimits { let mut inner = self.0.lock().unwrap_or_else(PoisonError::into_inner); let current = Arc::make_mut(&mut inner); - for limit in limits { - current.add(limit) + for mut limit in limits { + // To spice it up, we do have some special casing here for 'inherited categories', + // e.g. spans and transactions. + // + // The tldr; is, as transactions are just containers for spans, + // we can enforce span limits on transactions but also vice versa. + // + // So this is largely an enforcement problem, but since Relay propagates + // rate limits to clients, we clone the limits with the inherited category. + // This ensures old SDKs rate limit correctly, but also it simplifies client + // implementations. Only Relay needs to make this decision. + for i in 0..limit.categories.len() { + let Some(category) = limit.categories.get(i) else { + debug_assert!(false, "logical error"); + break; + }; + + for inherited in inherited_categories(category) { + if !limit.categories.contains(inherited) { + limit.categories.push(*inherited); + } + } + } + + current.add(limit); } } @@ -499,6 +523,23 @@ impl CachedRateLimits { } } +/// Returns inherited rate limit categories for the passed category. +/// +/// When a rate limit for a category can also be enforced in a different category, +/// then it's an inherited category. +/// +/// For example, a transaction rate limit can also be applied to spans and vice versa. +/// +/// For a detailed explanation on span/transaction enforcement see: +/// . +fn inherited_categories(category: &DataCategory) -> &'static [DataCategory] { + match category { + DataCategory::Transaction => &[DataCategory::Span], + DataCategory::Span => &[DataCategory::Transaction], + _ => &[], + } +} + #[cfg(test)] mod tests { use smallvec::smallvec; diff --git a/tests/integration/test_store.py b/tests/integration/test_store.py index 459221b82b1..a5dfb80fbc4 100644 --- a/tests/integration/test_store.py +++ b/tests/integration/test_store.py @@ -468,7 +468,11 @@ def transform(e): assert int(retry_after) <= max_rate_limit retry_after2, rest = headers["x-sentry-rate-limits"].split(":", 1) assert int(retry_after2) == int(retry_after) - assert rest == "%s:key:get_lost" % category + if event_type == "transaction": + # Transaction limits also apply to spans. + assert rest == "transaction;span:key:get_lost" + else: + assert rest == "%s:key:get_lost" % category if outcomes_consumer is not None: outcomes_consumer.assert_rate_limited( "get_lost", key_id=key_id, categories=[category]