feat: Implement category-based rate limiting (NATIVE-412) #396
Conversation
Parsing of headers and rate limits was implemented, but filtered the envelope was not so far.
|
|
||
| /// The Category of payload that a Rate Limit refers to. | ||
| #[non_exhaustive] | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
nice 🎉 would be neat if we could figure out a way to remove non_exhaustive too
There was a problem hiding this comment.
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.
| // filter again, removing attachments which do not make any sense without | ||
| // an event/transaction | ||
| if filtered.uuid().is_none() { |
There was a problem hiding this comment.
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.
| /// empty. | ||
| pub fn filter<P>(self, mut predicate: P) -> Option<Self> | ||
| where | ||
| P: FnMut(&EnvelopeItem) -> bool, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2 participants
Parsing of headers and rate limits was implemented, but filtered the
envelope was not so far.