Skip to content

Commit

Permalink
ref(spans): Remove conversion to transaction (#3801)
Browse files Browse the repository at this point in the history 
Double-writing segment spans as transactions was meant as a temporary
measure to transition to the spans dataset in the product. The feature
was never used by more than a handful of projects and can now be
removed.
  • Loading branch information
@jjbayer
jjbayer authored Jul 9, 2024
1 parent 368d69e commit 2c68339
Show file tree
Hide file tree
Showing 11 changed files with 76 additions and 514 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- 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))
- Remove experimental double-write from spans to transactions. ([#3801](https://github.com/getsentry/relay/pull/3801))

## 24.6.0

Expand Down
17 changes: 5 additions & 12 deletions relay-dynamic-config/src/defaults.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,18 +101,11 @@ pub fn add_span_metrics(project_config: &mut ProjectConfig) {
.is_enabled = true;
}

// Enable transaction metrics for span (score.total), but only if double-write to transactions
// is disabled.
if !project_config
.features
.has(Feature::ExtractTransactionFromSegmentSpan)
{
let span_metrics_tx = config
.global_groups
.entry(GroupKey::SpanMetricsTx)
.or_default();
span_metrics_tx.is_enabled = true;
}
let span_metrics_tx = config
.global_groups
.entry(GroupKey::SpanMetricsTx)
.or_default();
span_metrics_tx.is_enabled = true;

config._span_metrics_extended = true;
if config.version == 0 {
Expand Down
9 changes: 0 additions & 9 deletions relay-dynamic-config/src/feature.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,6 @@ pub enum Feature {
#[serde(rename = "organizations:continuous-profiling")]
ContinuousProfiling,

/// When enabled, every standalone segment span will be duplicated as a transaction.
///
/// This allows support of product features that rely on transactions for SDKs that only
/// send spans.
///
/// Serialized as `projects:extract-transaction-from-segment-span`.
#[serde(rename = "projects:extract-transaction-from-segment-span")]
ExtractTransactionFromSegmentSpan,

/// Enables metric extraction from spans for common modules.
///
/// Serialized as `projects:span-metrics-extraction`.
Expand Down
305 changes: 66 additions & 239 deletions relay-event-schema/src/protocol/span/convert.rs
View file
Original file line number Diff line number Diff line change
@@ -1,203 +1,84 @@
//! This module defines bidirectional field mappings between spans and transactions.

use crate::protocol::{BrowserContext, Contexts, Event, ProfileContext, Span, TraceContext};

use relay_base_schema::events::EventType;
use relay_protocol::Annotated;

macro_rules! write_path(
($root:expr, $path_root:ident $($path_segment:ident)*) => {
{
let annotated = &mut ($root).$path_root;
$(
let value = annotated.get_or_insert_with(Default::default);
let annotated = &mut value.$path_segment;
)*
annotated
}
};
);

macro_rules! read_value(
($span:expr, $path_root:ident $($path_segment:ident)*) => {
{
let value = ($span).$path_root.value();
$(
let value = value.and_then(|value| value.$path_segment.value());
)*
value
}
};
);

macro_rules! context_write_path (
($event:expr, $ContextType:ty, $context_field:ident) => {
{
let contexts = $event.contexts.get_or_insert_with(Contexts::default);
let context = contexts.get_or_default::<$ContextType>();
write_path!(context, $context_field)
}
};
);

macro_rules! event_write_path(
($event:expr, contexts browser $context_field:ident) => {
context_write_path!($event, BrowserContext, $context_field)
};
($event:expr, contexts trace $context_field:ident) => {
context_write_path!($event, TraceContext, $context_field)
};
($event:expr, contexts profile $context_field:ident) => {
context_write_path!($event, ProfileContext, $context_field)
};
($event:expr, $path_root:ident $($path_segment:ident)*) => {
{
write_path!($event, $path_root $($path_segment)*)
}
};
);

macro_rules! context_value (
($event:expr, $ContextType:ty, $context_field:ident) => {
{
let context = &($event).context::<$ContextType>();
context.map_or(None, |ctx|ctx.$context_field.value())
}
};
);

macro_rules! event_value(
($event:expr, contexts browser $context_field:ident) => {
context_value!($event, BrowserContext, $context_field)
};
($event:expr, contexts trace $context_field:ident) => {
context_value!($event, TraceContext, $context_field)
};
($event:expr, contexts profile $context_field:ident) => {
context_value!($event, ProfileContext, $context_field)
};
($event:expr, $path_root:ident $($path_segment:ident)*) => {
{
let value = ($event).$path_root.value();
$(
let value = value.and_then(|value|value.$path_segment.value());
)*
value
use crate::protocol::{BrowserContext, Event, ProfileContext, Span, SpanData, TraceContext};

impl From<&Event> for Span {
fn from(event: &Event) -> Self {
let Event {
transaction,

platform,
timestamp,
start_timestamp,
received,
release,
environment,
tags,

measurements,
_metrics,
_metrics_summary,
..
} = event;

let trace = event.context::<TraceContext>();

// Fill data from trace context:
let mut data = trace
.map(|c| c.data.clone().map_value(SpanData::from))
.unwrap_or_default();

// Overwrite specific fields:
let span_data = data.get_or_insert_with(Default::default);
span_data.segment_name = transaction.clone();
span_data.release = release.clone();
span_data.environment = environment.clone();
if let Some(browser) = event.context::<BrowserContext>() {
span_data.browser_name = browser.name.clone();
}
};
);

#[derive(Debug, thiserror::Error)]
pub enum TryFromSpanError {
#[error("span is not a segment")]
NotASegment,
#[error("span has no span ID")]
MissingSpanId,
#[error("failed to parse event ID")]
InvalidSpanId(#[from] uuid::Error),
}

/// Implements the conversion between transaction events and segment spans.
///
/// Invoking this macro implements both `From<&Event> for Span` and `From<&Span> for Event`.
macro_rules! map_fields {
(
$(span $(. $span_path:ident)+ <=> event $(. $event_path:ident)+),+
;
$(span . $fixed_span_path:tt <= $fixed_span_field:expr),+
;
$($fixed_event_field:expr => event . $fixed_event_path:tt),+
) => {
impl From<&Event> for Span {
fn from(event: &Event) -> Self {
let mut span = Span::default();
$(
if let Some(value) = event_value!(event, $($event_path)+) {
*write_path!(&mut span, $($span_path)+) = Annotated::new(value.clone()).map_value(Into::into);
}
)+
$(
*write_path!(&mut span, $fixed_span_path) = Annotated::new($fixed_span_field);
)+
span
}
if let Some(client_sdk) = event.client_sdk.value() {
span_data.sdk_name = client_sdk.name.clone();
span_data.sdk_version = client_sdk.version.clone();
}

impl<'a> TryFrom<&'a Span> for Event {
type Error = TryFromSpanError;

fn try_from(span: &Span) -> Result<Self, Self::Error> {
let mut event = Event::default();
let span_id = span.span_id.value().ok_or(TryFromSpanError::MissingSpanId)?;
event.id = Annotated::new(span_id.try_into()?);

if !span.is_segment.value().unwrap_or(&false) {
// Only segment spans can become transactions.
return Err(TryFromSpanError::NotASegment);
}

$(
if let Some(value) = read_value!(span, $($span_path)+) {
*event_write_path!(&mut event, $($event_path)+) = Annotated::new(value.clone()).map_value(Into::into)
}
)+
$(
*event_write_path!(&mut event, $fixed_event_path) = Annotated::new($fixed_event_field);
)+

Ok(event)
}
Self {
timestamp: timestamp.clone(),
start_timestamp: start_timestamp.clone(),
exclusive_time: trace.map(|c| c.exclusive_time.clone()).unwrap_or_default(),
op: trace.map(|c| c.op.clone()).unwrap_or_default(),
span_id: trace.map(|c| c.span_id.clone()).unwrap_or_default(),
parent_span_id: trace.map(|c| c.parent_span_id.clone()).unwrap_or_default(),
trace_id: trace.map(|c| c.trace_id.clone()).unwrap_or_default(),
segment_id: trace.map(|c| c.span_id.clone()).unwrap_or_default(),
is_segment: true.into(),
status: trace.map(|c| c.status.clone()).unwrap_or_default(),
description: transaction.clone(),
tags: tags.clone().map_value(|t| t.into()),
origin: trace.map(|c| c.origin.clone()).unwrap_or_default(),
profile_id: event
.context::<ProfileContext>()
.map(|c| c.profile_id.clone())
.unwrap_or_default(),
data,
sentry_tags: Default::default(),
received: received.clone(),
measurements: measurements.clone(),
_metrics_summary: _metrics_summary.clone(),
platform: platform.clone(),
was_transaction: true.into(),
other: Default::default(),
}
};
}
}

// 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,
span.measurements <=> event.measurements,
span.platform <=> event.platform,
span.received <=> event.received,
span.start_timestamp <=> event.start_timestamp,
span.tags <=> event.tags,
span.timestamp <=> event.timestamp,
span.exclusive_time <=> event.contexts.trace.exclusive_time,
span.op <=> event.contexts.trace.op,
span.parent_span_id <=> event.contexts.trace.parent_span_id,
// A transaction corresponds to a segment span, so span_id and segment_id have the same value:
span.span_id <=> event.contexts.trace.span_id,
span.segment_id <=> event.contexts.trace.span_id,
span.status <=> event.contexts.trace.status,
span.trace_id <=> event.contexts.trace.trace_id,
span.profile_id <=> event.contexts.profile.profile_id,
span.data.release <=> event.release,
span.data.environment <=> event.environment,
span.data.browser_name <=> event.contexts.browser.name,
span.data.sdk_name <=> event.client_sdk.name,
span.data.sdk_version <=> event.client_sdk.version,
span.origin <=> event.contexts.trace.origin
;
span.is_segment <= true,
span.was_transaction <= true
;
EventType::Transaction => event.ty
);

#[cfg(test)]
mod tests {
use relay_protocol::Annotated;
use similar_asserts::assert_eq;

use crate::protocol::{SpanData, SpanId};

use super::*;

#[test]
fn roundtrip() {
fn convert() {
let event = Annotated::<Event>::from_json(
r#"{
"type": "transaction",
Expand Down Expand Up @@ -366,59 +247,5 @@ mod tests {
other: {},
}
"###);

let mut event_from_span = Event::try_from(&span_from_event).unwrap();

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);
}

#[test]
fn segment_name_takes_precedence_over_description() {
let span = Span {
span_id: SpanId("fa90fdead5f74052".to_owned()).into(),
is_segment: true.into(),
description: "This is the description".to_owned().into(),
data: SpanData {
segment_name: "This is the segment name".to_owned().into(),
..Default::default()
}
.into(),
..Default::default()
};
let event = Event::try_from(&span).unwrap();

assert_eq!(event.transaction.as_str(), Some("This is the segment name"));
}

#[test]
fn no_empty_profile_context() {
let span = Span {
span_id: SpanId("fa90fdead5f74052".to_owned()).into(),
is_segment: true.into(),
..Default::default()
};
let event = Event::try_from(&span).unwrap();

// No profile context is set.
// profile_id is required on ProfileContext so we should not create an empty one.
assert!(event.context::<ProfileContext>().is_none());
}
}
Loading

0 comments on commit 2c68339

Please sign in to comment.