Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(profile): Introduce a common sample format #1462

Merged
merged 23 commits into from
Sep 27, 2022
Merged
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f2bb0bc
feat(profile): Introduce a common sample format
phacops Sep 12, 2022
81561f1
Remove unnecessary code
phacops Sep 12, 2022
edd820d
Support frame indexing
phacops Sep 12, 2022
07923cc
Support frames without addresses
phacops Sep 13, 2022
e1221c3
Add a changelog entry
phacops Sep 13, 2022
b49c885
Merge branch 'master' into pierre/profiling-sample-format
phacops Sep 13, 2022
c5b8146
Use more efficient syntax where suggested
phacops Sep 13, 2022
08e1210
Add a thread_id field to the transaction metadata to collect the thre…
phacops Sep 14, 2022
ab0f69c
Merge branch 'master' into pierre/profiling-sample-format
phacops Sep 14, 2022
d6048d5
Generate a new event_id when duplicating the profile
phacops Sep 14, 2022
20c33ba
Avoid potential panic if end < start
phacops Sep 14, 2022
b3fc60e
Match on specific version instead of just testing for existence
phacops Sep 14, 2022
ce2f060
Do not filter frames or stacks since they are indexed
phacops Sep 14, 2022
af1de3b
Explain why we're duplicating profiles
phacops Sep 14, 2022
fdae8a7
Adjust some types
phacops Sep 15, 2022
9adabfe
Do not serialize zero values
phacops Sep 15, 2022
9661180
Rename field to clarify its use
phacops Sep 15, 2022
cfb397f
Merge branch 'master' into pierre/profiling-sample-format
phacops Sep 15, 2022
760a62a
Merge branch 'master' into pierre/profiling-sample-format
phacops Sep 19, 2022
895c195
Rename a field for clarity
phacops Sep 26, 2022
77e5b01
Rename key in test data as well
phacops Sep 26, 2022
b4af3bc
Merge branch 'master' into pierre/profiling-sample-format
phacops Sep 27, 2022
0601e39
Remove check for thread_id since nodejs thread IDs can be equal to 0
phacops Sep 27, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)), ([#1463](https://github.com/getsentry/relay/pull/1463)), ([#1464](https://github.com/getsentry/relay/pull/1464))
- Introduce a new profile format called `sample`. ([#1462](https://github.com/getsentry/relay/pull/1462))

**Features**:

1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion relay-profiling/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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"
2 changes: 1 addition & 1 deletion relay-profiling/src/android.rs
Original file line number Diff line number Diff line change
@@ -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 {
57 changes: 5 additions & 52 deletions relay-profiling/src/cocoa.rs
Original file line number Diff line number Diff line change
@@ -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>,
@@ -137,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 {
@@ -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");
2 changes: 2 additions & 0 deletions relay-profiling/src/error.rs
Original file line number Diff line number Diff line change
@@ -18,4 +18,6 @@ pub enum ProfileError {
NoTransactionAssociated,
#[fail(display = "invalid transaction metadata")]
InvalidTransactionMetadata,
#[fail(display = "missing profile metadata")]
MissingProfileMetadata,
}
85 changes: 67 additions & 18 deletions relay-profiling/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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,36 +110,83 @@ 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> {
serde_json::from_slice(data).map_err(ProfileError::InvalidJson)
}

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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would match on v1 explicitly here. That would make the intent clearer, and if there's ever a v2, an old Relay instance would not try to expand it as a sample profile.

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),
};
Ok(vec![payload?])
}
}
},
}
}

#[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());
}
}
58 changes: 58 additions & 0 deletions relay-profiling/src/native_debug_image.rs
Original file line number Diff line number Diff line change
@@ -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);
}
}
2 changes: 1 addition & 1 deletion relay-profiling/src/rust.rs
Original file line number Diff line number Diff line change
@@ -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)]
Loading