From f2bb0bc18d050d269d04d579b54183f1b402a8e6 Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Mon, 12 Sep 2022 06:18:53 -0700 Subject: [PATCH 01/18] feat(profile): Introduce a common sample format --- Cargo.lock | 1 + relay-profiling/Cargo.toml | 3 +- relay-profiling/src/cocoa.rs | 55 +-- relay-profiling/src/error.rs | 2 + relay-profiling/src/lib.rs | 88 +++- relay-profiling/src/native_debug_image.rs | 58 +++ relay-profiling/src/rust.rs | 2 +- relay-profiling/src/sample.rs | 460 ++++++++++++++++++ .../cocoa/{valid.json => roundtrip.json} | 0 .../sample/multiple_transactions.json | 106 ++++ .../profiles/sample/no_transaction.json | 89 ++++ .../fixtures/profiles/sample/roundtrip.json | 99 ++++ 12 files changed, 892 insertions(+), 71 deletions(-) create mode 100644 relay-profiling/src/native_debug_image.rs create mode 100644 relay-profiling/src/sample.rs rename relay-profiling/tests/fixtures/profiles/cocoa/{valid.json => roundtrip.json} (100%) create mode 100644 relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json create mode 100644 relay-profiling/tests/fixtures/profiles/sample/no_transaction.json create mode 100644 relay-profiling/tests/fixtures/profiles/sample/roundtrip.json diff --git a/Cargo.lock b/Cargo.lock index fdc7500c128..37d5da87f2e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3377,6 +3377,7 @@ dependencies = [ "android_trace_log", "base64 0.10.1", "bytes 0.4.12", + "chrono", "failure", "relay-general", "serde", diff --git a/relay-profiling/Cargo.toml b/relay-profiling/Cargo.toml index ff315630afb..4e46ff1d5fe 100644 --- a/relay-profiling/Cargo.toml +++ b/relay-profiling/Cargo.toml @@ -10,10 +10,11 @@ license-file = "../LICENSE" publish = false [dependencies] -failure = "0.1.8" android_trace_log = { version = "0.2.0", features = ["serde"] } base64 = "0.10.1" bytes = { version = "0.4.12", features = ["serde"] } +chrono = { version = "0.4", features = ["serde"] } +failure = "0.1.8" relay-general = { path = "../relay-general" } serde = { version = "1.0.114", features = ["derive"] } serde_json = "1.0.55" diff --git a/relay-profiling/src/cocoa.rs b/relay-profiling/src/cocoa.rs index 6358cafd2bc..dd574678de1 100644 --- a/relay-profiling/src/cocoa.rs +++ b/relay-profiling/src/cocoa.rs @@ -2,11 +2,12 @@ use std::collections::HashMap; use serde::{de, Deserialize, Serialize}; -use relay_general::protocol::{Addr, DebugId, EventId, NativeImagePath}; +use relay_general::protocol::{Addr, EventId}; +use crate::error::ProfileError; +use crate::native_debug_image::NativeDebugImage; use crate::transaction_metadata::TransactionMetadata; use crate::utils::{deserialize_number_from_string, is_zero}; -use crate::ProfileError; fn strip_pointer_authentication_code<'de, D>(deserializer: D) -> Result<Addr, D::Error> where @@ -65,31 +66,6 @@ struct SampledProfile { queue_metadata: HashMap<String, QueueMetadata>, } -#[derive(Debug, Serialize, Deserialize, PartialEq, Clone)] -#[serde(rename_all = "lowercase")] -enum ImageType { - MachO, - Symbolic, -} - -#[derive(Debug, Serialize, Deserialize, PartialEq, Clone)] -pub struct NativeDebugImage { - #[serde(alias = "name")] - code_file: NativeImagePath, - #[serde(alias = "id")] - debug_id: DebugId, - image_addr: Addr, - - #[serde(default)] - image_vmaddr: Addr, - - #[serde(deserialize_with = "deserialize_number_from_string")] - image_size: u64, - - #[serde(rename = "type")] - image_type: ImageType, -} - #[derive(Debug, Serialize, Deserialize, Clone)] struct DebugMeta { images: Vec<NativeDebugImage>, @@ -233,38 +209,15 @@ fn parse_cocoa_profile(payload: &[u8]) -> Result<CocoaProfile, ProfileError> { mod tests { use super::*; - use relay_general::protocol::{DebugImage, NativeDebugImage as RelayNativeDebugImage}; - use relay_general::types::{Annotated, Map}; - #[test] fn test_roundtrip_cocoa() { - let payload = include_bytes!("../tests/fixtures/profiles/cocoa/valid.json"); + let payload = include_bytes!("../tests/fixtures/profiles/cocoa/roundtrip.json"); let profile = parse_cocoa_profile(payload); assert!(profile.is_ok()); let data = serde_json::to_vec(&profile.unwrap()); assert!(parse_cocoa_profile(&data.unwrap()[..]).is_ok()); } - #[test] - fn test_ios_debug_image_compatibility() { - let image_json = r#"{"debug_id":"32420279-25E2-34E6-8BC7-8A006A8F2425","image_addr":"0x000000010258c000","code_file":"/private/var/containers/Bundle/Application/C3511752-DD67-4FE8-9DA2-ACE18ADFAA61/TrendingMovies.app/TrendingMovies","type":"macho","image_size":1720320,"image_vmaddr":"0x0000000100000000"}"#; - let image: NativeDebugImage = serde_json::from_str(image_json).unwrap(); - let json = serde_json::to_string(&image).unwrap(); - let annotated = Annotated::from_json(&json[..]).unwrap(); - assert_eq!( - Annotated::new(DebugImage::MachO(Box::new(RelayNativeDebugImage { - arch: Annotated::empty(), - code_file: Annotated::new("/private/var/containers/Bundle/Application/C3511752-DD67-4FE8-9DA2-ACE18ADFAA61/TrendingMovies.app/TrendingMovies".into()), - code_id: Annotated::empty(), - debug_file: Annotated::empty(), - debug_id: Annotated::new("32420279-25E2-34E6-8BC7-8A006A8F2425".parse().unwrap()), - image_addr: Annotated::new(Addr(4334338048)), - image_size: Annotated::new(1720320), - image_vmaddr: Annotated::new(Addr(4294967296)), - other: Map::new(), - }))), annotated); - } - #[test] fn test_parse_multiple_transactions() { let payload = include_bytes!("../tests/fixtures/profiles/cocoa/multiple_transactions.json"); diff --git a/relay-profiling/src/error.rs b/relay-profiling/src/error.rs index 9535b825d04..b96f833a3e3 100644 --- a/relay-profiling/src/error.rs +++ b/relay-profiling/src/error.rs @@ -18,4 +18,6 @@ pub enum ProfileError { NoTransactionAssociated, #[fail(display = "invalid transaction metadata")] InvalidTransactionMetadata, + #[fail(display = "missing profile metadata")] + MissingProfileMetadata, } diff --git a/relay-profiling/src/lib.rs b/relay-profiling/src/lib.rs index e1286bc9d74..41d76cc0b20 100644 --- a/relay-profiling/src/lib.rs +++ b/relay-profiling/src/lib.rs @@ -93,13 +93,15 @@ //! } //! ``` -use serde::Deserialize; +use serde::{Deserialize, Serialize}; mod android; mod cocoa; mod error; +mod native_debug_image; mod python; mod rust; +mod sample; mod transaction_metadata; mod typescript; mod utils; @@ -108,13 +110,26 @@ use crate::android::expand_android_profile; use crate::cocoa::expand_cocoa_profile; use crate::python::parse_python_profile; use crate::rust::parse_rust_profile; +use crate::sample::expand_sample_profile; use crate::typescript::parse_typescript_profile; pub use crate::error::ProfileError; +#[derive(Debug, Serialize, Deserialize, Clone)] +#[serde(rename_all = "lowercase")] +enum Platform { + Android, + Cocoa, + Node, + Python, + Rust, + Typescript, +} + #[derive(Debug, Deserialize)] struct MinimalProfile { - platform: String, + platform: Platform, + version: Option<String>, } fn minimal_profile_from_json(data: &[u8]) -> Result<MinimalProfile, ProfileError> { @@ -122,22 +137,59 @@ fn minimal_profile_from_json(data: &[u8]) -> Result<MinimalProfile, ProfileError } pub fn expand_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileError> { - let minimal_profile: MinimalProfile = minimal_profile_from_json(payload)?; - let platform = minimal_profile.platform.as_str(); - match platform { - "android" => expand_android_profile(payload), - "cocoa" => expand_cocoa_profile(payload), - _ => { - let payload = match platform { - "python" => parse_python_profile(payload), - "rust" => parse_rust_profile(payload), - "typescript" => parse_typescript_profile(payload), - _ => Err(ProfileError::PlatformNotSupported), - }; - match payload { - Ok(payload) => Ok(vec![payload]), - Err(payload) => Err(payload), + let profile: MinimalProfile = minimal_profile_from_json(payload)?; + match profile.version { + Some(_) => expand_sample_profile(payload), + None => match profile.platform { + Platform::Android => expand_android_profile(payload), + Platform::Cocoa => expand_cocoa_profile(payload), + _ => { + let payload = match profile.platform { + Platform::Python => parse_python_profile(payload), + Platform::Rust => parse_rust_profile(payload), + Platform::Typescript => parse_typescript_profile(payload), + _ => Err(ProfileError::PlatformNotSupported), + }; + match payload { + Ok(payload) => Ok(vec![payload]), + Err(err) => Err(err), + } } - } + }, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_minimal_profile_with_version() { + let data = r#"{"version": "v1", "platform": "cocoa"}"#; + let profile = minimal_profile_from_json(data.as_bytes()); + assert!(profile.is_ok()); + assert!(profile.unwrap().version.is_some()); + } + + #[test] + fn test_minimal_profile_without_version() { + let data = r#"{"platform": "cocoa"}"#; + let profile = minimal_profile_from_json(data.as_bytes()); + assert!(profile.is_ok()); + assert!(profile.unwrap().version.is_none()); + } + + #[test] + fn test_expand_profile_with_version() { + let payload = include_bytes!("../tests/fixtures/profiles/sample/roundtrip.json"); + let profile = expand_profile(payload); + assert!(profile.is_ok()); + } + + #[test] + fn test_expand_profile_without_version() { + let payload = include_bytes!("../tests/fixtures/profiles/cocoa/roundtrip.json"); + let profile = expand_profile(payload); + assert!(profile.is_ok()); } } diff --git a/relay-profiling/src/native_debug_image.rs b/relay-profiling/src/native_debug_image.rs new file mode 100644 index 00000000000..d288c937f3b --- /dev/null +++ b/relay-profiling/src/native_debug_image.rs @@ -0,0 +1,58 @@ +use serde::{Deserialize, Serialize}; + +use relay_general::protocol::{Addr, DebugId, NativeImagePath}; + +use crate::utils::deserialize_number_from_string; + +#[derive(Debug, Serialize, Deserialize, PartialEq, Clone)] +#[serde(rename_all = "lowercase")] +enum ImageType { + MachO, + Symbolic, +} + +#[derive(Debug, Serialize, Deserialize, PartialEq, Clone)] +pub struct NativeDebugImage { + #[serde(alias = "name")] + code_file: NativeImagePath, + #[serde(alias = "id")] + debug_id: DebugId, + image_addr: Addr, + + #[serde(default)] + image_vmaddr: Addr, + + #[serde(deserialize_with = "deserialize_number_from_string")] + image_size: u64, + + #[serde(rename = "type")] + image_type: ImageType, +} + +#[cfg(test)] +mod tests { + use super::NativeDebugImage; + + use relay_general::protocol::{Addr, DebugImage, NativeDebugImage as RelayNativeDebugImage}; + use relay_general::types::{Annotated, Map}; + + #[test] + fn test_native_debug_image_compatibility() { + let image_json = r#"{"debug_id":"32420279-25E2-34E6-8BC7-8A006A8F2425","image_addr":"0x000000010258c000","code_file":"/private/var/containers/Bundle/Application/C3511752-DD67-4FE8-9DA2-ACE18ADFAA61/TrendingMovies.app/TrendingMovies","type":"macho","image_size":1720320,"image_vmaddr":"0x0000000100000000"}"#; + let image: NativeDebugImage = serde_json::from_str(image_json).unwrap(); + let json = serde_json::to_string(&image).unwrap(); + let annotated = Annotated::from_json(&json[..]).unwrap(); + assert_eq!( + Annotated::new(DebugImage::MachO(Box::new(RelayNativeDebugImage { + arch: Annotated::empty(), + code_file: Annotated::new("/private/var/containers/Bundle/Application/C3511752-DD67-4FE8-9DA2-ACE18ADFAA61/TrendingMovies.app/TrendingMovies".into()), + code_id: Annotated::empty(), + debug_file: Annotated::empty(), + debug_id: Annotated::new("32420279-25E2-34E6-8BC7-8A006A8F2425".parse().unwrap()), + image_addr: Annotated::new(Addr(4334338048)), + image_size: Annotated::new(1720320), + image_vmaddr: Annotated::new(Addr(4294967296)), + other: Map::new(), + }))), annotated); + } +} diff --git a/relay-profiling/src/rust.rs b/relay-profiling/src/rust.rs index b1e7ea63a17..4eb0a5a019e 100644 --- a/relay-profiling/src/rust.rs +++ b/relay-profiling/src/rust.rs @@ -2,7 +2,7 @@ use serde::{Deserialize, Serialize}; use relay_general::protocol::EventId; -use crate::cocoa::NativeDebugImage; +use crate::native_debug_image::NativeDebugImage; use crate::ProfileError; #[derive(Debug, Deserialize, Serialize)] diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs new file mode 100644 index 00000000000..7d902248b3e --- /dev/null +++ b/relay-profiling/src/sample.rs @@ -0,0 +1,460 @@ +use std::collections::HashMap; + +use chrono::{DateTime, Utc}; +use serde::{Deserialize, Serialize}; + +use relay_general::protocol::{Addr, EventId}; + +use crate::error::ProfileError; +use crate::native_debug_image::NativeDebugImage; +use crate::transaction_metadata::TransactionMetadata; +use crate::utils::{deserialize_number_from_string, is_zero}; +use crate::Platform; + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct Frame { + instruction_addr: Addr, +} + +impl Frame { + fn strip_pointer_authentication_code(&mut self, addr: u64) { + self.instruction_addr = Addr(self.instruction_addr.0 & addr); + } +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct Sample { + stack_id: u32, + #[serde(deserialize_with = "deserialize_number_from_string")] + thread_id: u64, + #[serde(deserialize_with = "deserialize_number_from_string")] + relative_timestamp_ns: u64, + + // cocoa only + #[serde(default, skip_serializing_if = "Option::is_none")] + queue_address: Option<String>, +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct ThreadMetadata { + #[serde(default, skip_serializing_if = "Option::is_none")] + is_main: Option<bool>, + #[serde(default, skip_serializing_if = "Option::is_none")] + is_active: Option<bool>, + #[serde(default, skip_serializing_if = "Option::is_none")] + name: Option<String>, + #[serde(default, skip_serializing_if = "Option::is_none")] + priority: Option<u32>, +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct QueueMetadata { + label: String, +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct Stack { + frames: Vec<Frame>, +} + +impl Stack { + fn strip_pointer_authentication_code(&mut self, addr: u64) { + for frame in &mut self.frames { + frame.strip_pointer_authentication_code(addr); + } + } +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct Profile { + samples: Vec<Sample>, + stacks: Vec<Stack>, + thread_metadata: HashMap<String, ThreadMetadata>, + + // cocoa only + #[serde(default, skip_serializing_if = "Option::is_none")] + queue_metadata: Option<HashMap<String, QueueMetadata>>, +} + +impl Profile { + fn strip_pointer_authentication_code(&mut self, platform: &Platform, architecture: &str) { + let addr = match (platform, architecture) { + // https://github.com/microsoft/plcrashreporter/blob/748087386cfc517936315c107f722b146b0ad1ab/Source/PLCrashAsyncThread_arm.c#L84 + (Platform::Cocoa, "arm64") | (Platform::Cocoa, "arm64e") => 0x0000000FFFFFFFFF, + _ => return, + }; + for stack in &mut self.stacks { + stack.strip_pointer_authentication_code(addr); + } + } +} + +#[derive(Default, Debug, Serialize, Deserialize, Clone)] +struct DebugMeta { + images: Vec<NativeDebugImage>, +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct OSMetadata { + name: String, + version: String, + + #[serde(default, skip_serializing_if = "Option::is_none")] + build_number: Option<String>, +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct RuntimeMetadata { + name: String, + version: String, +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct DeviceMetadata { + architecture: String, + + #[serde(default, skip_serializing_if = "Option::is_none")] + is_emulator: Option<bool>, + #[serde(default, skip_serializing_if = "Option::is_none")] + locale: Option<String>, + #[serde(default, skip_serializing_if = "Option::is_none")] + manufacturer: Option<String>, + #[serde(default, skip_serializing_if = "Option::is_none")] + model: Option<String>, +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +enum Version { + #[serde(rename = "1")] + V1, +} + +#[derive(Debug, Serialize, Deserialize, Clone)] +struct SampleProfile { + version: Version, + + #[serde(skip_serializing_if = "Option::is_none")] + debug_meta: Option<DebugMeta>, + + device: DeviceMetadata, + os: OSMetadata, + #[serde(skip_serializing_if = "Option::is_none")] + runtime: Option<RuntimeMetadata>, + + #[serde(default, skip_serializing_if = "String::is_empty")] + environment: String, + #[serde(alias = "profile_id")] + event_id: EventId, + platform: Platform, + profile: Profile, + release: String, + timestamp: DateTime<Utc>, + + #[serde( + default, + deserialize_with = "deserialize_number_from_string", + skip_serializing_if = "is_zero" + )] + duration_ns: u64, + #[serde(default, skip_serializing_if = "EventId::is_nil")] + trace_id: EventId, + #[serde(default, skip_serializing_if = "EventId::is_nil")] + transaction_id: EventId, + #[serde(default, skip_serializing_if = "String::is_empty")] + transaction_name: String, + + #[serde(default, skip_serializing_if = "Vec::is_empty")] + transactions: Vec<TransactionMetadata>, +} + +impl SampleProfile { + fn valid(&self) -> bool { + match self.platform { + Platform::Cocoa => { + self.os.build_number.is_some() + && self.device.is_emulator.is_some() + && self.device.locale.is_some() + && self.device.manufacturer.is_some() + && self.device.model.is_some() + } + Platform::Python => self.runtime.is_some(), + Platform::Node => self.runtime.is_some(), + _ => true, + } + } + + fn set_transaction(&mut self, transaction: &TransactionMetadata) { + self.transaction_name = transaction.name.clone(); + self.transaction_id = transaction.id; + self.trace_id = transaction.trace_id; + self.duration_ns = transaction.relative_end_ns - transaction.relative_start_ns; + } + + fn has_transaction_metadata(&self) -> bool { + !self.transaction_name.is_empty() && self.duration_ns > 0 + } + + /// Removes a sample when it's the only sample on its thread + fn remove_single_samples_per_thread(&mut self) { + let mut sample_count_by_thread_id: HashMap<u64, u32> = HashMap::new(); + + for sample in self.profile.samples.iter() { + *sample_count_by_thread_id + .entry(sample.thread_id) + .or_default() += 1; + } + + // Only keep data from threads with more than 1 sample so we can calculate a duration + sample_count_by_thread_id.retain(|_, count| *count > 1); + + self.profile + .samples + .retain(|sample| sample_count_by_thread_id.contains_key(&sample.thread_id)); + } + + fn strip_pointer_authentication_code(&mut self) { + self.profile + .strip_pointer_authentication_code(&self.platform, &self.device.architecture); + } +} + +pub fn expand_sample_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileError> { + let profile = parse_profile(payload)?; + + if !profile.valid() { + return Err(ProfileError::MissingProfileMetadata); + } + + let mut items: Vec<Vec<u8>> = Vec::new(); + + if profile.transactions.is_empty() && profile.has_transaction_metadata() { + match serde_json::to_vec(&profile) { + Ok(payload) => items.push(payload), + Err(_) => { + return Err(ProfileError::CannotSerializePayload); + } + }; + return Ok(items); + } + + for transaction in &profile.transactions { + let mut new_profile = profile.clone(); + + new_profile.set_transaction(transaction); + new_profile.transactions.clear(); + + new_profile.profile.samples.retain_mut(|sample| { + if transaction.relative_start_ns <= sample.relative_timestamp_ns + && sample.relative_timestamp_ns <= transaction.relative_end_ns + { + sample.relative_timestamp_ns -= transaction.relative_start_ns; + true + } else { + false + } + }); + + match serde_json::to_vec(&new_profile) { + Ok(payload) => items.push(payload), + Err(_) => { + return Err(ProfileError::CannotSerializePayload); + } + }; + } + + Ok(items) +} + +fn parse_profile(payload: &[u8]) -> Result<SampleProfile, ProfileError> { + let mut profile: SampleProfile = + serde_json::from_slice(payload).map_err(ProfileError::InvalidJson)?; + + profile.remove_single_samples_per_thread(); + + if profile.profile.samples.is_empty() { + return Err(ProfileError::NotEnoughSamples); + } + + if profile.transactions.is_empty() && !profile.has_transaction_metadata() { + return Err(ProfileError::NoTransactionAssociated); + } + + for transaction in profile.transactions.iter() { + if !transaction.valid() { + return Err(ProfileError::InvalidTransactionMetadata); + } + } + + profile.strip_pointer_authentication_code(); + + Ok(profile) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_roundtrip() { + let payload = include_bytes!("../tests/fixtures/profiles/sample/roundtrip.json"); + let profile = parse_profile(payload); + assert!(profile.is_ok()); + let data = serde_json::to_vec(&profile.unwrap()); + assert!(parse_profile(&data.unwrap()[..]).is_ok()); + } + + #[test] + fn test_expand() { + let payload = include_bytes!("../tests/fixtures/profiles/sample/roundtrip.json"); + let profile = expand_sample_profile(payload); + assert!(profile.is_ok()); + } + + #[test] + fn test_parse_multiple_transactions() { + let payload = + include_bytes!("../tests/fixtures/profiles/sample/multiple_transactions.json"); + let data = parse_profile(payload); + assert!(data.is_ok()); + } + + #[test] + fn test_no_transaction() { + let payload = include_bytes!("../tests/fixtures/profiles/sample/no_transaction.json"); + let data = parse_profile(payload); + assert!(data.is_err()); + } + + fn generate_profile() -> SampleProfile { + SampleProfile { + debug_meta: Option::None, + version: Version::V1, + timestamp: Utc::now(), + runtime: Option::None, + device: DeviceMetadata { + architecture: "arm64e".to_string(), + is_emulator: Some(true), + locale: Some("en_US".to_string()), + manufacturer: Some("Apple".to_string()), + model: Some("iPhome11,3".to_string()), + }, + os: OSMetadata { + build_number: Some("H3110".to_string()), + name: "iOS".to_string(), + version: "16.0".to_string(), + }, + duration_ns: 1337, + environment: "testing".to_string(), + platform: Platform::Cocoa, + event_id: EventId::new(), + profile: Profile { + queue_metadata: Some(HashMap::new()), + samples: Vec::new(), + stacks: Vec::new(), + thread_metadata: HashMap::new(), + }, + trace_id: EventId::new(), + transaction_id: EventId::new(), + transaction_name: "test".to_string(), + transactions: Vec::new(), + release: "1.0 (9999)".to_string(), + } + } + + #[test] + fn test_filter_samples() { + let mut profile = generate_profile(); + + profile.profile.stacks.push(Stack { frames: Vec::new() }); + profile.profile.samples.extend(vec![ + Sample { + stack_id: 0, + queue_address: Some("0xdeadbeef".to_string()), + relative_timestamp_ns: 1, + thread_id: 1, + }, + Sample { + stack_id: 0, + queue_address: Some("0xdeadbeef".to_string()), + relative_timestamp_ns: 1, + thread_id: 1, + }, + Sample { + stack_id: 0, + queue_address: Some("0xdeadbeef".to_string()), + relative_timestamp_ns: 1, + thread_id: 2, + }, + Sample { + stack_id: 0, + queue_address: Some("0xdeadbeef".to_string()), + relative_timestamp_ns: 1, + thread_id: 3, + }, + ]); + + profile.remove_single_samples_per_thread(); + + assert!(profile.profile.samples.len() == 2); + } + + #[test] + fn test_parse_profile_with_all_samples_filtered() { + let mut profile = generate_profile(); + + profile.profile.stacks.push(Stack { frames: Vec::new() }); + profile.profile.samples.extend(vec![ + Sample { + stack_id: 0, + queue_address: Some("0xdeadbeef".to_string()), + relative_timestamp_ns: 1, + thread_id: 1, + }, + Sample { + stack_id: 0, + queue_address: Some("0xdeadbeef".to_string()), + relative_timestamp_ns: 1, + thread_id: 2, + }, + Sample { + stack_id: 0, + queue_address: Some("0xdeadbeef".to_string()), + relative_timestamp_ns: 1, + thread_id: 3, + }, + Sample { + stack_id: 0, + queue_address: Some("0xdeadbeef".to_string()), + relative_timestamp_ns: 1, + thread_id: 4, + }, + ]); + + let payload = serde_json::to_vec(&profile).unwrap(); + assert!(parse_profile(&payload[..]).is_err()); + } + + #[test] + fn test_expand_multiple_transactions() { + let payload = + include_bytes!("../tests/fixtures/profiles/sample/multiple_transactions.json"); + let data = expand_sample_profile(payload); + assert!(data.is_ok()); + assert_eq!(data.as_ref().unwrap().len(), 2); + + let profile = match parse_profile(&data.as_ref().unwrap()[0][..]) { + Err(err) => panic!("cannot parse profile: {:?}", err), + Ok(profile) => profile, + }; + assert_eq!( + profile.transaction_id, + "30976f2ddbe04ac9b6bffe6e35d4710c".parse().unwrap() + ); + assert_eq!(profile.duration_ns, 50000000); + assert_eq!(profile.profile.samples.len(), 4); + + for sample in &profile.profile.samples { + assert!(sample.relative_timestamp_ns < profile.duration_ns); + } + } +} diff --git a/relay-profiling/tests/fixtures/profiles/cocoa/valid.json b/relay-profiling/tests/fixtures/profiles/cocoa/roundtrip.json similarity index 100% rename from relay-profiling/tests/fixtures/profiles/cocoa/valid.json rename to relay-profiling/tests/fixtures/profiles/cocoa/roundtrip.json diff --git a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json new file mode 100644 index 00000000000..6a8a1ebf1b8 --- /dev/null +++ b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json @@ -0,0 +1,106 @@ +{ + "event_id": "41fed0925670468bb0457f61a74688ec", + "version": "1", + "os": { + "name": "iOS", + "version": "16.0", + "build_number": "19H253" + }, + "device": { + "architecture": "arm64e", + "is_emulator": false, + "locale": "en_US", + "manufacturer": "Apple", + "model": "iPhone14,3" + }, + "timestamp": "2022-09-01T09:45:00.000Z", + "profile": { + "samples": [ + { + "stack_id": 0, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "10500500" + }, + { + "stack_id": 1, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "20500500" + }, + { + "stack_id": 0, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "30500500" + }, + { + "stack_id": 1, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "40500500" + } + ], + "stacks": [ + { + "frames": [ + { + "instruction_addr": "0xa722447ffffffffc" + } + ] + }, + { + "frames": [ + { + "instruction_addr": "0x442e4b81f5031e58" + } + ] + } + ], + "thread_metadata": { + "1": { + "priority": 31, + "is_main": true + }, + "2": {} + }, + "queue_metadata": { + "0x0000000102adc700": { + "label": "com.apple.main-thread" + }, + "0x000000016d8fb180": { + "label": "com.apple.network.connections" + } + } + }, + "release": "0.1 (199)", + "platform": "cocoa", + "debug_meta": { + "images": [ + { + "debug_id": "32420279-25E2-34E6-8BC7-8A006A8F2425", + "image_addr": "0x000000010258c000", + "code_file": "/private/var/containers/Bundle/Application/C3511752-DD67-4FE8-9DA2-ACE18ADFAA61/TrendingMovies.app/TrendingMovies", + "type": "macho", + "image_size": 1720320, + "image_vmaddr": "0x0000000100000000" + } + ] + }, + "transactions": [ + { + "name": "example_ios_movies_sources.MoviesViewController", + "trace_id": "4b25bc58f14243d8b208d1e22a054164", + "id": "30976f2ddbe04ac9b6bffe6e35d4710c", + "relative_start_ns": "500500", + "relative_end_ns": "50500500" + }, + { + "name": "example_ios_movies_sources.MoviesViewController", + "trace_id": "4b25bc58f14243d8b208d1e22a054164", + "id": "30976f2ddbe04ac9b6bffe6e35d4710c", + "relative_start_ns": "500500", + "relative_end_ns": "50500500" + } + ] +} diff --git a/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json b/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json new file mode 100644 index 00000000000..b662e330730 --- /dev/null +++ b/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json @@ -0,0 +1,89 @@ +{ + "event_id": "41fed0925670468bb0457f61a74688ec", + "version": "1", + "os": { + "name": "iOS", + "version": "16.0" + }, + "device": { + "architecture": "arm64e", + "is_emulator": false, + "locale": "en_US", + "manufacturer": "Apple", + "model": "iPhone14,3" + }, + "timestamp": "2022-09-01T09:45:00.000Z", + "profile": { + "samples": [ + { + "stack_id": 0, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "10500500" + }, + { + "stack_id": 1, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "20500500" + }, + { + "stack_id": 0, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "30500500" + }, + { + "stack_id": 1, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "40500500" + } + ], + "stacks": [ + { + "frames": [ + { + "instruction_addr": "0xa722447ffffffffc" + } + ] + }, + { + "frames": [ + { + "instruction_addr": "0x442e4b81f5031e58" + } + ] + } + ], + "thread_metadata": { + "1": { + "priority": 31, + "is_main": true + }, + "2": {} + }, + "queue_metadata": { + "0x0000000102adc700": { + "label": "com.apple.main-thread" + }, + "0x000000016d8fb180": { + "label": "com.apple.network.connections" + } + } + }, + "release": "0.1 (199)", + "platform": "cocoa", + "debug_meta": { + "images": [ + { + "debug_id": "32420279-25E2-34E6-8BC7-8A006A8F2425", + "image_addr": "0x000000010258c000", + "code_file": "/private/var/containers/Bundle/Application/C3511752-DD67-4FE8-9DA2-ACE18ADFAA61/TrendingMovies.app/TrendingMovies", + "type": "macho", + "image_size": 1720320, + "image_vmaddr": "0x0000000100000000" + } + ] + } +} diff --git a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json new file mode 100644 index 00000000000..c891af18777 --- /dev/null +++ b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json @@ -0,0 +1,99 @@ +{ + "event_id": "41fed0925670468bb0457f61a74688ec", + "version": "1", + "os": { + "name": "iOS", + "version": "16.0", + "build_number": "19H253" + }, + "device": { + "architecture": "arm64e", + "is_emulator": false, + "locale": "en_US", + "manufacturer": "Apple", + "model": "iPhone14,3" + }, + "timestamp": "2022-09-01T09:45:00.000Z", + "profile": { + "samples": [ + { + "stack_id": 0, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "10500500" + }, + { + "stack_id": 1, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "20500500" + }, + { + "stack_id": 0, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "30500500" + }, + { + "stack_id": 1, + "thread_id": "1", + "queue_address": "0x0000000102adc700", + "relative_timestamp_ns": "40500500" + } + ], + "stacks": [ + { + "frames": [ + { + "instruction_addr": "0xa722447ffffffffc" + } + ] + }, + { + "frames": [ + { + "instruction_addr": "0x442e4b81f5031e58" + } + ] + } + ], + "thread_metadata": { + "1": { + "priority": 31, + "is_main": true + }, + "2": {} + }, + "queue_metadata": { + "0x0000000102adc700": { + "label": "com.apple.main-thread" + }, + "0x000000016d8fb180": { + "label": "com.apple.network.connections" + } + } + }, + "release": "0.1 (199)", + "platform": "cocoa", + "debug_meta": { + "images": [ + { + "debug_id": "32420279-25E2-34E6-8BC7-8A006A8F2425", + "image_addr": "0x000000010258c000", + "code_file": "/private/var/containers/Bundle/Application/C3511752-DD67-4FE8-9DA2-ACE18ADFAA61/TrendingMovies.app/TrendingMovies", + "type": "macho", + "image_size": 1720320, + "image_vmaddr": "0x0000000100000000" + } + ] + }, + "transactions": [ + { + "name": "example_ios_movies_sources.MoviesViewController", + "trace_id": "4b25bc58f14243d8b208d1e22a054164", + "id": "30976f2ddbe04ac9b6bffe6e35d4710c", + "relative_start_ns": "500500", + "relative_end_ns": "50500500" + } + ] +} From 81561f1a2a62ec933a0c7529ba9650275b198fde Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Mon, 12 Sep 2022 16:03:28 -0700 Subject: [PATCH 02/18] Remove unnecessary code --- relay-profiling/src/android.rs | 2 +- relay-profiling/src/cocoa.rs | 2 +- relay-profiling/src/sample.rs | 62 +++++---------------- relay-profiling/src/transaction_metadata.rs | 6 +- 4 files changed, 21 insertions(+), 51 deletions(-) diff --git a/relay-profiling/src/android.rs b/relay-profiling/src/android.rs index c75167511b0..da7a96acc01 100644 --- a/relay-profiling/src/android.rs +++ b/relay-profiling/src/android.rs @@ -94,7 +94,7 @@ impl AndroidProfile { self.transaction_name = transaction.name.clone(); self.transaction_id = transaction.id; self.trace_id = transaction.trace_id; - self.duration_ns = transaction.relative_end_ns - transaction.relative_start_ns; + self.duration_ns = transaction.duration_ns(); } fn has_transaction_metadata(&self) -> bool { diff --git a/relay-profiling/src/cocoa.rs b/relay-profiling/src/cocoa.rs index dd574678de1..8147df3810a 100644 --- a/relay-profiling/src/cocoa.rs +++ b/relay-profiling/src/cocoa.rs @@ -113,7 +113,7 @@ impl CocoaProfile { self.transaction_name = transaction.name.clone(); self.transaction_id = transaction.id; self.trace_id = transaction.trace_id; - self.duration_ns = transaction.relative_end_ns - transaction.relative_start_ns; + self.duration_ns = transaction.duration_ns(); } fn has_transaction_metadata(&self) -> bool { diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 7d902248b3e..98b5d3fb0dc 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -8,7 +8,7 @@ use relay_general::protocol::{Addr, EventId}; use crate::error::ProfileError; use crate::native_debug_image::NativeDebugImage; use crate::transaction_metadata::TransactionMetadata; -use crate::utils::{deserialize_number_from_string, is_zero}; +use crate::utils::deserialize_number_from_string; use crate::Platform; #[derive(Debug, Serialize, Deserialize, Clone)] @@ -150,19 +150,6 @@ struct SampleProfile { release: String, timestamp: DateTime<Utc>, - #[serde( - default, - deserialize_with = "deserialize_number_from_string", - skip_serializing_if = "is_zero" - )] - duration_ns: u64, - #[serde(default, skip_serializing_if = "EventId::is_nil")] - trace_id: EventId, - #[serde(default, skip_serializing_if = "EventId::is_nil")] - transaction_id: EventId, - #[serde(default, skip_serializing_if = "String::is_empty")] - transaction_name: String, - #[serde(default, skip_serializing_if = "Vec::is_empty")] transactions: Vec<TransactionMetadata>, } @@ -183,17 +170,6 @@ impl SampleProfile { } } - fn set_transaction(&mut self, transaction: &TransactionMetadata) { - self.transaction_name = transaction.name.clone(); - self.transaction_id = transaction.id; - self.trace_id = transaction.trace_id; - self.duration_ns = transaction.relative_end_ns - transaction.relative_start_ns; - } - - fn has_transaction_metadata(&self) -> bool { - !self.transaction_name.is_empty() && self.duration_ns > 0 - } - /// Removes a sample when it's the only sample on its thread fn remove_single_samples_per_thread(&mut self) { let mut sample_count_by_thread_id: HashMap<u64, u32> = HashMap::new(); @@ -227,21 +203,11 @@ pub fn expand_sample_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileErro let mut items: Vec<Vec<u8>> = Vec::new(); - if profile.transactions.is_empty() && profile.has_transaction_metadata() { - match serde_json::to_vec(&profile) { - Ok(payload) => items.push(payload), - Err(_) => { - return Err(ProfileError::CannotSerializePayload); - } - }; - return Ok(items); - } - for transaction in &profile.transactions { let mut new_profile = profile.clone(); - new_profile.set_transaction(transaction); new_profile.transactions.clear(); + new_profile.transactions.push(transaction.clone()); new_profile.profile.samples.retain_mut(|sample| { if transaction.relative_start_ns <= sample.relative_timestamp_ns @@ -275,7 +241,7 @@ fn parse_profile(payload: &[u8]) -> Result<SampleProfile, ProfileError> { return Err(ProfileError::NotEnoughSamples); } - if profile.transactions.is_empty() && !profile.has_transaction_metadata() { + if profile.transactions.is_empty() { return Err(ProfileError::NoTransactionAssociated); } @@ -343,7 +309,6 @@ mod tests { name: "iOS".to_string(), version: "16.0".to_string(), }, - duration_ns: 1337, environment: "testing".to_string(), platform: Platform::Cocoa, event_id: EventId::new(), @@ -353,9 +318,6 @@ mod tests { stacks: Vec::new(), thread_metadata: HashMap::new(), }, - trace_id: EventId::new(), - transaction_id: EventId::new(), - transaction_name: "test".to_string(), transactions: Vec::new(), release: "1.0 (9999)".to_string(), } @@ -438,23 +400,27 @@ mod tests { fn test_expand_multiple_transactions() { let payload = include_bytes!("../tests/fixtures/profiles/sample/multiple_transactions.json"); + let data = expand_sample_profile(payload); assert!(data.is_ok()); assert_eq!(data.as_ref().unwrap().len(), 2); - let profile = match parse_profile(&data.as_ref().unwrap()[0][..]) { + let original_profile = match parse_profile(payload) { + Err(err) => panic!("cannot parse profile: {:?}", err), + Ok(profile) => profile, + }; + let expanded_profile = match parse_profile(&data.as_ref().unwrap()[0][..]) { Err(err) => panic!("cannot parse profile: {:?}", err), Ok(profile) => profile, }; assert_eq!( - profile.transaction_id, - "30976f2ddbe04ac9b6bffe6e35d4710c".parse().unwrap() + original_profile.transactions[0], + expanded_profile.transactions[0] ); - assert_eq!(profile.duration_ns, 50000000); - assert_eq!(profile.profile.samples.len(), 4); + assert_eq!(expanded_profile.profile.samples.len(), 4); - for sample in &profile.profile.samples { - assert!(sample.relative_timestamp_ns < profile.duration_ns); + for sample in &expanded_profile.profile.samples { + assert!(sample.relative_timestamp_ns < expanded_profile.transactions[0].duration_ns()); } } } diff --git a/relay-profiling/src/transaction_metadata.rs b/relay-profiling/src/transaction_metadata.rs index 08444bf9e02..fa6493160e9 100644 --- a/relay-profiling/src/transaction_metadata.rs +++ b/relay-profiling/src/transaction_metadata.rs @@ -4,7 +4,7 @@ use relay_general::protocol::EventId; use crate::utils::deserialize_number_from_string; -#[derive(Debug, Serialize, Deserialize, Clone)] +#[derive(Debug, Serialize, Deserialize, Clone, Eq, PartialEq)] pub struct TransactionMetadata { pub id: EventId, pub name: String, @@ -30,6 +30,10 @@ impl TransactionMetadata { && self.relative_start_ns < self.relative_end_ns && self.relative_cpu_start_ms <= self.relative_cpu_end_ms } + + pub fn duration_ns(&self) -> u64 { + self.relative_end_ns - self.relative_start_ns + } } #[cfg(test)] From edd820ddb64829f26ded02f2bff8b4879907c5be Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Mon, 12 Sep 2022 16:42:24 -0700 Subject: [PATCH 03/18] Support frame indexing --- relay-profiling/src/sample.rs | 25 ++++++------------- .../sample/multiple_transactions.json | 18 ++++++------- .../profiles/sample/no_transaction.json | 18 ++++++------- .../fixtures/profiles/sample/roundtrip.json | 16 +++++------- 4 files changed, 27 insertions(+), 50 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 98b5d3fb0dc..84e0136d624 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -52,23 +52,11 @@ struct QueueMetadata { label: String, } -#[derive(Debug, Serialize, Deserialize, Clone)] -struct Stack { - frames: Vec<Frame>, -} - -impl Stack { - fn strip_pointer_authentication_code(&mut self, addr: u64) { - for frame in &mut self.frames { - frame.strip_pointer_authentication_code(addr); - } - } -} - #[derive(Debug, Serialize, Deserialize, Clone)] struct Profile { samples: Vec<Sample>, - stacks: Vec<Stack>, + stacks: Vec<Vec<u32>>, + frames: Vec<Frame>, thread_metadata: HashMap<String, ThreadMetadata>, // cocoa only @@ -83,8 +71,8 @@ impl Profile { (Platform::Cocoa, "arm64") | (Platform::Cocoa, "arm64e") => 0x0000000FFFFFFFFF, _ => return, }; - for stack in &mut self.stacks { - stack.strip_pointer_authentication_code(addr); + for frame in &mut self.frames { + frame.strip_pointer_authentication_code(addr); } } } @@ -316,6 +304,7 @@ mod tests { queue_metadata: Some(HashMap::new()), samples: Vec::new(), stacks: Vec::new(), + frames: Vec::new(), thread_metadata: HashMap::new(), }, transactions: Vec::new(), @@ -327,7 +316,7 @@ mod tests { fn test_filter_samples() { let mut profile = generate_profile(); - profile.profile.stacks.push(Stack { frames: Vec::new() }); + profile.profile.stacks.push(Vec::new()); profile.profile.samples.extend(vec![ Sample { stack_id: 0, @@ -364,7 +353,7 @@ mod tests { fn test_parse_profile_with_all_samples_filtered() { let mut profile = generate_profile(); - profile.profile.stacks.push(Stack { frames: Vec::new() }); + profile.profile.stacks.push(Vec::new()); profile.profile.samples.extend(vec![ Sample { stack_id: 0, diff --git a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json index 6a8a1ebf1b8..2ba304444b2 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json +++ b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json @@ -41,22 +41,18 @@ "relative_timestamp_ns": "40500500" } ], - "stacks": [ + "frames": [ { - "frames": [ - { - "instruction_addr": "0xa722447ffffffffc" - } - ] + "instruction_addr": "0xa722447ffffffffc" }, { - "frames": [ - { - "instruction_addr": "0x442e4b81f5031e58" - } - ] + "instruction_addr": "0x442e4b81f5031e58" } ], + "stacks": [ + [0], + [1] + ], "thread_metadata": { "1": { "priority": 31, diff --git a/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json b/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json index b662e330730..d0209581eca 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json +++ b/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json @@ -40,22 +40,18 @@ "relative_timestamp_ns": "40500500" } ], - "stacks": [ + "frames": [ { - "frames": [ - { - "instruction_addr": "0xa722447ffffffffc" - } - ] + "instruction_addr": "0xa722447ffffffffc" }, { - "frames": [ - { - "instruction_addr": "0x442e4b81f5031e58" - } - ] + "instruction_addr": "0x442e4b81f5031e58" } ], + "stacks": [ + [0], + [1] + ], "thread_metadata": { "1": { "priority": 31, diff --git a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json index c891af18777..0ca9147d13d 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json +++ b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json @@ -42,19 +42,15 @@ } ], "stacks": [ + [0], + [1] + ], + "frames": [ { - "frames": [ - { - "instruction_addr": "0xa722447ffffffffc" - } - ] + "instruction_addr": "0xa722447ffffffffc" }, { - "frames": [ - { - "instruction_addr": "0x442e4b81f5031e58" - } - ] + "instruction_addr": "0x442e4b81f5031e58" } ], "thread_metadata": { From 07923cc68a119169a2774ef8bc9b1acfee9ae7bb Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Mon, 12 Sep 2022 17:05:39 -0700 Subject: [PATCH 04/18] Support frames without addresses --- relay-profiling/src/sample.rs | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 84e0136d624..eb4fba0ea0a 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -13,12 +13,26 @@ use crate::Platform; #[derive(Debug, Serialize, Deserialize, Clone)] struct Frame { - instruction_addr: Addr, + #[serde(default, skip_serializing_if = "Option::is_none")] + instruction_addr: Option<Addr>, + #[serde(default, skip_serializing_if = "Option::is_none")] + name: Option<String>, + #[serde(default, skip_serializing_if = "Option::is_none")] + line: Option<String>, + #[serde(default, skip_serializing_if = "Option::is_none")] + file: Option<String>, } impl Frame { - fn strip_pointer_authentication_code(&mut self, addr: u64) { - self.instruction_addr = Addr(self.instruction_addr.0 & addr); + fn valid(&self) -> bool { + self.instruction_addr.is_some() + || (self.name.is_some() && self.line.is_some() && self.file.is_some()) + } + + fn strip_pointer_authentication_code(&mut self, pac_code: u64) { + if let Some(address) = self.instruction_addr { + self.instruction_addr = Some(Addr(address.0 & pac_code)); + } } } @@ -229,15 +243,15 @@ fn parse_profile(payload: &[u8]) -> Result<SampleProfile, ProfileError> { return Err(ProfileError::NotEnoughSamples); } + profile + .transactions + .retain(|transaction| transaction.valid()); + if profile.transactions.is_empty() { return Err(ProfileError::NoTransactionAssociated); } - for transaction in profile.transactions.iter() { - if !transaction.valid() { - return Err(ProfileError::InvalidTransactionMetadata); - } - } + profile.profile.frames.retain(|frame| frame.valid()); profile.strip_pointer_authentication_code(); From e1221c3498270e3dc03c34cafc118d2a4240ea7e Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Mon, 12 Sep 2022 17:05:46 -0700 Subject: [PATCH 05/18] Add a changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5992a77db99..6fd0b1d1a0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ - Add InvalidReplayEvent outcome. ([#1455](https://github.com/getsentry/relay/pull/1455)) - Add replay and replay-recording rate limiter. ([#1456](https://github.com/getsentry/relay/pull/1456)) - Support profiles tagged for many transactions. ([#1444](https://github.com/getsentry/relay/pull/1444)) +- Introduce a new profile format called `sample`. ([#1462](https://github.com/getsentry/relay/pull/1462)) **Features**: From c5b814628150c674575e24b47143dd0b4397999a Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Tue, 13 Sep 2022 13:58:36 -0700 Subject: [PATCH 06/18] Use more efficient syntax where suggested --- relay-profiling/src/lib.rs | 5 +---- relay-profiling/src/sample.rs | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/relay-profiling/src/lib.rs b/relay-profiling/src/lib.rs index 41d76cc0b20..b605e0406ec 100644 --- a/relay-profiling/src/lib.rs +++ b/relay-profiling/src/lib.rs @@ -150,10 +150,7 @@ pub fn expand_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileError> { Platform::Typescript => parse_typescript_profile(payload), _ => Err(ProfileError::PlatformNotSupported), }; - match payload { - Ok(payload) => Ok(vec![payload]), - Err(err) => Err(err), - } + Ok(vec![payload?]) } }, } diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index eb4fba0ea0a..141c9c9e3d5 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -176,7 +176,7 @@ impl SampleProfile { fn remove_single_samples_per_thread(&mut self) { let mut sample_count_by_thread_id: HashMap<u64, u32> = HashMap::new(); - for sample in self.profile.samples.iter() { + for sample in &self.profile.samples { *sample_count_by_thread_id .entry(sample.thread_id) .or_default() += 1; From 08e1210f7aca6a96a9c0301418e3bbda55b64bcd Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Wed, 14 Sep 2022 08:24:30 -0700 Subject: [PATCH 07/18] Add a thread_id field to the transaction metadata to collect the thread ID the transaction is started on --- relay-profiling/src/sample.rs | 19 +++++------- relay-profiling/src/transaction_metadata.rs | 31 ++++++++++++------- .../sample/multiple_transactions.json | 5 +-- .../profiles/sample/no_transaction.json | 3 +- .../fixtures/profiles/sample/roundtrip.json | 4 +-- 5 files changed, 33 insertions(+), 29 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 141c9c9e3d5..3e0b6295977 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -51,10 +51,6 @@ struct Sample { #[derive(Debug, Serialize, Deserialize, Clone)] struct ThreadMetadata { - #[serde(default, skip_serializing_if = "Option::is_none")] - is_main: Option<bool>, - #[serde(default, skip_serializing_if = "Option::is_none")] - is_active: Option<bool>, #[serde(default, skip_serializing_if = "Option::is_none")] name: Option<String>, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -237,22 +233,21 @@ fn parse_profile(payload: &[u8]) -> Result<SampleProfile, ProfileError> { let mut profile: SampleProfile = serde_json::from_slice(payload).map_err(ProfileError::InvalidJson)?; - profile.remove_single_samples_per_thread(); - - if profile.profile.samples.is_empty() { - return Err(ProfileError::NotEnoughSamples); - } - profile .transactions - .retain(|transaction| transaction.valid()); + .retain(|transaction| transaction.valid() && transaction.thread_id > 0); if profile.transactions.is_empty() { return Err(ProfileError::NoTransactionAssociated); } - profile.profile.frames.retain(|frame| frame.valid()); + profile.remove_single_samples_per_thread(); + if profile.profile.samples.is_empty() { + return Err(ProfileError::NotEnoughSamples); + } + + profile.profile.frames.retain(|frame| frame.valid()); profile.strip_pointer_authentication_code(); Ok(profile) diff --git a/relay-profiling/src/transaction_metadata.rs b/relay-profiling/src/transaction_metadata.rs index fa6493160e9..2ba58250f58 100644 --- a/relay-profiling/src/transaction_metadata.rs +++ b/relay-profiling/src/transaction_metadata.rs @@ -2,7 +2,7 @@ use serde::{Deserialize, Serialize}; use relay_general::protocol::EventId; -use crate::utils::deserialize_number_from_string; +use crate::utils::{deserialize_number_from_string, is_zero}; #[derive(Debug, Serialize, Deserialize, Clone, Eq, PartialEq)] pub struct TransactionMetadata { @@ -10,6 +10,13 @@ pub struct TransactionMetadata { pub name: String, pub trace_id: EventId, + #[serde( + default, + deserialize_with = "deserialize_number_from_string", + skip_serializing_if = "is_zero" + )] + pub thread_id: u64, + #[serde(deserialize_with = "deserialize_number_from_string")] pub relative_start_ns: u64, #[serde(deserialize_with = "deserialize_number_from_string")] @@ -25,10 +32,10 @@ pub struct TransactionMetadata { impl TransactionMetadata { pub fn valid(&self) -> bool { !self.id.is_nil() - && !self.trace_id.is_nil() && !self.name.is_empty() - && self.relative_start_ns < self.relative_end_ns + && !self.trace_id.is_nil() && self.relative_cpu_start_ms <= self.relative_cpu_end_ms + && self.relative_start_ns < self.relative_end_ns } pub fn duration_ns(&self) -> u64 { @@ -45,11 +52,12 @@ mod tests { let metadata = TransactionMetadata { id: "A2669CD2-C7E0-47ED-8298-4AAF9666A6B6".parse().unwrap(), name: "SomeTransaction".to_string(), - trace_id: "4705BD13-368A-499A-AA48-439DAFD9CFB0".parse().unwrap(), - relative_start_ns: 1, - relative_end_ns: 133, - relative_cpu_start_ms: 0, relative_cpu_end_ms: 0, + relative_cpu_start_ms: 0, + relative_end_ns: 133, + relative_start_ns: 1, + thread_id: 259, + trace_id: "4705BD13-368A-499A-AA48-439DAFD9CFB0".parse().unwrap(), }; assert!(metadata.valid()); } @@ -59,11 +67,12 @@ mod tests { let metadata = TransactionMetadata { id: "A2669CD2-C7E0-47ED-8298-4AAF9666A6B6".parse().unwrap(), name: "".to_string(), - trace_id: "4705BD13-368A-499A-AA48-439DAFD9CFB0".parse().unwrap(), - relative_start_ns: 1, - relative_end_ns: 133, - relative_cpu_start_ms: 0, relative_cpu_end_ms: 0, + relative_cpu_start_ms: 0, + relative_end_ns: 133, + relative_start_ns: 1, + thread_id: 259, + trace_id: "4705BD13-368A-499A-AA48-439DAFD9CFB0".parse().unwrap(), }; assert!(!metadata.valid()); } diff --git a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json index 2ba304444b2..b13d64e5fa9 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json +++ b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json @@ -55,8 +55,7 @@ ], "thread_metadata": { "1": { - "priority": 31, - "is_main": true + "priority": 31 }, "2": {} }, @@ -88,6 +87,7 @@ "name": "example_ios_movies_sources.MoviesViewController", "trace_id": "4b25bc58f14243d8b208d1e22a054164", "id": "30976f2ddbe04ac9b6bffe6e35d4710c", + "thread_id": "259", "relative_start_ns": "500500", "relative_end_ns": "50500500" }, @@ -95,6 +95,7 @@ "name": "example_ios_movies_sources.MoviesViewController", "trace_id": "4b25bc58f14243d8b208d1e22a054164", "id": "30976f2ddbe04ac9b6bffe6e35d4710c", + "thread_id": "259", "relative_start_ns": "500500", "relative_end_ns": "50500500" } diff --git a/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json b/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json index d0209581eca..bf8c5de872e 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json +++ b/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json @@ -54,8 +54,7 @@ ], "thread_metadata": { "1": { - "priority": 31, - "is_main": true + "priority": 31 }, "2": {} }, diff --git a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json index 0ca9147d13d..ce7b3ff201a 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json +++ b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json @@ -55,8 +55,7 @@ ], "thread_metadata": { "1": { - "priority": 31, - "is_main": true + "priority": 31 }, "2": {} }, @@ -88,6 +87,7 @@ "name": "example_ios_movies_sources.MoviesViewController", "trace_id": "4b25bc58f14243d8b208d1e22a054164", "id": "30976f2ddbe04ac9b6bffe6e35d4710c", + "thread_id": "259", "relative_start_ns": "500500", "relative_end_ns": "50500500" } From d6048d5c7546ca05ad6a18789eba0daad3d0efdf Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Wed, 14 Sep 2022 12:53:29 -0700 Subject: [PATCH 08/18] Generate a new event_id when duplicating the profile --- relay-profiling/src/sample.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 3e0b6295977..72c2bdb61d0 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -204,6 +204,7 @@ pub fn expand_sample_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileErro for transaction in &profile.transactions { let mut new_profile = profile.clone(); + new_profile.event_id = EventId::new(); new_profile.transactions.clear(); new_profile.transactions.push(transaction.clone()); From 20c33ba61fe5b934f266e8d68817994ba811c745 Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Wed, 14 Sep 2022 12:56:03 -0700 Subject: [PATCH 09/18] Avoid potential panic if end < start --- relay-profiling/src/transaction_metadata.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/relay-profiling/src/transaction_metadata.rs b/relay-profiling/src/transaction_metadata.rs index 2ba58250f58..a92b21c1edf 100644 --- a/relay-profiling/src/transaction_metadata.rs +++ b/relay-profiling/src/transaction_metadata.rs @@ -39,7 +39,7 @@ impl TransactionMetadata { } pub fn duration_ns(&self) -> u64 { - self.relative_end_ns - self.relative_start_ns + self.relative_end_ns.saturating_sub(self.relative_start_ns) } } From b3fc60efff823d40449b1a72876a41f7223650c5 Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Wed, 14 Sep 2022 13:06:13 -0700 Subject: [PATCH 10/18] Match on specific version instead of just testing for existence --- relay-profiling/src/lib.rs | 15 ++++++++------- relay-profiling/src/sample.rs | 6 ++++-- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/relay-profiling/src/lib.rs b/relay-profiling/src/lib.rs index b605e0406ec..5c2f30a7ca0 100644 --- a/relay-profiling/src/lib.rs +++ b/relay-profiling/src/lib.rs @@ -110,7 +110,7 @@ use crate::android::expand_android_profile; use crate::cocoa::expand_cocoa_profile; use crate::python::parse_python_profile; use crate::rust::parse_rust_profile; -use crate::sample::expand_sample_profile; +use crate::sample::{expand_sample_profile, Version}; use crate::typescript::parse_typescript_profile; pub use crate::error::ProfileError; @@ -129,7 +129,8 @@ enum Platform { #[derive(Debug, Deserialize)] struct MinimalProfile { platform: Platform, - version: Option<String>, + #[serde(default)] + version: Version, } fn minimal_profile_from_json(data: &[u8]) -> Result<MinimalProfile, ProfileError> { @@ -139,8 +140,8 @@ fn minimal_profile_from_json(data: &[u8]) -> Result<MinimalProfile, ProfileError pub fn expand_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileError> { let profile: MinimalProfile = minimal_profile_from_json(payload)?; match profile.version { - Some(_) => expand_sample_profile(payload), - None => match profile.platform { + Version::V1 => expand_sample_profile(payload), + Version::Unknown => match profile.platform { Platform::Android => expand_android_profile(payload), Platform::Cocoa => expand_cocoa_profile(payload), _ => { @@ -162,10 +163,10 @@ mod tests { #[test] fn test_minimal_profile_with_version() { - let data = r#"{"version": "v1", "platform": "cocoa"}"#; + let data = r#"{"version": "1", "platform": "cocoa"}"#; let profile = minimal_profile_from_json(data.as_bytes()); assert!(profile.is_ok()); - assert!(profile.unwrap().version.is_some()); + assert_eq!(profile.unwrap().version, Version::V1); } #[test] @@ -173,7 +174,7 @@ mod tests { let data = r#"{"platform": "cocoa"}"#; let profile = minimal_profile_from_json(data.as_bytes()); assert!(profile.is_ok()); - assert!(profile.unwrap().version.is_none()); + assert_eq!(profile.unwrap().version, Version::Unknown); } #[test] diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 72c2bdb61d0..d8d365af469 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -121,8 +121,10 @@ struct DeviceMetadata { model: Option<String>, } -#[derive(Debug, Serialize, Deserialize, Clone)] -enum Version { +#[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq)] +pub enum Version { + #[default] + Unknown, #[serde(rename = "1")] V1, } From ce2f0607a647495f1e9dc598e245a64c82f3b138 Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Wed, 14 Sep 2022 15:23:56 -0700 Subject: [PATCH 11/18] Do not filter frames or stacks since they are indexed --- relay-profiling/src/sample.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index d8d365af469..e81ceacbf64 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -24,11 +24,6 @@ struct Frame { } impl Frame { - fn valid(&self) -> bool { - self.instruction_addr.is_some() - || (self.name.is_some() && self.line.is_some() && self.file.is_some()) - } - fn strip_pointer_authentication_code(&mut self, pac_code: u64) { if let Some(address) = self.instruction_addr { self.instruction_addr = Some(Addr(address.0 & pac_code)); @@ -250,7 +245,6 @@ fn parse_profile(payload: &[u8]) -> Result<SampleProfile, ProfileError> { return Err(ProfileError::NotEnoughSamples); } - profile.profile.frames.retain(|frame| frame.valid()); profile.strip_pointer_authentication_code(); Ok(profile) From af1de3b481b094fad11f63d8bdd8c3bc0aaa59bd Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Wed, 14 Sep 2022 15:29:10 -0700 Subject: [PATCH 12/18] Explain why we're duplicating profiles --- relay-profiling/src/sample.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index e81ceacbf64..738f53cb9a4 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -198,6 +198,9 @@ pub fn expand_sample_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileErro let mut items: Vec<Vec<u8>> = Vec::new(); + // As we're getting one profile for multiple transactions and our backend doesn't support this, + // we need to duplicate the profile to have one profile for one transaction, filter the samples + // to match the transaction bounds and generate a new profile ID. for transaction in &profile.transactions { let mut new_profile = profile.clone(); From fdae8a7aba155af128b41712b781917e50aea07b Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Wed, 14 Sep 2022 19:06:06 -0700 Subject: [PATCH 13/18] Adjust some types --- relay-profiling/src/sample.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 738f53cb9a4..b62cd9b39b0 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -18,7 +18,7 @@ struct Frame { #[serde(default, skip_serializing_if = "Option::is_none")] name: Option<String>, #[serde(default, skip_serializing_if = "Option::is_none")] - line: Option<String>, + line: Option<u32>, #[serde(default, skip_serializing_if = "Option::is_none")] file: Option<String>, } @@ -62,7 +62,9 @@ struct Profile { samples: Vec<Sample>, stacks: Vec<Vec<u32>>, frames: Vec<Frame>, - thread_metadata: HashMap<String, ThreadMetadata>, + + #[serde(default, skip_serializing_if = "Option::is_none")] + thread_metadata: Option<HashMap<String, ThreadMetadata>>, // cocoa only #[serde(default, skip_serializing_if = "Option::is_none")] From 9adabfea24e3b3f00b1c6d2883b903e800af585f Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Wed, 14 Sep 2022 19:13:43 -0700 Subject: [PATCH 14/18] Do not serialize zero values --- relay-profiling/src/sample.rs | 2 +- relay-profiling/src/transaction_metadata.rs | 12 ++++++++++-- relay-server/src/actors/processor.rs | 1 + 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index b62cd9b39b0..bb827ddfe9f 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -316,7 +316,7 @@ mod tests { samples: Vec::new(), stacks: Vec::new(), frames: Vec::new(), - thread_metadata: HashMap::new(), + thread_metadata: Some(HashMap::new()), }, transactions: Vec::new(), release: "1.0 (9999)".to_string(), diff --git a/relay-profiling/src/transaction_metadata.rs b/relay-profiling/src/transaction_metadata.rs index a92b21c1edf..34dc98558dd 100644 --- a/relay-profiling/src/transaction_metadata.rs +++ b/relay-profiling/src/transaction_metadata.rs @@ -23,9 +23,17 @@ pub struct TransactionMetadata { pub relative_end_ns: u64, // Android might have a CPU clock for the trace - #[serde(default, deserialize_with = "deserialize_number_from_string")] + #[serde( + default, + deserialize_with = "deserialize_number_from_string", + skip_serializing_if = "is_zero" + )] pub relative_cpu_start_ms: u64, - #[serde(default, deserialize_with = "deserialize_number_from_string")] + #[serde( + default, + deserialize_with = "deserialize_number_from_string", + skip_serializing_if = "is_zero" + )] pub relative_cpu_end_ms: u64, } diff --git a/relay-server/src/actors/processor.rs b/relay-server/src/actors/processor.rs index 52ac23d0b9e..49e5dbd3112 100644 --- a/relay-server/src/actors/processor.rs +++ b/relay-server/src/actors/processor.rs @@ -958,6 +958,7 @@ impl EnvelopeProcessor { match relay_profiling::expand_profile(&item.payload()[..]) { Ok(payloads) => new_profiles.extend(payloads), Err(err) => { + relay_log::debug!("invalid profile: {:#?}", err); context.track_outcome( outcome_from_profile_error(err), DataCategory::Profile, From 96611808de86d6abdae51fc587986934d1d113a8 Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Thu, 15 Sep 2022 13:42:38 -0700 Subject: [PATCH 15/18] Rename field to clarify its use --- relay-profiling/src/sample.rs | 2 +- relay-profiling/src/transaction_metadata.rs | 6 +++--- .../fixtures/profiles/sample/multiple_transactions.json | 4 ++-- .../tests/fixtures/profiles/sample/roundtrip.json | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index bb827ddfe9f..5d360a402ef 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -238,7 +238,7 @@ fn parse_profile(payload: &[u8]) -> Result<SampleProfile, ProfileError> { profile .transactions - .retain(|transaction| transaction.valid() && transaction.thread_id > 0); + .retain(|transaction| transaction.valid() && transaction.active_thread_id > 0); if profile.transactions.is_empty() { return Err(ProfileError::NoTransactionAssociated); diff --git a/relay-profiling/src/transaction_metadata.rs b/relay-profiling/src/transaction_metadata.rs index 34dc98558dd..ef89f770bc8 100644 --- a/relay-profiling/src/transaction_metadata.rs +++ b/relay-profiling/src/transaction_metadata.rs @@ -15,7 +15,7 @@ pub struct TransactionMetadata { deserialize_with = "deserialize_number_from_string", skip_serializing_if = "is_zero" )] - pub thread_id: u64, + pub active_thread_id: u64, #[serde(deserialize_with = "deserialize_number_from_string")] pub relative_start_ns: u64, @@ -64,7 +64,7 @@ mod tests { relative_cpu_start_ms: 0, relative_end_ns: 133, relative_start_ns: 1, - thread_id: 259, + active_thread_id: 259, trace_id: "4705BD13-368A-499A-AA48-439DAFD9CFB0".parse().unwrap(), }; assert!(metadata.valid()); @@ -79,7 +79,7 @@ mod tests { relative_cpu_start_ms: 0, relative_end_ns: 133, relative_start_ns: 1, - thread_id: 259, + active_thread_id: 259, trace_id: "4705BD13-368A-499A-AA48-439DAFD9CFB0".parse().unwrap(), }; assert!(!metadata.valid()); diff --git a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json index b13d64e5fa9..579cca3ca1c 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json +++ b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json @@ -87,7 +87,7 @@ "name": "example_ios_movies_sources.MoviesViewController", "trace_id": "4b25bc58f14243d8b208d1e22a054164", "id": "30976f2ddbe04ac9b6bffe6e35d4710c", - "thread_id": "259", + "active_thread_id": "259", "relative_start_ns": "500500", "relative_end_ns": "50500500" }, @@ -95,7 +95,7 @@ "name": "example_ios_movies_sources.MoviesViewController", "trace_id": "4b25bc58f14243d8b208d1e22a054164", "id": "30976f2ddbe04ac9b6bffe6e35d4710c", - "thread_id": "259", + "active_thread_id": "259", "relative_start_ns": "500500", "relative_end_ns": "50500500" } diff --git a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json index ce7b3ff201a..81074629f90 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json +++ b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json @@ -87,7 +87,7 @@ "name": "example_ios_movies_sources.MoviesViewController", "trace_id": "4b25bc58f14243d8b208d1e22a054164", "id": "30976f2ddbe04ac9b6bffe6e35d4710c", - "thread_id": "259", + "active_thread_id": "259", "relative_start_ns": "500500", "relative_end_ns": "50500500" } From 895c19593ffdae3b992171c34b71b3e555327ef9 Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Sun, 25 Sep 2022 22:13:37 -0400 Subject: [PATCH 16/18] Rename a field for clarity --- relay-profiling/src/sample.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 5d360a402ef..1bfdcef85bb 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -37,7 +37,7 @@ struct Sample { #[serde(deserialize_with = "deserialize_number_from_string")] thread_id: u64, #[serde(deserialize_with = "deserialize_number_from_string")] - relative_timestamp_ns: u64, + elapsed_since_start_ns: u64, // cocoa only #[serde(default, skip_serializing_if = "Option::is_none")] @@ -211,10 +211,10 @@ pub fn expand_sample_profile(payload: &[u8]) -> Result<Vec<Vec<u8>>, ProfileErro new_profile.transactions.push(transaction.clone()); new_profile.profile.samples.retain_mut(|sample| { - if transaction.relative_start_ns <= sample.relative_timestamp_ns - && sample.relative_timestamp_ns <= transaction.relative_end_ns + if transaction.relative_start_ns <= sample.elapsed_since_start_ns + && sample.elapsed_since_start_ns <= transaction.relative_end_ns { - sample.relative_timestamp_ns -= transaction.relative_start_ns; + sample.elapsed_since_start_ns -= transaction.relative_start_ns; true } else { false @@ -332,25 +332,25 @@ mod tests { Sample { stack_id: 0, queue_address: Some("0xdeadbeef".to_string()), - relative_timestamp_ns: 1, + elapsed_since_start_ns: 1, thread_id: 1, }, Sample { stack_id: 0, queue_address: Some("0xdeadbeef".to_string()), - relative_timestamp_ns: 1, + elapsed_since_start_ns: 1, thread_id: 1, }, Sample { stack_id: 0, queue_address: Some("0xdeadbeef".to_string()), - relative_timestamp_ns: 1, + elapsed_since_start_ns: 1, thread_id: 2, }, Sample { stack_id: 0, queue_address: Some("0xdeadbeef".to_string()), - relative_timestamp_ns: 1, + elapsed_since_start_ns: 1, thread_id: 3, }, ]); @@ -369,25 +369,25 @@ mod tests { Sample { stack_id: 0, queue_address: Some("0xdeadbeef".to_string()), - relative_timestamp_ns: 1, + elapsed_since_start_ns: 1, thread_id: 1, }, Sample { stack_id: 0, queue_address: Some("0xdeadbeef".to_string()), - relative_timestamp_ns: 1, + elapsed_since_start_ns: 1, thread_id: 2, }, Sample { stack_id: 0, queue_address: Some("0xdeadbeef".to_string()), - relative_timestamp_ns: 1, + elapsed_since_start_ns: 1, thread_id: 3, }, Sample { stack_id: 0, queue_address: Some("0xdeadbeef".to_string()), - relative_timestamp_ns: 1, + elapsed_since_start_ns: 1, thread_id: 4, }, ]); @@ -420,7 +420,7 @@ mod tests { assert_eq!(expanded_profile.profile.samples.len(), 4); for sample in &expanded_profile.profile.samples { - assert!(sample.relative_timestamp_ns < expanded_profile.transactions[0].duration_ns()); + assert!(sample.elapsed_since_start_ns < expanded_profile.transactions[0].duration_ns()); } } } From 77e5b01ca207484714e14af95429f96af7b659b3 Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Mon, 26 Sep 2022 10:09:02 -0400 Subject: [PATCH 17/18] Rename key in test data as well --- .../fixtures/profiles/sample/multiple_transactions.json | 8 ++++---- .../tests/fixtures/profiles/sample/no_transaction.json | 8 ++++---- .../tests/fixtures/profiles/sample/roundtrip.json | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json index 579cca3ca1c..0cba6a69337 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json +++ b/relay-profiling/tests/fixtures/profiles/sample/multiple_transactions.json @@ -20,25 +20,25 @@ "stack_id": 0, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "10500500" + "elapsed_since_start_ns": "10500500" }, { "stack_id": 1, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "20500500" + "elapsed_since_start_ns": "20500500" }, { "stack_id": 0, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "30500500" + "elapsed_since_start_ns": "30500500" }, { "stack_id": 1, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "40500500" + "elapsed_since_start_ns": "40500500" } ], "frames": [ diff --git a/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json b/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json index bf8c5de872e..c73c11a5d36 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json +++ b/relay-profiling/tests/fixtures/profiles/sample/no_transaction.json @@ -19,25 +19,25 @@ "stack_id": 0, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "10500500" + "elapsed_since_start_ns": "10500500" }, { "stack_id": 1, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "20500500" + "elapsed_since_start_ns": "20500500" }, { "stack_id": 0, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "30500500" + "elapsed_since_start_ns": "30500500" }, { "stack_id": 1, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "40500500" + "elapsed_since_start_ns": "40500500" } ], "frames": [ diff --git a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json index 81074629f90..608ca05de69 100644 --- a/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json +++ b/relay-profiling/tests/fixtures/profiles/sample/roundtrip.json @@ -20,25 +20,25 @@ "stack_id": 0, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "10500500" + "elapsed_since_start_ns": "10500500" }, { "stack_id": 1, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "20500500" + "elapsed_since_start_ns": "20500500" }, { "stack_id": 0, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "30500500" + "elapsed_since_start_ns": "30500500" }, { "stack_id": 1, "thread_id": "1", "queue_address": "0x0000000102adc700", - "relative_timestamp_ns": "40500500" + "elapsed_since_start_ns": "40500500" } ], "stacks": [ From 0601e39d7da7876074dcfbf28f06db59e389b2cb Mon Sep 17 00:00:00 2001 From: Pierre Massat <pierre.massat@sentry.io> Date: Tue, 27 Sep 2022 15:21:17 -0400 Subject: [PATCH 18/18] Remove check for thread_id since nodejs thread IDs can be equal to 0 --- relay-profiling/src/sample.rs | 2 +- relay-profiling/src/transaction_metadata.rs | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/relay-profiling/src/sample.rs b/relay-profiling/src/sample.rs index 1bfdcef85bb..59f92afc95f 100644 --- a/relay-profiling/src/sample.rs +++ b/relay-profiling/src/sample.rs @@ -238,7 +238,7 @@ fn parse_profile(payload: &[u8]) -> Result<SampleProfile, ProfileError> { profile .transactions - .retain(|transaction| transaction.valid() && transaction.active_thread_id > 0); + .retain(|transaction| transaction.valid()); if profile.transactions.is_empty() { return Err(ProfileError::NoTransactionAssociated); diff --git a/relay-profiling/src/transaction_metadata.rs b/relay-profiling/src/transaction_metadata.rs index ef89f770bc8..e6bdf14c9e2 100644 --- a/relay-profiling/src/transaction_metadata.rs +++ b/relay-profiling/src/transaction_metadata.rs @@ -10,11 +10,7 @@ pub struct TransactionMetadata { pub name: String, pub trace_id: EventId, - #[serde( - default, - deserialize_with = "deserialize_number_from_string", - skip_serializing_if = "is_zero" - )] + #[serde(default, deserialize_with = "deserialize_number_from_string")] pub active_thread_id: u64, #[serde(deserialize_with = "deserialize_number_from_string")]