ref(processing): Use new transactions processor #5379
Conversation
| metrics_extracted, | ||
| spans_extracted, | ||
| } = ctx; | ||
| // TODO(follow-up): this function should always extract metrics. Dynamic sampling should validate |
| #[cfg(debug_assertions)] | ||
| event.ensure_span_count(); |
There was a problem hiding this comment.
Doesn't that mess with the Managed assertions and we may get misaccounting in prod, but not in tests because this is ran in tests but not in production?
Or is this there to specifically defuse these Managed assertions and it's okay if we misaccount in prod until the transaction is parsed?
There was a problem hiding this comment.
Or is this there to specifically defuse these Managed assertions and it's okay if we misaccount in prod until the transaction is parsed?
Yes, I believe it's not worth shallow-parsing in prod, because it gets properly parsed immediately afterwards.
| category: _, | ||
| } = self; | ||
| debug_assert!(flags.metrics_extracted); | ||
| let mut quantities = smallvec![(DataCategory::TransactionIndexed, 1),]; |
There was a problem hiding this comment.
| let mut quantities = smallvec![(DataCategory::TransactionIndexed, 1),]; | |
| let mut quantities = smallvec![(DataCategory::TransactionIndexed, 1)]; |
| let limits = rate_limiter | ||
| .try_consume(scoping.item(DataCategory::Transaction), 1) | ||
| .await; |
There was a problem hiding this comment.
If the transaction is limited, we can already drop the entire payload and don't need to consider attachments and profiles, right?
There was a problem hiding this comment.
Yes, I thought I had an early return here, good catch.
| // If there is a transaction limit, drop everything. | ||
| // This also affects profiles that lost their transaction due to sampling. | ||
| let limits = rate_limiter | ||
| .try_consume(scoping.item(DataCategory::TransactionIndexed), 1) |
There was a problem hiding this comment.
I think you need to check both categories here, that's what the EnvelopeLimiter does.
Note fn is_event_active() checks both categories, indexed and non-indexed as well.
There was a problem hiding this comment.
Naivly I would now expect transaction rate limits never to apply, what is strange, I would've expected a test to fail here. Am I missing something?
The spans processor always only rate limits total + indexed, it extracts the metrics only after rate limiting. I think this is also what we should do here.
Then you also don't have to implement RateLimited twice.
| ($($variant:ident => $ty:ty,)*) => { | ||
| /// All known [`Processor`] outputs. | ||
| #[derive(Debug)] | ||
| #[allow(clippy::large_enum_variant)] |
There was a problem hiding this comment.
| #[allow(clippy::large_enum_variant)] | |
| #[expect(clippy::large_enum_variant), reason = "..."] |
Alternatively we can also box the transaction output, I think that kinda depends why the lint triggers, if it's just because of a small-ish amount it's fine, if there is a large discrepancy maybe it's worth boxing.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Bug: Rate limiting doesn't check indexed transaction quota
The rate limiting implementation for ExpandedTransaction<TotalAndIndexed> only checks the Transaction category quota, but the quantities() method reports both Transaction and TransactionIndexed categories. This means TransactionIndexed quota limits are never enforced for transactions before metrics extraction. In contrast, the ExpandedTransaction<Indexed> rate limiting correctly checks both categories (lines 537-546). Without checking the indexed category, rate limits on indexed transactions can be bypassed.
Additional Locations (1)
jjbayer
force-pushed
the
ref/use-new-processor
branch
from
3c0b62d to
1c0aea5
Compare
Closes INGEST-610.