feat(profiling): Filter out lonely samples #1445
Conversation
…`Vec` of keys Co-authored-by: Oleksandr <[email protected]>
| .or_default() += 1; | ||
| } | ||
|
|
||
| sample_count_by_thread_id.retain(|_, count| *count > 1); |
There was a problem hiding this comment.
Is 1 some sort of natural limit? Or might this value change in the future? In the latter case, I would put it in a constant or even make it configurable.
nit: An explanation in the form of a code comment or as part of the function's documentation would be nice.
There was a problem hiding this comment.
1 is the only limit we'd want. Basically, we need at least 2 samples (list of frames executed at a given timestamp) to calculate a duration. So it's not going to change, I'll remove every thread with only 1 sample since I can't do anything interesting with this data.
I added a comment to make that clear.
relay-profiling/src/cocoa.rs
Outdated
| @@ -121,11 +121,31 @@ struct CocoaProfile { | |||
| version_name: String, | |||
| } | |||
|
|
|||
| impl CocoaProfile { | |||
| fn filter_samples(&mut self) { | |||
There was a problem hiding this comment.
nit: Would remove_singleton_samples be a better name?
There was a problem hiding this comment.
I changed the name for something much more descriptive.
relay-profiling/src/android.rs
Outdated
|
|
||
| /// Removes an event with a duration of 0 | ||
| fn remove_events_with_no_duration(&mut self) { | ||
| let mut android_profile = self.profile.as_ref().unwrap().clone(); |
There was a problem hiding this comment.
Do we really need to clone here?
| let mut android_profile = self.profile.as_ref().unwrap().clone(); | |
| let android_profile = self.profile.as_mut().unwrap(); |
Also, the unwrap() is unfortunate, even if calling parse() guarantees that the option has a value. Maybe we can make AndroidProfile::profile non-optional, and convert parse() into a function that is called during deserialization, something like
#[serde(deserialize_with="parse_android_trace_log")]
profile: AndroidTraceLog,There was a problem hiding this comment.
It is unfortunate and the reason for that was to not have to parse if the relay node was not in processing mode. Now, I think we can still make it so we only parse when we need to while still getting rid of the Option.
This PR aims to be more strict on what profiles we accept. We're seeing profiles with only 1 sample on some threads, sometimes all threads. These profiles are useless and we don't want to ingest them.
This is removing samples which are alone on a given thread and rejecting the profile if we end up without any sample.