feat(processor): Fast path rate limiting for trace attachments #5475
Conversation
tobias-wilfert
changed the title
tobias-wilfert
changed the title
| Event, | ||
| Span, | ||
| Trace, |
There was a problem hiding this comment.
Surprised clippy isn't mad at this, usually need to document every public symbol and enum variants are always public if the enum is.
Would be good to just add documentation of what we consider an Event attachment parent etc. (can also just be generic docs on the type) with an example.
| } | ||
| } | ||
|
|
||
| /// The type of parent entity an attachment is associated with. |
There was a problem hiding this comment.
Would be good just to add some examples or what this is used for.
| } | ||
|
|
||
| /// Recreates the category limit, regardless of if it is active, for a new category with the same reason. | ||
| pub fn clone_for_unchecked(&self, category: DataCategory, quantity: usize) -> CategoryLimit { |
There was a problem hiding this comment.
Discussed offline, we change the logic of clone_for instead if we change the implementation of is_active to self != Default::default().
| /// Stronger check than [`!is_active`](Self::is_active) since this will return true only if the | ||
| /// limit has not be set. So setting an 'in_active' limit will this to return false. | ||
| pub fn is_default(&self) -> bool { | ||
| *self == Self::default() |
There was a problem hiding this comment.
Discussed offline, we want to have an explicit default state, e.g. Option<DataCategory>
| let span_limit = if !enforcement.spans.is_default() { | ||
| &enforcement.spans | ||
| } else { | ||
| &enforcement.spans_indexed | ||
| }; | ||
| enforcement.attachments_limits.span.bytes = span_limit.clone_for_unchecked( |
There was a problem hiding this comment.
Discussed offline, the span_indexed limit is enough as it already inherits the total limit.
| if !summary.attachment_quantities.span.is_empty() | ||
| && (!enforcement.spans.is_default() || !enforcement.spans_indexed.is_default()) |
There was a problem hiding this comment.
Discussed offline, we can simplify that check to enforcement.span.is_active()
Will still need some local fixing but to have them all in before I start the local rework. Co-authored-by: David Herberth <[email protected]>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
relay-server/src/envelope/item.rs
Outdated
| } else { | ||
| Some(ParentType::Event) | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| /// Returns the attachment payload size. | ||
| /// | ||
| /// For AttachmentV2, returns only the size of the actual payload, excluding the attachment meta. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| /// | ||
| /// For standard attachments (V1) always returns [`ParentType::Event`]. | ||
| pub fn attachment_parent_type(&self) -> Option<ParentType> { | ||
| let is_attachment = self.ty() == &ItemType::Attachment; |
There was a problem hiding this comment.
nit: I would make this an early return, i.e.
if self.ty() != &ItemType::Attachment { return None; }| let AttachmentsQuantities { | ||
| event: | ||
| AttachmentQuantities { | ||
| count: event_count, | ||
| bytes: _, | ||
| }, | ||
| trace: | ||
| AttachmentQuantities { | ||
| count: trace_count, | ||
| bytes: _, | ||
| }, | ||
| span: | ||
| AttachmentQuantities { | ||
| count: span_count, | ||
| bytes: _, | ||
| }, | ||
| } = self; | ||
|
|
||
| event_count + trace_count + span_count |
There was a problem hiding this comment.
nit: IMO it's OK to not destructure everything here, since you're only interested in the count field:
| let AttachmentsQuantities { | |
| event: | |
| AttachmentQuantities { | |
| count: event_count, | |
| bytes: _, | |
| }, | |
| trace: | |
| AttachmentQuantities { | |
| count: trace_count, | |
| bytes: _, | |
| }, | |
| span: | |
| AttachmentQuantities { | |
| count: span_count, | |
| bytes: _, | |
| }, | |
| } = self; | |
| event_count + trace_count + span_count | |
| let AttachmentsQuantities {event, trace, span} = self; | |
| event.count + trace.count + span.count |
| /// | ||
| /// Tracks both the count of attachments and size in bytes. | ||
| #[derive(Clone, Copy, Debug, Default)] | ||
| pub struct AttachmentQuantities { |
There was a problem hiding this comment.
nit(naming): I would name this AttachmentQuantity and the other thing AttachmentQuantities, just because multi-level plurals make code hard to read IMO.
| false | ||
| } | ||
| }, | ||
| None => true, // TODO: Not sure if we want to log an error here and if true makes sense rather than false? |
There was a problem hiding this comment.
Yes, let's make attachment_parent_type return a non-Optional ParentType and add a debug_assert inside that function.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| let AttachmentQuantities { | ||
| event: | ||
| AttachmentQuantity { | ||
| count: _, | ||
| bytes: event_bytes, | ||
| }, | ||
| trace: | ||
| AttachmentQuantity { | ||
| count: _, | ||
| bytes: trace_bytes, | ||
| }, | ||
| span: | ||
| AttachmentQuantity { | ||
| count: _, | ||
| bytes: span_bytes, | ||
| }, | ||
| } = self; | ||
|
|
||
| event_bytes + trace_bytes + span_bytes |
There was a problem hiding this comment.
nit: Same as above (no need to destructure).
| type RateLimitTestCase = ( | ||
| &'static str, // name | ||
| &'static [DataCategory], // denied categories | ||
| bool, // expect attachment limit active | ||
| &'static [(DataCategory, usize)], // expected limiter calls | ||
| &'static [(DataCategory, usize)], // expected rate limit outcomes | ||
| ); |
There was a problem hiding this comment.
nit: This tuple has so many entries it would be better to use a struct instead.
tobias-wilfert
deleted the
tobias-wilfert/feat/span-attachment-fast-path-ratelimiting
branch
Follow up to #5457 now adding fast path rate limiting for trace attachments.
Closes INGEST-645