Skip to content

Commit

Permalink
ref(normalization): Restore span processing to transactionprocessor (#…
Browse files Browse the repository at this point in the history 
…2764)
  • Loading branch information
@iker-barriocanal
iker-barriocanal authored Nov 27, 2023
1 parent c44dde2 commit 41a2859
Show file tree
Hide file tree
Showing 3 changed files with 251 additions and 250 deletions.
210 changes: 6 additions & 204 deletions relay-event-normalization/src/normalize/processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,16 @@ use chrono::{DateTime, Duration, Utc};
use relay_base_schema::metrics::{is_valid_metric_name, DurationUnit, FractionUnit, MetricUnit};
use relay_common::time::UnixTimestamp;
use relay_event_schema::processor::{
self, MaxChars, ProcessValue, ProcessingAction, ProcessingResult, ProcessingState, Processor,
self, MaxChars, ProcessingAction, ProcessingResult, ProcessingState, Processor,
};
use relay_event_schema::protocol::{
AsPair, Context, ContextInner, Contexts, DeviceClass, Event, EventType, Exception, Headers,
IpAddr, LogEntry, Measurement, Measurements, NelContext, Request, Span, SpanAttribute,
SpanStatus, Tags, TraceContext, User,
IpAddr, LogEntry, Measurement, Measurements, NelContext, Request, SpanAttribute, SpanStatus,
Tags, TraceContext, User,
};
use relay_protocol::{Annotated, Empty, Error, ErrorKind, Meta, Object, Value};
use smallvec::SmallVec;

use crate::normalize::utils::validate_span;
use crate::normalize::{mechanism, stacktrace};
use crate::span::tag_extraction::{self, extract_span_tags};
use crate::timestamp::TimestampProcessor;
Expand Down Expand Up @@ -188,20 +187,12 @@ impl<'a> Processor for NormalizeProcessor<'a> {
&mut self,
event: &mut Event,
meta: &mut Meta,
state: &ProcessingState<'_>,
_: &ProcessingState<'_>,
) -> ProcessingResult {
if self.config.is_renormalize {
return Ok(());
}

// XXX(iker): processing child values should be the last step. The logic
// below this call is being moved (WIP) to the processor appropriately.
event.process_child_values(self, state)?;

if self.config.is_renormalize {
return Ok(());
}

// Validate and normalize transaction
// (internally noops for non-transaction events).
// TODO: Parts of this processor should probably be a filter so we
Expand Down Expand Up @@ -318,20 +309,6 @@ impl<'a> Processor for NormalizeProcessor<'a> {

Ok(())
}

fn process_span(
&mut self,
span: &mut Span,
_: &mut Meta,
state: &ProcessingState<'_>,
) -> ProcessingResult {
validate_span(span)?;
span.op.get_or_insert_with(|| "default".to_owned());

span.process_child_values(self, state)?;

Ok(())
}
}

/// Backfills the client IP address on for the NEL reports.
Expand Down Expand Up @@ -1089,23 +1066,19 @@ fn normalize_app_start_measurements(measurements: &mut Measurements) {
mod tests {
use std::collections::BTreeMap;

use chrono::{TimeZone, Utc};
use insta::assert_debug_snapshot;
use relay_base_schema::events::EventType;
use relay_base_schema::metrics::{DurationUnit, MetricUnit};
use relay_event_schema::processor::{process_value, ProcessingAction, ProcessingState};
use relay_event_schema::protocol::{
Contexts, Csp, DeviceContext, Event, Headers, IpAddr, Measurement, Measurements, Request,
Span, SpanId, Tags, TraceContext, TraceId,
Tags,
};
use relay_protocol::{get_value, Annotated, Meta, Object, SerializableAnnotated};
use relay_protocol::{Annotated, Meta, Object, SerializableAnnotated};
use serde_json::json;

use crate::normalize::processor::{
filter_mobile_outliers, normalize_app_start_measurements, normalize_device_class,
normalize_dist, normalize_measurements, normalize_performance_score,
normalize_security_report, normalize_units, remove_invalid_measurements,
NormalizeProcessor, NormalizeProcessorConfig,
};
use crate::{
ClientHints, DynamicMeasurementsConfig, MeasurementsConfig, PerformanceScoreConfig,
Expand Down Expand Up @@ -1731,177 +1704,6 @@ mod tests {
assert_eq!(measurements.len(), 0);
}

#[test]
fn test_span_default_op_when_missing() {
let json = r#"{
"start_timestamp": 1,
"timestamp": 2,
"span_id": "fa90fdead5f74052",
"trace_id": "4c79f60c11214eb38604f4ae0781bfb2"
}"#;
let mut span = Annotated::<Span>::from_json(json).unwrap();

process_value(
&mut span,
&mut NormalizeProcessor::default(),
ProcessingState::root(),
)
.unwrap();
assert_eq!(get_value!(span.op).unwrap(), &"default".to_owned());
}

#[test]
fn test_discards_transaction_event_with_span_with_missing_span_id() {
let mut event = Annotated::new(Event {
ty: Annotated::new(EventType::Transaction),
timestamp: Annotated::new(Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into()),
start_timestamp: Annotated::new(
Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into(),
),
contexts: {
let mut contexts = Contexts::new();
contexts.add(TraceContext {
trace_id: Annotated::new(TraceId("4c79f60c11214eb38604f4ae0781bfb2".into())),
span_id: Annotated::new(SpanId("fa90fdead5f74053".into())),
op: Annotated::new("http.server".to_owned()),
..Default::default()
});
Annotated::new(contexts)
},
spans: Annotated::new(vec![Annotated::new(Span {
timestamp: Annotated::new(
Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into(),
),
start_timestamp: Annotated::new(
Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into(),
),
trace_id: Annotated::new(TraceId("4c79f60c11214eb38604f4ae0781bfb2".into())),
..Default::default()
})]),
..Default::default()
});

assert_eq!(
process_value(
&mut event,
&mut NormalizeProcessor::default(),
ProcessingState::root()
),
Err(ProcessingAction::InvalidTransaction(
"span is missing span_id"
))
);
}

#[test]
fn test_discards_transaction_event_with_span_with_missing_start_timestamp() {
let mut event = Annotated::new(Event {
ty: Annotated::new(EventType::Transaction),
timestamp: Annotated::new(Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into()),
start_timestamp: Annotated::new(
Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into(),
),
contexts: {
let mut contexts = Contexts::new();
contexts.add(TraceContext {
trace_id: Annotated::new(TraceId("4c79f60c11214eb38604f4ae0781bfb2".into())),
span_id: Annotated::new(SpanId("fa90fdead5f74053".into())),
op: Annotated::new("http.server".to_owned()),
..Default::default()
});
Annotated::new(contexts)
},
spans: Annotated::new(vec![Annotated::new(Span {
timestamp: Annotated::new(
Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into(),
),
..Default::default()
})]),
..Default::default()
});

assert_eq!(
process_value(
&mut event,
&mut NormalizeProcessor::default(),
ProcessingState::root()
),
Err(ProcessingAction::InvalidTransaction(
"span is missing start_timestamp"
))
);
}

#[test]
fn test_discards_transaction_event_with_span_with_missing_trace_id() {
let mut event = Annotated::new(Event {
ty: Annotated::new(EventType::Transaction),
timestamp: Annotated::new(Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into()),
start_timestamp: Annotated::new(
Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into(),
),
contexts: {
let mut contexts = Contexts::new();
contexts.add(TraceContext {
trace_id: Annotated::new(TraceId("4c79f60c11214eb38604f4ae0781bfb2".into())),
span_id: Annotated::new(SpanId("fa90fdead5f74053".into())),
op: Annotated::new("http.server".to_owned()),
..Default::default()
});
Annotated::new(contexts)
},
spans: Annotated::new(vec![Annotated::new(Span {
timestamp: Annotated::new(
Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into(),
),
start_timestamp: Annotated::new(
Utc.with_ymd_and_hms(2000, 1, 1, 0, 0, 0).unwrap().into(),
),
..Default::default()
})]),
..Default::default()
});

assert_eq!(
process_value(
&mut event,
&mut NormalizeProcessor::default(),
ProcessingState::root()
),
Err(ProcessingAction::InvalidTransaction(
"span is missing trace_id"
))
);
}

#[test]
fn test_renormalize_spans_is_idempotent() {
let json = r#"{
"start_timestamp": 1,
"timestamp": 2,
"span_id": "fa90fdead5f74052",
"trace_id": "4c79f60c11214eb38604f4ae0781bfb2"
}"#;

let mut processed = Annotated::<Span>::from_json(json).unwrap();
let processor_config = NormalizeProcessorConfig::default();
let mut processor = NormalizeProcessor::new(processor_config.clone());
process_value(&mut processed, &mut processor, ProcessingState::root()).unwrap();
assert_eq!(get_value!(processed.op!), &"default".to_owned());

let mut reprocess_config = processor_config.clone();
reprocess_config.is_renormalize = true;
let mut processor = NormalizeProcessor::new(processor_config.clone());

let mut reprocessed = processed.clone();
process_value(&mut reprocessed, &mut processor, ProcessingState::root()).unwrap();
assert_eq!(processed, reprocessed);

let mut reprocessed2 = reprocessed.clone();
process_value(&mut reprocessed2, &mut processor, ProcessingState::root()).unwrap();
assert_eq!(reprocessed, reprocessed2);
}

#[test]
fn test_computed_performance_score() {
let json = r#"
Expand Down
44 changes: 0 additions & 44 deletions relay-event-normalization/src/normalize/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,52 +5,8 @@

use std::f64::consts::SQRT_2;

use relay_event_schema::processor::{ProcessingAction, ProcessingResult};
use relay_event_schema::protocol::{Event, ResponseContext, Span, TraceContext, User};

/// Returns a [`ProcessingResult`] error if the span is invalid.
///
/// A span is valid in the following cases:
/// - The span has a span id and trace id.
/// - The span has a start and end timestamp.
/// - The start timestamp is no greater than the end timestamp.
pub(crate) fn validate_span(span: &Span) -> ProcessingResult {
match (span.start_timestamp.value(), span.timestamp.value()) {
(Some(start), Some(end)) => {
if end < start {
return Err(ProcessingAction::InvalidTransaction(
"end timestamp in span is smaller than start timestamp",
));
}
}
(_, None) => {
// XXX: Maybe do the same as event.timestamp?
return Err(ProcessingAction::InvalidTransaction(
"span is missing timestamp",
));
}
(None, _) => {
// XXX: Maybe copy timestamp over?
return Err(ProcessingAction::InvalidTransaction(
"span is missing start_timestamp",
));
}
}

if span.trace_id.value().is_none() {
return Err(ProcessingAction::InvalidTransaction(
"span is missing trace_id",
));
}
if span.span_id.value().is_none() {
return Err(ProcessingAction::InvalidTransaction(
"span is missing span_id",
));
}

Ok(())
}

/// Used to decide when to extract mobile-specific tags.
pub const MOBILE_SDKS: [&str; 4] = [
"sentry.cocoa",
Expand Down
Loading

0 comments on commit 41a2859

Please sign in to comment.