ref(transactions): Ensure that every span's parent exists (#4661)

In the span buffer we rely on the parent_span_id to cluster spans back
into transactions/segments. If we send out transactions that contain
"dangling" spans, then flatten those transactions into individual spans,
we won't be able to build the same segment back together.

---------

Co-authored-by: Jan Michael Auer <[email protected]>

Author: untitaker

CHANGELOG.md

@@ -18,6 +18,7 @@
 - Normalize trace context information before writing it into transaction and span data. This ensures the correct sampling rates are stored for extrapolation in Sentry. ([#4625](https://github.com/getsentry/relay/pull/4625))
 - Adds u16 validation to the replay protocol's segment_id field. ([#4635](https://github.com/getsentry/relay/pull/4635))
 - Exposes all service utilization with instance labels instead of the last. ([#4654](https://github.com/getsentry/relay/pull/4654))
+- Ensure that every span's parent exists. ([#4661](https://github.com/getsentry/relay/pull/4661))
 
 **Internal**:
 

relay-event-normalization/src/event.rs

@@ -335,6 +335,7 @@ fn normalize(event: &mut Event, meta: &mut Meta, config: &NormalizationConfig) {
     normalize_contexts(&mut event.contexts);
 
     if config.normalize_spans && event.ty.value() == Some(&EventType::Transaction) {
+        span::reparent_broken_spans::reparent_broken_spans(event);
         crate::normalize::normalize_app_start_spans(event);
         span::exclusive_time::compute_span_exclusive_time(event);
     }

relay-event-normalization/src/normalize/span/mod.rs

@@ -7,6 +7,7 @@ pub mod ai;
 pub mod country_subregion;
 pub mod description;
 pub mod exclusive_time;
+pub mod reparent_broken_spans;
 pub mod tag_extraction;
 
 /// Regex used to scrub hex IDs and multi-digit numbers from table names and other identifiers.

relay-event-normalization/src/normalize/span/reparent_broken_spans.rs

@@ -0,0 +1,164 @@
+//! Span tree normalization.
+
+use std::collections::BTreeSet;
+use std::mem;
+
+use relay_event_schema::protocol::{Event, TraceContext};
+use relay_protocol::Error;
+
+/// Enforce that every span has a valid parent. If any `parent_span_id` is pointing nowhere, the
+/// span is re-parented onto the root span.
+///
+/// This is to avoid any nasty surprises in the span buffer specifically. Other readers of spans
+/// (such as the frontend's tree component) were already able to deal with detached spans.
+pub fn reparent_broken_spans(event: &mut Event) {
+    let Some(ref mut spans) = event.spans.value_mut() else {
+        return;
+    };
+
+    let Some(contexts) = event.contexts.value_mut() else {
+        return;
+    };
+
+    let Some(trace_context) = contexts.get_mut::<TraceContext>() else {
+        return;
+    };
+
+    let Some(root_span_id) = trace_context.span_id.value() else {
+        return;
+    };
+
+    let valid_span_ids = spans
+        .iter()
+        .filter_map(|span| span.value())
+        .filter_map(|span| span.span_id.value())
+        .chain(Some(root_span_id))
+        .cloned()
+        .collect::<BTreeSet<_>>();
+
+    for span in spans {
+        let Some(span) = span.value_mut() else {
+            continue;
+        };
+
+        let Some(parent_span_id) = span.parent_span_id.value_mut() else {
+            continue;
+        };
+
+        if valid_span_ids.contains(&*parent_span_id) {
+            continue;
+        };
+
+        let invalid_parent = mem::replace(parent_span_id, root_span_id.clone());
+        let meta = span.parent_span_id.meta_mut();
+        meta.add_error(Error::invalid("span ID does not exist"));
+        meta.set_original_value(Some(invalid_parent));
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use relay_protocol::FromValue;
+    use relay_protocol::SerializableAnnotated;
+
+    use super::*;
+
+    #[test]
+    fn basic() {
+        let mut data = Event::from_value(
+            serde_json::json!({
+                "type": "transaction",
+                "start_timestamp": 1609455600,
+                "end_timestamp": 1609455605,
+                "contexts": {
+                    "trace": {
+                        "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
+                        "span_id": "aaaaaaaaaaaaaaaa",
+                        "parent_span_id": "ffffffffffffffff",
+                    }
+                },
+                "spans": [
+                    {
+                        "span_id": "bbbbbbbbbbbbbbbb",
+                        "parent_span_id": "aaaaaaaaaaaaaaaa"
+                    },
+                    {
+                        "span_id": "bbbbbbbbbbbbbbbb",
+                        "parent_span_id": "dddddddddddddddd",
+                    },
+                    {
+                        "span_id": "bbbbbbbbbbbbbbbb",
+                        "parent_span_id": "eeeeeeeeeeeeeeee",
+                    },
+                ],
+            })
+            .into(),
+        );
+
+        reparent_broken_spans(data.value_mut().as_mut().unwrap());
+
+        insta::assert_json_snapshot!(SerializableAnnotated(&data), @r###"
+        {
+          "type": "transaction",
+          "start_timestamp": 1609455600.0,
+          "contexts": {
+            "trace": {
+              "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
+              "span_id": "aaaaaaaaaaaaaaaa",
+              "parent_span_id": "ffffffffffffffff",
+              "type": "trace"
+            }
+          },
+          "spans": [
+            {
+              "span_id": "bbbbbbbbbbbbbbbb",
+              "parent_span_id": "aaaaaaaaaaaaaaaa"
+            },
+            {
+              "span_id": "bbbbbbbbbbbbbbbb",
+              "parent_span_id": "aaaaaaaaaaaaaaaa"
+            },
+            {
+              "span_id": "bbbbbbbbbbbbbbbb",
+              "parent_span_id": "aaaaaaaaaaaaaaaa"
+            }
+          ],
+          "end_timestamp": 1609455605,
+          "_meta": {
+            "spans": {
+              "1": {
+                "parent_span_id": {
+                  "": {
+                    "err": [
+                      [
+                        "invalid_data",
+                        {
+                          "reason": "span ID does not exist"
+                        }
+                      ]
+                    ],
+                    "val": "dddddddddddddddd"
+                  }
+                }
+              },
+              "2": {
+                "parent_span_id": {
+                  "": {
+                    "err": [
+                      [
+                        "invalid_data",
+                        {
+                          "reason": "span ID does not exist"
+                        }
+                      ]
+                    ],
+                    "val": "eeeeeeeeeeeeeeee"
+                  }
+                }
+              }
+            }
+          }
+        }
+        "###);
+    }
+}

relay-event-schema/src/protocol/contexts/trace.rs

@@ -40,7 +40,9 @@ impl AsRef<str> for TraceId {
 }
 
 /// A 16-character hex string as described in the W3C trace context spec.
-#[derive(Clone, Debug, Default, Eq, Hash, PartialEq, Empty, IntoValue, ProcessValue)]
+#[derive(
+    Clone, Debug, Default, Eq, Hash, PartialEq, Ord, PartialOrd, Empty, IntoValue, ProcessValue,
+)]
 pub struct SpanId(pub String);
 
 relay_common::impl_str_serde!(SpanId, "a span identifier");