From f437f203e8b5b5f1ad1dfd45ec0c0155f9b76d5f Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Wed, 24 Jan 2024 15:08:53 +0100 Subject: [PATCH] ref(normalization): Remove duplicated functions (#2994) Currently, there are two normalization functions that are exactly the same for the following steps: - IP addresses. - GeoInfo. - App start spans. This PR removes one of them and uses the other. --- relay-event-normalization/src/event.rs | 38 +------- .../src/normalize/mod.rs | 87 ++----------------- relay-event-normalization/src/replay.rs | 4 +- 3 files changed, 12 insertions(+), 117 deletions(-) diff --git a/relay-event-normalization/src/event.rs b/relay-event-normalization/src/event.rs index ba9dbbbce8..e3d3edfca7 100644 --- a/relay-event-normalization/src/event.rs +++ b/relay-event-normalization/src/event.rs @@ -227,7 +227,7 @@ fn normalize(event: &mut Event, meta: &mut Meta, config: &NormalizationConfig) - if let Some(geoip_lookup) = config.geoip_lookup { if let Some(user) = event.user.value_mut() { - normalize_user_geoinfo(geoip_lookup, user) + normalize_user_geoinfo(geoip_lookup, user); } } @@ -284,7 +284,7 @@ fn normalize(event: &mut Event, meta: &mut Meta, config: &NormalizationConfig) - // transactions don't have many spans, but if this is no longer the // case and we roll this flag out for most projects, we may want to // reconsider this approach. - normalize_app_start_spans(event); + crate::normalize::normalize_app_start_spans(event); span::attributes::normalize_spans(event, &BTreeSet::from([SpanAttribute::ExclusiveTime])); } @@ -419,7 +419,7 @@ pub fn normalize_ip_addresses( } /// Sets the user's GeoIp info based on user's IP address. -fn normalize_user_geoinfo(geoip_lookup: &GeoIpLookup, user: &mut User) { +pub fn normalize_user_geoinfo(geoip_lookup: &GeoIpLookup, user: &mut User) { // Infer user.geo from user.ip_address if user.geo.value().is_none() { if let Some(ip_address) = user.ip_address.value() { @@ -890,38 +890,6 @@ fn normalize_breakdowns(event: &mut Event, breakdowns_config: Option<&Breakdowns } } -/// Replaces snake_case app start spans op with dot.case op. -/// -/// This is done for the affected React Native SDK versions (from 3 to 4.4). -fn normalize_app_start_spans(event: &mut Event) { - if !event.sdk_name().eq("sentry.javascript.react-native") - || !(event.sdk_version().starts_with("4.4") - || event.sdk_version().starts_with("4.3") - || event.sdk_version().starts_with("4.2") - || event.sdk_version().starts_with("4.1") - || event.sdk_version().starts_with("4.0") - || event.sdk_version().starts_with('3')) - { - return; - } - - if let Some(spans) = event.spans.value_mut() { - for span in spans { - if let Some(span) = span.value_mut() { - if let Some(op) = span.op.value() { - if op == "app_start_cold" { - span.op.set_value(Some("app.start.cold".to_string())); - break; - } else if op == "app_start_warm" { - span.op.set_value(Some("app.start.warm".to_string())); - break; - } - } - } - } - } -} - /// Normalizes incoming contexts for the downstream metric extraction. fn normalize_contexts(contexts: &mut Annotated) { let _ = processor::apply(contexts, |contexts, _meta| { diff --git a/relay-event-normalization/src/normalize/mod.rs b/relay-event-normalization/src/normalize/mod.rs index 20114d6376..91cf4ab77c 100644 --- a/relay-event-normalization/src/normalize/mod.rs +++ b/relay-event-normalization/src/normalize/mod.rs @@ -10,8 +10,8 @@ use relay_event_schema::processor::{ }; use relay_event_schema::protocol::{ Breadcrumb, ClientSdkInfo, Context, Contexts, DebugImage, Event, EventId, EventType, Exception, - Frame, IpAddr, Level, MetricSummaryMapping, NelContext, ReplayContext, Request, Stacktrace, - TraceContext, User, VALID_PLATFORMS, + Frame, Level, MetricSummaryMapping, NelContext, ReplayContext, Stacktrace, TraceContext, User, + VALID_PLATFORMS, }; use relay_protocol::{ Annotated, Empty, Error, ErrorKind, FromValue, IntoValue, Meta, Object, Remark, RemarkType, @@ -184,7 +184,7 @@ impl<'a> StoreNormalizeProcessor<'a> { /// Replaces snake_case app start spans op with dot.case op. /// /// This is done for the affected React Native SDK versions (from 3 to 4.4). -fn normalize_app_start_spans(event: &mut Event) { +pub fn normalize_app_start_spans(event: &mut Event) { if !event.sdk_name().eq("sentry.javascript.react-native") || !(event.sdk_version().starts_with("4.4") || event.sdk_version().starts_with("4.3") @@ -252,80 +252,6 @@ fn normalize_all_metrics_summaries(event: &mut Event) { } } -/// Backfills IP addresses in various places. -pub fn normalize_ip_addresses( - request: &mut Annotated, - user: &mut Annotated, - platform: Option<&str>, - client_ip: Option<&IpAddr>, -) { - // NOTE: This is highly order dependent, in the sense that both the statements within this - // function need to be executed in a certain order, and that other normalization code - // (geoip lookup) needs to run after this. - // - // After a series of regressions over the old Python spaghetti code we decided to put it - // back into one function. If a desire to split this code up overcomes you, put this in a - // new processor and make sure all of it runs before the rest of normalization. - - // Resolve {{auto}} - if let Some(client_ip) = client_ip { - if let Some(ref mut request) = request.value_mut() { - if let Some(ref mut env) = request.env.value_mut() { - if let Some(&mut Value::String(ref mut http_ip)) = env - .get_mut("REMOTE_ADDR") - .and_then(|annotated| annotated.value_mut().as_mut()) - { - if http_ip == "{{auto}}" { - *http_ip = client_ip.to_string(); - } - } - } - } - - if let Some(ref mut user) = user.value_mut() { - if let Some(ref mut user_ip) = user.ip_address.value_mut() { - if user_ip.is_auto() { - *user_ip = client_ip.to_owned(); - } - } - } - } - - // Copy IPs from request interface to user, and resolve platform-specific backfilling - let http_ip = request - .value() - .and_then(|request| request.env.value()) - .and_then(|env| env.get("REMOTE_ADDR")) - .and_then(Annotated::::as_str) - .and_then(|ip| IpAddr::parse(ip).ok()); - - if let Some(http_ip) = http_ip { - let user = user.value_mut().get_or_insert_with(User::default); - user.ip_address.value_mut().get_or_insert(http_ip); - } else if let Some(client_ip) = client_ip { - let user = user.value_mut().get_or_insert_with(User::default); - // auto is already handled above - if user.ip_address.value().is_none() { - // In an ideal world all SDKs would set {{auto}} explicitly. - if let Some("javascript") | Some("cocoa") | Some("objc") = platform { - user.ip_address = Annotated::new(client_ip.to_owned()); - } - } - } -} - -// Sets the user's GeoIp info based on user's IP address. -fn normalize_user_geoinfo(geoip_lookup: &GeoIpLookup, user: &mut User) { - // Infer user.geo from user.ip_address - if user.geo.value().is_none() { - if let Some(ip_address) = user.ip_address.value() { - if let Ok(Some(geo)) = geoip_lookup.lookup(ip_address.as_str()) { - user.geo.set_value(Some(geo)); - } - } - } -} - /// Container for global and project level [`MeasurementsConfig`]. The purpose is to handle /// the merging logic. #[derive(Clone, Debug)] @@ -524,7 +450,7 @@ impl<'a> Processor for StoreNormalizeProcessor<'a> { // Infer user.geo from user.ip_address if let Some(geoip_lookup) = self.geoip_lookup { - normalize_user_geoinfo(geoip_lookup, user) + crate::event::normalize_user_geoinfo(geoip_lookup, user) } Ok(()) @@ -759,8 +685,9 @@ mod tests { use relay_base_schema::spans::SpanStatus; use relay_event_schema::processor::process_value; use relay_event_schema::protocol::{ - ContextInner, DebugMeta, Frame, Geo, LenientString, LogEntry, MetricSummary, - MetricsSummary, PairList, RawStacktrace, Span, SpanId, TagEntry, Tags, TraceId, Values, + ContextInner, DebugMeta, Frame, Geo, IpAddr, LenientString, LogEntry, MetricSummary, + MetricsSummary, PairList, RawStacktrace, Request, Span, SpanId, TagEntry, Tags, TraceId, + Values, }; use relay_protocol::{ assert_annotated_snapshot, get_path, get_value, FromValue, SerializableAnnotated, diff --git a/relay-event-normalization/src/replay.rs b/relay-event-normalization/src/replay.rs index b7248ef8b0..8d2d294bc5 100644 --- a/relay-event-normalization/src/replay.rs +++ b/relay-event-normalization/src/replay.rs @@ -112,12 +112,12 @@ fn normalize_array_fields(replay: &mut Replay) { } fn normalize_ip_address(replay: &mut Replay, ip_address: Option) { - crate::normalize_ip_addresses( + crate::event::normalize_ip_addresses( &mut replay.request, &mut replay.user, replay.platform.as_str(), ip_address.map(|ip| IpAddr(ip.to_string())).as_ref(), - ) + ); } fn normalize_user_agent(replay: &mut Replay, default_user_agent: &RawUserAgentInfo<&str>) {