diff --git a/CHANGELOG.md b/CHANGELOG.md index 0de8c032f3..bb7e8fb534 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Features**: - Double write to legacy attributes for backwards compatibility. ([#5490](https://github.com/getsentry/relay/pull/5490)) +- Forbid invalid profile chunks without a platform header. ([#5507](https://github.com/getsentry/relay/pull/5507)) **Internal**: diff --git a/relay-server/src/envelope/item.rs b/relay-server/src/envelope/item.rs index 1322d684c5..ba83670e4a 100644 --- a/relay-server/src/envelope/item.rs +++ b/relay-server/src/envelope/item.rs @@ -159,11 +159,12 @@ impl Item { (DataCategory::Span, item_count), (DataCategory::SpanIndexed, item_count), ], - // NOTE: semantically wrong, but too expensive to parse. ItemType::ProfileChunk => match self.profile_type() { Some(ProfileType::Backend) => smallvec![(DataCategory::ProfileChunk, item_count)], Some(ProfileType::Ui) => smallvec![(DataCategory::ProfileChunkUi, item_count)], - None => smallvec![], + // Profile chunks without platform/profile type are considered invalid, + // fallback to `profile chunk` to still get outcomes. + None => smallvec![(DataCategory::ProfileChunk, item_count)], }, ItemType::Integration => match self.integration() { Some(Integration::Logs(LogsIntegration::OtelV1 { .. })) => smallvec![ diff --git a/relay-server/src/processing/profile_chunks/mod.rs b/relay-server/src/processing/profile_chunks/mod.rs index cdb458a1b9..f0ba3ed08b 100644 --- a/relay-server/src/processing/profile_chunks/mod.rs +++ b/relay-server/src/processing/profile_chunks/mod.rs @@ -168,7 +168,9 @@ impl Counted for SerializedProfileChunks { match pc.profile_type() { Some(ProfileType::Ui) => ui += 1, Some(ProfileType::Backend) => backend += 1, - None => {} + // These are invalid and will be dropped, but we default outcomes to the backend + // profile chunk category. + None => backend += 1, } } diff --git a/relay-server/src/processing/profile_chunks/process.rs b/relay-server/src/processing/profile_chunks/process.rs index e4ac2d81a0..ff017a24b8 100644 --- a/relay-server/src/processing/profile_chunks/process.rs +++ b/relay-server/src/processing/profile_chunks/process.rs @@ -1,6 +1,3 @@ -use relay_profiling::ProfileType; -use relay_quotas::DataCategory; - use crate::envelope::{ContentType, Item, ItemType}; use crate::processing::Context; use crate::processing::Managed; @@ -18,40 +15,21 @@ pub fn process(profile_chunks: &mut Managed, ctx: Conte profile_chunks.retain( |pc| &mut pc.profile_chunks, - |item, records| -> Result<()> { + |item, _| -> Result<()> { let pc = relay_profiling::ProfileChunk::new(item.payload())?; - // Validate the item inferred profile type with the one from the payload, - // or if missing set it. - // - // This is currently necessary to ensure profile chunks are emitted in the correct - // data category, as well as rate limited with the correct data category. - // - // In the future we plan to make the profile type on the item header a necessity. - // For more context see also: . - if item - .profile_type() - .is_some_and(|pt| pt != pc.profile_type()) - { + // Profile chunks must carry the same profile type in the item header as well as the + // payload. + if item.profile_type().is_none_or(|pt| pt != pc.profile_type()) { + relay_log::debug!("dropping profile chunk due to profile type/platform mismatch"); return Err(relay_profiling::ProfileError::InvalidProfileType.into()); } - // Update the profile type to ensure the following outcomes are emitted in the correct - // data category. - // - // Once the item header on the item is required, this is no longer required. - if item.profile_type().is_none() { - item.set_profile_type(pc.profile_type()); - match pc.profile_type() { - ProfileType::Ui => records.modify_by(DataCategory::ProfileChunkUi, 1), - ProfileType::Backend => records.modify_by(DataCategory::ProfileChunk, 1), - } - } - pc.filter(client_ip, filter_settings, ctx.global_config)?; let expanded = pc.expand()?; if expanded.len() > ctx.config.max_profile_size() { + relay_log::debug!("dropping profile chunk exceeding the size limit"); return Err(relay_profiling::ProfileError::ExceedSizeLimit.into()); } diff --git a/relay-server/src/utils/rate_limits.rs b/relay-server/src/utils/rate_limits.rs index 5592a7e196..cd1cb99615 100644 --- a/relay-server/src/utils/rate_limits.rs +++ b/relay-server/src/utils/rate_limits.rs @@ -643,9 +643,8 @@ impl Enforcement { } ItemType::Span => !self.spans_indexed.is_active(), ItemType::ProfileChunk => match item.profile_type() { - Some(ProfileType::Backend) => !self.profile_chunks.is_active(), + Some(ProfileType::Backend) | None => !self.profile_chunks.is_active(), Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(), - None => true, }, ItemType::TraceMetric => !self.trace_metrics.is_active(), ItemType::Integration => match item.integration() { @@ -1618,28 +1617,9 @@ mod tests { } /// Limit profile chunks. - #[tokio::test] - async fn test_enforce_limit_profile_chunks_no_profile_type() { - // In this test we have profile chunks which have not yet been classified, which means they - // should not be rate limited. - let mut envelope = envelope![ProfileChunk, ProfileChunk]; - - let mock = mock_limiter(&[DataCategory::ProfileChunk]); - let (enforcement, limits) = enforce_and_apply(mock.clone(), &mut envelope, None).await; - assert!(!limits.is_limited()); - assert_eq!(get_outcomes(enforcement), vec![]); - - let mock = mock_limiter(&[DataCategory::ProfileChunkUi]); - let (enforcement, limits) = enforce_and_apply(mock.clone(), &mut envelope, None).await; - assert!(!limits.is_limited()); - assert_eq!(get_outcomes(enforcement), vec![]); - - assert_eq!(envelope.envelope().len(), 2); - } - #[tokio::test] async fn test_enforce_limit_profile_chunks_ui() { - let mut envelope = envelope![]; + let mut envelope = envelope![ProfileChunk]; let mut item = Item::new(ItemType::ProfileChunk); item.set_profile_type(ProfileType::Backend); @@ -1652,11 +1632,11 @@ mod tests { let (enforcement, limits) = enforce_and_apply(mock.clone(), &mut envelope, None).await; assert!(limits.is_limited()); - assert_eq!(envelope.envelope().len(), 1); + assert_eq!(envelope.envelope().len(), 2); mock.lock() .await .assert_call(DataCategory::ProfileChunkUi, 1); - mock.lock().await.assert_call(DataCategory::ProfileChunk, 1); + mock.lock().await.assert_call(DataCategory::ProfileChunk, 2); assert_eq!( get_outcomes(enforcement), @@ -1666,7 +1646,7 @@ mod tests { #[tokio::test] async fn test_enforce_limit_profile_chunks_backend() { - let mut envelope = envelope![]; + let mut envelope = envelope![ProfileChunk]; let mut item = Item::new(ItemType::ProfileChunk); item.set_profile_type(ProfileType::Backend); @@ -1683,11 +1663,11 @@ mod tests { mock.lock() .await .assert_call(DataCategory::ProfileChunkUi, 1); - mock.lock().await.assert_call(DataCategory::ProfileChunk, 1); + mock.lock().await.assert_call(DataCategory::ProfileChunk, 2); assert_eq!( get_outcomes(enforcement), - vec![(DataCategory::ProfileChunk, 1)] + vec![(DataCategory::ProfileChunk, 2)] ); } diff --git a/tests/integration/test_profile_chunks.py b/tests/integration/test_profile_chunks.py index d5b1f49ceb..94793d6bbe 100644 --- a/tests/integration/test_profile_chunks.py +++ b/tests/integration/test_profile_chunks.py @@ -28,7 +28,7 @@ } -def sample_profile_v2_envelope(platform=None): +def sample_profile_v2_envelope(platform="cocoa"): envelope = Envelope() with open( @@ -47,7 +47,7 @@ def sample_profile_v2_envelope(platform=None): return envelope -def android_profile_chunk_envelope(platform=None): +def android_profile_chunk_envelope(platform="android"): envelope = Envelope() with open( @@ -155,7 +155,13 @@ def test_profile_chunk_outcomes_invalid( "chunk_id": "11111111111111111111111111111111", "platform": "thisisnotvalid", } - envelope.add_item(Item(payload=PayloadRef(json=payload), type="profile_chunk")) + envelope.add_item( + Item( + payload=PayloadRef(json=payload), + type="profile_chunk", + headers={"platform": "thisisnotvalid"}, + ) + ) upstream.send_envelope(project_id, envelope) @@ -178,7 +184,6 @@ def test_profile_chunk_outcomes_invalid( profiles_consumer.assert_empty() -@pytest.mark.parametrize("item_header_platform", [None, "cocoa"]) @pytest.mark.parametrize( ["envelope"], [ @@ -195,7 +200,6 @@ def test_profile_chunk_outcomes_rate_limited( outcomes_consumer, profiles_consumer, envelope, - item_header_platform, ): """ Tests that Relay reports correct outcomes when profile chunks are rate limited. @@ -227,7 +231,7 @@ def test_profile_chunk_outcomes_rate_limited( ] # Create and send envelope containing the profile chunk - envelope = envelope(item_header_platform) + envelope = envelope() upstream = relay_with_processing(TEST_CONFIG) upstream.send_envelope(project_id, envelope) @@ -257,7 +261,7 @@ def test_profile_chunk_outcomes_rate_limited( [ ("cocoa", "profile_chunk_ui"), ("node", "profile_chunk"), - (None, "profile_chunk"), # Special case, currently this will forward + (None, "profile_chunk"), ], ) @pytest.mark.parametrize( @@ -280,10 +284,6 @@ def test_profile_chunk_outcomes_rate_limited_fast( """ Tests that Relay reports correct outcomes when profile chunks are rate limited already in the fast-path, using the item header. - - The test is parameterized to also *not* send the necessary item header, in which case this currently - asserts the chunk is let through. Once Relay's behaviour is changed to reject or profile chunks - without the necessary headers or the profile type is defaulted this test needs to be adjusted accordingly. """ project_id = 42 project_config = mini_sentry.add_full_project_config(project_id)["config"] @@ -305,15 +305,11 @@ def test_profile_chunk_outcomes_rate_limited_fast( upstream = relay(mini_sentry) upstream.send_envelope(project_id, envelope) - if platform is None: - envelope = mini_sentry.get_captured_envelope() - assert [item.type for item in envelope.items] == ["profile_chunk"] - else: - outcome = mini_sentry.get_client_report() - assert outcome["rate_limited_events"] == [ - {"category": category, "quantity": 1, "reason": "profile_chunks_exceeded"} - ] - assert mini_sentry.captured_envelopes.empty() + outcome = mini_sentry.get_client_report() + assert outcome["rate_limited_events"] == [ + {"category": category, "quantity": 1, "reason": "profile_chunks_exceeded"} + ] + assert mini_sentry.captured_envelopes.empty() @pytest.mark.parametrize(