ref(profiling): Normalize all profiles #1250

phacops · 2022-04-29T02:14:10Z

This PR aims to normalize all profiles and will reject invalid profiles with not enough samples or the wrong platform.

jan-auer · 2022-04-29T11:55:19Z

relay-server/src/utils/profile.rs

+    queue_address: Option<String>,
+
+    #[serde(deserialize_with = "deserialize_number_from_string")]
+    relative_timestamp_ns: u64,


Shouldn't you also serialize that type as string to allow for roundtrips?

It also looks like this is the only use of the serde_aux crate, so maybe you could vendor the implementation in? There's two ways to do this:

define your own functions/module to do the serialization and deserialization

create a named newtype

Note that in other places of the event payload we currently use floating point timestamps.

What do you mean by roundtrips? I don't serialize as string since I want to pass it as an integer downstream. Python supports decoding a uint64 from a JSON object even if it's not in the standard.

I think I'd rather deserialize as an integer pass it downstream than use a float. This seems simpler as we're not going to have anymore type conversion.

@phacops assuming this is in envelopes, our protocol has to roundtrip. so if you want to serialize this as integer, you have to be able to deserialize it as such. you can always be more lenient on deserialization than serialization, but not the other way around

example: customer relay forwards profiles to us. or pop relays forward profiles to internal relays

OK, I understand. The deserialization function I'm using now supports deserializing from string or number now.

ok you're right. I think I'm fine with this then.

jan-auer · 2022-04-29T12:00:50Z

relay-server/src/utils/profile.rs

+}
+
+#[derive(Debug, Serialize, Deserialize)]
+struct Image {


This and other types are very similar to the existing types in the event protocol. Depending on how much you still want to refactor this, it would be good if we start looking into sharing the definitions.

The main challenge with this is that we base most of our protocol on the Annotated, which requires its own serializing and deserializing implementation. It's notably slower than plain serde, and we've been long looking to re-align it with serde.

I'm willing to refactor things to make them more cohesive. I think we should still ship that though because it sounds like a way larger effort that should really be done on its own as opposed to doing it on top of another change.

I'm fine with not aligning for now, unless we're planning to write this struct back into the event in some sort of capacity.

This won't be written back anywhere. I'll use in for symbolication later and then discard it.

If this goes through symbolicator we should find a way to make those structs align and fail compilation if they don't. For the time being I would add yet another roundtrip test that ensures this: profiling image <-> big json string <-> event image

I made a test for that but there's a few caveats. The iOS SDK is using a function get generate debug meta that was already part of the SDK and this function gets us apple debug images. Two problems from there:

It seems the Symbolicator wants Macho now, and that's why we have to rename the uuid field to debug_id, but then the image would be more of type macho.

It doesn't provide some fields that seems required when building an apple debug image (cpu_type, cpu_subtype and arch). I added those fields in our image struct for now.

So, we're investigating to see how to generate macho images and then I'll make this test a little bit less weird and will also likely remove the need to rename the uuid field to debug_id.

untitaker

I think this is conceptually fine, but serde annotations are easy to mess up. I would add roundtrip and snapshot tests for serialization and deserialization behavior. You can use insta to make test assertions a little easier

untitaker · 2022-05-05T18:09:04Z

relay-server/src/utils/profile.rs

+
+#[derive(Debug, Serialize, Deserialize)]
+struct ThreadMetadata {
+    #[serde(skip_serializing_if = "Option::is_none")]


This won't roundtrip: name: null is required, but not serialized

I think there are more like this. Can you add roundtrip tests?

… the event debug image schema

…atform

jan-auer

Looks good to me now, thanks for incorporating the suggestions.

Before merging this, could you please move the large test payloads to a test fixture? We do not have reusable test fixtures in our Rust tests yet, but you can just create a file, for instance in <crate>/tests/fixtures like we did in relay-general.

jan-auer · 2022-05-09T18:08:30Z

relay-general/src/protocol/debugmeta.rs

+pub struct ParseNativeImagePathError;
+
+impl FromStr for NativeImagePath {
+    type Err = ParseNativeImagePathError;


Since this never fails, you can use std::convert::Infallible.

jan-auer · 2022-05-09T18:09:05Z

relay-general/src/protocol/debugmeta.rs

@@ -75,6 +94,8 @@ impl ProcessValue for NativeImagePath {
    }
 }

+relay_common::impl_str_serde!(NativeImagePath, "a native image path");


As discussed offline, feel free to derive Deserialize, Serialize directly.

…il or empty

* master: feat(sampling): Support custom tags and more contexts (#1268) deps: Update symbolic to pull in Unreal parser fixes (#1266) Bring in CLA Lite (#1257) release: 0.8.11 ref(profiling): Normalize all profiles (#1250) feat(profiling): Add a profile data category and count profiles in an envelope to apply rate limits (#1259) fix: Raise a new InvalidCompression Outcome for invalid Unreal compression (#1237) chore: Update logo for dark or light theme (#1262) ref: Use Duration to compute breakdowns and span durations (#1260)

phacops requested a review from a team April 29, 2022 02:14

phacops mentioned this pull request Apr 29, 2022

ref(profiling): Normalize more in Relay instead of here getsentry/sentry#34095

Merged

jan-auer reviewed Apr 29, 2022

View reviewed changes

phacops force-pushed the pierre/profiling-normalize-profiles-all-platform branch from 2ea8176 to d07ebb6 Compare April 29, 2022 23:03

phacops added 4 commits May 4, 2022 10:29

ref(profiling): Normalize all profiles

e807a27

Add a changelog entry

03bc54c

Remove serde-aux

f504164

Use Addr and DebugId types where necessary

828ae4f

phacops force-pushed the pierre/profiling-normalize-profiles-all-platform branch from e242c24 to 828ae4f Compare May 4, 2022 15:07

phacops added 2 commits May 4, 2022 11:20

Use an enum for the image type

ada4b86

Skip some optional fields when serializing

14868c3

untitaker reviewed May 5, 2022

View reviewed changes

Add tests for roundtrips

79fbd11

phacops requested a review from jan-auer May 6, 2022 15:59

Add a test to make sure the debug image we receive is compatible with…

e2a73ca

… the event debug image schema

phacops requested a review from untitaker May 6, 2022 18:01

phacops added 2 commits May 9, 2022 10:28

Switch to using Mach-O debug images instead of the deprecated Apple type

9c3600b

Merge branch 'master' into pierre/profiling-normalize-profiles-all-pl…

cc6b3da

…atform

jan-auer approved these changes May 9, 2022

View reviewed changes

phacops added 3 commits May 9, 2022 15:03

Move profiles to fixtures and fix a type and key name

b8a7df1

Use a derive to serialize and deserialize NativeImagePath

6228d42

Accept more optional fields and do not serialize if some values are n…

3a93a06

…il or empty

phacops enabled auto-merge (squash) May 9, 2022 23:14

phacops merged commit 7d0dc11 into master May 9, 2022

phacops deleted the pierre/profiling-normalize-profiles-all-platform branch May 9, 2022 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(profiling): Normalize all profiles #1250

ref(profiling): Normalize all profiles #1250

phacops commented Apr 29, 2022

jan-auer Apr 29, 2022

phacops Apr 29, 2022

untitaker May 5, 2022

phacops May 5, 2022

untitaker May 5, 2022

jan-auer Apr 29, 2022

phacops Apr 29, 2022

untitaker May 5, 2022

phacops May 5, 2022

untitaker May 5, 2022 •

edited

Loading

phacops May 6, 2022

untitaker left a comment

untitaker May 5, 2022

jan-auer left a comment

jan-auer May 9, 2022

jan-auer May 9, 2022

ref(profiling): Normalize all profiles #1250

ref(profiling): Normalize all profiles #1250

Conversation

phacops commented Apr 29, 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

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

untitaker May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker May 5, 2022 •

edited

Loading