feat(profiling): Expand the discard reasons

CHANGELOG.md

@@ -21,6 +21,7 @@
 - Track metrics for OpenTelemetry events. ([#1618](https://github.com/getsentry/relay/pull/1618))
 - Normalize transaction name for URLs transaction source, by replacing UUIDs, SHAs and numerical IDs in transaction names by placeholders. ([#1621](https://github.com/getsentry/relay/pull/1621))
 - Parse string as number to handle a release bug. ([#1637](https://github.com/getsentry/relay/pull/1637))
+- Expand Profiling's discard reasons. ([#1661](https://github.com/getsentry/relay/pull/1661))
 
 ## 22.11.0
 

relay-profiling/src/error.rs

@@ -2,8 +2,8 @@ use thiserror::Error;
 
 #[derive(Debug, Error)]
 pub enum ProfileError {
-    #[error("invalid json in profile")]
-    InvalidJson(#[source] serde_json::Error),
+    #[error(transparent)]
+    InvalidJson(#[from] serde_json::Error),
     #[error("invalid base64 value")]
     InvalidBase64Value,
     #[error("invalid sampled profile")]
@@ -20,4 +20,8 @@ pub enum ProfileError {
     InvalidTransactionMetadata,
     #[error("missing profile metadata")]
     MissingProfileMetadata,
+    #[error("malformed stacks")]
+    MalformedStacks,
+    #[error("malformed samples")]
+    MalformedSamples,
 }

relay-profiling/src/lib.rs

@@ -100,6 +100,7 @@ mod cocoa;
 mod error;
 mod measurements;
 mod native_debug_image;
+mod outcomes;
 mod python;
 mod rust;
 mod sample;
@@ -115,6 +116,7 @@ use crate::sample::{expand_sample_profile, Version};
 use crate::typescript::parse_typescript_profile;
 
 pub use crate::error::ProfileError;
+pub use crate::outcomes::discard_reason;
 
 #[derive(Debug, Serialize, Deserialize, Clone)]
 #[serde(rename_all = "lowercase")]

relay-profiling/src/outcomes.rs

@@ -0,0 +1,15 @@
+use crate::ProfileError;
+
+pub fn discard_reason(err: ProfileError) -> &'static str {
+    match err {
+        ProfileError::CannotSerializePayload => "profiling_failed_serialization",
+        ProfileError::InvalidTransactionMetadata => "profiling_invalid_transaction_metadata",
+        ProfileError::MalformedSamples => "profiling_malformed_samples",
+        ProfileError::MalformedStacks => "profiling_malformed_stacks",
+        ProfileError::MissingProfileMetadata => "profiling_invalid_profile_metadata",
+        ProfileError::NoTransactionAssociated => "profiling_no_transaction_associated",
+        ProfileError::NotEnoughSamples => "profiling_not_enough_samples",
+        ProfileError::PlatformNotSupported => "profiling_platform_not_supported",
+        _ => "profiling_unknown",
+    }
+}

relay-profiling/src/sample.rs

@@ -42,7 +42,7 @@ impl Frame {
 
 #[derive(Debug, Serialize, Deserialize, Clone)]
 struct Sample {
-    stack_id: u32,
+    stack_id: usize,
     #[serde(deserialize_with = "deserialize_number_from_string")]
     thread_id: u64,
     #[serde(deserialize_with = "deserialize_number_from_string")]
@@ -69,7 +69,7 @@ struct QueueMetadata {
 #[derive(Debug, Serialize, Deserialize, Clone)]
 struct Profile {
     samples: Vec<Sample>,
-    stacks: Vec<Vec<u32>>,
+    stacks: Vec<Vec<usize>>,
     frames: Vec<Frame>,
 
     #[serde(default, skip_serializing_if = "Option::is_none")]
@@ -179,6 +179,26 @@ impl SampleProfile {
         }
     }
 
+    fn check_samples(&self) -> bool {
+        for sample in &self.profile.samples {
+            if self.profile.stacks.get(sample.stack_id).is_none() {
+                return false;
+            }
+        }
+        true
+    }
+
+    fn check_stacks(&self) -> bool {
+        for stack in &self.profile.stacks {
+            for frame_id in stack {
+                if self.profile.frames.get(*frame_id).is_none() {
+                    return false;
+                }
+            }
+        }
+        true
+    }
+
     /// Removes a sample when it's the only sample on its thread
     fn remove_single_samples_per_thread(&mut self) {
         let mut sample_count_by_thread_id: HashMap<u64, u32> = HashMap::new();
@@ -210,6 +230,14 @@ pub fn expand_sample_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileErro
         return Err(ProfileError::MissingProfileMetadata);
     }
 
+    if !profile.check_samples() {
+        return Err(ProfileError::MalformedSamples);
+    }
+
+    if !profile.check_stacks() {
+        return Err(ProfileError::MalformedStacks);
+    }
+
     let mut items: Vec<Vec<u8>> = Vec::new();
 
     // As we're getting one profile for multiple transactions and our backend doesn't support this,
@@ -233,6 +261,10 @@ pub fn expand_sample_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileErro
             }
         });
 
+        if new_profile.profile.samples.is_empty() {
+            continue;
+        }
+
         match serde_json::to_vec(&new_profile) {
             Ok(payload) => items.push(payload),
             Err(_) => {
@@ -409,6 +441,64 @@ mod tests {
         assert!(parse_profile(&payload[..]).is_err());
     }
 
+    #[test]
+    fn test_expand_with_samples_outside_transaction() {
+        let mut profile = generate_profile();
+
+        profile.profile.frames.push(Frame {
+            abs_path: Some("".to_string()),
+            colno: Some(0),
+            filename: Some("".to_string()),
+            function: Some("".to_string()),
+            in_app: Some(false),
+            instruction_addr: Some(Addr(0)),
+            lineno: Some(0),
+            module: Some("".to_string()),
+        });
+        profile.transactions.push(TransactionMetadata {
+            active_thread_id: 1,
+            id: EventId::new(),
+            name: "blah".to_string(),
+            relative_cpu_end_ms: 0,
+            relative_cpu_start_ms: 0,
+            relative_end_ns: 100,
+            relative_start_ns: 50,
+            trace_id: EventId::new(),
+        });
+        profile.profile.stacks.push(vec![0]);
+        profile.profile.samples.extend(vec![
+            Sample {
+                stack_id: 0,
+                queue_address: Some("0xdeadbeef".to_string()),
+                elapsed_since_start_ns: 10,
+                thread_id: 1,
+            },
+            Sample {
+                stack_id: 0,
+                queue_address: Some("0xdeadbeef".to_string()),
+                elapsed_since_start_ns: 20,
+                thread_id: 1,
+            },
+            Sample {
+                stack_id: 0,
+                queue_address: Some("0xdeadbeef".to_string()),
+                elapsed_since_start_ns: 30,
+                thread_id: 1,
+            },
+            Sample {
+                stack_id: 0,
+                queue_address: Some("0xdeadbeef".to_string()),
+                elapsed_since_start_ns: 40,
+                thread_id: 1,
+            },
+        ]);
+
+        let payload = serde_json::to_vec(&profile).unwrap();
+        let data = expand_sample_profile(&payload[..]);
+
+        assert!(data.is_err());
+    }
+
     #[test]
     fn test_expand_multiple_transactions() {
         let payload =

relay-server/src/actors/outcome.rs

@@ -356,15 +356,11 @@ pub enum DiscardReason {
     /// dynamic sampling rules.
     TransactionSampled,
 
-    /// (Relay) We failed to parse the profile so we discard the profile.
-    ProcessProfile,
-
-    /// (Relay) The profile is parseable but semantically invalid. This could happen if
-    /// profiles lack sufficient samples.
-    InvalidProfile,
-
-    // (Relay) We failed to parse the replay so we discard it.
+    /// (Relay) We failed to parse the replay so we discard it.
     InvalidReplayEvent,
+
+    /// (Relay) Profiling related discard reasons
+    Profiling(&'static str),
 }
 
 impl DiscardReason {
@@ -385,7 +381,6 @@ impl DiscardReason {
             DiscardReason::SecurityReport => "security_report",
             DiscardReason::Cors => "cors",
             DiscardReason::ProcessUnreal => "process_unreal",
-            DiscardReason::ProcessProfile => "process_profile",
 
             // Relay specific reasons (not present in Sentry)
             DiscardReason::Payload => "payload",
@@ -403,8 +398,8 @@ impl DiscardReason {
             DiscardReason::Internal => "internal",
             DiscardReason::TransactionSampled => "transaction_sampled",
             DiscardReason::EmptyEnvelope => "empty_envelope",
-            DiscardReason::InvalidProfile => "invalid_profile",
             DiscardReason::InvalidReplayEvent => "invalid_replay",
+            DiscardReason::Profiling(reason) => reason,
         }
     }
 }

relay-server/src/actors/processor.rs

@@ -349,15 +349,6 @@ fn outcome_from_parts(field: ClientReportField, reason: &str) -> Result<Outcome,
     }
 }
 
-fn outcome_from_profile_error(err: relay_profiling::ProfileError) -> Outcome {
-    let discard_reason = match err {
-        relay_profiling::ProfileError::CannotSerializePayload => DiscardReason::Internal,
-        relay_profiling::ProfileError::NotEnoughSamples => DiscardReason::InvalidProfile,
-        _ => DiscardReason::ProcessProfile,
-    };
-    Outcome::Invalid(discard_reason)
-}
-
 /// Response of the [`ProcessEnvelope`] message.
 #[cfg_attr(not(feature = "processing"), allow(dead_code))]
 pub struct ProcessEnvelopeResponse {
@@ -971,9 +962,16 @@ impl EnvelopeProcessorService {
             match relay_profiling::expand_profile(&item.payload()[..]) {
                 Ok(payloads) => new_profiles.extend(payloads),
                 Err(err) => {
-                    relay_log::debug!("invalid profile: {:#?}", err);
+                    match err {
+                        relay_profiling::ProfileError::InvalidJson(_) => {
+                            relay_log::error!("invalid profile: {}", LogError(&err));
+                        }
+                        _ => relay_log::debug!("invalid profile: {}", err),
+                    };
                     context.track_outcome(
-                        outcome_from_profile_error(err),
+                        Outcome::Invalid(DiscardReason::Profiling(
+                            relay_profiling::discard_reason(err),
+                        )),
                         DataCategory::Profile,
                         1,
                     );