fix(quotas): correct behavior of per-project keys #6013
Conversation
Migration Checklist
Generated by 🚫 danger |
|
|
||
| ProjectKey = orm['sentry.ProjectKey'] | ||
|
|
||
| queryset = ProjectKey.objects.filter(rate_limit_window__isnull=False) |
There was a problem hiding this comment.
>>> ProjectKey.objects.filter(rate_limit_window__isnull=False).count()
61| ProjectKey.objects.filter(pk=key.pk).update( | ||
| rate_limit_window=key.rate_limit_window * 60) | ||
|
|
||
| def backwards(self, orm): |
There was a problem hiding this comment.
Not required but I added it since it was easy and used it for manual testing
There was a problem hiding this comment.
So I'd also recommend shipping out the migration after the change. Otherwise, it's possible that new values will get stored wrong between the time migration ran, and the time deploy finishes.
Also, since this is a frontend only change, this time window might be significantly larger. Is there any way that we can also fix this on the backend? So even if the frontend sends a bad value, we just * 60 it to make it right?
| queryset = ProjectKey.objects.filter(rate_limit_window__isnull=False) | ||
| for key in RangeQuerySetWrapperWithProgressBar(queryset): | ||
| ProjectKey.objects.filter(pk=key.pk).update( | ||
| rate_limit_window=key.rate_limit_window * 60) |
There was a problem hiding this comment.
Just as a little bit of protection from race here:
ProjectKey.objects.filter(pk=key.pk, rate_limit_window=key.rate_limit_window).update(rate_limit_window=key.rate_limit_window * 60)Just in case for the small chance it were updated in between, we don't clobber the value.
|
For context, this affects 0.018988917942970987% of all project keys. |
|
Yeah, you're not at all wrong with the points above — I just wasn't really sure that it was worth the effort to jump through all of the hoops to comprehensively fix what looks to be a feature that is used by only a few users. I don't think there'd be a very reliable way to handle this without pushing it down to the model layer to "migrate" it on the fly on the way in and out of the database, and I didn't think it was worth special casing this for an indefinite period of time since it would only make already complicated code even more complicated. |
The frontend was sending this data in minute units, while the quota backend expects them to be seconds. So, what the UI was representing as "12 hour" window (720 minutes) was actually being treated as a "12 minute" (720 second) window by the backend.
Fixes GH-6007 (a detailed description of the issue is contained within that ticket), references GH-5227.