Prime Restores - Periodic Task #35875
Conversation
The theory is that there are smartlink exclusive workflows resulting in users never launching the app from the homescreen and getting a full restore. This results in users with a cold cache.
Jtang-1
requested review from
a team,
MartinRiese,
minhaminha,
Robert-Costello and
esoergel
and removed request for
a team and
Robert-Costello
|
@coderabbitai review |
WalkthroughThe changes update the source of several functions used for priming and clearing Formplayer databases by moving their definitions from the old module ( Sequence Diagram(s)sequenceDiagram
participant Scheduler as Celery Beat
participant Task as bha_prime_formplayer_dbs Task
participant Toggle as PRIME_FORMPLAYER_DBS_BHA
participant UserFetcher as get_users_for_priming
participant Worker as prime_formplayer_db_for_user
Scheduler->>Task: Trigger daily execution
Task->>Toggle: Retrieve enabled domains
Toggle-->>Task: Return list of domains
Task->>Task: Calculate sync date window
Task->>UserFetcher: Get eligible users for each domain
UserFetcher-->>Task: Return list of user IDs
Task->>Worker: Asynchronously invoke prime_formplayer_db_for_user for each user
Worker-->>Task: Process synchronization and clear DB if needed
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
corehq/toggles/__init__.py (1)
2295-2302: Add descriptive docstring or description parameter.While defining a new toggle is straightforward, consider adding a brief
descriptionargument to provide context on how or why this toggle should be used. This helps future developers understand its purpose without having to check external documentation.custom/bha/tasks.py (3)
17-21: Use environment variables for rate-limit.
USH_PRIME_RESTORE_RATE_LIMIT = 100is a reasonable default. Ensure it is documented or easily overridden to facilitate quick adjustments in production if needed.
27-29: Promote consistent naming of tasks.
bha_prime_formplayer_dbsis descriptive, but consider a docstring clarifying its purpose, for instance: "This daily task primes the Formplayer DB for BHA domains enabled under the toggle."
30-35: Validate user set retrieval and task queue load.Looping over domains, then users, can generate many subtasks. Ensure you monitor the queue load to prevent backlogs. Also confirm that large volumes of async tasks won’t exceed Celery concurrency limits or overshadow other tasks.
custom/covid/tasks.py (1)
12-12: Remove unused imports if possible.After deleting the local implementation of the priming functions, confirm all references to
FormplayerAPIException,clear_user_data, or others remain necessary and used in this module. Cleaning imports reduces confusion for future maintainers.custom/formplayer/restore_priming.py (3)
31-52: Exception handling with retries
The approach of retrying on generic exceptions and explicitly handlingFormplayerAPIExceptionis solid. Consider logging the actual exception message and more context (e.g., user/domain) to help diagnose transient errors.
74-82: Guard against missing CouchUser
CouchUser.get_by_user_id()can returnNoneif the user is missing or deleted, but the code tries to useusername. Consider adding a check to avoid aNoneTypeerror if the user is not found.
84-112: Large dataset query performance
Queries onSyncLogSQLwith.filter()and.distinct()can become expensive if the table grows large. Ensure proper indexing on fields likedomainanddateto maintain performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
corehq/apps/formplayer_api/management/commands/clear_formplayer_dbs.py(1 hunks)corehq/apps/formplayer_api/management/commands/prime_formplayer_restores.py(1 hunks)corehq/toggles/__init__.py(1 hunks)custom/bha/tasks.py(1 hunks)custom/covid/tasks.py(1 hunks)custom/covid/tests/test_prime_restore.py(2 hunks)custom/formplayer/restore_priming.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- corehq/apps/formplayer_api/management/commands/clear_formplayer_dbs.py
- custom/covid/tests/test_prime_restore.py
- corehq/apps/formplayer_api/management/commands/prime_formplayer_restores.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: - Review the code following best practises and st...
**/*.py: - Review the code following best practises and standards
custom/bha/tasks.pycorehq/toggles/__init__.pycustom/formplayer/restore_priming.pycustom/covid/tasks.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (python-sharded-and-javascript)
- GitHub Check: tests (python, bf)
- GitHub Check: tests (python, 6a)
- GitHub Check: tests (python, 05)
- GitHub Check: test
🔇 Additional comments (11)
custom/bha/tasks.py (4)
1-6: Validate date/time imports and time zone handling.It's good to see usage of
datetimeandrelativedelta. However, ensure that UTC is the desired timezone throughout, and consider using timezone-aware datetime objects (e.g.,datetime.now(tz=utc)or Django’stimezone.now()) to avoid potential confusion if future code runs in different time zones.
8-10: Leverage global queue settings consistently.The code designates
ush_background_tasksas the queue. Confirm that other tasks that prime or manage Formplayer also use consistent queue naming, or specify a rationale for splitting tasks across multiple queues.
11-14: Confirm cross-file references.The imports from
custom.formplayer.restore_priminglook correct. Verify thatprime_formplayer_db_for_userandget_users_for_primingreferences remain consistent if the code is reorganized again, to avoid broken dependencies.
22-24: Recheck scheduled run times vs. user workflows.The
TASK_WINDOW_CUTOFF_HOUR = 14(2PM UTC) gives a 2-hour window for completion. Double check with real usage patterns to confirm it fully covers the prime window for BHA daily usage.custom/covid/tasks.py (1)
16-20: Validate cross-module migrations.These lines import the new helper functions from
custom.formplayer.restore_priming. Make sure the removed functionality in this file is fully covered there, including error handling or case load checks, so that no logic remains accidentally lost or partially duplicated.custom/formplayer/restore_priming.py (6)
1-4: All imports are standard and correct
No issues with the usage; everything is consistent with best practices.
5-9: Dependent library imports
Everything is properly referenced and organized.
10-17: Additional imports
No concerns here. Imports align well with the tasks’ requirements.
19-20: Review the default rate limit
Confirm that the defaultUSH_PRIME_RESTORE_RATE_LIMITof 100 is appropriate for your environment. If the number of Celery tasks spikes above this threshold, tasks may be excessively delayed.
22-30: Time-based cutoff logic
datetime.utcnow().hour >= task_cutoff_hourmay cause confusion if your team specifically expects local time. Make sure UTC-based cutoff aligns with your operational requirements.
54-72: Clear function consistency
clear_formplayer_db_for_userparallelsprime_formplayer_db_for_userin design and retry logic. This consistency simplifies maintenance.
Product Description
No user facing effects on the UI side. But the intended effect is to improve performance upon form submission for BHA users.
Technical Summary
USH-5780
The cc-skip-fixtures-after-submit custom app property was removed from the BHA application, and the corresponding code was deleted. Following this change, Datadog performance monitoring indicated an increase in slow submissions for BHA forms.
Our current hypothesis is that this slowdown is due to additional fixture data being included in submission incremental syncs, particularly during the first form submission of the day. Typically, users with a cold cache receive a full restore when launching the app. However, past issues have shown that some users access CommCare exclusively through bookmarked links or smartlinks, bypassing the expected full restore process.
This PR will ensure that users in domains with the feature flag enabled get their restores primed, as long as they’ve used Web Apps in the past week (specifically, if they’ve synced in the last seven days). These tasks will run daily from 5 AM to 7 AM GMT-7.
Feature Flag
Adds FF PRIME_FORMPLAYER_DBS_BHA
Safety Assurance
Safety story
Changes are restricted to custom async tasks and management command so should not impact any user facing features. It also behind a feature flag. Tested locally.
Automated test coverage
Tests exist that the correct users are fetched for priming.
QA Plan
no QA
Rollback instructions
Labels & Review