Skip to content

feat(quota): Rate limit attachments by item count#4377

Merged
jjbayer merged 15 commits into
masterfrom
feat/rate-limit-attachment-items
Dec 16, 2024
Merged

feat(quota): Rate limit attachments by item count#4377
jjbayer merged 15 commits into
masterfrom
feat/rate-limit-attachment-items

Conversation

@jjbayer
Copy link
Copy Markdown
Member

@jjbayer jjbayer commented Dec 12, 2024

Attachment quotas and rate limits are currently defined in bytes, so we have no way to prevent an abusively high number of very small (or even empty) attachments.

ref: #4175

Comment thread relay-quotas/src/quota.rs
| DataCategory::AttachmentItem
| DataCategory::Session => Some(Self::Count),
DataCategory::Attachment => Some(Self::Bytes),
DataCategory::Session => Some(Self::Batched),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An aggregate session item does not support accurate counts, but this does not matter since sessions don't produce outcomes and are only rate possibly limited by setting quota to zero.

///
/// For attachments, we count the number of bytes. Other items are counted as 1.
pub fn quantity(&self) -> usize {
pub fn quantities(&self, purpose: CountFor) -> SmallVec<[(DataCategory, usize); 1]> {
Copy link
Copy Markdown
Member Author

@jjbayer jjbayer Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function now unites the old quantity and outcome_category functions.

We might be able to absorb the index_category into this function in the future, and also replace the EnvelopeSummary with this.

DataCategory::Monitor => &mut self.monitor_quantity,
DataCategory::Span => &mut self.span_quantity,
DataCategory::ProfileChunk => &mut self.profile_chunk_quantity,
// TODO: This catch-all return looks dangerous
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to remove the add_quantity function in a future PR, that will hopefully also resolve this TODO.

"source": expected_source,
},
{
"category": 22, # attachment item
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now get outcomes for attachment items.

attachment_item_limits.longest(),
);
attachment_limits.merge(attachment_item_limits);
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is getting messier with every addition. I will attempt to generalize it in a future PR.

@jjbayer jjbayer marked this pull request as ready for review December 16, 2024 08:51
@jjbayer jjbayer requested a review from a team as a code owner December 16, 2024 08:51
pub event_indexed: CategoryLimit,
/// The combined attachment item rate limit.
/// The combined attachment bytes rate limit.
pub attachments: CategoryLimit,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this name is somewhat set in stone? Because in principle I think something like attachment_bytes would be better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it, but I did not want to mess with the serialization / python code generation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair.

@jjbayer jjbayer merged commit a8305c8 into master Dec 16, 2024
@jjbayer jjbayer deleted the feat/rate-limit-attachment-items branch December 16, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants