feat(event-schema): Copy root span data when converting from events (#…

…3790)

SDKs place span data for root spans into `event.contexts.trace.data`.
Therefore, top-level span data was missing for metric extraction and in
indexed spans. This PR copies trace data over to the root span when
events are converted to spans.

The two types used for trace data and span data are different (called
`Data` and `SpanData`, respectively). Each of the types is partially
typed out, where some fields overlap but have different types, and
otherwise most fields are different. In a follow-up, these two types
will be
merged, but in the meanwhile we can convert between them via `IntoValue`
and `FromValue`.

The current approach carries two noteworthy caveats:

1. Roundtrip behavior is loosened, since the conversion function places
   certain event attributes into span.data. These are then cloned back
   into trace.data, where they did not reside in the original event.
2. `IntoValue` does not honor `skip_serialization`, so all typed fields
   are inserted into the `other` field as `Annotated::empty()`. This is
   a cosmetic issue in tests and leads to suboptimal performance, but
   these fields will be omitted properly in JSON serialization.

CHANGELOG.md

@@ -19,6 +19,7 @@
 - Extract thread ID and name in spans. ([#3771](https://github.com/getsentry/relay/pull/3771))
 - Compute metrics summary on the extracted custom metrics. ([#3769](https://github.com/getsentry/relay/pull/3769))
 - Add support for `all` and `any` `RuleCondition`(s). ([#3791](https://github.com/getsentry/relay/pull/3791))
+- Copy root span data from `contexts.trace.data` when converting transaction events into raw spans. ([#3790](https://github.com/getsentry/relay/pull/3790))
 
 ## 24.6.0
 

relay-event-schema/src/protocol/span.rs

@@ -6,8 +6,8 @@ use relay_protocol::{Annotated, Empty, FromValue, Getter, IntoValue, Object, Val
 
 use crate::processor::ProcessValue;
 use crate::protocol::{
-    EventId, JsonLenientString, LenientString, Measurements, MetricsSummary, OperationType,
-    OriginType, SpanId, SpanStatus, ThreadId, Timestamp, TraceId,
+    Data as TraceData, EventId, JsonLenientString, LenientString, Measurements, MetricsSummary,
+    OperationType, OriginType, SpanId, SpanStatus, ThreadId, Timestamp, TraceId,
 };
 
 #[derive(Clone, Debug, Default, PartialEq, Empty, FromValue, IntoValue, ProcessValue)]
@@ -454,6 +454,22 @@ impl Getter for SpanData {
     }
 }
 
+impl From<TraceData> for SpanData {
+    fn from(trace_data: TraceData) -> Self {
+        FromValue::from_value(Annotated::new(trace_data.into_value()))
+            .into_value()
+            .unwrap_or_default()
+    }
+}
+
+impl From<SpanData> for TraceData {
+    fn from(span_data: SpanData) -> Self {
+        FromValue::from_value(Annotated::new(span_data.into_value()))
+            .into_value()
+            .unwrap_or_default()
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::protocol::Measurement;

relay-event-schema/src/protocol/span/convert.rs

@@ -154,6 +154,8 @@ macro_rules! map_fields {
 // This macro call implements a bidirectional mapping between transaction event and segment spans,
 // allowing users to call both `Event::from(&span)` and `Span::from(&event)`.
 map_fields!(
+    // Data must go first to ensure it doesn't overwrite more specific fields
+    span.data <=> event.contexts.trace.data,
     span._metrics_summary <=> event._metrics_summary,
     span.description <=> event.transaction,
     span.data.segment_name <=> event.transaction,
@@ -188,6 +190,7 @@ map_fields!(
 #[cfg(test)]
 mod tests {
     use relay_protocol::Annotated;
+    use similar_asserts::assert_eq;
 
     use crate::protocol::{SpanData, SpanId};
 
@@ -214,7 +217,10 @@ mod tests {
                         "op": "myop",
                         "status": "ok",
                         "exclusive_time": 123.4,
-                        "parent_span_id": "FA90FDEAD5F74051"
+                        "parent_span_id": "FA90FDEAD5F74051",
+                        "data": {
+                            "custom_attribute": 42
+                        }
                     }
                 },
                 "_metrics_summary": {
@@ -320,7 +326,13 @@ mod tests {
                 user_agent_original: ~,
                 url_full: ~,
                 client_address: ~,
-                other: {},
+                other: {
+                    "custom_attribute": I64(
+                        42,
+                    ),
+                    "previousRoute": ~,
+                    "route": ~,
+                },
             },
             sentry_tags: ~,
             received: ~,
@@ -360,6 +372,21 @@ mod tests {
         let event_id = event_from_span.id.value_mut().take().unwrap();
         assert_eq!(&event_id.to_string(), "0000000000000000fa90fdead5f74052");
 
+        // Before comparing, remove any additional data that was injected into trace data during
+        // span conversion. Note that the keys are renamed on the `SpanData` struct and mostly start
+        // with `sentry.`.
+        let trace = event_from_span.context_mut::<TraceContext>().unwrap();
+        let trace_data = trace.data.value_mut().as_mut().unwrap();
+
+        trace_data.other.retain(|k, v| {
+            if v.value().is_none() || k.starts_with("sentry.") {
+                return false;
+            }
+
+            // Seems to be a special case
+            k != "browser.name"
+        });
+
         assert_eq!(event, event_from_span);
     }