perf(cohorts): cache behavioral cohort exclusion for flag list#65602
perf(cohorts): cache behavioral cohort exclusion for flag list#65602gustavohstrassburger wants to merge 8 commits into
Conversation
Generated-By: PostHog Code Task-Id: b094aee2-93a9-4790-bb1a-c5c4670f2d49
There was a problem hiding this comment.
Pull request overview
This PR improves performance of the cohorts list endpoint when hide_behavioral_cohorts=true (used by the feature flag cohort typeahead) by avoiding per-request full-team dependency graph rebuilds and reusing a cached per-team set of flag-incompatible (behavioral) cohorts.
Changes:
- Added a cached helper in
products/cohorts/backend/models/dependencies.pyto compute and cache flag-excluded behavioral cohort IDs per team (with SQL prefiltering to trim the graph input). - Updated the cohorts list queryset logic to call the cached helper instead of rebuilding the dependency graph in the viewset.
- Updated and added tests to cover the extracted graph logic and the new cache/invalidation behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| products/workflows/backend/api/hog_flow.py | Updates an internal comment reference to the new dependency-graph location. |
| products/cohorts/backend/models/test/test_dependencies.py | Adds a new test suite for cached flag-excluded behavioral cohort IDs and invalidation behavior. |
| products/cohorts/backend/models/dependencies.py | Introduces cached computation + invalidation hooks for behavioral cohort exclusion, and hosts the extracted graph-walk helpers. |
| posthog/api/test/test_cohort.py | Updates graph-walk tests to use find_behavioral_cohorts from the new module (removes viewset dependency). |
| posthog/api/cohort.py | Switches list filtering for hide_behavioral_cohorts to the cached helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Reviews (1): Last reviewed commit: "perf(cohorts): cache behavioral cohort e..." | Re-trigger Greptile |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from |
|
Reviews (2): Last reviewed commit: "test(cohorts): improve behavioral cohort..." | Re-trigger Greptile |
haacked
left a comment
There was a problem hiding this comment.
Clean perf win. No blocking issues; a couple of test-coverage suggestions inline.
| set(cache.get(_behavioral_cohort_ids_key(self.team.id, allow_realtime_backfilled=False))), excluded | ||
| ) | ||
|
|
||
| def test_cache_invalidated_when_cohort_changes(self) -> None: |
There was a problem hiding this comment.
suggestion: Only the save path has a test for behavioral-cache invalidation. cohort_deleted and the Team post_delete handler both call _invalidate_team_behavioral_cohort_cache (dependencies.py:544), but nothing deletes a cohort and re-checks, so a regression there would leave a behavioral cohort hidden from the flag picker after its dependency is deleted, with no test to catch it.
Mirror test_cache_invalidated_when_cohort_changes with a behavioral.delete(): prime both cache keys, delete, then assert both are None.
There was a problem hiding this comment.
Done in a7a1a27 — added test_cache_invalidated_when_cohort_deleted: primes both cache-key variants, hard-deletes the behavioral cohort, asserts both are None.
| with mock.patch("django.db.transaction.on_commit", side_effect=lambda func: func()): | ||
| yield | ||
|
|
||
| def test_excludes_behavioral_and_transitive_referrers_but_not_leaves(self) -> None: |
There was a problem hiding this comment.
suggestion: Every test here uses the flat BEHAVIORAL_FILTERS (a top-level OR with one value), so the recursion into nested AND/OR groups in check_property_values never runs. A behavioral node buried a group deeper still has to be detected: the prefilter's icontains catches it, then the walk has to recurse into the group to mark it.
Add a case mirroring this test with the behavioral value nested inside an inner AND/OR group. (A two-hop referrer chain, A → referrer → behavioral, would also exercise the reverse-walk traversal instead of a single edge lookup.)
There was a problem hiding this comment.
Done in a7a1a27 — added test_detects_behavioral_nested_in_group_and_two_hop_referrers: behavioral node buried in an inner AND group (exercises the check_property_values recursion) plus a two-hop chain hop2 → hop1 → behavioral (exercises the reverse-walk traversal).
…multi-hop graph Generated-By: PostHog Code Task-Id: b094aee2-93a9-4790-bb1a-c5c4670f2d49
Query snapshots: Backend query snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
Query snapshots: Backend query snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
Problem
The cohorts list endpoint (
GET /api/projects/:id/cohorts/?hide_behavioral_cohorts=true) could take several seconds server-side. The feature flag UI's cohort typeahead hits this endpoint on every keystroke, so the flag cohort picker felt broken — the list never populated fast enough to select a cohort.The cost wasn't search or indexing. On every request the endpoint rebuilt the behavioral-cohort dependency graph: it loaded every non-deleted cohort for the team (with its
filtersJSONB) into memory, parsed all that JSON, and walked the graph — ignoring pagination and thesearch/typefilters. Teams with many or large cohorts paid the full-team cost on each keystroke.Changes
filtersreference abehavioralnode or anothercohort(via a SQL prefilter). Leaf cohorts with plain person-property filters are skipped. The bare-word match has no false negatives (those node types always serialize the literal substrings); a false positive just loads one extra leaf the walk ignores.allow_realtime_backfilled. Typeahead keystrokes reuse it instead of rebuilding the graph. The cache is invalidated on any cohort write/delete through the existing signal hooks independencies.py. Flag-save validation remains the real safety net for the brief staleness window.products/cohorts/backend/models/dependencies.py, next to the existing cohort-dependency cache and invalidation infrastructure. The viewset now just calls the cached helper.How did you test this code?
I'm an agent (PostHog Code), so no manual testing. Automated tests run locally:
TestFlagExcludedBehavioralCohortIdssuite intest_dependencies.py— trim correctness, static-cohort exemption, caching,Nonenormalization ofallow_realtime_backfilled, and cache invalidation on cohort change.find_behavioral_cohortsgraph test continue to pass (23 relevant tests green).ruffandty checkclean (via lint-staged on commit).👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Diagnosed from a DevTools waterfall showing ~9s TTFB on the cohorts list endpoint with
hide_behavioral_cohorts=true. Traced the cost to the per-request full-team graph build (not search/indexing or the local feature-flag eval). Considered three fixes — caching only, trimming the load only, or both — and went with both: the trim shrinks each computation, the cache reuses it across the typeahead burst.Chose a 1h TTL plus signal-based invalidation (reusing the existing
dependencies.pycohort-cache hooks) rather than a longer TTL, sincecohort_typeclears during recalculation go through queryset.update()and don't fire signals; the flag-save validation backstops any stale window. No skills were required for this change.@greptile
Created with PostHog Code