From 8806f9cb495d049fc65ef90ef9ef1dd36c3ef03b Mon Sep 17 00:00:00 2001
From: Tony Xiao <txiao@sentry.io>
Date: Fri, 12 Jul 2024 09:43:00 -0400
Subject: [PATCH] fix(profiling): Only transfer valid profile ids (#3809)

This was transfering the profile id onto the transaction all the time
which is not desirable since we need to fully validate the profile
first.
---
 CHANGELOG.md                                  |   1 +
 relay-server/src/services/processor.rs        |   1 +
 .../src/services/processor/profile.rs         | 211 ++++++++++++++++--
 3 files changed, 192 insertions(+), 21 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 404a28170c..5e4371dd23 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@
 - Fixes metrics dropped due to missing project state. ([#3553](https://github.com/getsentry/relay/issues/3553))
 - Incorrect span outcomes when generated from a indexed transaction quota. ([#3793](https://github.com/getsentry/relay/pull/3793))
 - Report outcomes for spans when transactions are rate limited. ([#3749](https://github.com/getsentry/relay/pull/3749))
+- Only transfer valid profile ids. ([#3809](https://github.com/getsentry/relay/pull/3809))
 
 **Internal**:
 
diff --git a/relay-server/src/services/processor.rs b/relay-server/src/services/processor.rs
index d7dfeeafd0..4b8bba9ca0 100644
--- a/relay-server/src/services/processor.rs
+++ b/relay-server/src/services/processor.rs
@@ -1699,6 +1699,7 @@ impl EnvelopeProcessorService {
         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);
+            profile::transfer_id(state, profile_id);
 
             // Always extract metrics in processing Relays for sampled items.
             self.extract_transaction_metrics(state, SamplingDecision::Keep, profile_id)?;
diff --git a/relay-server/src/services/processor/profile.rs b/relay-server/src/services/processor/profile.rs
index 00b98ddebe..be8e3fb800 100644
--- a/relay-server/src/services/processor/profile.rs
+++ b/relay-server/src/services/processor/profile.rs
@@ -146,6 +146,7 @@ fn expand_profile(item: &mut Item, event: &Event, config: &Config) -> Result<Pro
 mod tests {
     use std::sync::Arc;
 
+    #[cfg(feature = "processing")]
     use insta::assert_debug_snapshot;
     #[cfg(not(feature = "processing"))]
     use relay_dynamic_config::Feature;
@@ -162,10 +163,18 @@ mod tests {
 
     use super::*;
 
+    #[cfg(feature = "processing")]
     #[tokio::test]
     async fn test_profile_id_transfered() {
         // Setup
-        let processor = create_test_processor(Default::default());
+        let config = Config::from_json_value(serde_json::json!({
+            "processing": {
+                "enabled": true,
+                "kafka_config": []
+            }
+        }))
+        .unwrap();
+        let processor = create_test_processor(config);
         let event_id = EventId::new();
         let dsn = "https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42"
             .parse()
@@ -179,25 +188,24 @@ mod tests {
 
             item.set_payload(
                 ContentType::Json,
-                r#"
-            {
-                "type": "transaction",
-                "transaction": "/foo/",
-                "timestamp": 946684810.0,
-                "start_timestamp": 946684800.0,
-                "contexts": {
-                    "trace": {
-                    "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
-                    "span_id": "fa90fdead5f74053",
-                    "op": "http.server",
-                    "type": "trace"
+                r#"{
+                    "event_id": "9b73438f70e044ecbd006b7fd15b7373",
+                    "type": "transaction",
+                    "transaction": "/foo/",
+                    "timestamp": 946684810.0,
+                    "start_timestamp": 946684800.0,
+                    "contexts": {
+                        "trace": {
+                        "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
+                        "span_id": "fa90fdead5f74053",
+                        "op": "http.server",
+                        "type": "trace"
+                        }
+                    },
+                    "transaction_info": {
+                        "source": "url"
                     }
-                },
-                "transaction_info": {
-                    "source": "url"
-                }
-            }
-            "#,
+                }"#,
             );
             item
         });
@@ -213,7 +221,32 @@ mod tests {
                     "platform": "android",
                     "os": {"name": "foo", "version": "bar"},
                     "device": {"architecture": "zap"},
-                    "timestamp": "2023-10-10 00:00:00Z"
+                    "timestamp": "2023-10-10 00:00:00Z",
+                    "profile": {
+                        "samples":[
+                            {
+                                "stack_id":0,
+                                "elapsed_since_start_ns":1,
+                                "thread_id":1
+                            },
+                            {
+                                "stack_id":0,
+                                "elapsed_since_start_ns":2,
+                                "thread_id":1
+                            }
+                        ],
+                        "stacks":[[0]],
+                        "frames":[{
+                            "function":"main"
+                        }]
+                    },
+                    "transactions": [
+                        {
+                            "id": "9b73438f70e044ecbd006b7fd15b7373",
+                            "name": "/foo/",
+                            "trace_id": "4c79f60c11214eb38604f4ae0781bfb2"
+                        }
+                    ]
                 }"#,
             );
             item
@@ -260,6 +293,134 @@ mod tests {
         "###);
     }
 
+    #[cfg(feature = "processing")]
+    #[tokio::test]
+    async fn test_invalid_profile_id_not_transfered() {
+        // Setup
+        let config = Config::from_json_value(serde_json::json!({
+            "processing": {
+                "enabled": true,
+                "kafka_config": []
+            }
+        }))
+        .unwrap();
+        let processor = create_test_processor(config);
+        let event_id = EventId::new();
+        let dsn = "https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42"
+            .parse()
+            .unwrap();
+        let request_meta = RequestMeta::new(dsn);
+        let mut envelope = Envelope::from_request(Some(event_id), request_meta);
+
+        // Add a valid transaction item.
+        envelope.add_item({
+            let mut item = Item::new(ItemType::Transaction);
+
+            item.set_payload(
+                ContentType::Json,
+                r#"{
+                    "event_id": "9b73438f70e044ecbd006b7fd15b7373",
+                    "type": "transaction",
+                    "transaction": "/foo/",
+                    "timestamp": 946684810.0,
+                    "start_timestamp": 946684800.0,
+                    "contexts": {
+                        "trace": {
+                        "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
+                        "span_id": "fa90fdead5f74053",
+                        "op": "http.server",
+                        "type": "trace"
+                        }
+                    },
+                    "transaction_info": {
+                        "source": "url"
+                    }
+                }"#,
+            );
+            item
+        });
+
+        // Add a profile to the same envelope.
+        envelope.add_item({
+            let mut item = Item::new(ItemType::Profile);
+            item.set_payload(
+                ContentType::Json,
+                r#"{
+                    "profile_id": "012d836b15bb49d7bbf99e64295d995b",
+                    "version": "1",
+                    "platform": "android",
+                    "os": {"name": "foo", "version": "bar"},
+                    "device": {"architecture": "zap"},
+                    "timestamp": "2023-10-10 00:00:00Z",
+                    "profile": {
+                        "samples":[
+                            {
+                                "stack_id":0,
+                                "elapsed_since_start_ns":1,
+                                "thread_id":1
+                            },
+                            {
+                                "stack_id":1,
+                                "elapsed_since_start_ns":2,
+                                "thread_id":1
+                            }
+                        ],
+                        "stacks":[[0],[]],
+                        "frames":[{
+                            "function":"main"
+                        }]
+                    },
+                    "transactions": [
+                        {
+                            "id": "9b73438f70e044ecbd006b7fd15b7373",
+                            "name": "/foo/",
+                            "trace_id": "4c79f60c11214eb38604f4ae0781bfb2"
+                        }
+                    ]
+                }"#,
+            );
+            item
+        });
+
+        let mut project_state = ProjectState::allowed();
+        project_state.config.features.0.insert(Feature::Profiling);
+
+        let mut envelopes = ProcessingGroup::split_envelope(*envelope);
+        assert_eq!(envelopes.len(), 1);
+
+        let (group, envelope) = envelopes.pop().unwrap();
+        let envelope = ManagedEnvelope::standalone(envelope, Addr::dummy(), Addr::dummy(), group);
+
+        let message = ProcessEnvelope {
+            envelope,
+            project_state: Arc::new(project_state),
+            sampling_project_state: None,
+            reservoir_counters: ReservoirCounters::default(),
+        };
+
+        let envelope_response = processor.process(message).unwrap();
+        let ctx = envelope_response.envelope.unwrap();
+        let new_envelope = ctx.envelope();
+
+        // Get the re-serialized context.
+        let item = new_envelope
+            .get_item_by(|item| item.ty() == &ItemType::Transaction)
+            .unwrap();
+        let transaction = Annotated::<Event>::from_json_bytes(&item.payload()).unwrap();
+        let context = transaction
+            .value()
+            .unwrap()
+            .context::<ProfileContext>()
+            .unwrap();
+
+        assert_debug_snapshot!(context, @r###"
+        ProfileContext {
+            profile_id: ~,
+            profiler_id: ~,
+        }
+        "###);
+    }
+
     #[tokio::test]
     async fn filter_standalone_profile() {
         relay_log::init_test!();
@@ -311,10 +472,18 @@ mod tests {
         assert!(envelope_response.envelope.is_none());
     }
 
+    #[cfg(feature = "processing")]
     #[tokio::test]
     async fn test_profile_id_removed_profiler_id_kept() {
         // Setup
-        let processor = create_test_processor(Default::default());
+        let config = Config::from_json_value(serde_json::json!({
+            "processing": {
+                "enabled": true,
+                "kafka_config": []
+            }
+        }))
+        .unwrap();
+        let processor = create_test_processor(config);
         let event_id = EventId::new();
         let dsn = "https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42"
             .parse()