feat(stats): Emit outcomes for applied rate limits #951

RyanSkonnord · 2021-03-16T13:08:21Z

Emit outcomes to represent events and attachments being removed from an
envelope by project rate limits. The main difference is in
EnvelopeLimiter, which returns a tuple of Enforcement and
RateLimits used for emitting outcomes:

Enforcements declare the quantities of categories that have been rate
limited with the individual reason codes that caused rate limiting. If
multiple rate limits applied to a category, then the longest limit is
reported.
Rate limits declare all active rate limits, regardless of whether they
have been applied to items in the envelope.
Rate limits for sessions are not reported.

Example

Interaction between Events and Attachments

An envelope with an Error event and an Attachment. Two quotas
specify to drop all attachments (reason "a") and all errors
(reason "e"). The result of enforcement will be:

All items are removed from the envelope.
Enforcements report both the event and the attachment dropped with
reason "e", since dropping an event automatically drops all
attachments with the same reason.
Rate limits report the single event limit "e", since attachment
limits do not need to be checked in this case.

Required Attachments

An envelope with a single Minidump Attachment, and a single quota
specifying to drop all attachments with reason "a":

Since the minidump creates an event and is required for processing,
it remains in the envelope and is marked as rate_limited.
Enforcements report the attachment dropped with reason "a".
Rate limits are empty since it is allowed to send required
attachments even when rate limited.

Previously Rate Limited Attachments

An envelope with a single item marked as rate_limited, and a quota
specifying to drop everything with reason "d":

The item remains in the envelope.
Enforcements are empty. Rate limiting has occurred at an earlier
stage in the pipeline.
Rate limits are empty.

relay-server/src/endpoints/common.rs

TODO: - Fix derive(Debug) on CheckEnvelope - Add envelope summary updates per existing "TODO" comments - Integration test updates

RyanSkonnord · 2021-03-17T02:32:12Z

relay-server/src/actors/project.rs

+    fn emit_rate_limit_outcomes(&self, applied_limits: &RateLimits) {
+        for applied_limit in applied_limits.iter() {
+            if applied_limit.categories.is_empty() {
+                // Empty categories value indicates that the rate limit applies to all data.


Per https://github.com/getsentry/relay/blob/master/relay-quotas/src/rate_limit.rs#L140-L141. So far I have only blind faith in that comment to support that this is correct/necessary behavior, but maybe the integration tests will clarify things as I continue digging.

I believe this approach is problematic since you cannot reconstruct the accurate drop reason from the RateLimits structure, unfortunately. This is an inherent flaw of RateLimits and we can consider changing that, too.

To illustrate, consider the following example:

There is a single quota category:error limit:0. It drops all error events.

EnvelopeLimiter will also drop all attachments in the same envelope as a result of that.

When you check RateLimits here, you won't find the attachment category, even though you just dropped them.

From the top of my head, I have two ideas to solve that:

Move emitting outcomes into EnvelopeLimiter, because that's where you can keep track of the dropped quantities.

From EnvelopeLimiter::enforce, return an instance of EnvelopeSummary that contains the dropped quantities. Then, use that summary (+ scoping) to emit outcomes. This approach is preferable as it decouples concerns.

RyanSkonnord · 2021-03-18T08:34:11Z

Revised to-do list:

Move outcome producer from CheckEnvelope to Project (this removes the earlier problem with the Debug trait)
Add envelope summary update to common.rs
Add envelope summary update to events.rs
Fix failing integration tests afterward, if any
Add new integration test coverage, if needed (on that weird "empty categories" case?)

The current integration test failures are because there are redundant outcomes in the event category. ~~I'm guessing this is because they aren't being removed from the envelope summary in events.rs after rate-limiting. I think there should be no failures once that's fixed.~~ [Update: Nope. See below.]

RyanSkonnord · 2021-03-18T08:35:38Z

relay-server/src/actors/events.rs

-                    Some(envelope) => Ok(envelope),
+                    Some(envelope) => {
+                        // TODO: Fix scope problem and uncomment
+                        // envelope_summary.replace(EnvelopeSummary::compute(&envelope));


@jan-auer Can you advise on this? I'm not sure whether I need to propagate envelope_summary through all the and_then closures up to this point, enclose them all in one big parent closure (which is what makes this simpler in common.rs, AFAICT), or something else.

Update: Never mind, resolved in be73b2b. That turned out to be embarrassingly simple. 🤦 (In my defense, I had to take another pass at wrapping my head around what clone! does in order to understand why that works. Though I'm a little surprised I hadn't tried the same thing by accident already.)

RyanSkonnord · 2021-03-24T03:23:26Z

relay-server/src/actors/events.rs

+                    None => {
+                        envelope_summary.replace(EnvelopeSummary::empty());
+                        Err(ProcessingError::RateLimited(rate_limits))
+                    }


I believe at least one integration test is failing because a rate-limited item is still in the envelope here, making the outcome emitted below redundant to the new one. That means that updating the envelope summary is essentially a no-op. From debug logs, it looks like the item is being correctly removed by RateLimit::enforce, so I'm not clear why a non-empty item list would in the CheckedEnvelope. The order of the debug logs also implies that RateLimit::enforce is happening after this code section, so I seem to be misunderstanding something here.

[Update: This may have changed something but I think the tests are still failing for the same reason.)

There is indeed one case in which we retain rate limited items. Minidump attachments are both errors and attachments. It works like this:

Assume an organization has run out of attachment quota, we will need to reject all attachments now.

A minidump comes in. Since the organization still has errors quota, we need to process the minidump.

We check the rate limiter and it tells us to drop the attachment. Now we take two actions:

We emit an outcome for the dropped attachment.

Leave the minidump item in but mark it as "rate_limited" in its header.

In all subsequent rate limiting checks, the rate limited item is ignored since we already emitted an outcome.

After processing, we store the processed event but drop the attachment without another outcome.

EnvelopeSummary already has a check to compensate for that and ignores items with the rate_limited header set. However, we might have missed a case there.

jan-auer · 2021-03-30T09:22:48Z

relay-server/src/actors/project.rs

+    event_id: Option<EventId>,
+    remote_addr: Option<IpAddr>,


You could move these two into EnvelopeSummary. It looks like this type is a utility that would fit well into utils::rate_limits

jan-auer · 2021-03-30T09:29:34Z

relay-server/src/actors/project.rs

+    fn emit_rate_limit_outcomes(&self, applied_limits: &RateLimits) {
+        for applied_limit in applied_limits.iter() {
+            if applied_limit.categories.is_empty() {
+                // Empty categories value indicates that the rate limit applies to all data.


I believe this approach is problematic since you cannot reconstruct the accurate drop reason from the RateLimits structure, unfortunately. This is an inherent flaw of RateLimits and we can consider changing that, too.

To illustrate, consider the following example:

There is a single quota category:error limit:0. It drops all error events.

EnvelopeLimiter will also drop all attachments in the same envelope as a result of that.

When you check RateLimits here, you won't find the attachment category, even though you just dropped them.

From the top of my head, I have two ideas to solve that:

Move emitting outcomes into EnvelopeLimiter, because that's where you can keep track of the dropped quantities.

From EnvelopeLimiter::enforce, return an instance of EnvelopeSummary that contains the dropped quantities. Then, use that summary (+ scoping) to emit outcomes. This approach is preferable as it decouples concerns.

TODO: Integration test changes still needed?

…rivate

RyanSkonnord · 2021-04-01T02:54:42Z

relay-server/src/actors/project.rs

-        let rate_limits = envelope_limiter.enforce(&mut envelope, scoping)?;
+        let rate_limits = envelope_limiter.enforce(&mut envelope, scoping, |outcome| {
+            self.outcome_producer.do_send(outcome)
+        })?;


I'd like to be able to just pass self.outcome_producer to enforce rather than this awkward closure. I did it this way to accommodate the EnvelopeLimiter unit tests, so that we can use a simple closure as a mock in place of the outcome producer. Let me know if you can suggest a better way to mock out the outcome producer.

The goal of injecting the outcome producer into enforce is to obviate the RateLimitEnforcement struct, as in d4af165. I'm not certain it's worth it. If it isn't, we can just revert that commit.

[EDIT: Never mind, it broke some stuff I hadn't noticed. I've reverted it. The aforementioned commit is still in the history if you want to explore my idea but I don't think it's a high priority at all.]

This reverts commit d4af165.

* master: test(server): Fix flaky shutdown test (#970) fix(stacktrace): Skip serializing some null values in frames interface (#944) release: 0.8.5

tests/integration/test_outcome.py

* master: release: 21.3.1

* master: feat(stats): Emit outcomes for applied rate limits (#951) release: 21.3.1

RyanSkonnord requested a review from jan-auer March 16, 2021 13:08

RyanSkonnord commented Mar 16, 2021

View reviewed changes

relay-server/src/endpoints/common.rs Outdated Show resolved Hide resolved

Base automatically changed from add-track-outcome-quantity to master March 16, 2021 15:06

RyanSkonnord force-pushed the emit-rate-limit-outcomes branch from 0d6e45f to cdf9933 Compare March 16, 2021 16:03

RyanSkonnord added 4 commits March 16, 2021 17:37

feat(stats): Emit outcomes for applied rate limits

328b762

TODO: - Fix derive(Debug) on CheckEnvelope - Add envelope summary updates per existing "TODO" comments - Integration test updates

Attempt quick-and-dirty Debug for CheckEnvelope

79ccc02

Account for RateLimit with empty categories

307f931

Extract rate limit outcomes into struct

3c556b6

RyanSkonnord force-pushed the emit-rate-limit-outcomes branch from cdf9933 to 3c556b6 Compare March 17, 2021 00:39

RyanSkonnord commented Mar 17, 2021

View reviewed changes

RyanSkonnord added 2 commits March 18, 2021 00:36

Move OutcomeProducer from CheckEnvelope to ProjectCache

9b36fc0

Add new envelope summary update in common.rs

f76e797

RyanSkonnord commented Mar 18, 2021

View reviewed changes

RyanSkonnord added 2 commits March 23, 2021 16:34

Add new envelope summary update in events.rs

be73b2b

Empty the envelope summary if no items are left after rate limiting

531bf2b

RyanSkonnord commented Mar 24, 2021

View reviewed changes

RyanSkonnord added 3 commits March 26, 2021 05:09

Some more places to mpty the envelope summary after rate limiting

168b6e4

Update test_outcome assertions

4c87e85

Merge branch 'master' into emit-rate-limit-outcomes

4acba6d

jan-auer reviewed Mar 30, 2021

View reviewed changes

RyanSkonnord added 9 commits March 30, 2021 20:30

Move attrs from RateLimitEnvelope to EnvelopeSummary

3de6c5b

Rename RateLimitEnvelope and move to rate_limits.rs

c43bd1a

Refactor with RateLimitEnforcement - wip

e3dc62f

Refactor with RateLimitEnforcement - wip 2

6dcf4de

Refactor with RateLimitEnforcement - make Envelope optional

4e37a7a

Attempt to emit outcomes correctly

697381b

TODO: Integration test changes still needed?

Small clarification on name

e76b6e5

Merge branch 'master' into emit-rate-limit-outcomes

44e892b

oops

afc85a7

RyanSkonnord added 3 commits March 31, 2021 08:41

Move item retention from envelope.rs to rate_limits.rs; make things p…

686fdd0

…rivate

Do category inference earlier to avoid Item clone

ae00b96

Refactor RateLimitEnforcement struct into a method

d4af165

RyanSkonnord commented Apr 1, 2021

View reviewed changes

RyanSkonnord and others added 4 commits March 31, 2021 20:03

Revert "Refactor RateLimitEnforcement struct into a method"

c8394e6

This reverts commit d4af165.

No longer need an Envelope::empty_copy method after refactoring

d971c52

ref: Replace get_active_limit with RateLimits::longest

bd6071f

ref: Reorder project cache dependencies

d1d2ff3

jan-auer force-pushed the emit-rate-limit-outcomes branch from 7279497 to d1d2ff3 Compare April 1, 2021 11:55

jan-auer added 5 commits April 1, 2021 14:01

fix: Do not double-track rate limited outcomes

f045d7b

ref: Avoid redundant envelope_summary updates.

4719355

ref: Move outcome meta closer to enforcement

c226d61

fix: Lints and test compilation

6845720

doc: Add a description to EnvelopeLimiter::enforce

a126f91

jan-auer marked this pull request as ready for review April 2, 2021 07:01

jan-auer requested review from a team and untitaker April 2, 2021 07:01

jan-auer added 4 commits April 2, 2021 09:04

Merge branch 'master' into emit-rate-limit-outcomes

760dac5

* master: test(server): Fix flaky shutdown test (#970) fix(stacktrace): Skip serializing some null values in frames interface (#944) release: 0.8.5

meta: Changelog

8faf98c

doc: Update doc comment

1a042b1

ref: More docs

22f9328

untitaker approved these changes Apr 2, 2021

View reviewed changes

tests/integration/test_outcome.py Outdated Show resolved Hide resolved

tests/integration/test_outcome.py Outdated Show resolved Hide resolved

jan-auer added 4 commits April 6, 2021 18:34

ref: Apply review suggestions

504a8bd

fix: Do not emit outcomes for sessions

56cd51f

Merge branch 'master' into emit-rate-limit-outcomes

ef105a5

* master: release: 21.3.1

meta: Changelog

2f65537

jan-auer enabled auto-merge (squash) April 9, 2021 10:17

jan-auer merged commit 85acb41 into master Apr 9, 2021

jan-auer deleted the emit-rate-limit-outcomes branch April 9, 2021 10:21

jan-auer added a commit that referenced this pull request Apr 9, 2021

Merge branch 'master' into build/rust-minidump

92ed083

* master: feat(stats): Emit outcomes for applied rate limits (#951) release: 21.3.1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stats): Emit outcomes for applied rate limits #951

feat(stats): Emit outcomes for applied rate limits #951

RyanSkonnord commented Mar 16, 2021 •

edited by jan-auer

Loading

RyanSkonnord Mar 17, 2021

jan-auer Mar 30, 2021 •

edited

Loading

RyanSkonnord commented Mar 18, 2021 •

edited by jan-auer

Loading

RyanSkonnord Mar 18, 2021 •

edited

Loading

RyanSkonnord Mar 24, 2021 •

edited

Loading

jan-auer Mar 30, 2021 •

edited

Loading

jan-auer Mar 30, 2021

jan-auer Mar 30, 2021 •

edited

Loading

RyanSkonnord Apr 1, 2021 •

edited

Loading

feat(stats): Emit outcomes for applied rate limits #951

feat(stats): Emit outcomes for applied rate limits #951

Conversation

RyanSkonnord commented Mar 16, 2021 • edited by jan-auer Loading

Example

RyanSkonnord Mar 17, 2021

Choose a reason for hiding this comment

jan-auer Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

RyanSkonnord commented Mar 18, 2021 • edited by jan-auer Loading

RyanSkonnord Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

RyanSkonnord Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

jan-auer Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

jan-auer Mar 30, 2021

Choose a reason for hiding this comment

jan-auer Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

RyanSkonnord Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

RyanSkonnord commented Mar 16, 2021 •

edited by jan-auer

Loading

jan-auer Mar 30, 2021 •

edited

Loading

RyanSkonnord commented Mar 18, 2021 •

edited by jan-auer

Loading

RyanSkonnord Mar 18, 2021 •

edited

Loading

RyanSkonnord Mar 24, 2021 •

edited

Loading

jan-auer Mar 30, 2021 •

edited

Loading

jan-auer Mar 30, 2021 •

edited

Loading

RyanSkonnord Apr 1, 2021 •

edited

Loading