feat(server): Expose rate limits for metrics #3347

jan-auer · 2024-03-27T10:44:51Z

Adds a new rate limit component to quota limits in the x-sentry-rate-limits header that communicates the namespace(s) that a metrics rate limit should apply to. The data category of limits that have this component is always metric_bucket, in other quotas this component will not be set.

Clients will receive rate limit entries in the response of every envelope that contains a metric item, regardless of whether the contents match the specific namespace. This is a performance optimization so Relay can avoid to parse item contents in the fast path while the request is still open.

Limitations:

As soon as a single metric rate limit is active, all envelope requests containing a metrics item will result in a 429 status code. It is the client's responsibility to parse the header and ignore irrelevant rate limits.
These rate limits are not propagated on the batch metrics endpoint. External Relays do not use this endpoint and send envelopes instead, so backoff will work as expected. However, if external Relays would start to use that endpoint, additional changes would be necessary as they also do not receive quotas.

Documentation is added via getsentry/develop#1205

Epic: https://github.com/getsentry/team-ingest/issues/303

* master: fix(statsd): Prefix kafka client metrics (#3341)

jan-auer · 2024-03-27T13:33:24Z

relay-server/src/services/project.rs

+        // because `EnvelopeLimiter` cannot not check metrics without parsing item contents.
+        if envelope.envelope().items().any(|i| i.ty().is_metrics()) {
+            let metrics_scoping = scoping.item(DataCategory::MetricBucket);
+            rate_limits.merge(self.rate_limits.check_with_quotas(quotas, metrics_scoping));


This does not work yet. The scoping does not have a namespace, hence no quotas with namespaces match.

Updated. We now check with MetricNamespaceScoping::Any, which will resolve all metrics. This also makes the API a bit more explicit by adding specific doc comments for the semantics of None.

* master: ref(cardinality): Pipeline Redis script invocations (#3321) ref(normalization): Remove StoreProcessor (#3097) feat(cardinality): Implement name based cardinality limits (#3313) instr(kafka): Tag existing metrics with variant (#3352) instr(kafka): More broker stats (#3349) instr(kafka): Improve produce error handling (#3351) feat(schema): Allow integers as username (#3328)

jan-auer · 2024-03-28T10:36:20Z

relay-server/src/services/processor.rs

@@ -2010,44 +2012,31 @@ impl EnvelopeProcessorService {
            // We set over_accept_once such that the limit is actually reached, which allows subsequent
            // calls with quantity=0 to be rate limited.
            let over_accept_once = true;
+            let mut rate_limits = RateLimits::new();
+
+            for category in [DataCategory::Transaction, DataCategory::Span] {


Drive-by rewrite for readability.

jan-auer · 2024-03-28T10:37:30Z

relay-server/src/services/project.rs

-            .check_with_quotas(project_state.get_quotas(), item_scoping);
+        let limits = self.rate_limits().check_with_quotas(
+            project_state.get_quotas(),
+            scoping.item(DataCategory::MetricBucket),


This might be a bug, filed in #3354

jjbayer

Just a couple of nits.

jjbayer · 2024-03-28T12:03:27Z

relay-quotas/src/rate_limit.rs

+            scope: RateLimitScope::Organization(42),
+            reason_code: None,
+            retry_after: RetryAfter::from_secs(1),
+            namespace: smallvec![MetricNamespace::Custom],


Should we run the same test against a RateLimit with empty namespaces?

There are tests above that test with empty list on different data categories -- a rate limit on MetricBuckets without namespace would actually be semantically incorrect. Though it will work as the other tests ensure.

relay-quotas/src/rate_limit.rs

jjbayer · 2024-03-28T12:06:46Z

relay-server/src/services/processor.rs

@@ -2010,44 +2012,31 @@ impl EnvelopeProcessorService {
            // We set over_accept_once such that the limit is actually reached, which allows subsequent
            // calls with quantity=0 to be rate limited.
            let over_accept_once = true;
+            let mut rate_limits = RateLimits::new();
+
+            for category in [DataCategory::Transaction, DataCategory::Span] {


jjbayer · 2024-03-28T12:14:21Z

relay-server/src/utils/rate_limits.rs

@@ -35,6 +35,13 @@ pub fn format_rate_limits(rate_limits: &RateLimits) -> String {

        if let Some(ref reason_code) = rate_limit.reason_code {
            write!(header, ":{reason_code}").ok();
+        } else if !rate_limit.namespace.is_empty() {
+            write!(header, ":").ok();


It took me a while to understand this code.

Suggested change

write!(header, ":").ok();

write!(header, ":").ok(); // delimits the empty reason code

Thanks, the comment helps. I was hoping to rewrite this in a better way, but everything else I could come up with ended up much more complex.

relay-server/src/utils/rate_limits.rs

feat(server): Expose rate limits for metrics

f987757

jan-auer requested a review from a team as a code owner March 27, 2024 10:44

jan-auer marked this pull request as draft March 27, 2024 10:47

jan-auer self-assigned this Mar 27, 2024

jan-auer added 3 commits March 27, 2024 13:26

feat: Support multiple namespaces per rate limit

ae523f3

ref: Remove new item header again

cc18860

fix: Ensure namespaces are only matched on metrics

5395933

jan-auer force-pushed the feat/metrics-rate-limits-namespace branch from 094a864 to 5395933 Compare March 27, 2024 12:34

jan-auer mentioned this pull request Mar 27, 2024

feat(sdk): Add name spaces for metrics rate limits getsentry/develop#1205

Merged

jan-auer marked this pull request as ready for review March 27, 2024 12:38

jan-auer added 4 commits March 27, 2024 13:39

Merge branch 'master' into feat/metrics-rate-limits-namespace

c0dbdd1

* master: fix(statsd): Prefix kafka client metrics (#3341)

meta: Changelog

17a9af2

test: Update snapshots

7246772

test: Add integration tests

b5663b5

jan-auer commented Mar 27, 2024

View reviewed changes

jan-auer mentioned this pull request Mar 28, 2024

Potential bug: metric rate limits checked without namespace #3354

Closed

jan-auer added 3 commits March 28, 2024 11:26

fix: Properly check rate limits in the fast path

59dc5e6

doc: More docs

0f7de8a

jan-auer commented Mar 28, 2024

View reviewed changes

jan-auer added 2 commits March 28, 2024 11:39

fix: Lint

3a92dcb

test: Fix integration tests

1be454b

jjbayer approved these changes Mar 28, 2024

View reviewed changes

ref: Adress review feedback

697e236

jan-auer enabled auto-merge (squash) March 28, 2024 12:47

jan-auer merged commit ea9683d into master Mar 28, 2024
20 checks passed

jan-auer deleted the feat/metrics-rate-limits-namespace branch March 28, 2024 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): Expose rate limits for metrics #3347

feat(server): Expose rate limits for metrics #3347

jan-auer commented Mar 27, 2024 •

edited

Loading

jan-auer Mar 27, 2024

jan-auer Mar 28, 2024

jan-auer Mar 28, 2024

jjbayer Mar 28, 2024

jan-auer Mar 28, 2024

jjbayer left a comment

jjbayer Mar 28, 2024

jan-auer Mar 28, 2024

jjbayer Mar 28, 2024

jjbayer Mar 28, 2024

jan-auer Mar 28, 2024

	write!(header, ":").ok();
	write!(header, ":").ok(); // delimits the empty reason code

feat(server): Expose rate limits for metrics #3347

feat(server): Expose rate limits for metrics #3347

Conversation

jan-auer commented Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer commented Mar 27, 2024 •

edited

Loading