From a18e44eadb3e2c3ded75711c477e4feb7014fd65 Mon Sep 17 00:00:00 2001 From: David Herberth Date: Wed, 26 Jun 2024 16:56:08 +0200 Subject: [PATCH] ref(profiles): Process profiles before metric extraction --- CHANGELOG.md | 1 + relay-server/src/services/processor.rs | 23 +++++----- .../src/services/processor/profile.rs | 46 +++++++++++-------- tests/integration/test_outcome.py | 20 +++++++- 4 files changed, 57 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 578a252b8b..77128f20b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ **Internal**: - Aggregate metrics before rate limiting. ([#3746](https://github.com/getsentry/relay/pull/3746)) +- Make sure outcomes for dropped profiles are consistent between indexed and non-indexed categories. ([#3767](https://github.com/getsentry/relay/pull/3767)) - Add web vitals support for mobile browsers. ([#3762](https://github.com/getsentry/relay/pull/3762)) - Accept profiler_id in the profile context. ([#3714](https://github.com/getsentry/relay/pull/3714)) diff --git a/relay-server/src/services/processor.rs b/relay-server/src/services/processor.rs index 859171a1db..80b9f275f5 100644 --- a/relay-server/src/services/processor.rs +++ b/relay-server/src/services/processor.rs @@ -1632,8 +1632,7 @@ impl EnvelopeProcessorService { &self.inner.global_config.current(), )?; - let profile_id = profile::filter(state); - profile::transfer_id(state, profile_id); + profile::filter(state); if_processing!(self.inner.config, { attachment::create_placeholders(state); @@ -1657,15 +1656,16 @@ impl EnvelopeProcessorService { }; if let Some(outcome) = sampling_result.into_dropped_outcome() { - // Extract metrics here, we're about to drop the event/transaction. - self.extract_transaction_metrics(state, SamplingDecision::Drop, profile_id)?; - let keep_profiles = dynamic_sampling::forward_unsampled_profiles(state, &global_config); - // Process profiles before dropping the transaction, if necessary. - if keep_profiles { - profile::process(state, &self.inner.config); - } + // Before metric extraction to make sure the profile count is reflected correctly. + let profile_id = match keep_profiles { + true => profile::process(state, &self.inner.config), + false => None, + }; + + // Extract metrics here, we're about to drop the event/transaction. + self.extract_transaction_metrics(state, SamplingDecision::Drop, profile_id)?; dynamic_sampling::drop_unsampled_items(state, outcome, keep_profiles); @@ -1687,11 +1687,12 @@ impl EnvelopeProcessorService { attachment::scrub(state); if_processing!(self.inner.config, { + // Process profiles before extracting metrics, to make sure they are removed if they are invalid. + let profile_id = profile::process(state, &self.inner.config); + // Always extract metrics in processing Relays for sampled items. self.extract_transaction_metrics(state, SamplingDecision::Keep, profile_id)?; - profile::process(state, &self.inner.config); - if state .project_state .has_feature(Feature::ExtractSpansFromEvent) diff --git a/relay-server/src/services/processor/profile.rs b/relay-server/src/services/processor/profile.rs index d302f87312..3d948130ce 100644 --- a/relay-server/src/services/processor/profile.rs +++ b/relay-server/src/services/processor/profile.rs @@ -16,7 +16,7 @@ use crate::utils::ItemAction; /// Filters out invalid and duplicate profiles. /// /// Returns the profile id of the single remaining profile, if there is one. -pub fn filter(state: &mut ProcessEnvelopeState) -> Option { +pub fn filter(state: &mut ProcessEnvelopeState) { let profiling_enabled = state.project_state.has_feature(Feature::Profiling); let has_transaction = state.event_type() == Some(EventType::Transaction); let keep_unsampled_profiles = state @@ -55,18 +55,11 @@ pub fn filter(state: &mut ProcessEnvelopeState) -> Option { _ => ItemAction::Keep, }); - profile_id -} - -/// Transfers the profile ID from the profile item to the transaction item. -/// -/// The profile id may be `None` when the envelope does not contain a profile, -/// in that case the profile context is removed. -/// Some SDKs send transactions with profile ids but omit the profile in the envelope. -pub fn transfer_id( - state: &mut ProcessEnvelopeState, - profile_id: Option, -) { + // Transfers the profile ID from the profile item to the transaction item. + // + // The profile id may be `None` when the envelope does not contain a profile, + // in that case the profile context is removed. + // Some SDKs send transactions with profile ids but omit the profile in the envelope. let Some(event) = state.event.value_mut() else { return; }; @@ -90,8 +83,13 @@ pub fn transfer_id( } /// Processes profiles and set the profile ID in the profile context on the transaction if successful. -pub fn process(state: &mut ProcessEnvelopeState, config: &Config) { +pub fn process( + state: &mut ProcessEnvelopeState, + config: &Config, +) -> Option { let profiling_enabled = state.project_state.has_feature(Feature::Profiling); + let mut profile_id = None; + state.managed_envelope.retain_items(|item| match item.ty() { ItemType::Profile => { if !profiling_enabled { @@ -104,26 +102,34 @@ pub fn process(state: &mut ProcessEnvelopeState, config: &Conf return ItemAction::DropSilently; }; - expand_profile(item, event, config) + match expand_profile(item, event, config) { + Ok(id) => { + profile_id = Some(id); + ItemAction::Keep + } + Err(outcome) => ItemAction::Drop(outcome), + } } _ => ItemAction::Keep, }); + + profile_id } /// Transfers transaction metadata to profile and check its size. -fn expand_profile(item: &mut Item, event: &Event, config: &Config) -> ItemAction { +fn expand_profile(item: &mut Item, event: &Event, config: &Config) -> Result { match relay_profiling::expand_profile(&item.payload(), event) { - Ok((_id, payload)) => { + Ok((id, payload)) => { if payload.len() <= config.max_profile_size() { item.set_payload(ContentType::Json, payload); - ItemAction::Keep + Ok(id) } else { - ItemAction::Drop(Outcome::Invalid(DiscardReason::Profiling( + Err(Outcome::Invalid(DiscardReason::Profiling( relay_profiling::discard_reason(relay_profiling::ProfileError::ExceedSizeLimit), ))) } } - Err(err) => ItemAction::Drop(Outcome::Invalid(DiscardReason::Profiling( + Err(err) => Err(Outcome::Invalid(DiscardReason::Profiling( relay_profiling::discard_reason(err), ))), } diff --git a/tests/integration/test_outcome.py b/tests/integration/test_outcome.py index bb7804ce3a..1d6d8b5daa 100644 --- a/tests/integration/test_outcome.py +++ b/tests/integration/test_outcome.py @@ -1302,11 +1302,25 @@ def make_envelope(transaction_name): assert outcomes == expected_outcomes, outcomes +@pytest.mark.parametrize( + "profile_payload,expected_outcome", + [ + # Completely invalid header + (b"foobar", "profiling_invalid_json"), + # Invalid platform -> invalid profile, but a valid profile header + ( + b"""{"profile_id":"11111111111111111111111111111111","version":"2","platform":""}""", + "profiling_platform_not_supported", + ), + ], +) def test_profile_outcomes_invalid( mini_sentry, relay_with_processing, outcomes_consumer, metrics_consumer, + profile_payload, + expected_outcome, ): """ Tests that Relay reports correct outcomes for invalid profiles as `Profile`. @@ -1351,7 +1365,9 @@ def make_envelope(): type="transaction", ) ) - envelope.add_item(Item(payload=PayloadRef(bytes=b""), type="profile")) + envelope.add_item( + Item(payload=PayloadRef(bytes=profile_payload), type="profile") + ) return envelope @@ -1369,7 +1385,7 @@ def make_envelope(): "outcome": 3, # Invalid "project_id": 42, "quantity": 1, - "reason": "profiling_invalid_json", + "reason": expected_outcome, "source": "pop-relay", "timestamp": time_within_delta(), }