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

phacops · 2022-09-12T13:26:51Z

We have several formats for each platform based around sampling. This format is meant to consolidate all of them into one common format, the Sample format.

Compared to current formats, this one lets us deduplicate the stacks of frames to reduce the size of the profile and standardize common metadata for all platforms and in returns, in the rest of the pipeline, the logic to transform into calltrees will be adapted for this format only instead of having to be adapted for each platform for which it is missing (rust, python and node so far). Some of the fields were added or renamed to match what is in the event format as well (for example timestamp or release).

There are still some fields specific to cocoa. It's not great since it's meant to be a more generic format but we still need some things that exists only for cocoa. Not sure if we can do so much better here. I toyed with the idea of adding a generic tags field so specific metadata but we'd still have to manually validate it since that metadata is mandatory in our data model right now. I think once we'll adapt our data model to be more generic, we'd be able to update this format as well.

JonasBa · 2022-09-12T13:49:14Z

One worry about indexing stacks that I have is that for some environments (node for example), we do not have control over sample scheduling meaning we only get access to the entire profile once the profile stop method has been called at which point creating an index of stacks may add a non negligible amount of CPU and memory overhead. In any case, I think we need to benchmark this format to see how it impacts our overhead.

As an alternative, could we maybe think about supporting both indexed and non indexed stacks? It could be a progressive enhancement where we just distinguish between samples that are int[][] vs int[] where int[][] is the non-indexed version and int[] is the indexed stacks version.

phacops · 2022-09-12T17:29:10Z

One worry about indexing stacks that I have is that for some environments (node for example), we do not have control over sample scheduling meaning we only get access to the entire profile once the profile stop method has been called at which point creating an index of stacks may add a non negligible amount of CPU and memory overhead. In any case, I think we need to benchmark this format to see how it impacts our overhead.

You don't need to use this feature if you don't want to. Push each stack as a unique one and give it the latest ID.

JonasBa · 2022-09-13T13:30:31Z

One worry about indexing stacks that I have is that for some environments (node for example), we do not have control over sample scheduling meaning we only get access to the entire profile once the profile stop method has been called at which point creating an index of stacks may add a non negligible amount of CPU and memory overhead. In any case, I think we need to benchmark this format to see how it impacts our overhead.

You don't need to use this feature if you don't want to. Push each stack as a unique one and give it the latest ID.

@phacops does that mean we no longer index frames thought? That could be a pretty big hit to the profile size

olksdr · 2022-09-13T14:50:57Z

relay-profiling/src/lib.rs

+                match payload {
+                    Ok(payload) => Ok(vec![payload]),
+                    Err(err) => Err(err),
+                }


Suggested change

match payload {

Ok(payload) => Ok(vec![payload]),

Err(err) => Err(err),

}

Ok(vec![payload?])

This might be simpler and there is no need for another match here

olksdr · 2022-09-13T14:53:24Z

relay-profiling/src/sample.rs

+    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() {


Suggested change

for sample in self.profile.samples.iter() {

for sample in self.profile.samples {

you shouldn't have to use .iter() explicitly here

I removed it but I needed to borrow.

phacops · 2022-09-13T20:55:39Z

@phacops does that mean we no longer index frames thought? That could be a pretty big hit to the profile size

It wasn't there in the first commit but I added it in a different commit. It would be a shame not to have it.

jjbayer · 2022-09-14T12:20:06Z

relay-profiling/src/lib.rs

-                Err(payload) => Err(payload),
+    let profile: MinimalProfile = minimal_profile_from_json(payload)?;
+    match profile.version {
+        Some(_) => expand_sample_profile(payload),


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.

jjbayer · 2022-09-14T12:34:18Z

relay-profiling/src/sample.rs

+    let mut items: Vec<Vec<u8>> = Vec::new();
+
+    for transaction in &profile.transactions {
+        let mut new_profile = profile.clone();


What is the purpose of splitting each transaction into a separate profile? A doc comment would help on this function.

I left a comment, let me know if it's clear.

Yes, thank you 👍

jjbayer · 2022-09-14T12:38:12Z

relay-profiling/src/sample.rs

+        let mut new_profile = profile.clone();
+
+        new_profile.transactions.clear();
+        new_profile.transactions.push(transaction.clone());


It should be possible to consume profile.transactions and prevent the clone() here.

I'm not sure how to do this. I need to be able to copy the profile but Profile doesn't implement Copy since I have a bunch of Vec and String in the struct. So I explicitely call clone. Then, this is what the compiler suggests:

error[E0382]: borrow of partially moved value: `profile` --> relay-profiling/src/sample.rs:205:31 | 204 | for transaction in profile.transactions { | -------------------- `profile.transactions` partially moved due to this implicit call to `.into_iter()` 205 | let mut new_profile = profile.clone(); | ^^^^^^^^^^^^^^^ value borrowed here after partial move | note: this function takes ownership of the receiver `self`, which moves `profile.transactions` = note: partial move occurs because `profile.transactions` has type `Vec<transaction_metadata::TransactionMetadata>`, which does not implement the `Copy` trait help: consider iterating over a slice of the `Vec<transaction_metadata::TransactionMetadata>`'s content to avoid moving into the `for` loop | 204 | for transaction in &profile.transactions {

And from there, it also makes me clone the transaction. What would you do differently?

This is indeed trickier than I thought, but what should work is

let mut profile = parse_profile(payload)?; // [...] // Replace profile.transactions with an empty vector // (side benefit: makes calling .clear() unnecessary). let transactions = std::mem::take(&mut profile.transactions); for transaction in transactions { // [...] new_profile.profile.samples.retain_mut(|sample| { // [...] }); new_profile.transactions.push(transaction);

jjbayer · 2022-09-14T12:39:22Z

relay-profiling/src/sample.rs

+        return Err(ProfileError::NoTransactionAssociated);
+    }
+
+    profile.profile.frames.retain(|frame| frame.valid());


Could frames be empty after this? If so, should an error be returned in that case?

Actually, since frames are indexed (and stacks as well), I can't remove any of them, valid or invalid. I'll remove this.

jjbayer · 2022-09-14T12:44:00Z

relay-profiling/src/transaction_metadata.rs

@@ -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


This could panic if end < start, you might want to use saturating_sub.

…ad ID the transaction is started on

jjbayer · 2022-09-15T09:15:53Z

relay-profiling/src/sample.rs

+        let mut new_profile = profile.clone();
+
+        new_profile.transactions.clear();
+        new_profile.transactions.push(transaction.clone());


This is indeed trickier than I thought, but what should work is

let mut profile = parse_profile(payload)?; // [...] // Replace profile.transactions with an empty vector // (side benefit: makes calling .clear() unnecessary). let transactions = std::mem::take(&mut profile.transactions); for transaction in transactions { // [...] new_profile.profile.samples.retain_mut(|sample| { // [...] }); new_profile.transactions.push(transaction);

jjbayer · 2022-09-15T09:19:34Z

relay-profiling/src/sample.rs

+    let mut items: Vec<Vec<u8>> = Vec::new();
+
+    for transaction in &profile.transactions {
+        let mut new_profile = profile.clone();


Yes, thank you 👍

viglia · 2022-09-15T14:32:13Z

relay-profiling/src/transaction_metadata.rs

+        deserialize_with = "deserialize_number_from_string",
+        skip_serializing_if = "is_zero"
+    )]
+    pub thread_id: u64,


@phacops could we call thread_id active_thread_id instead?

This way it's easier to figure out what it refers to.

viglia

@phacops I saw, that when you replaced is_active in thread_metadata with active_thread_id in transaction_metadata you've also removed is_main.

Is it intentional not to have that field anymore?

phacops · 2022-09-19T11:02:35Z

Yes, it's intentional to not have is_main.

The objective with is_main was to indicate which thread to be shown in the UI and since on mobile platforms, the main thread is the thread we want to focus on, we named it that way.

Once we live the mobile SDKs world, main thread and active thread become different. At the begining, adding a second metadata field to indicate the active thread seemed like a natural thing to do but it turns out we are not interested in the main thread, we're interested into the active thread. So is_main can disappear.

Zylphrex

Just a few small questions, but otherwise LGTM!

Zylphrex · 2022-09-19T14:19:31Z

relay-profiling/src/transaction_metadata.rs

            && self.relative_cpu_start_ms <= self.relative_cpu_end_ms
+            && self.relative_start_ns < self.relative_end_ns


Why does relative_start_ns use < but relative_cpu_start_ms use <=? Are 0 duration profiles allowed when using relative_cpu_start_ms (this is android only right?)?

It's to avoid considering the profile invalid when we don't have a value (fields will be defaulted to 0). Ideally we'd do a check based on the platform but we don't have access to that. We might also have to align both to check for <= because we might have CPU time only anyway.

Zylphrex · 2022-09-19T14:20:59Z

relay-server/src/actors/processor.rs

@@ -1036,6 +1036,7 @@ impl EnvelopeProcessorService {
            match relay_profiling::expand_profile(&item.payload()[..]) {
                Ok(payloads) => new_profiles.extend(payloads),
                Err(err) => {
+                    relay_log::debug!("invalid profile: {:#?}", err);


are we able to see these logs in prod somewhere?

No, that's local only.

Zylphrex · 2022-09-19T14:22:31Z

relay-profiling/src/sample.rs

+    #[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<u32>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    file: Option<String>,


Since all the attributes are optional, is it possible the SDK sends an empty dict as the frame? Should we invalidate this case?

It would be possible yes. Since we do frame indexing, I wouldn't be really able to remove the frame if it's invalid. We could reject the whole profile after checking if frames referenced in a sample are all valid or not though.

JonasBa · 2022-09-23T12:59:49Z

relay-profiling/tests/fixtures/profiles/sample/no_transaction.json

+        "stack_id": 0,
+        "thread_id": "1",
+        "queue_address": "0x0000000102adc700",
+        "relative_timestamp_ns": "10500500"


@phacops since this is not really a timestamp, should we call it elapsed_ns instead to be more accurate?

Let's call it elapsed_since_start_ns, makes it very clear that way. @viglia @armcknight

feat(profile): Introduce a common sample format

f2bb0bc

phacops requested a review from a team as a code owner September 12, 2022 13:26

phacops requested a review from a team September 12, 2022 13:26

phacops added 2 commits September 12, 2022 16:03

Remove unnecessary code

81561f1

Support frame indexing

edd820d

phacops mentioned this pull request Sep 13, 2022

feat(profiling): Convert profile output to the sample format getsentry/sentry-python#1611

Merged

phacops added 3 commits September 12, 2022 17:05

Support frames without addresses

07923cc

Add a changelog entry

e1221c3

Merge branch 'master' into pierre/profiling-sample-format

b49c885

olksdr reviewed Sep 13, 2022

View reviewed changes

Use more efficient syntax where suggested

c5b8146

phacops requested a review from olksdr September 13, 2022 20:59

JonasBa mentioned this pull request Sep 14, 2022

TODO: Alpha release getsentry/profiling-node#5

Closed

26 tasks

jjbayer reviewed Sep 14, 2022

View reviewed changes

phacops added 9 commits September 14, 2022 08:24

Add a thread_id field to the transaction metadata to collect the thre…

08e1210

…ad ID the transaction is started on

Merge branch 'master' into pierre/profiling-sample-format

ab0f69c

Generate a new event_id when duplicating the profile

d6048d5

Avoid potential panic if end < start

20c33ba

Match on specific version instead of just testing for existence

b3fc60e

Do not filter frames or stacks since they are indexed

ce2f060

Explain why we're duplicating profiles

af1de3b

Adjust some types

fdae8a7

Do not serialize zero values

9adabfe

jjbayer approved these changes Sep 15, 2022

View reviewed changes

viglia reviewed Sep 15, 2022

View reviewed changes

Rename field to clarify its use

9661180

phacops requested review from viglia and a team September 15, 2022 20:44

Merge branch 'master' into pierre/profiling-sample-format

cfb397f

viglia reviewed Sep 16, 2022

View reviewed changes

Merge branch 'master' into pierre/profiling-sample-format

760a62a

Zylphrex approved these changes Sep 19, 2022

View reviewed changes

phacops requested a review from viglia September 19, 2022 15:10

armcknight mentioned this pull request Sep 20, 2022

ref: update profile payload schema getsentry/sentry-cocoa#2203

Merged

1 task

viglia mentioned this pull request Sep 21, 2022

feat(profiling): Update to new standard Profile format getsentry/sentry-rust#504

Merged

phacops mentioned this pull request Sep 21, 2022

feat(profiling): Process sample format getsentry/sentry#39106

Merged

JonasBa reviewed Sep 23, 2022

View reviewed changes

Rename a field for clarity

895c195

JonasBa mentioned this pull request Sep 26, 2022

feat(format): adopt sentry sampled format getsentry/profiling-node#14

Merged

Rename key in test data as well

77e5b01

viglia approved these changes Sep 26, 2022

View reviewed changes

viglia mentioned this pull request Sep 27, 2022

feat(profiling): add profiling to debug_files commands getsentry/sentry-cli#1325

Closed

phacops added 2 commits September 27, 2022 11:02

Merge branch 'master' into pierre/profiling-sample-format

b4af3bc

Remove check for thread_id since nodejs thread IDs can be equal to 0

0601e39

phacops enabled auto-merge (squash) September 27, 2022 19:21

phacops merged commit 4894c1d into master Sep 27, 2022

phacops deleted the pierre/profiling-sample-format branch September 27, 2022 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

phacops commented Sep 12, 2022 •

edited

JonasBa commented Sep 12, 2022

phacops commented Sep 12, 2022

JonasBa commented Sep 13, 2022

olksdr Sep 13, 2022

olksdr Sep 13, 2022

phacops Sep 13, 2022

phacops commented Sep 13, 2022

jjbayer Sep 14, 2022

jjbayer Sep 14, 2022

phacops Sep 14, 2022

jjbayer Sep 15, 2022

jjbayer Sep 14, 2022

phacops Sep 14, 2022 •

edited

jjbayer Sep 15, 2022

jjbayer Sep 14, 2022

phacops Sep 14, 2022

jjbayer Sep 14, 2022

jjbayer Sep 15, 2022

jjbayer Sep 15, 2022

viglia Sep 15, 2022

viglia left a comment

phacops commented Sep 19, 2022

Zylphrex left a comment

Zylphrex Sep 19, 2022

phacops Sep 20, 2022

Zylphrex Sep 19, 2022

phacops Sep 20, 2022

Zylphrex Sep 19, 2022

phacops Sep 20, 2022

JonasBa Sep 23, 2022

phacops Sep 26, 2022

	for sample in self.profile.samples.iter() {
	for sample in self.profile.samples {

		&& self.relative_cpu_start_ms <= self.relative_cpu_end_ms
		&& self.relative_start_ns < self.relative_end_ns

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

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

Conversation

phacops commented Sep 12, 2022 • edited

JonasBa commented Sep 12, 2022

phacops commented Sep 12, 2022

JonasBa commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phacops commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phacops Sep 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viglia left a comment

Choose a reason for hiding this comment

phacops commented Sep 19, 2022

Zylphrex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phacops commented Sep 12, 2022 •

edited

phacops Sep 14, 2022 •

edited