diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f42a774722..28feadb266d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ **Bug Fixes**: +- Fix Android trace and ANR profile parsing. Serialize Android trace chunks with `version: "2.android-trace"`. Custom + Android trace `profile_chunk` producers should send `version: "2.android-trace"`; legacy `version: "2"` is accepted + only for Android `sampled_profile` payloads. `version: "1"` and versionless Android trace chunks are rejected. + ([#6183](https://github.com/getsentry/relay/pull/6183)) - Defer dynamic sampling until metrics config is valid. ([#6246](https://github.com/getsentry/relay/pull/6246)) ## 26.7.1 diff --git a/relay-profiling/src/android/chunk.rs b/relay-profiling/src/android/chunk.rs index 62f14489e27..599777f25f2 100644 --- a/relay-profiling/src/android/chunk.rs +++ b/relay-profiling/src/android/chunk.rs @@ -17,6 +17,7 @@ use serde::{Deserialize, Serialize}; use crate::debug_image::get_proguard_image; use crate::measurements::ChunkMeasurement; +use crate::sample::Version; use crate::sample::v2::ProfileData; use crate::types::{ClientSdk, DebugMeta}; use crate::{MAX_PROFILE_CHUNK_DURATION, ProfileError}; @@ -35,6 +36,9 @@ pub struct Metadata { platform: String, release: String, + #[serde(default)] + version: Version, + #[serde(skip_serializing_if = "Option::is_none")] debug_meta: Option, @@ -114,6 +118,11 @@ impl Chunk { // Use duration given by the profiler and not reported by the SDK. profile.metadata.duration_ns = profile.profile.elapsed_time.as_nanos() as u64; + // Convert legacy Android trace version ("2") to the corrected version + // ("2.android-trace"). We do so during parsing rather than + // serialization because raw serde doesn't validate the trace payload. + profile.metadata.version = Version::V2AndroidTrace; + // If build_id is not empty but we don't have any DebugImage set, // we create the proper Proguard image and set the uuid. if !profile.metadata.build_id.is_empty() && profile.metadata.debug_meta.is_none() { @@ -179,6 +188,21 @@ mod tests { assert!(Chunk::parse(&(data.unwrap())[..]).is_ok()); } + #[test] + fn test_parse_corrects_android_trace_profile_version() { + let payload = include_bytes!("../../tests/fixtures/android/chunk/valid.json"); + let input: serde_json::Value = serde_json::from_slice(payload).unwrap(); + assert_eq!(input["version"], "2"); + + let profile = Chunk::parse(payload).unwrap(); + assert_eq!(profile.metadata.version, Version::V2AndroidTrace); + + let output = serde_json::to_value(&profile).unwrap(); + + assert_eq!(output["version"], "2.android-trace"); + assert!(output.get("sampled_profile").is_none()); + } + #[test] fn test_remove_invalid_events() { let payload = diff --git a/relay-profiling/src/lib.rs b/relay-profiling/src/lib.rs index 30469e719fb..fc6c4ace868 100644 --- a/relay-profiling/src/lib.rs +++ b/relay-profiling/src/lib.rs @@ -33,7 +33,7 @@ //! Each item type expects a different format. //! //! For `Profile` item type, we expect the Sample format v1 or Android format. -//! For `ProfileChunk` item type, we expect the Sample format v2. +//! For `ProfileChunk` item type, we expect the Sample format v2 or Android trace chunk format. //! //! # Ingestion //! diff --git a/relay-profiling/src/profile_chunk.rs b/relay-profiling/src/profile_chunk.rs index 78e928600b3..9751642b58e 100644 --- a/relay-profiling/src/profile_chunk.rs +++ b/relay-profiling/src/profile_chunk.rs @@ -1,7 +1,8 @@ use serde::Deserialize; use crate::{ - AndroidProfileChunk, PerfettoProfileChunk, ProfileError, ProfileType, V2ProfileChunk, sample, + AndroidProfileChunk, PerfettoProfileChunk, ProfileError, ProfileType, V2ProfileChunk, + sample::Version, }; /// Minimum interface all profile chunk types must implement. @@ -94,6 +95,7 @@ impl relay_filter::Filterable for AnyProfileChunk { } /// Either an [`AndroidProfileChunk`] or a [`V2ProfileChunk`]. +#[derive(Debug)] pub enum AndroidOrV2ProfileChunk { Android(Box), V2(Box), @@ -122,7 +124,9 @@ impl AndroidOrV2ProfileChunk { struct MinimalProfile { platform: String, #[serde(default)] - version: sample::Version, + version: Version, + #[serde(default)] + sampled_profile: Option, } let minimal: MinimalProfile = { @@ -130,16 +134,142 @@ impl AndroidOrV2ProfileChunk { serde_path_to_error::deserialize(d) }?; - match (minimal.platform.as_str(), minimal.version) { - // This has always been parsed with higher priority than `v2`, so this was kept as-is - // when refactoring, but from the looks of it, this may cause issues with v2 profiles - // which happen to be sent from android. - ("android", _) => AndroidProfileChunk::parse(data) + // Android SDKs produce two profile_chunk types that pass through this method: trace + // profiles and Application-Not-Responding (ANR) profiles. They come in multiple + // varieties, each of which needs to be accounted for. + + // Android trace profiles: + // --------------- + // Version: 2 (incorrect), 2.android-trace (corrected) + // Platform: android + // Content field: sampled_profile (i.e., Android Runtime's event-based format, aka + // "traces") + // Destination type: AndroidProfileChunk + + // Android ANR profiles: + // --------------- + // Version: 2 + // Platform: java (incorrect), android (corrected) + // Content field: profile (i.e., standardized stacks/frames/samples format) + // Destination type: V2ProfileChunk + + // We also need to handle non-Android profile chunks. + + // Non-Android profiles: + // --------------- + // Version: 2 + // Platform: cocoa, javascript, etc. + // Content field: profile (i.e., standardized stacks/frames/samples format) + // Destination type: V2ProfileChunk + + let is_android_trace_profile = + minimal.platform == "android" && minimal.sampled_profile.is_some(); + + match minimal.version { + Version::V2AndroidTrace => AndroidProfileChunk::parse(data) + .map(Box::new) + .map(Self::Android), + // Account for legacy submissions that don't use the 2.android-trace version. + Version::V2 if is_android_trace_profile => AndroidProfileChunk::parse(data) .map(Box::new) .map(Self::Android), - (_, sample::Version::V2) => V2ProfileChunk::parse(data).map(Box::new).map(Self::V2), - (_, sample::Version::V1) => Err(ProfileError::PlatformNotSupported), - (_, sample::Version::Unknown) => Err(ProfileError::PlatformNotSupported), + Version::V2 => V2ProfileChunk::parse(data).map(Box::new).map(Self::V2), + Version::V1 | Version::Unknown => Err(ProfileError::PlatformNotSupported), + } + } +} + +#[cfg(test)] +mod tests { + use std::assert_matches; + + use serde_json::{Value, json}; + + use super::*; + + #[test] + fn test_parse_correctly_versioned_android_trace_profile_into_android_profile_chunk() { + let mut payload: Value = + serde_json::from_slice(include_bytes!("../tests/fixtures/android/chunk/valid.json")) + .unwrap(); + payload["version"] = json!("2.android-trace"); + let data = serde_json::to_vec(&payload).unwrap(); + + // 1. Fresh SDK-shaped payload: `sampled_profile` populated, `profile` absent. + let sdk_chunk = AndroidOrV2ProfileChunk::parse(&data).unwrap(); + assert_matches!(sdk_chunk, AndroidOrV2ProfileChunk::Android(_)); + + // 2. Relay's own re-serialized shape: `sampled_profile` absent, `profile` populated. + let AndroidOrV2ProfileChunk::Android(android_chunk) = sdk_chunk else { + unreachable!() + }; + let reserialized = serde_json::to_vec(&android_chunk).unwrap(); + let value: Value = serde_json::from_slice(&reserialized).unwrap(); + assert!(value.get("sampled_profile").is_none()); + assert!(value.get("profile").is_some()); + + let round_tripped = AndroidOrV2ProfileChunk::parse(&reserialized).unwrap(); + assert_matches!(round_tripped, AndroidOrV2ProfileChunk::Android(_)); + } + + #[test] + fn test_parse_legacy_versioned_android_trace_profile_into_android_profile_chunk() { + let mut payload: Value = + serde_json::from_slice(include_bytes!("../tests/fixtures/android/chunk/valid.json")) + .unwrap(); + payload["version"] = json!("2"); + let data = serde_json::to_vec(&payload).unwrap(); + + let chunk = AndroidOrV2ProfileChunk::parse(&data).unwrap(); + assert_matches!(chunk, AndroidOrV2ProfileChunk::Android(_)); + } + + #[test] + fn test_parse_sample_v2_profile_into_v2_profile_chunk() { + let base_payload: Value = + serde_json::from_slice(include_bytes!("../tests/fixtures/sample/v2/valid.json")) + .unwrap(); + + for platform in ["android", "cocoa", "javascript", "python"] { + let mut payload = base_payload.clone(); + payload["platform"] = json!(platform); + let data = serde_json::to_vec(&payload).unwrap(); + + let chunk = AndroidOrV2ProfileChunk::parse(&data).unwrap(); + + assert_matches!(chunk, AndroidOrV2ProfileChunk::V2(_)); + } + } + + #[test] + fn test_return_error_for_version_1_profile() { + for payload in [ + &include_bytes!("../tests/fixtures/sample/v2/valid.json")[..], + &include_bytes!("../tests/fixtures/android/chunk/valid.json")[..], + &include_bytes!("../tests/fixtures/android/chunk/valid-rn.json")[..], + ] { + let mut payload: Value = serde_json::from_slice(payload).unwrap(); + payload["version"] = json!("1"); + let data = serde_json::to_vec(&payload).unwrap(); + + let err = AndroidOrV2ProfileChunk::parse(&data).unwrap_err(); + assert_matches!(err, ProfileError::PlatformNotSupported); + } + } + + #[test] + fn test_return_error_for_unknown_version_profile() { + for payload in [ + &include_bytes!("../tests/fixtures/sample/v2/valid.json")[..], + &include_bytes!("../tests/fixtures/android/chunk/valid.json")[..], + &include_bytes!("../tests/fixtures/android/chunk/valid-rn.json")[..], + ] { + let mut payload: Value = serde_json::from_slice(payload).unwrap(); + payload.as_object_mut().unwrap().remove("version"); + let data = serde_json::to_vec(&payload).unwrap(); + + let err = AndroidOrV2ProfileChunk::parse(&data).unwrap_err(); + assert_matches!(err, ProfileError::PlatformNotSupported); } } } diff --git a/relay-profiling/src/sample/mod.rs b/relay-profiling/src/sample/mod.rs index dd36828aebe..59241d29657 100644 --- a/relay-profiling/src/sample/mod.rs +++ b/relay-profiling/src/sample/mod.rs @@ -6,7 +6,7 @@ use relay_event_schema::protocol::Addr; pub mod v1; pub mod v2; -/// Possible values for the version field of the Sample Format. +/// Possible values for profile payload versions. #[derive(Debug, Serialize, Deserialize, Copy, Clone, Default, PartialEq, Eq)] pub enum Version { #[default] @@ -15,6 +15,9 @@ pub enum Version { V1, #[serde(rename = "2")] V2, + /// Special-cased chunk format for Android trace profiles, distinct from Sample Format V2. + #[serde(rename = "2.android-trace")] + V2AndroidTrace, } /// Holds information about a single stacktrace frame. diff --git a/tests/integration/test_profile_chunks.py b/tests/integration/test_profile_chunks.py index 0c4727758a2..92caded6884 100644 --- a/tests/integration/test_profile_chunks.py +++ b/tests/integration/test_profile_chunks.py @@ -1,3 +1,4 @@ +import json import uuid from copy import deepcopy from pathlib import Path @@ -321,6 +322,41 @@ def test_profile_chunk_outcomes_rate_limited_fast( assert mini_sentry.captured_envelopes.empty() +@pytest.mark.parametrize( + ["envelope_factory", "expected_version"], + [ + pytest.param(sample_profile_v2_envelope, "2", id="profile v2"), + pytest.param( + android_profile_chunk_envelope, + "2.android-trace", + id="android chunk", + ), + ], +) +def test_profile_chunk_version_is_forwarded( + mini_sentry, + relay_with_processing, + profiles_consumer, + envelope_factory, + expected_version, +): + profiles_consumer = profiles_consumer() + + project_id = 42 + project_config = mini_sentry.add_full_project_config(project_id)["config"] + + project_config.setdefault("features", []).append( + "organizations:continuous-profiling" + ) + + upstream = relay_with_processing(TEST_CONFIG) + upstream.send_envelope(project_id, envelope_factory()) + + profile, headers = profiles_consumer.get_profile() + assert headers == [("project_id", b"42")] + assert json.loads(profile["payload"])["version"] == expected_version + + @pytest.mark.parametrize( "platform, category, filter_context", [