feat(relay): do not assume empty means {{auto}} for some sdks

CHANGELOG.md

@@ -11,6 +11,7 @@
 
 - Track an utilization metric for internal services. ([#4501](https://github.com/getsentry/relay/pull/4501))
 - Add new `relay-threading` crate with asynchronous thread pool. ([#4500](https://github.com/getsentry/relay/pull/4500))
+- Do not convert empty ip address into `{{auto}}` for newer SDKs. ([#4519](https://github.com/getsentry/relay/pull/4519))
 
 ## 25.2.0
 

relay-event-normalization/src/event.rs

@@ -27,6 +27,7 @@ use smallvec::SmallVec;
 use uuid::Uuid;
 
 use crate::normalize::request;
+use crate::sdk_version::SdkVersion;
 use crate::span::ai::normalize_ai_measurements;
 use crate::span::tag_extraction::extract_span_tags_from_event;
 use crate::utils::{self, get_event_user_tag, MAX_DURATION_MOBILE_MS};
@@ -37,6 +38,15 @@ use crate::{
     RawUserAgentInfo, SpanDescriptionRule, TransactionNameConfig,
 };
 
+/// This is the first Javascript SDK version that does explicitly send `{{auto}}` instead of
+/// empty string to signal that the user IP address should be inferred.
+/// See: <https://github.com/getsentry/sentry-javascript/releases/tag/9.0.0>
+const CUTOFF_JAVASCRIPT_VERSION: SdkVersion = SdkVersion::new(9, 0, 0);
+/// This is the first Cocoa SDK version that explicitly send `{{auto}}` instead of
+/// empty string to signal that the user IP address should be inferred.
+/// See: <https://github.com/getsentry/sentry-cocoa/releases/tag/6.2.0>
+const CUTOFF_COCOA_VERSION: SdkVersion = SdkVersion::new(6, 2, 0);
+
 /// Configuration for [`normalize_event`].
 #[derive(Clone, Debug)]
 pub struct NormalizationConfig<'a> {
@@ -260,6 +270,7 @@ fn normalize(event: &mut Event, meta: &mut Meta, config: &NormalizationConfig) {
         &mut event.user,
         event.platform.as_str(),
         config.client_ip,
+        &event.client_sdk,
     );
 
     if let Some(geoip_lookup) = config.geoip_lookup {
@@ -412,6 +423,7 @@ pub fn normalize_ip_addresses(
     user: &mut Annotated<User>,
     platform: Option<&str>,
     client_ip: Option<&IpAddr>,
+    client_sdk: &Annotated<ClientSdkInfo>,
 ) {
     // 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
@@ -467,9 +479,35 @@ pub fn normalize_ip_addresses(
                 .iter_remarks()
                 .any(|r| r.ty == RemarkType::Removed);
             if !scrubbed_before {
-                // In an ideal world all SDKs would set {{auto}} explicitly.
+                // We stop assuming that empty means {{auto}} for the affected platforms
+                // starting with the versions listed below.
                 if let Some("javascript") | Some("cocoa") | Some("objc") = platform {
-                    user.ip_address = Annotated::new(client_ip.to_owned());
+                    if let Some(Ok(sdk_version)) = client_sdk
+                        .0
+                        .as_ref()
+                        .and_then(|sdk| sdk.version.0.as_ref())
+                        .map(|sdk_version| sdk_version.as_str().parse::<SdkVersion>())
+                    {
+                        match platform {
+                            Some("javascript") => {
+                                if sdk_version < CUTOFF_JAVASCRIPT_VERSION {
+                                    user.ip_address = Annotated::new(client_ip.to_owned());
+                                }
+                            }
+                            Some("cocoa") => {
+                                if sdk_version < CUTOFF_COCOA_VERSION {
+                                    user.ip_address = Annotated::new(client_ip.to_owned());
+                                }
+                            }
+                            // The obj-c SDK is deprecated in favour of cocoa so we keep the
+                            // old behavior of empty == {{auto}}.
+                            // With the `scrubbed_before` check we made sure that we only
+                            // derive empty as {{auto}} if the empty string comes from the SDK
+                            // directly and is not the result of scrubbing.
+                            Some("objc") => user.ip_address = Annotated::new(client_ip.to_owned()),
+                            _ => {}
+                        }
+                    }
                 }
             }
         }
@@ -1462,16 +1500,15 @@ mod tests {
 
     use std::collections::BTreeMap;
 
+    use super::*;
+    use crate::{ClientHints, MeasurementsConfig, ModelCost};
     use insta::assert_debug_snapshot;
     use itertools::Itertools;
     use relay_common::glob2::LazyGlob;
     use relay_event_schema::protocol::{Breadcrumb, Csp, DebugMeta, DeviceContext, Values};
     use relay_protocol::{get_value, SerializableAnnotated};
     use serde_json::json;
 
-    use super::*;
-    use crate::{ClientHints, MeasurementsConfig, ModelCost};
-
     const IOS_MOBILE_EVENT: &str = r#"
         {
             "sdk": {"name": "sentry.cocoa"},

relay-event-normalization/src/lib.rs

@@ -17,6 +17,7 @@ mod normalize;
 mod regexes;
 mod remove_other;
 mod schema;
+mod sdk_version;
 mod stacktrace;
 mod statsd;
 mod timestamp;
@@ -26,6 +27,7 @@ mod validation;
 
 pub use validation::{validate_event, validate_span, EventValidationConfig};
 pub mod replay;
+
 pub use event::{
     normalize_event, normalize_measurements, normalize_performance_score, NormalizationConfig,
 };

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

@@ -454,8 +454,10 @@ mod tests {
             },
         );
 
-        let ip_addr = get_value!(event.user.ip_address!);
-        assert_eq!(ip_addr, &IpAddr("2.125.160.216".to_string()));
+        assert_eq!(
+            Annotated::empty(),
+            event.0.unwrap().user.0.unwrap().ip_address
+        );
     }
 
     #[test]

relay-event-normalization/src/replay.rs

@@ -128,6 +128,7 @@ fn normalize_ip_address(replay: &mut Replay, ip_address: Option<StdIpAddr>) {
         &mut replay.user,
         replay.platform.as_str(),
         ip_address.map(|ip| IpAddr(ip.to_string())).as_ref(),
+        &replay.sdk,
     );
 }
 
@@ -310,8 +311,10 @@ mod tests {
             None,
         );
 
-        let ipaddr = get_value!(replay.user!).ip_address.as_str();
-        assert_eq!(Some("127.0.0.1"), ipaddr);
+        assert_eq!(
+            Annotated::empty(),
+            replay.0.unwrap().user.0.unwrap().ip_address
+        );
     }
 
     #[test]

relay-event-normalization/src/sdk_version.rs

@@ -0,0 +1,183 @@
+use std::str::FromStr;
+
+const DEFAULT_VERSION_IF_EMPTY: usize = 0;
+
+#[derive(PartialEq, Eq, Debug, thiserror::Error)]
+pub enum SdkVersionParseError {
+    #[error("Version contains invalid non numeric parts")]
+    VersionNotNumeric,
+
+    #[error("Release type was neither 'alpha' nor 'beta'")]
+    ReleaseTypeInvalid,
+}
+
+/// Represents an SDK Version using the semvar versioning, meaning MAJOR.MINOR.PATCH.
+/// An optional release type can be specified, then it becomes
+/// MAJOR.MINOR.PATCH-(alpha|beta).RELEASE_VERSION
+#[derive(Debug, PartialOrd, Eq, PartialEq, Ord)]
+pub struct SdkVersion {
+    major: usize,
+    minor: usize,
+    patch: usize,
+    release_type: ReleaseType,
+}
+
+/// Represents the release type which might be present in a version string,
+/// for example: 9.0.0-alpha.0.
+/// Release types are also comparable to each other, using the rules:
+/// alpha < beta < release.
+#[derive(Debug, Eq, PartialEq, Ord, PartialOrd)]
+enum ReleaseType {
+    Alpha(usize),
+    Beta(usize),
+    Release,
+}
+
+impl SdkVersion {
+    pub const fn new(major: usize, minor: usize, patch: usize) -> Self {
+        Self {
+            major,
+            minor,
+            patch,
+            release_type: ReleaseType::Release,
+        }
+    }
+}
+
+impl FromStr for SdkVersion {
+    type Err = SdkVersionParseError;
+
+    /// Attempts to parse a version string and returning a [`SdkVersionParseError`] if it contains any
+    /// non-numerical characters apart from dots or the release type.
+    /// If a version part is not provided, 0 will be assumed.
+    /// For example:
+    /// 1.2 -> 1.2.0
+    /// 1   -> 1.0.0
+    ///
+    /// Also supports the release types `alpha` and `beta` in the form `1.2.3-beta.1`.
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let mut split = s.split(".");
+        let major = split
+            .next()
+            .map(str::parse)
+            .transpose()
+            .map_err(|_| SdkVersionParseError::VersionNotNumeric)?
+            .unwrap_or(DEFAULT_VERSION_IF_EMPTY);
+        let minor = split
+            .next()
+            .map(str::parse)
+            .transpose()
+            .map_err(|_| SdkVersionParseError::VersionNotNumeric)?
+            .unwrap_or(DEFAULT_VERSION_IF_EMPTY);
+        let patch_segment = split.next();
+        let release_version = split
+            .next()
+            .map(str::parse)
+            .transpose()
+            .map_err(|_| SdkVersionParseError::VersionNotNumeric)?
+            .unwrap_or(DEFAULT_VERSION_IF_EMPTY);
+        let (patch, release_type) = if let Some(next) = patch_segment {
+            match next.split_once("-") {
+                Some((patch, release_type)) => (
+                    patch
+                        .parse()
+                        .map_err(|_| SdkVersionParseError::VersionNotNumeric)?,
+                    match release_type {
+                        "alpha" => ReleaseType::Alpha(release_version),
+                        "beta" => ReleaseType::Beta(release_version),
+                        _ => return Err(SdkVersionParseError::ReleaseTypeInvalid),
+                    },
+                ),
+                None => (
+                    next.parse()
+                        .map_err(|_| SdkVersionParseError::VersionNotNumeric)?,
+                    ReleaseType::Release,
+                ),
+            }
+        } else {
+            (DEFAULT_VERSION_IF_EMPTY, ReleaseType::Release)
+        };
+        Ok(Self {
+            major,
+            minor,
+            patch,
+            release_type,
+        })
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use crate::sdk_version::{SdkVersion, SdkVersionParseError};
+
+    #[test]
+    fn test_version_compare() {
+        let main_version = SdkVersion::new(1, 2, 3);
+        let less = SdkVersion::new(1, 2, 1);
+        let greater = SdkVersion::new(2, 1, 1);
+        let equal = SdkVersion::new(1, 2, 3);
+        assert!(main_version > less);
+        assert!(main_version < greater);
+        assert_eq!(main_version, equal);
+    }
+
+    #[test]
+    fn test_version_string_compare() {
+        let main_version: SdkVersion = "1.2.3".parse().unwrap();
+        let less = "1.2.1".parse().unwrap();
+        let greater = "2.1.1".parse().unwrap();
+        let equal = "1.2.3".parse().unwrap();
+        assert!(main_version > less);
+        assert!(main_version < greater);
+        assert_eq!(main_version, equal);
+    }
+
+    #[test]
+    fn test_release_type() {
+        let alpha: SdkVersion = "9.0.0-alpha.2".parse().unwrap();
+        let beta = "9.0.0-beta.2".parse().unwrap();
+        let release = "9.0.0".parse().unwrap();
+
+        assert!(alpha < beta);
+        assert!(beta < release);
+        assert!(alpha < release)
+    }
+
+    #[test]
+    fn test_invalid_release_type() {
+        assert_eq!(
+            "9.0.0-foobar.3".parse::<SdkVersion>(),
+            Err(SdkVersionParseError::ReleaseTypeInvalid)
+        );
+    }
+
+    #[test]
+    fn test_release_type_versions() {
+        let first_alpha: SdkVersion = "9.0.0-alpha.1".parse().unwrap();
+        let second_alpha = "9.0.0-alpha.2".parse().unwrap();
+
+        assert!(first_alpha < second_alpha);
+    }
+
+    #[test]
+    fn test_alpha_always_before_beta() {
+        let large_alpha: SdkVersion = "9.0.0-alpha.150".parse().unwrap();
+        let small_beta = "9.0.0-beta.0".parse().unwrap();
+
+        assert!(large_alpha < small_beta);
+    }
+
+    #[test]
+    fn test_version_defaults_to_zero() {
+        let only_major: SdkVersion = "9".parse().unwrap();
+        assert_eq!(only_major, SdkVersion::new(9, 0, 0))
+    }
+
+    #[test]
+    fn test_version_string_parse_failed() {
+        assert_eq!(
+            "amd64".parse::<SdkVersion>(),
+            Err(SdkVersionParseError::VersionNotNumeric)
+        );
+    }
+}

tests/fixtures/replay_missing_user_ip_address.json

@@ -30,7 +30,8 @@
     "user": {
         "id": "123",
         "username": "user",
-        "email": "user@site.com"
+        "email": "user@site.com",
+        "ip_address": "{{auto}}"
     },
     "request": {
         "url": null,