fix: batch-load waffle flags to avoid per-flag Redis N+1 - #14
Conversation
Evaluate FRONTEND_* flags from a single prefetched queryset instead of calling flag_is_active (Flag.get) once per flag. Bind prefetched users/groups membership so is_active skips Redis M2M lookups. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: loadsmart/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough
ChangesFlag evaluation optimization
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend_settings/utils.py (1)
56-74: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a regression test for the waffle hook contract This depends on waffle’s private
_get_user_ids/_get_group_idsmethods; a small test will catch an upgrade that changes that contract and silently drops back to the slower path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend_settings/utils.py` around lines 56 - 74, Add a regression test covering _bind_prefetched_membership that verifies the bound _get_user_ids and _get_group_ids hooks return the prefetched user and group ID sets without invoking the original lookup methods. Use a flag with prefetched memberships and ensure the test fails if waffle removes or changes either private method contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend_settings/utils.py`:
- Around line 56-74: Add a regression test covering _bind_prefetched_membership
that verifies the bound _get_user_ids and _get_group_ids hooks return the
prefetched user and group ID sets without invoking the original lookup methods.
Use a flag with prefetched memberships and ensure the test fails if waffle
removes or changes either private method contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: loadsmart/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e77fb71-a0c5-4b74-9405-13c21a62c410
📒 Files selected for processing (2)
frontend_settings/utils.pytests/test_views.py
Rewrite _select_queries_matching without a line-broken binary operator so CI lint (flake8 --ignore=E501) passes. Co-authored-by: Cursor <cursoragent@cursor.com>
Wrap the long function signature so CI black --check passes. Co-authored-by: Cursor <cursoragent@cursor.com>
Motivation and context for the change
Alice
GET /api/frontend-settings/was still slow in Datadog APM (~250ms avg, ~550ms p95) with dozens of repeated Redis GETs onwaffle:2.7.*keys per request.Why PR #11 did not fix this: that change only removed the SQL N+1 on
request.user.groups(oneauth_user_groupsquery per flag). It left the evaluation loop as “list flag names, then callwaffle.flag_is_active(request, name)once per name.” Each call doesFlag.get(name)→ one Redis GET (and on miss, a DB get). For user/group-targeted flags, waffle’s_get_user_ids/_get_group_idscan add more per-flag Redis GETs. So after #11, Postgres group lookups were bounded, but per-flag Redis round-trips remained — that is what Datadog still showed.Why this PR fixes it: we stop resolving flags by name. We load matching flag rows in one queryset with
prefetch_related("users", "groups"), then callflag.is_active(request)on those instances. That removes N×Flag.get(). We also bind_get_user_ids/_get_group_idsto the prefetched id sets so membership checks stay in memory and skip waffle’s per-flag Redis M2M caches. PR #11’s request-local user-groups cache is kept.Review path:
frontend_settings.views.settings→get_flags→_request_with_cached_user_groups→ batch queryset + prefetch →_evaluate_flags/_bind_prefetched_membership→flag.is_active.A clear description of the change
get_flagsto batch-loadFRONTEND_*(configurable prefix) flags withprefetch_related("users", "groups")instead ofvalues_list("name")+flag_is_activeper name.is_active, so waffle evaluation does not hit Redis/DB per flag for M2M._request_with_cached_user_groups.waffle_flagSELECT, bounded M2M prefetches, and that total query count does not grow linearly with flag count.Testing
Testing instructions
Rollback
Rollback instructions
Revert this PR / release and pin consumers (e.g. Alice) back to the previous
django-frontend-settingsversion.Summary by CodeRabbit