From e7d40a35a7a8a35b6c162b19161f387abfac122f Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Wed, 30 Oct 2024 16:53:45 +0100 Subject: [PATCH 1/6] test --- relay-server/src/utils/rate_limits.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/relay-server/src/utils/rate_limits.rs b/relay-server/src/utils/rate_limits.rs index eb20b519a6..d92eab22d5 100644 --- a/relay-server/src/utils/rate_limits.rs +++ b/relay-server/src/utils/rate_limits.rs @@ -1427,6 +1427,28 @@ mod tests { ); } + #[test] + fn test_enforce_transaction_standalone_profile_enforced() { + // When the transaction is sampled, the profile survives as standalone. + let mut envelope = envelope![Profile]; + + let mut mock = MockLimiter::default().deny(DataCategory::Transaction); + let (enforcement, _) = enforce_and_apply(&mut mock, &mut envelope, None); + + assert!(enforcement.profiles.is_active()); + mock.assert_call(DataCategory::Transaction, 1); + + assert_eq!( + get_outcomes(enforcement), + vec![ + (DataCategory::Transaction, 1), + (DataCategory::TransactionIndexed, 1), + (DataCategory::Profile, 1), + (DataCategory::ProfileIndexed, 1), + ] + ); + } + #[test] fn test_enforce_transaction_attachment_enforced_indexing_quota() { let mut envelope = envelope![Transaction, Attachment]; From ff52ec601f525df4649dba207f2055408b807529 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 3 Dec 2024 16:18:01 +0100 Subject: [PATCH 2/6] fix --- relay-server/src/utils/rate_limits.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/relay-server/src/utils/rate_limits.rs b/relay-server/src/utils/rate_limits.rs index d92eab22d5..729a0feaae 100644 --- a/relay-server/src/utils/rate_limits.rs +++ b/relay-server/src/utils/rate_limits.rs @@ -677,6 +677,16 @@ where scoping.item(DataCategory::Profile), summary.profile_quantity, )?; + + // Profiles can persist in envelopes without transaction if the transaction item + // was dropped by dynamic sampling. + if profile_limits.is_empty() { + profile_limits.merge(self.check.apply( + scoping.item(DataCategory::Transaction), + summary.profile_quantity, + )?); + } + enforcement.profiles = CategoryLimit::new( DataCategory::Profile, summary.profile_quantity, @@ -1436,13 +1446,12 @@ mod tests { let (enforcement, _) = enforce_and_apply(&mut mock, &mut envelope, None); assert!(enforcement.profiles.is_active()); + mock.assert_call(DataCategory::Profile, 1); mock.assert_call(DataCategory::Transaction, 1); assert_eq!( get_outcomes(enforcement), vec![ - (DataCategory::Transaction, 1), - (DataCategory::TransactionIndexed, 1), (DataCategory::Profile, 1), (DataCategory::ProfileIndexed, 1), ] From e7efe1ae2fa93d948b35440dbdc7fc472644235f Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 3 Dec 2024 16:49:43 +0100 Subject: [PATCH 3/6] fix --- relay-server/src/utils/rate_limits.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/relay-server/src/utils/rate_limits.rs b/relay-server/src/utils/rate_limits.rs index 729a0feaae..28b90ba823 100644 --- a/relay-server/src/utils/rate_limits.rs +++ b/relay-server/src/utils/rate_limits.rs @@ -680,11 +680,10 @@ where // Profiles can persist in envelopes without transaction if the transaction item // was dropped by dynamic sampling. - if profile_limits.is_empty() { - profile_limits.merge(self.check.apply( - scoping.item(DataCategory::Transaction), - summary.profile_quantity, - )?); + if profile_limits.is_empty() && summary.event_category.is_none() { + profile_limits = self + .check + .apply(scoping.item(DataCategory::Transaction), 0)?; } enforcement.profiles = CategoryLimit::new( @@ -1447,7 +1446,7 @@ mod tests { assert!(enforcement.profiles.is_active()); mock.assert_call(DataCategory::Profile, 1); - mock.assert_call(DataCategory::Transaction, 1); + mock.assert_call(DataCategory::Transaction, 0); assert_eq!( get_outcomes(enforcement), From 31b7b8aa94f8bd53dc6f40987f6dffdb485220ea Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 3 Dec 2024 16:57:39 +0100 Subject: [PATCH 4/6] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb8d1c8189..73b60b0cfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Bug Fixes**: - Accept incoming requests even if there was an error fetching their project config. ([#4140](https://github.com/getsentry/relay/pull/4140)) +- Rate limit profiles when transaction was sampled. ([#4195](https://github.com/getsentry/relay/pull/4195)) ## 24.11.1 From 35b4b61d86056829eecbda5772fe22346d0f02ce Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 5 Dec 2024 10:24:43 +0100 Subject: [PATCH 5/6] integration test --- tests/integration/test_outcome.py | 70 +++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/integration/test_outcome.py b/tests/integration/test_outcome.py index f21061892a..37a0e1a286 100644 --- a/tests/integration/test_outcome.py +++ b/tests/integration/test_outcome.py @@ -1666,6 +1666,76 @@ def test_profile_outcomes_rate_limited( assert outcomes == expected_outcomes, outcomes +@pytest.mark.parametrize("quota_category", ["transaction", "transaction_indexed"]) +def test_profile_outcomes_rate_limited_when_dynamic_sampling_drops( + mini_sentry, + relay, + quota_category, +): + project_id = 42 + project_config = mini_sentry.add_full_project_config(project_id)["config"] + + project_config.setdefault("features", []).append("organizations:profiling") + project_config["quotas"] = [ + { + "id": f"test_rate_limiting_{uuid.uuid4().hex}", + "categories": [quota_category], + "limit": 0, + "reasonCode": "profiles_exceeded", + } + ] + + config = { + "outcomes": { + "emit_outcomes": True, + "batch_size": 1, + "batch_interval": 1, + "aggregator": { + "bucket_interval": 1, + "flush_interval": 0, + }, + }, + "aggregator": { + "bucket_interval": 1, + "initial_delay": 0, + }, + } + + relay = relay(mini_sentry, options=config) + + with open( + RELAY_ROOT / "relay-profiling/tests/fixtures/sample/v1/valid.json", + "rb", + ) as f: + profile = f.read() + + # Create an envelope with an invalid profile: + envelope = Envelope() + profile_item = Item(payload=PayloadRef(bytes=profile), type="profile") + profile_item.headers["sampled"] = False + envelope.add_item(profile_item) + relay.send_envelope(project_id, envelope) + + if quota_category == "transaction": + (outcome1,) = mini_sentry.captured_outcomes.get()["outcomes"] + (outcome2,) = mini_sentry.captured_outcomes.get(timeout=1)["outcomes"] + outcome1, outcome2 = sorted([outcome1, outcome2], key=lambda o: o["category"]) + assert outcome1["outcome"] == 2 # rate limited + assert outcome1["category"] == DataCategory.PROFILE + assert outcome1["quantity"] == 1 + assert outcome2["outcome"] == 2 # rate limited + assert outcome2["category"] == DataCategory.PROFILE_INDEXED + assert outcome2["quantity"] == 1 + + assert mini_sentry.captured_events.empty() + else: + # Do not rate limit if there is only a transaction_indexed quota. + envelope = mini_sentry.captured_events.get() + assert envelope.items[0].headers["type"] == "profile" + + assert mini_sentry.captured_outcomes.empty() + + def test_global_rate_limit( mini_sentry, relay_with_processing, metrics_consumer, outcomes_consumer ): From 2f81db3cb83c53793d6cf64b02572e44a5eb2758 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 5 Dec 2024 10:27:44 +0100 Subject: [PATCH 6/6] timeouts --- tests/integration/test_outcome.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_outcome.py b/tests/integration/test_outcome.py index 37a0e1a286..5ee3d553a8 100644 --- a/tests/integration/test_outcome.py +++ b/tests/integration/test_outcome.py @@ -1717,7 +1717,7 @@ def test_profile_outcomes_rate_limited_when_dynamic_sampling_drops( relay.send_envelope(project_id, envelope) if quota_category == "transaction": - (outcome1,) = mini_sentry.captured_outcomes.get()["outcomes"] + (outcome1,) = mini_sentry.captured_outcomes.get(timeout=10)["outcomes"] (outcome2,) = mini_sentry.captured_outcomes.get(timeout=1)["outcomes"] outcome1, outcome2 = sorted([outcome1, outcome2], key=lambda o: o["category"]) assert outcome1["outcome"] == 2 # rate limited @@ -1730,7 +1730,7 @@ def test_profile_outcomes_rate_limited_when_dynamic_sampling_drops( assert mini_sentry.captured_events.empty() else: # Do not rate limit if there is only a transaction_indexed quota. - envelope = mini_sentry.captured_events.get() + envelope = mini_sentry.captured_events.get(timeout=10) assert envelope.items[0].headers["type"] == "profile" assert mini_sentry.captured_outcomes.empty()