fix(profiles): Rate limit profiles when transaction was sampled #4195
Conversation
jjbayer
force-pushed
the
fix/profiles-rate-limit-wo-txn
branch
from
f296745 to
ff52ec6
Compare
| if profile_limits.is_empty() && summary.event_category.is_none() { | ||
| profile_limits = self | ||
| .check | ||
| .apply(scoping.item(DataCategory::Transaction), 0)?; |
There was a problem hiding this comment.
We don't need to check for TransactionIndexed here, because indexed transactions will trigger the if let Some(category) = summary.event_category { branch above.
There was a problem hiding this comment.
I think you're correct that we don't have to check indexed here, but for the wrong reason.
It won't trigger the branch, because here we never have an event_category. But we don't have to check the indexed payload because I assume we don't want to enforce the indexed profile quota on profiles.
But this really is awkward and tricky, because ideally we should persist the information that we dropped indexed transactions on the profile (so it is treated like dynamic sampling), because then the profiling consumers later know that there won't be any transactions for the profiles (like it would be the case if the transaction is dynamically sampled).
As a side note, god damn I hate this function.
There was a problem hiding this comment.
It won't trigger the branch, because here we never have an
event_category.
Let's break it down:
- If the dynamic sampling decision was "keep" and there is a limit on either
transactionortransaction_indexed, the profile is already dropped here. - If the dynamic sampling decision was "drop" and there is a limit on
transaction, the new code drops the profile. - If the dynamic sampling decision was "drop" and there is a limit on
transaction_indexed, we are in undefined territory. I want to err on the side of not enforcingtransaction_indexedon profiles, but it is awkward like you said, because in the "keep" case we would drop the profile.
As a side note, god damn I hate this function.
Yes.
| if profile_limits.is_empty() && summary.event_category.is_none() { | ||
| profile_limits = self | ||
| .check | ||
| .apply(scoping.item(DataCategory::Transaction), 0)?; |
There was a problem hiding this comment.
I think you're correct that we don't have to check indexed here, but for the wrong reason.
It won't trigger the branch, because here we never have an event_category. But we don't have to check the indexed payload because I assume we don't want to enforce the indexed profile quota on profiles.
But this really is awkward and tricky, because ideally we should persist the information that we dropped indexed transactions on the profile (so it is treated like dynamic sampling), because then the profiling consumers later know that there won't be any transactions for the profiles (like it would be the case if the transaction is dynamically sampled).
As a side note, god damn I hate this function.
fixes: #4280