Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Mar 28, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f987757
feat(server): Expose rate limits for metrics
jan-auer Mar 27, 2024
ae523f3
feat: Support multiple namespaces per rate limit
jan-auer Mar 27, 2024
cc18860
ref: Remove new item header again
jan-auer Mar 27, 2024
5395933
fix: Ensure namespaces are only matched on metrics
jan-auer Mar 27, 2024
c0dbdd1
Merge branch 'master' into feat/metrics-rate-limits-namespace
jan-auer Mar 27, 2024
17a9af2
meta: Changelog
jan-auer Mar 27, 2024
7246772
test: Update snapshots
jan-auer Mar 27, 2024
b5663b5
test: Add integration tests
jan-auer Mar 27, 2024
59dc5e6
fix: Properly check rate limits in the fast path
jan-auer Mar 28, 2024
804711f
Merge branch 'master' into feat/metrics-rate-limits-namespace
jan-auer Mar 28, 2024
0f7de8a
doc: More docs
jan-auer Mar 28, 2024
3a92dcb
fix: Lint
jan-auer Mar 28, 2024
1be454b
test: Fix integration tests
jan-auer Mar 28, 2024
697e236
ref: Adress review feedback
jan-auer Mar 28, 2024
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
Prev Previous commit
Next Next commit
feat: Support multiple namespaces per rate limit
  • Loading branch information
@jan-auer
jan-auer committed Mar 27, 2024
commit ae523f31c9b7b432f3cbe2d88681dcce2c0f661b
125 changes: 94 additions & 31 deletions relay-quotas/src/rate_limit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::time::{Duration, Instant};

use relay_base_schema::metrics::MetricNamespace;
use relay_base_schema::project::{ProjectId, ProjectKey};
use smallvec::SmallVec;

use crate::quota::{DataCategories, ItemScoping, Quota, QuotaScope, ReasonCode, Scoping};
use crate::REJECT_ALL_SECS;
Expand Down Expand Up @@ -157,7 +158,7 @@ pub struct RateLimit {
pub retry_after: RetryAfter,

/// The namespace of this rate limit.
pub namespace: Option<MetricNamespace>,
pub namespace: SmallVec<[MetricNamespace; 1]>,
jan-auer marked this conversation as resolved.
Show resolved Hide resolved
}

impl RateLimit {
Expand All @@ -168,20 +169,26 @@ impl RateLimit {
scope: RateLimitScope::for_quota(scoping, quota.scope),
reason_code: quota.reason_code.clone(),
retry_after,
namespace: quota.namespace,
namespace: quota.namespace.into_iter().collect(),
}
}

/// Checks whether the rate limit applies to the given item.
pub fn matches(&self, scoping: ItemScoping<'_>) -> bool {
self.matches_scope(scoping)
&& self.matches_namespace(scoping)
&& scoping.matches_categories(&self.categories)
&& self.matches_namespace(scoping)
}

/// Returns `true` if the rate limit namespace matches the namespace of the item.
fn matches_namespace(&self, scoping: ItemScoping<'_>) -> bool {
self.namespace.is_none() || self.namespace == scoping.namespace
if self.namespace.is_empty() {
true
} else if let Some(namespace) = scoping.namespace {
self.namespace.contains(&namespace)
} else {
false
}
}

/// Returns `true` if the rate limiting scope matches the given item.
Expand Down Expand Up @@ -221,9 +228,15 @@ impl RateLimits {
limit.categories.sort();

let limit_opt = self.limits.iter_mut().find(|l| {
l.categories == limit.categories
&& l.scope == limit.scope
&& l.namespace == limit.namespace
let RateLimit {
categories,
scope,
reason_code: _,
retry_after: _,
namespace,
} = &limit;

*categories == l.categories && *scope == l.scope && *namespace == l.namespace
});

match limit_opt {
Expand Down Expand Up @@ -417,7 +430,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
};

assert!(rate_limit.matches(ItemScoping {
Expand Down Expand Up @@ -450,7 +463,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
};

assert!(rate_limit.matches(ItemScoping {
Expand Down Expand Up @@ -483,7 +496,7 @@ mod tests {
scope: RateLimitScope::Project(ProjectId::new(21)),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
};

assert!(rate_limit.matches(ItemScoping {
Expand All @@ -509,6 +522,56 @@ mod tests {
}));
}

#[test]
fn test_rate_limit_matches_namespaces() {
let rate_limit = RateLimit {
categories: smallvec![],
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: smallvec![MetricNamespace::Custom],
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

};

let scoping = Scoping {
organization_id: 42,
project_id: ProjectId::new(21),
project_key: ProjectKey::parse("a94ae32be2584e0bbd7a4cbb95971fee").unwrap(),
key_id: None,
};

assert!(rate_limit.matches(ItemScoping {
category: DataCategory::MetricBucket,
scoping: &scoping,
namespace: Some(MetricNamespace::Custom),
}));

assert!(!rate_limit.matches(ItemScoping {
category: DataCategory::MetricBucket,
scoping: &scoping,
namespace: Some(MetricNamespace::Spans),
}));

let general_rate_limit = RateLimit {
categories: smallvec![],
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: smallvec![], // all namespaces
};

assert!(general_rate_limit.matches(ItemScoping {
category: DataCategory::MetricBucket,
scoping: &scoping,
namespace: Some(MetricNamespace::Spans),
}));

assert!(general_rate_limit.matches(ItemScoping {
category: DataCategory::MetricBucket,
scoping: &scoping,
namespace: None,
}));
}

#[test]
fn test_rate_limit_matches_key() {
let rate_limit = RateLimit {
Expand All @@ -518,7 +581,7 @@ mod tests {
),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
};

assert!(rate_limit.matches(ItemScoping {
Expand Down Expand Up @@ -553,7 +616,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("first")),
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

// longer rate limit shadows shorter one
Expand All @@ -562,7 +625,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("second")),
retry_after: RetryAfter::from_secs(10),
namespace: None,
namespace: smallvec![],
});

insta::assert_ron_snapshot!(rate_limits, @r###"
Expand Down Expand Up @@ -592,7 +655,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("first")),
retry_after: RetryAfter::from_secs(10),
namespace: None,
namespace: smallvec![],
});

// shorter rate limit is shadowed by existing one
Expand All @@ -601,7 +664,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("second")),
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

insta::assert_ron_snapshot!(rate_limits, @r###"
Expand Down Expand Up @@ -631,7 +694,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

// Same scope but different categories
Expand All @@ -640,7 +703,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

// Same categories but different scope
Expand All @@ -649,7 +712,7 @@ mod tests {
scope: RateLimitScope::Project(ProjectId::new(21)),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

insta::assert_ron_snapshot!(rate_limits, @r###"
Expand Down Expand Up @@ -697,7 +760,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: Some(MetricNamespace::Custom),
namespace: smallvec![MetricNamespace::Custom],
});

// Same scope but different categories
jan-auer marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -706,7 +769,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: Some(MetricNamespace::Spans),
namespace: smallvec![MetricNamespace::Spans],
});

insta::assert_ron_snapshot!(rate_limits, @r###"
Expand Down Expand Up @@ -744,7 +807,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("first")),
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

// Distinct scope to prevent deduplication
Expand All @@ -753,7 +816,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("second")),
retry_after: RetryAfter::from_secs(10),
namespace: None,
namespace: smallvec![],
});

let rate_limit = rate_limits.longest().unwrap();
Expand All @@ -780,7 +843,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

// Inactive error limit with distinct scope
Expand All @@ -789,7 +852,7 @@ mod tests {
scope: RateLimitScope::Project(ProjectId::new(21)),
reason_code: None,
retry_after: RetryAfter::from_secs(0),
namespace: None,
namespace: smallvec![],
});

// Sanity check before running `clean_expired`
Expand Down Expand Up @@ -825,7 +888,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

// Active transaction limit
Expand All @@ -834,7 +897,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

let applied_limits = rate_limits.check(ItemScoping {
Expand Down Expand Up @@ -876,7 +939,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

// Active transaction limit
Expand All @@ -885,7 +948,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

let item_scoping = ItemScoping {
Expand Down Expand Up @@ -939,23 +1002,23 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("first")),
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

rate_limits1.add(RateLimit {
categories: smallvec![DataCategory::Transaction],
scope: RateLimitScope::Organization(42),
reason_code: None,
retry_after: RetryAfter::from_secs(1),
namespace: None,
namespace: smallvec![],
});

rate_limits2.add(RateLimit {
categories: smallvec![DataCategory::Error],
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("second")),
retry_after: RetryAfter::from_secs(10),
namespace: None,
namespace: smallvec![],
});

rate_limits1.merge(rate_limits2);
Expand Down
11 changes: 6 additions & 5 deletions relay-quotas/src/redis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ mod tests {
use relay_base_schema::project::{ProjectId, ProjectKey};
use relay_redis::redis::Commands;
use relay_redis::RedisConfigOptions;
use smallvec::smallvec;

use super::*;
use crate::quota::{DataCategories, DataCategory, ReasonCode, Scoping};
Expand Down Expand Up @@ -379,7 +380,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("get_lost")),
retry_after: rate_limits[0].retry_after,
namespace: None,
namespace: smallvec![],
}]
);
}
Expand Down Expand Up @@ -495,7 +496,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("get_lost")),
retry_after: rate_limits[0].retry_after,
namespace: None,
namespace: smallvec![],
}]
);
} else {
Expand Down Expand Up @@ -545,7 +546,7 @@ mod tests {
scope: RateLimitScope::Global,
reason_code: Some(ReasonCode::new("get_lost")),
retry_after: rate_limits[0].retry_after,
namespace: None,
namespace: smallvec![],
}]
);
} else {
Expand Down Expand Up @@ -737,7 +738,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("project_quota1")),
retry_after: rate_limits[0].retry_after,
namespace: None,
namespace: smallvec![],
}]
);
}
Expand Down Expand Up @@ -785,7 +786,7 @@ mod tests {
scope: RateLimitScope::Organization(42),
reason_code: Some(ReasonCode::new("get_lost")),
retry_after: rate_limits[0].retry_after,
namespace: None,
namespace: smallvec![],
}]
);
} else {
Expand Down
Loading