ref(profiles): Process profiles before metric extraction

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))
 

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)

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<G>(state: &mut ProcessEnvelopeState<G>) -> Option<ProfileId> {
+pub fn filter<G>(state: &mut ProcessEnvelopeState<G>) {
     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<G>(state: &mut ProcessEnvelopeState<G>) -> Option<ProfileId> {
         _ => 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<TransactionGroup>,
-    profile_id: Option<ProfileId>,
-) {
+    // 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<TransactionGroup>, config: &Config) {
+pub fn process(
+    state: &mut ProcessEnvelopeState<TransactionGroup>,
+    config: &Config,
+) -> Option<ProfileId> {
     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<TransactionGroup>, 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<ProfileId, Outcome> {
     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),
         ))),
     }

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":"<this does not exist>"}""",
+            "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(),
         }