feat(spans): Correctly emit negative outcomes for rate limited transactions that have nested spans #3749

iambriccardo · 2024-06-20T07:01:14Z

This PR correctly emits negative outcomes for rate-limited transactions with nested spans by also emitting negative outcomes for the nested spans themselves.

Closes: #3705

…ctions that have nested spans

iambriccardo · 2024-06-20T07:06:45Z

relay-server/src/utils/rate_limits.rs

            profile_chunks,
        } = self;

+        let nested_spans = event.clone_for(DataCategory::Span, spans_length);


In case spans_length is 0, we will have both categories filtered out downstream since they won't be active.

iambriccardo · 2024-06-24T08:56:45Z

relay-server/src/services/project.rs

+                .and_then(|i| serde_json::from_slice::<PartialEvent>(&i.payload()).ok())
+                .map_or(0, |p| p.spans.0)
+        } else {
+            0


Not very nice, but I wanted to avoid unnecessary work in case the flag is not set.

relay-server/src/services/project.rs

Dav1dde · 2024-06-24T12:27:04Z

tests/integration/test_spans.py


 @pytest.mark.parametrize(
-    "category,raises_rate_limited",
+    "category,hits_fast_path",


I prefer the raises_rate_limit.

Yeah, will change it back. I have changed it because of a test refactor but I rolled it back.

CHANGELOG.md

relay-server/src/utils/rate_limits.rs

jjbayer · 2024-06-24T13:05:58Z

relay-server/src/services/project.rs

            envelope_limiter.enforce(envelope.envelope_mut(), &scoping)?;
+
+        if check_nested_spans {
+            track_nested_spans_outcomes(&envelope, &enforcement, spans);


I think we should integrate the new logic a little bit more with the Enforcement, to prevent having multiple code paths that do similar things. It's a good call to not inject the span count into the envelope summary, but it could still be part of the Enforcement. That is:

Pass a flag into enforce to conditionally check for spans in the transaction item and set Enforcement::spans.

Enforcement::track_outcomes automatically does the right thing

We could possibly pass the managed envelope and some options to the track_outcomes and do it there, I preferred this at the moment, because track_outcomes already does work correctly for rate limits with the envelope. Modifying it will affect both cached/non-cached rate limits.

Having track_outcomes inspect the (managed) envelope seems like a footgun to me, since it is potentially built from completely different values than what the envelope contains (assume_event).

Co-authored-by: Joris Bayer <[email protected]>

…getsentry/relay into riccardo/feat/rate-limiting-improvement

CHANGELOG.md

relay-server/src/services/project.rs

relay-server/src/utils/rate_limits.rs

relay-server/src/services/project.rs

iambriccardo · 2024-06-25T08:18:10Z

relay-server/src/services/processor.rs

-        enforcement.track_outcomes(
-            state.envelope(),
-            &state.managed_envelope.scoping(),
-            self.inner.addrs.outcome_aggregator.clone(),
-        );


It's now done inside apply_with_outcomes

iambriccardo · 2024-06-25T08:19:04Z

relay-server/src/services/processor.rs

-        if enforcement.event_active() {
-            state.remove_event();
-            debug_assert!(state.envelope().is_empty());
-        }


Moved up, since it requires enforcement which is consumed by the apply_with_outcomes. I verified that the ordering doesn't seem to impact the end result.

iambriccardo · 2024-06-25T08:20:01Z

relay-server/src/utils/rate_limits.rs

 /// Rate limiting information for a data category.
 #[derive(Debug)]
-struct CategoryLimit {
+#[cfg_attr(test, derive(Clone))]


I wanted to have the clone() in tests. Either we implement manually the Clone in the tests or we go this route. Idk which would be preferred. The manual definition might be more verbose.

Fine with me!

iambriccardo · 2024-06-25T08:21:04Z

relay-server/src/utils/rate_limits.rs

        }
    }

+    fn enforce_and_apply(


Generalized all the code into this function, since I wanted tests to be leaner since a lot of repetition was there previously and some tests were also passing without calling the apply_with_outcomes so to avoid that risk in the future, I decided to have everything into one.

iambriccardo · 2024-06-25T08:21:54Z

relay-server/src/utils/rate_limits.rs

-        let timestamp = relay_common::time::instant_to_date_time(envelope.meta().start_time());
-        let scoping = *scoping;
-        let event_id = envelope.event_id();
-        let remote_addr = envelope.meta().remote_addr();


I made sure the fields were all the same as the ones set by the track_outcome of the ManagedEnvelope.

iambriccardo · 2024-06-25T08:22:14Z

relay-server/src/utils/rate_limits.rs

+                (
+                    Outcome::RateLimited(limit.reason_code),
+                    limit.category,
+                    limit.quantity,
+                )


Felt like there was no need for a struct

relay-server/src/services/project.rs

jjbayer · 2024-06-25T09:31:08Z

relay-server/src/services/project.rs

+            assert_eq!(outcome.category, expected_category);
+            assert_eq!(outcome.quantity, expected_quantity);
+        }
+    }


No python, nice!

jjbayer · 2024-06-25T09:32:29Z

relay-server/src/utils/rate_limits.rs

 /// Rate limiting information for a data category.
 #[derive(Debug)]
-struct CategoryLimit {
+#[cfg_attr(test, derive(Clone))]


Fine with me!

relay-server/src/utils/managed_envelope.rs

Co-authored-by: Joris Bayer <[email protected]>

Dav1dde · 2024-06-25T09:48:31Z

relay-server/src/utils/rate_limits.rs

    ///   to items in the envelope. This excludes rate limits applied to required attachments, since
    ///   clients are allowed to continue sending them.
-    ///
-    /// # Example


Can we keep these docs, they seem to be still relevant? We just don't drop the items anymore directly.

I wanted to rewrite them in the new function. Will do in a follow up.

* master: chore(dynamic-sampling): Remove metrics for dsc tracking (#3766) feat(web-vitals): add support for mobile browsers (#3762) feat(profiles): Support profiler_id in context (#3714) ref(normalization): Add origin and event_type tags to normalization decision (#3764) feat(rate-limiting): Add back docs with examples on rate limiting (#3761) feat(spans): Correctly emit negative outcomes for rate limited transactions that have nested spans (#3749) ref(metrics): Remove unused sentry extra data (#3758) feat(statsd): Emit tokio runtime metrics via statsd (#3755) ref(metrics): Aggregate metrics before rate limiting (#3746) ref(cogs): Remove unused metric, revert to released usage accountant (#3756) build(cargo): Update curve25519-dalek from 4.0.0 to 4.1.3 (#3745) test(deps): Bump requests from 2.31.0 to 2.32.2 (#3752)

Riccardo Busetti added 3 commits June 20, 2024 09:00

feat(spans): Correctly emit negative outcomes for rate limited transa…

6b06e5f

…ctions that have nested spans

Simplify imports

ee0e056

Simplify imports

a482504

iambriccardo commented Jun 20, 2024

View reviewed changes

Riccardo Busetti added 7 commits June 20, 2024 13:47

Rework

c570954

Rework

be678e3

Rework

45536f6

Rework

deac884

Rework

95a3062

Fix

cd2d3b7

Fix

bfe0c24

iambriccardo commented Jun 24, 2024

View reviewed changes

Riccardo Busetti added 4 commits June 24, 2024 12:58

Fix

ee0208e

Merge

9635368

Merge

a2afb5b

Fix

4bc9cb7

iambriccardo marked this pull request as ready for review June 24, 2024 11:20

iambriccardo requested a review from a team as a code owner June 24, 2024 11:21

iambriccardo requested a review from Dav1dde June 24, 2024 11:21

Fix

52b2252

Dav1dde reviewed Jun 24, 2024

View reviewed changes

Fix

853bb55

iambriccardo requested a review from Dav1dde June 24, 2024 12:58

Dav1dde approved these changes Jun 24, 2024

View reviewed changes

jjbayer reviewed Jun 24, 2024

View reviewed changes

iambriccardo and others added 5 commits June 24, 2024 16:21

Update CHANGELOG.md

4ab15b7

Co-authored-by: Joris Bayer <[email protected]>

Fix

5e3c501

Merge branch 'riccardo/feat/rate-limiting-improvement' of github.com:…

cc420c0

…getsentry/relay into riccardo/feat/rate-limiting-improvement

Fix

a11bae6

Fix

baeb987

Fix

d8cd3a3

jjbayer requested changes Jun 25, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

relay-server/src/services/project.rs Outdated Show resolved Hide resolved

relay-server/src/utils/rate_limits.rs Show resolved Hide resolved

relay-server/src/utils/rate_limits.rs Outdated Show resolved Hide resolved

Dav1dde reviewed Jun 25, 2024

View reviewed changes

relay-server/src/utils/rate_limits.rs Outdated Show resolved Hide resolved

relay-server/src/services/project.rs Outdated Show resolved Hide resolved

Riccardo Busetti added 4 commits June 25, 2024 09:33

Fix

b00b16c

Fix

fcd3432

Fix

1352ec6

Fix

88ad88a

iambriccardo requested review from jjbayer and Dav1dde June 25, 2024 08:17

iambriccardo commented Jun 25, 2024

View reviewed changes

Fix

b8fa22e

Dav1dde reviewed Jun 25, 2024

View reviewed changes

relay-server/src/services/project.rs Outdated Show resolved Hide resolved

relay-server/src/services/project.rs Outdated Show resolved Hide resolved

relay-server/src/services/project.rs Outdated Show resolved Hide resolved

Riccardo Busetti added 3 commits June 25, 2024 10:48

Fix

8b37f47

Fix

02e93ad

Fix

5112d99

jjbayer approved these changes Jun 25, 2024

View reviewed changes

iambriccardo and others added 2 commits June 25, 2024 11:38

Update relay-server/src/utils/managed_envelope.rs

97a956e

Co-authored-by: Joris Bayer <[email protected]>

Update relay-server/src/services/project.rs

b24f44e

Co-authored-by: Joris Bayer <[email protected]>

Dav1dde approved these changes Jun 25, 2024

View reviewed changes

iambriccardo merged commit bc3ce51 into master Jun 25, 2024

iambriccardo deleted the riccardo/feat/rate-limiting-improvement branch June 25, 2024 10:55

iambriccardo mentioned this pull request Jun 25, 2024

feat(rate-limiting): Add back docs with examples on rate limiting #3761

Merged

feat(spans): Correctly emit negative outcomes for rate limited transactions that have nested spans #3749

feat(spans): Correctly emit negative outcomes for rate limited transactions that have nested spans #3749

Uh oh!

Conversation

iambriccardo commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iambriccardo commented Jun 20, 2024 •

edited

Loading