Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions sentry-types/src/protocol/envelope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,40 @@ impl Envelope {
.next()
}

/// Filters the Envelope's [`EnvelopeItem`]s based on a predicate,
/// and returns a new Envelope containing only the filtered items.
///
/// Retains the [`EnvelopeItem`]s for which the predicate returns `true`.
/// Additionally, [`EnvelopeItem::Attachment`]s are only kept if the Envelope
/// contains an [`EnvelopeItem::Event`] or [`EnvelopeItem::Transaction`].
///
/// [`None`] is returned if no items remain in the Envelope after filtering.
pub fn filter<P>(self, mut predicate: P) -> Option<Self>
where
P: FnMut(&EnvelopeItem) -> bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

why FnMut over Fn? it doesn't look like self nor the EnvelopeItem are being mutated. filter itself appears to re-construct the envelope. does the predicate also need mut? it looks like this might be leftovers of an attempt at an implementation that mutates and filters the envelope items out of the original envelope.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not about us being mutable, rather that it makes the callback as flexible as possible, so it can possibly mutate its environment if it so choses.
I think the idea is to prefer FnOnce > FnMut > Fn if possible. For example if we save the callback on the struct, and need access to it inside a &self method, we can only go as far as Fn.

{
let mut filtered = Envelope::new();
for item in self.items {
if predicate(&item) {
filtered.add_item(item);
}
}

// filter again, removing attachments which do not make any sense without
// an event/transaction
if filtered.uuid().is_none() {
Comment on lines +162 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

mostly nitpicky comment as this does work: it's a little unfortunate that this is the best way to determine whether an envelope contains a transaction or an event. it relies on a detail noted in the spec, but i think the envelope api could be improved to make this line a little better (and not require a comment).

i think in the future, particularly when transactions are added in a more self-descriptive check would look like self.event().is_some() || self.transaction().is_some(). there's already a getter for an event in an envelope; adding one in for a transaction seems natural once performance monitoring is added to the sdk, which would then allow us to use this predicate instead of checking the uuid.

filtered
.items
.retain(|item| !matches!(item, EnvelopeItem::Attachment(..)))
}

if filtered.items.is_empty() {
None
} else {
Some(filtered)
}
}

/// Serialize the Envelope into the given [`Write`].
///
/// [`Write`]: https://doc.rust-lang.org/std/io/trait.Write.html
Expand Down
38 changes: 35 additions & 3 deletions sentry/src/transports/ratelimit.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use httpdate::parse_http_date;
use std::time::{Duration, SystemTime};

// TODO: maybe move this someplace where we can filter an `Envelope`s items.
use crate::protocol::EnvelopeItem;
use crate::Envelope;

/// A Utility that helps with rate limiting sentry requests.
#[derive(Debug, Default)]
Expand All @@ -10,6 +11,7 @@ pub struct RateLimiter {
error: Option<SystemTime>,
session: Option<SystemTime>,
transaction: Option<SystemTime>,
attachment: Option<SystemTime>,
}

impl RateLimiter {
Expand Down Expand Up @@ -55,6 +57,7 @@ impl RateLimiter {
"error" => self.error = new_time,
"session" => self.session = new_time,
"transaction" => self.transaction = new_time,
"attachment" => self.attachment = new_time,
_ => {}
}
}
Expand All @@ -66,7 +69,10 @@ impl RateLimiter {
}
}

/// Query the RateLimiter for a certain category of event.
/// Query the RateLimiter if a certain category of event is currently rate limited.
///
/// If the given category is rate limited, it will return the remaining
/// [`Duration`] for which it is.
pub fn is_disabled(&self, category: RateLimitingCategory) -> Option<Duration> {
if let Some(ts) = self.global {
let time_left = ts.duration_since(SystemTime::now()).ok();
Expand All @@ -79,14 +85,38 @@ impl RateLimiter {
RateLimitingCategory::Error => self.error,
RateLimitingCategory::Session => self.session,
RateLimitingCategory::Transaction => self.transaction,
RateLimitingCategory::Attachment => self.attachment,
}?;
time_left.duration_since(SystemTime::now()).ok()
}

/// Query the RateLimiter for a certain category of event.
///
/// Returns `true` if the category is *not* rate limited and should be sent.
pub fn is_enabled(&self, category: RateLimitingCategory) -> bool {
self.is_disabled(category).is_none()
}

/// Filters the [`Envelope`] according to the current rate limits.
///
/// Returns [`None`] if all the envelope items were filtered out.
pub fn filter_envelope(&self, envelope: Envelope) -> Option<Envelope> {
envelope.filter(|item| {
self.is_enabled(match item {
EnvelopeItem::Event(_) => RateLimitingCategory::Error,
EnvelopeItem::SessionUpdate(_) | EnvelopeItem::SessionAggregates(_) => {
RateLimitingCategory::Session
}
EnvelopeItem::Transaction(_) => RateLimitingCategory::Transaction,
EnvelopeItem::Attachment(_) => RateLimitingCategory::Attachment,
_ => RateLimitingCategory::Any,
})
})
}
}

/// The Category of payload that a Rate Limit refers to.
#[non_exhaustive]
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 🎉 would be neat if we could figure out a way to remove non_exhaustive too

Copy link
Member Author

Choose a reason for hiding this comment

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

We wouldn’t want that. Right now this is completely private, but once we make it public, we would want to have the freedom to add more categories later on.
Also the compiler is smart enough to exhaustively match inside the crate that defines the #[non_exhaustive] enum.

pub enum RateLimitingCategory {
/// Rate Limit for any kind of payload.
Any,
Expand All @@ -96,6 +126,8 @@ pub enum RateLimitingCategory {
Session,
/// Rate Limit pertaining to Transactions.
Transaction,
/// Rate Limit pertaining to Attachments.
Attachment,
}

#[cfg(test)]
Expand Down
21 changes: 14 additions & 7 deletions sentry/src/transports/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,20 @@ impl TransportThread {
};

if let Some(time_left) = rl.is_disabled(RateLimitingCategory::Any) {
sentry_debug!(
"Skipping event send because we're disabled due to rate limits for {}s",
time_left.as_secs()
);
continue;
}
rl = send(envelope, rl).await;
sentry_debug!(
"Skipping event send because we're disabled due to rate limits for {}s",
time_left.as_secs()
);
continue;
}
match rl.filter_envelope(envelope) {
Some(envelope) => {
rl = send(envelope, rl).await;
},
None => {
sentry_debug!("Envelope was discarded due to per-item rate limits");
},
};
}
})
})
Expand Down