Revert "feat(sampling): Add profile context early and expose getter f…

…or it" (#2732)
getsentry · Nov 16, 2023 · 592091f · 592091f
1 parent 063df70
commit 592091f
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 58 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,7 +16,7 @@
 - Group resource spans by scrubbed domain and filename. ([#2654](https://github.com/getsentry/relay/pull/2654))
 - Convert transactions to spans for all organizations. ([#2659](https://github.com/getsentry/relay/pull/2659))
 - Filter outliers (>180s) for mobile measurements. ([#2649](https://github.com/getsentry/relay/pull/2649))
-- Allow access to more context fields in dynamic sampling and metric extraction. ([#2607](https://github.com/getsentry/relay/pull/2607), [#2640](https://github.com/getsentry/relay/pull/2640), [#2675](https://github.com/getsentry/relay/pull/2675), [#2707](https://github.com/getsentry/relay/pull/2707), [#2715](https://github.com/getsentry/relay/pull/2715))
+- Allow access to more context fields in dynamic sampling and metric extraction. ([#2607](https://github.com/getsentry/relay/pull/2607), [#2640](https://github.com/getsentry/relay/pull/2640), [#2675](https://github.com/getsentry/relay/pull/2675), [#2707](https://github.com/getsentry/relay/pull/2707))
 - Allow advanced scrubbing expressions for datascrubbing safe fields. ([#2670](https://github.com/getsentry/relay/pull/2670))
 - Disable graphql scrubbing when datascrubbing is disabled. ([#2689](https://github.com/getsentry/relay/pull/2689))
 - Track when a span was received. ([#2688](https://github.com/getsentry/relay/pull/2688))

diff --git a/relay-event-schema/src/protocol/event.rs b/relay-event-schema/src/protocol/event.rs
@@ -14,9 +14,9 @@ use crate::processor::ProcessValue;
 use crate::protocol::{
     AppContext, Breadcrumb, Breakdowns, BrowserContext, ClientSdkInfo, Contexts, Csp, DebugMeta,
     DefaultContext, DeviceContext, EventType, Exception, ExpectCt, ExpectStaple, Fingerprint, Hpkp,
-    LenientString, Level, LogEntry, Measurements, Metrics, OsContext, ProfileContext, RelayInfo,
-    Request, ResponseContext, Span, Stacktrace, Tags, TemplateInfo, Thread, Timestamp,
-    TraceContext, TransactionInfo, User, Values,
+    LenientString, Level, LogEntry, Measurements, Metrics, OsContext, RelayInfo, Request,
+    ResponseContext, Span, Stacktrace, Tags, TemplateInfo, Thread, Timestamp, TraceContext,
+    TransactionInfo, User, Values,
 };
 
 /// Wrapper around a UUID with slightly different formatting.
@@ -709,12 +709,6 @@ impl Getter for Event {
             "contexts.browser.version" => {
                 self.context::<BrowserContext>()?.version.as_str()?.into()
             }
-            "contexts.profile.profile_id" => self
-                .context::<ProfileContext>()?
-                .profile_id
-                .value()?
-                .0
-                .into(),
             "contexts.device.uuid" => self.context::<DeviceContext>()?.uuid.value()?.into(),
             "contexts.trace.status" => self
                 .context::<TraceContext>()?
@@ -1109,11 +1103,6 @@ mod tests {
                     kernel_version: Annotated::new("17.4.0".to_string()),
                     ..OsContext::default()
                 });
-                contexts.add(ProfileContext {
-                    profile_id: Annotated::new(EventId(uuid!(
-                        "abadcade-feed-dead-beef-8addadfeedaa"
-                    ))),
-                });
                 contexts
             }),
             ..Default::default()
@@ -1197,10 +1186,6 @@ mod tests {
             Some(Val::String("https://sentry.io")),
             event.get_value("event.request.url")
         );
-        assert_eq!(
-            Some(Val::Uuid(uuid!("abadcade-feed-dead-beef-8addadfeedaa"))),
-            event.get_value("event.contexts.profile.profile_id")
-        )
     }
 
     #[test]

diff --git a/relay-profiling/src/lib.rs b/relay-profiling/src/lib.rs
@@ -118,15 +118,10 @@ mod utils;
 
 const MAX_PROFILE_DURATION: Duration = Duration::from_secs(30);
 
-/// Unique identifier for a profile.
-///
-/// Same format as event IDs.
-pub type ProfileId = EventId;
-
 #[derive(Debug, Deserialize)]
 struct MinimalProfile {
     #[serde(alias = "profile_id")]
-    event_id: ProfileId,
+    event_id: EventId,
     platform: String,
     #[serde(default)]
     version: sample::Version,
@@ -139,7 +134,7 @@ fn minimal_profile_from_json(
     serde_path_to_error::deserialize(d)
 }
 
-pub fn parse_metadata(payload: &[u8], project_id: ProjectId) -> Result<ProfileId, ProfileError> {
+pub fn parse_metadata(payload: &[u8], project_id: ProjectId) -> Result<(), ProfileError> {
     let profile = match minimal_profile_from_json(payload) {
         Ok(profile) => profile,
         Err(err) => {
@@ -188,10 +183,10 @@ pub fn parse_metadata(payload: &[u8], project_id: ProjectId) -> Result<ProfileId
             _ => return Err(ProfileError::PlatformNotSupported),
         },
     };
-    Ok(profile.event_id)
+    Ok(())
 }
 
-pub fn expand_profile(payload: &[u8], event: &Event) -> Result<(ProfileId, Vec<u8>), ProfileError> {
+pub fn expand_profile(payload: &[u8], event: &Event) -> Result<(EventId, Vec<u8>), ProfileError> {
     let profile = match minimal_profile_from_json(payload) {
         Ok(profile) => profile,
         Err(err) => {

diff --git a/relay-server/src/actors/processor.rs b/relay-server/src/actors/processor.rs
@@ -31,8 +31,8 @@ use relay_event_normalization::{GeoIpLookup, RawUserAgentInfo};
 use relay_event_schema::processor::{self, ProcessingAction, ProcessingState};
 use relay_event_schema::protocol::{
     Breadcrumb, ClientReport, Contexts, Csp, Event, EventType, ExpectCt, ExpectStaple, Hpkp,
-    IpAddr, LenientString, Metrics, NetworkReportError, OtelContext, ProfileContext, RelayInfo,
-    Replay, SecurityReportType, SessionAggregates, SessionAttributes, SessionStatus, SessionUpdate,
+    IpAddr, LenientString, Metrics, NetworkReportError, OtelContext, RelayInfo, Replay,
+    SecurityReportType, SessionAggregates, SessionAttributes, SessionStatus, SessionUpdate,
     Timestamp, TraceContext, UserReport, Values,
 };
 use relay_filter::FilterStatKey;
@@ -58,7 +58,7 @@ use {
     crate::actors::project_cache::UpdateRateLimits,
     crate::utils::{EnvelopeLimiter, MetricsLimiter},
     relay_event_normalization::{span, StoreConfig, StoreProcessor},
-    relay_event_schema::protocol::Span,
+    relay_event_schema::protocol::{ProfileContext, Span},
     relay_metrics::Aggregator,
     relay_quotas::{RateLimitingError, RedisRateLimiter},
     relay_redis::RedisPool,
@@ -1096,15 +1096,15 @@ impl EnvelopeProcessorService {
             .items()
             .filter(|item| item.ty() == &ItemType::Transaction)
             .count();
-        let mut profile_id = None;
+        let mut found_profile = false;
         state.managed_envelope.retain_items(|item| match item.ty() {
             // Drop profile without a transaction in the same envelope.
             ItemType::Profile if transaction_count == 0 => ItemAction::DropSilently,
             // First profile found in the envelope, we'll keep it if metadata are valid.
-            ItemType::Profile if profile_id.is_none() => {
+            ItemType::Profile if !found_profile => {
                 match relay_profiling::parse_metadata(&item.payload(), state.project_id) {
-                    Ok(id) => {
-                        profile_id = Some(id);
+                    Ok(_) => {
+                        found_profile = true;
                         ItemAction::Keep
                     }
                     Err(err) => ItemAction::Drop(Outcome::Invalid(DiscardReason::Profiling(
@@ -1118,22 +1118,7 @@ impl EnvelopeProcessorService {
             ))),
             _ => ItemAction::Keep,
         });
-        state.has_profile = profile_id.is_some();
-
-        if let Some(event) = state.event.value_mut() {
-            if event.ty.value() == Some(&EventType::Transaction) {
-                let contexts = event.contexts.get_or_insert_with(Contexts::new);
-                // If we found a profile, add its ID to the profile context on the transaction.
-                if let Some(profile_id) = profile_id {
-                    contexts.add(ProfileContext {
-                        profile_id: Annotated::new(profile_id),
-                    });
-                // If not, we delete the profile context.
-                } else {
-                    contexts.remove::<ProfileContext>();
-                }
-            }
-        }
+        state.has_profile = found_profile;
     }
 
     /// Normalize monitor check-ins and remove invalid ones.
@@ -1197,13 +1182,17 @@ impl EnvelopeProcessorService {
             }
             _ => ItemAction::Keep,
         });
-        if found_profile_id.is_none() {
-            // Remove profile context from event.
-            if let Some(event) = state.event.value_mut() {
-                if event.ty.value() == Some(&EventType::Transaction) {
-                    if let Some(contexts) = event.contexts.value_mut() {
-                        contexts.remove::<ProfileContext>();
-                    }
+        if let Some(event) = state.event.value_mut() {
+            if event.ty.value() == Some(&EventType::Transaction) {
+                let contexts = event.contexts.get_or_insert_with(Contexts::new);
+                // If we found a profile, add its ID to the profile context on the transaction.
+                if let Some(profile_id) = found_profile_id {
+                    contexts.add(ProfileContext {
+                        profile_id: Annotated::new(profile_id),
+                    });
+                // If not, we delete the profile context.
+                } else {
+                    contexts.remove::<ProfileContext>();
                 }
             }
         }
@@ -2794,6 +2783,7 @@ impl EnvelopeProcessorService {
 
         if_processing!({
             self.enforce_quotas(state)?;
+            // We need the event parsed in order to set the profile context on it
             self.process_profiles(state);
             self.process_check_ins(state);
         });