fix(profiling): Classify profile chunks for rate limits and outcomes #4595
Conversation
Dav1dde
force-pushed
the
dav1d/ratelimit-profile-chunks
branch
3 times, most recently
from
f6de8a0 to
65c2067
Compare
Dav1dde
force-pushed
the
dav1d/ratelimit-profile-chunks
branch
from
65c2067 to
d53c559
Compare
Dav1dde
force-pushed
the
dav1d/ratelimit-profile-chunks
branch
from
d53c559 to
4df7283
Compare
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
| pub fn profile_type(&self) -> ProfileType { | ||
| match self.profile.platform.as_str() { | ||
| "cocoa" | "android" | "javascript" => ProfileType::Ui, | ||
| _ => ProfileType::Backend, | ||
| } | ||
| } |
| /// The profile type is currently determined based on the contained profile | ||
| /// platform. It determines the data category this profile chunk belongs to. | ||
| /// | ||
| /// This needs to be synchronized with the implementation in Sentry: |
There was a problem hiding this comment.
If we can add the profile_type in the payload before we send it, this would become the source of truth.
There was a problem hiding this comment.
I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.
There was a problem hiding this comment.
Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?
There was a problem hiding this comment.
I would add it to the payload and not the header, it's easier to manipulate later on.
| ItemType::ProfileChunk => match item.profile_type() { | ||
| Some(ProfileType::Backend) => !self.profile_chunks.is_active(), | ||
| Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(), | ||
| None => true, |
There was a problem hiding this comment.
Will this reject or accept profiles when we don't have a type?
There was a problem hiding this comment.
It will accept when there is no type.
|
We started adding a
Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default. |
This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.
This does have several downsides:
The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.
Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).
We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.
The test
test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limitwas never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.Fixes: https://github.com/getsentry/team-ingest/issues/679