Rate limit devices per user on short window #35613
Conversation
gherceg
force-pushed
the
gh/rate-limiting/devices-per-user
branch
6 times, most recently
from
3e22f49 to
6f6f461
Compare
TestAuditLoggingForFormSubmission::test_api_user_api_endpoint failed due to attempting to pass a Mock object to convert_xform_to_json
gherceg
force-pushed
the
gh/rate-limiting/devices-per-user
branch
from
6f6f461 to
bc9d976
Compare
|
Moving the remaining TODOs to the JIRA ticket:
The first one will be easier to do based on the datadog metric, and I can work on the confluence page while this is in review. |
corehq/apps/receiverwrapper/views.py
Outdated
| # let normal response handle invalid xml | ||
| pass | ||
| else: | ||
| device_id = form_json.get('meta', {}).get('deviceID') |
There was a problem hiding this comment.
Can this be retrieved directly from the instance with instance.metadata.deviceID? Possibly it should be made it forgiving of missing attributes with getattr()? I'm concerned about the overhead of adding convert_xform_to_json() here.
There was a problem hiding this comment.
@gherceg pointed out in an offline discussion that instance is a byte string here, not a form object as it is in SubmissionPost later on.
It would be nice to pass form_json on from here to anything else that subsequently needs parsed form JSON to avoid having to re-parse in those places.
There was a problem hiding this comment.
Attempted in d3e70de
Reproduced issue this caused with test: device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id') self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id')) which failed since the second rate_limit_device call removed 'existing-device-id', and the next call with 'new-device-id' effectively takes its place in the allowed device list.
This avoids unnecessarily converting the instance xml to json multiple times
dimagimon
added
the
Risk: High
|
I'm not proud of d3e70de, but wanted to make as minimal of a change as I could to reduce risk. |
A histogram was the wrong choice for this metric since we report each individual usage as it happens, not the total number at the end of a window.
Don't code while you're sick
Product Description
A 406 http response is returned when this limit is reached, which allows us to send a custom, non-translatable message. However given the mobile worker will be connected to the internet, I'm optimistic they will be able to translate it or send it to someone who can make better use of the error message if needed. While a 429 response would have fit here as well and is translatable, the message is too vague in my opinion. This way we can ensure we are communicating to the user that it is usage for this specific user that is too high.
Based on android code, I'm pretty sure this will be displayed similarly on mobile, but will confirm with a physical device. Update: Confirmed this looks good on mobile device.
Technical Summary
https://dimagi.atlassian.net/browse/SAAS-16355
Initial PR: #35515
Note for reviewers
By default, the device rate limiter is not enabled, but the code is in place to have the ability to enable this via
update-config. Review this code as if it were being enabled immediately, but note that there will be time after this is deployed to collect metrics in datadog to determine how the current limits fit into current usage.Summary
The changes in this PR track usage for each user action that leads to updating user metadata which are:
If the combined usage on these endpoints reaches 10 unique device IDs within a fixed minute time window, any other requests for that user within that minute from a new device ID are rejected. The exceptions are if a device ID is None or blank, or if the device ID is from Web Apps.
Understanding past usage
The easiest way to get a rough sense of current usage at a larger scale is to look at the
SyncLogSQLtable and see how many different devices are used by a single user in different windows of time. This fails to capture all activity like form submissions, but gives a good sense of restore activity for a specific user. I've periodically looked at this over the last month for different days/weeks for day windows and have seen that these numbers are relatively consistent, with the exception of December 6th.The smaller the time window, the less useful this data is, but gives a sense of the difference. The datadog metrics included in this PR will give a much better sense of usage at the minute window level.
1 day window
December 6th: [2781, 349, 346, 328, 54, 46, ...] # the top 4 device counts are from the problematic domain
January 15th: [420, 224, 42, 41, 40, ...]
Random 1 hour window
December 6th: [776, 52, 36, 24, 5, ...]
January 17th: [17, 7, 5, 4, 3, ...]
Random 1 minute window
December 6th: [34, 3, 1, ...]
January 17th: [2, 1, ...]
Again, I don't think it is worth diving too deep into these numbers as the metric will be far more useful and accurate.
Feature Flag
Safety Assurance
Safety story
Based on Clayton's comment, here are the considerations outlined for this change.
How will users become aware of this change?
Assuming that this limit is currently being reached or exceeded (we won't know until metrics are collected), here is how users will become aware.
The mobile users will attempt an action like a sync or form submission, and receive an error message explaining, in english, that current usage for this user is too high and that they should try again in a minute. Based on numbers pulled for devices used per user in a day (hardly ever exceeds 100 devices), in most cases where this limit is hit, trying again in a minute should succeed. If however there are thousands of devices attempting to take an action at roughly the same time, it will be a painful process to have every device be able to restore or submit successfully. We suspect this scenario is limited to large events like trainings.
How are other legitimate use cases impacted?
Other use cases outside of simultaneous usage of a user across different physical devices that can lead to multiple device ids for one user are:
Uninstalling and reinstalling the CommCare app
It seems nearly impossible to uninstall and reinstall CommCare 9 times in one minute, let alone logging into your user and triggering a restore or submission, so this scenario should not be a concern.
Clearing user data
Clearing user data is easier to do frequently, and could lead to frequent device ID changes, but again, a user would have to do this 9 times in one minute. That seems well past the point of debugging an issue.
Multiple CommCare app installations
Running with multiple installations of CommCare seems to be the most plausible for actually sending off 10 requests from 10 different installations for one user, but just because it is plausible doesn't mean it makes sense. I'm not too familiar with the use cases for different installations, but I imagine it is testing behavior between different mobile app versions or something along those lines, which should be limited to a few different device IDs at most.
Does this impact our offering?
I would say this does not impact our offering, but does make it more painful to use the product in a way we did not intend for (a significant number of devices for one user). This is a temporary rate limit for what we, SaaS, consider to be a reasonable number of devices per user in a minute window.
Automated test coverage
Created tests for the DeviceRateLimiter class (
corehq.apps.users.tests.test_device_rate_limiter.TestDeviceRateLimiter)QA Plan
Rollback instructions
Labels & Review