Per-project rate limits not working correctly

[MeredithAnya]

As reported from a customer, per-project rate limiting is not respecting the limit they have set. They have limit set to 1000 events in 12 hrs but are seeing upwards of 5K errors in one day.

@getsentry/platform

[dcramer]

@mattrobenolt @tkaemming could one of you take a look at this today? its a high priority concern

[tkaemming]

The frontend is sending this data in minute units, while the quota backend expects them to be seconds. So, what the UI is representing as "12 hour" window (720 minutes) is actually being treated as a "12 minute" (720 second) window by the backend.

Backend Implementation

The key quota query object is constructed as part of RedisQuota.get_quotas:

sentry/src/sentry/quotas/redis.py

Lines 81 to 90 in 3935302

	if key:
	kquota = self.get_key_quota(key)
	results.append(
	BasicRedisQuota(
	key='k:{}'.format(key.id),
	limit=kquota[0],
	window=kquota[1],
	reason_code='key_quota',
	)
	)

The parameters used to construct the quota query object are derived from BaseQuota.get_key_quota, which returns the value directly from the ProjectKey.rate_limit property:

sentry/src/sentry/quotas/base.py

Lines 69 to 74 in 3935302

	def get_key_quota(self, key):
	from sentry import features
	if features.has('projects:rate-limits', key.project):
	return key.rate_limit
	return (0, 0)

sentry/src/sentry/models/projectkey.py

Lines 123 to 127 in 3935302

	@property
	def rate_limit(self):
	if self.rate_limit_count and self.rate_limit_window:
	return (self.rate_limit_count, self.rate_limit_window)
	return (0, 0)

So, we have the ProjectKey.rate_limit_window value directly propagating to the BasicRedisQuota quota query object without any transformation and being set as BasicRedisQuota.window, which is defined in second units:

sentry/src/sentry/quotas/redis.py

Lines 29 to 30 in 3935302

	# time in seconds that this quota reflects
	self.window = window

Management UI

The PUT method on the ProjectKeyDetailsEndpoint saves the rate limit details directly from the RateLimitSerializer, which also does no transformation of the input data:

sentry/src/sentry/api/endpoints/project_key_details.py

Lines 99 to 105 in 3935302

	if features.has('projects:rate-limits', project):
	if result.get('rateLimit', -1) is None:
	key.rate_limit_count = None
	key.rate_limit_window = None
	elif result.get('rateLimit'):
	key.rate_limit_count = result['rateLimit']['count']
	key.rate_limit_window = result['rateLimit']['window']

sentry/src/sentry/api/endpoints/project_key_details.py

Lines 36 to 38 in 3935302

	class RateLimitSerializer(serializers.Serializer):
	count = serializers.IntegerField(min_value=0, required=False)
	window = serializers.IntegerField(min_value=0, max_value=60 * 24, required=False)

On the front end, the rate limit windows are defined here with minute units:

sentry/src/sentry/static/sentry/app/views/projectKeyDetails.jsx

Lines 234 to 247 in 3935302

	getRateLimitWindows() {
	return [
	['', ''],
	[1, '1 minute'],
	[5, '5 minutes'],
	[15, '15 minutes'],
	[60, '1 hour'],
	[120, '2 hours'],
	[240, '4 hours'],
	[360, '6 hours'],
	[720, '12 hours'],
	[1440, '24 hours']
	];
	},

sentry/src/sentry/static/sentry/app/views/projectKeyDetails.jsx

Lines 343 to 356 in 3935302

	<div style={{width: 150, display: 'inline-block'}}>
	<Select2Field
	width="100%"
	key="rateLimit.window"
	name="rateLimit.window"
	choices={this.getRateLimitWindows()}
	value={idx(formData, _ => _.rateLimit.window)}
	required={false}
	error={errors.rateLimit}
	placeholder={t('window')}
	allowClear={true}
	onChange={this.onRateLimitChange.bind(this, 'window')}
	className=""
	/>

[evanpurkhiser]

Great investigation overview @tkaemming 🥇

[tkaemming]

I also checked on the customer account that reported this — the UI shows them with a "1000 event(s) in 12 hours" rate limit (as noted in the original issue comment) that was being stored as 'rate_limit_count': 1000, 'rate_limit_window': 720 on the ProjectKey instance to verify.

[dcramer]

@tkaemming would you mind putting up a quick fix for it? we could add a data migration as well, but there's only a handful of accounts using it, so fixing it in prod would also be fine.

[tkaemming]

Yup, was planning on doing that today. I was thinking about fixing it at the model level to avoid the data migration, but I can do the data migration instead if that's your preference.

[dcramer]

no strong opinion -- it'll never be less than 1 minute