Repository navigation
fix(flags): size flags by recently active persons, not all-time count - #81547
Closed
posthog[bot] wants to merge 4 commits into
Closed
posthog[bot] wants to merge 4 commits into
posthog[bot] wants to merge 4 commits into
Conversation
…me count
The release-conditions sizing panel showed `team.persons_seen_so_far`, an
all-time, unbounded person count, as the reachable user total. Projects with
high anonymous person churn (a fresh distinct id per page load) accumulate a
long tail of one-shot persons, so the number ran several times larger than the
real audience.
Replace the denominator with a flags-specific count of persons active in the
last 30 days (`recently_active_persons_count`), computed via a HogQL query over
recent events. The old tooltip claim ("identified users only") is replaced with
copy that states the number is estimated from recently active persons.
`Team.persons_seen_so_far` is left unchanged so the workflows batch-audience
caller keeps its current behavior.
Generated-By: PostHog Desktop
Task-Id: 3df3ebe2-7f2d-41e5-a57f-d6b84ede009a
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
github-actions
Bot
added
feature/feature-flags
Contributor
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 67.72 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 285.3 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 238.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 231.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.5 KiB | src/lib/api.ts |
| 94.6 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.20 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.20 MiB · 17 files | no change | ████░░░░░░ 38.4% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 725.4 KiB | dist/toolbar/toolbar-app-7UNN7IH6.css |
| 552.7 KiB | dist/toolbar/chunk-chunk-M7ULUETD.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-R7NG4MFN.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-LTEMLIGF.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.2 KiB | dist/toolbar/toolbar-app-573LGHQJ.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-CR4TFIDQ.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-PRHPNAF7.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🟢 -890 B (-0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1420.95 MiB · 🟢 -890 B (-0.0%)
posthog
Bot
added
the
run-ci-frontend
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
Contributor
Author
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 2 should fix, 1 consider. Published 3 findings (view the review). Resolved comments: 2 fixed, 1 left for you |
Contributor
Author
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Comment on lines
+151
to
+173
| def recently_active_persons_count(team: Team) -> int: | ||
| """Count distinct persons active in the last RECENTLY_ACTIVE_DAYS days. | ||
|
|
||
| This is the flag-sizing denominator. It replaces an all-time person count so anonymous churn | ||
| does not inflate the audience shown to a flag author (see RECENTLY_ACTIVE_DAYS). `uniq` is an | ||
| estimate, which matches the "~" the sizing panel already renders. | ||
| """ | ||
| from posthog.hogql import ast | ||
| from posthog.hogql.parser import parse_select | ||
| from posthog.hogql.query import execute_hogql_query | ||
|
|
||
| cutoff = timezone.now() - timedelta(days=RECENTLY_ACTIVE_DAYS) | ||
| query = parse_select( | ||
| "SELECT uniq(person_id) FROM events WHERE timestamp >= {cutoff}", | ||
| placeholders={"cutoff": ast.Constant(value=cutoff)}, | ||
| ) | ||
|
|
||
| tag_queries(product=Product.FEATURE_FLAGS, feature=Feature.QUERY) | ||
| response = execute_hogql_query(query=query, team=team) | ||
|
|
||
| return response.results[0][0] if response.results else 0 | ||
|
|
||
|
|
Contributor
Author
There was a problem hiding this comment.
Cache the repeated 30-day distinct-person scan
Why we think it's a valid issue
- Checked: the new helper and its two call sites in the diff, the API action that serves each request, the frontend triggers that fan out those requests, what the replaced query cost on
master, whether any layer dedupes or caches the result, and whether the repo already has a short-lived cache helper. - Found: the helper carries no caching.
recently_active_persons_countbuilds and runsSELECT uniq(person_id) FROM events WHERE timestamp >= {cutoff}on every call (products/feature_flags/backend/user_blast_radius.py:151-171), and it is called on both blast-radius paths (:182,:196). The endpoint runs oneget_user_blast_radiusper POST (products/feature_flags/backend/api/feature_flag.py:4140-4149), so each request re-runs the team-wide scan. - Found: the request fan-out is one per condition set.
calculateBlastRadiusloops overvalues.filters.groupsand dispatchescalculateBlastRadiusForConditionfor each (frontend/src/scenes/feature-flags/featureFlagReleaseConditionsLogic.ts:828-836), and it fires on mount (:1206-1210), on filter load (:692-695), and on aggregation change (:781). Editing a condition adds another call after a 1 second breakpoint (:741-746). Opening a flag with several condition sets therefore issues that many concurrent identical team-wide queries. - Found: nothing absorbs the repeats.
execute_hogql_queryis a thin wrapper overHogQLQueryExecutor(...).execute()(posthog/hogql/query.py:794-795) with no result cache, and the helper is a plain module function, so there is no per-team or per-process memoization. - Found: the diff makes the per-request query much heavier than what it replaced.
masterread the person table once per request viateam.persons_seen_so_far(posthog/models/team/team.py:872-888), whose@cached_propertyonly spans a single request. The replacement reads 30 days of the team's events plus aperson_distinct_id_overridessubquery join — the snapshots regenerated in this PR show that exact shape withmax_execution_time=60. For a high-volume project, 30 days of events is orders of magnitude more rows than the person table, so the fan-out that already existed now multiplies a far larger query. - Found: the new query runs on the default workload. The neighboring group path deliberately sends its heavy count to
Workload.OFFLINEbecause "groups queries can be massive" (products/feature_flags/backend/user_blast_radius.py:251-257), while the new events scan gets no such treatment and lands on the online cluster that serves interactive traffic. - Impact: confirmed duplicated load on an interactive path: opening the release conditions panel of a flag with N condition sets triggers N identical 30-day
uniq(person_id)scans, concurrently, per viewer, and again after each condition edit. The 60 second execution ceiling in the generated query also means the panel can fail outright for the largest teams, which the frontend renders as the blast-radius error state. - Impact: the remedy is cheap and has in-repo precedent —
cache_forinposthog/cache_utils.py:67is already used for exactly this kind of team-level count (posthog/models/team/util.py:379), so a short TTL keyed by team removes the duplication without changing the filtered per-condition query.
Issue description
Each condition request scans 30 days of events and calculates uniq(person_id). The frontend sends one request for every condition. This repeats the same team-wide query concurrently and can add significant ClickHouse load for active projects.
Suggested fix
Cache the count by team for a short period, such as five minutes. Alternatively, return one shared total through a batch request. Keep the filtered count separate because it changes by condition.
Prompt to fix with AI (copy-paste)
## Context
@products/feature_flags/backend/user_blast_radius.py#L151-173
@products/feature_flags/backend/user_blast_radius.py#L196
<issue_description>
Each condition request scans 30 days of events and calculates `uniq(person_id)`. The frontend sends one request for every condition. This repeats the same team-wide query concurrently and can add significant ClickHouse load for active projects.
</issue_description>
<issue_validation>
- **Checked:** the new helper and its two call sites in the diff, the API action that serves each request, the frontend triggers that fan out those requests, what the replaced query cost on `master`, whether any layer dedupes or caches the result, and whether the repo already has a short-lived cache helper.
- **Found:** the helper carries no caching. `recently_active_persons_count` builds and runs `SELECT uniq(person_id) FROM events WHERE timestamp >= {cutoff}` on every call (`products/feature_flags/backend/user_blast_radius.py:151-171`), and it is called on both blast-radius paths (`:182`, `:196`). The endpoint runs one `get_user_blast_radius` per POST (`products/feature_flags/backend/api/feature_flag.py:4140-4149`), so each request re-runs the team-wide scan.
- **Found:** the request fan-out is one per condition set. `calculateBlastRadius` loops over `values.filters.groups` and dispatches `calculateBlastRadiusForCondition` for each (`frontend/src/scenes/feature-flags/featureFlagReleaseConditionsLogic.ts:828-836`), and it fires on mount (`:1206-1210`), on filter load (`:692-695`), and on aggregation change (`:781`). Editing a condition adds another call after a 1 second breakpoint (`:741-746`). Opening a flag with several condition sets therefore issues that many concurrent identical team-wide queries.
- **Found:** nothing absorbs the repeats. `execute_hogql_query` is a thin wrapper over `HogQLQueryExecutor(...).execute()` (`posthog/hogql/query.py:794-795`) with no result cache, and the helper is a plain module function, so there is no per-team or per-process memoization.
- **Found:** the diff makes the per-request query much heavier than what it replaced. `master` read the person table once per request via `team.persons_seen_so_far` (`posthog/models/team/team.py:872-888`), whose `@cached_property` only spans a single request. The replacement reads 30 days of the team's events plus a `person_distinct_id_overrides` subquery join — the snapshots regenerated in this PR show that exact shape with `max_execution_time=60`. For a high-volume project, 30 days of events is orders of magnitude more rows than the person table, so the fan-out that already existed now multiplies a far larger query.
- **Found:** the new query runs on the default workload. The neighboring group path deliberately sends its heavy count to `Workload.OFFLINE` because "groups queries can be massive" (`products/feature_flags/backend/user_blast_radius.py:251-257`), while the new events scan gets no such treatment and lands on the online cluster that serves interactive traffic.
- **Impact:** confirmed duplicated load on an interactive path: opening the release conditions panel of a flag with N condition sets triggers N identical 30-day `uniq(person_id)` scans, concurrently, per viewer, and again after each condition edit. The 60 second execution ceiling in the generated query also means the panel can fail outright for the largest teams, which the frontend renders as the blast-radius error state.
- **Impact:** the remedy is cheap and has in-repo precedent — `cache_for` in `posthog/cache_utils.py:67` is already used for exactly this kind of team-level count (`posthog/models/team/util.py:379`), so a short TTL keyed by team removes the duplication without changing the filtered per-condition query.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Cache the count by team for a short period, such as five minutes. Alternatively, return one shared total through a batch request. Keep the filtered count separate because it changes by condition.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
This is a real concern, not noise: the release-conditions panel does fan out one request per condition set, and each request runs the same team-wide 30-day uniq(person_id) denominator scan, so opening a flag with several condition sets issues that many concurrent identical scans. I'm escalating rather than applying the cache unattended, because the safe implementation is a design decision with a correctness interaction:
- The suggested helper
cache_for(posthog/cache_utils.py) keys its cache on the function arguments and is a deliberate no-op under TEST. Decorating the count function as-is would key on the per-requestcutofftimestamp (now minus 30 days, recomputed each request), so it would rarely dedupe across the fan-out. To actually dedupe you must key by team only and let the helper compute its own window — which decouples the denominator's 30-day window from the numerator's request-computed cutoff. That same-instant pairing was the point of the earlier fix on this PR (the matched count and total describing one population), so loosening it by up to the cache TTL is a deliberate tradeoff a human should sign off on. It's fine for a~estimate, but it shouldn't be decided automatically. - Because
cache_fordisables itself under TEST, the dedup can't be demonstrated by the test suite — the benefit only shows up in real multi-request, multi-process traffic, and the cache is per-process in-memory so dedup across web workers is partial. A TTL also has to be chosen (the existing precedent uses 1 minute). - The separate suggestion to run the scan on
Workload.OFFLINEis its own latency-vs-load call: that cluster is used for the massive groups count precisely because it's not interactive, so moving this interactive panel query there could make the panel slower. That also warrants a human decision.
What a maintainer needs to decide: (a) whether to add a per-team TTL cache to the denominator, accepting the window-drift from the numerator, and at what TTL; and (b) whether to route the events scan to the offline cluster. Happy to implement whichever direction is chosen.
The release-conditions sizing panel took its total from a 30-day recently
active person count, but the matched ("affected") count still counted all-time
persons matching the condition. The two numbers described different
populations, so on projects with high anonymous churn inactive matches could
inflate the matched count up to the full active total (100% blast radius).
Count matched persons from the same recently-active population: root the
numerator in events since the same cutoff and intersect with persons matching
the filter, mirroring the denominator. Both are now uniq() estimates over the
same window, so the matched count is a strict subset of the total; the min()
clamp is kept for estimator noise.
The shared _build_person_query (used by the workflows batch-audience persons
list) is left untouched, so that caller keeps targeting all matching persons.
Adds a regression test where a person matching the filter but with no recent
event is excluded from the count, and regenerates the affected query snapshots.
Generated-By: PostHog Desktop
Task-Id: e4320472-35c7-481b-9cff-90d43f3cfb67
Comment on lines
+195
to
+228
| if cutoff is None: | ||
| cutoff = _recently_active_cutoff() | ||
|
|
||
| matched_persons = ast.SelectQuery( | ||
| select=[ast.Field(chain=["persons", "id"])], | ||
| select_from=ast.JoinExpr(table=ast.Field(chain=["persons"])), | ||
| where=ast.And( | ||
| exprs=[ | ||
| ast.CompareOperation( | ||
| op=ast.CompareOperationOp.Eq, | ||
| left=ast.Field(chain=["persons", "team_id"]), | ||
| right=ast.Constant(value=team.pk), | ||
| ), | ||
| property_to_expr(filter.property_groups, team, scope="person"), | ||
| ] | ||
| ), | ||
| ) | ||
| query = ast.SelectQuery( | ||
| select=[ast.Call(name="uniq", args=[ast.Field(chain=["person_id"])])], | ||
| select_from=ast.JoinExpr(table=ast.Field(chain=["events"])), | ||
| where=ast.And( | ||
| exprs=[ | ||
| ast.CompareOperation( | ||
| op=ast.CompareOperationOp.GtEq, | ||
| left=ast.Field(chain=["timestamp"]), | ||
| right=ast.Constant(value=cutoff), | ||
| ), | ||
| ast.CompareOperation( | ||
| op=ast.CompareOperationOp.In, | ||
| left=ast.Field(chain=["person_id"]), | ||
| right=matched_persons, | ||
| ), | ||
| ] | ||
| ), |
Contributor
Author
There was a problem hiding this comment.
Avoid materializing all historical matches
Why we think it's a valid issue
- Checked: the generated ClickHouse SQL the PR itself regenerated in
products/feature_flags/backend/api/test/__snapshots__/test_feature_flag.ambr, the pre-PR query shapes, the query settings applied, the workload each path uses, and how often the endpoint fires from the flag editor. - Found: the snapshot confirms the
INright-hand side carries no time bound.TestBlastRadius.test_user_blast_radius.1showsin(if(not(empty(events__override.distinct_id)), events__override.person_id, events.person_id), (SELECT persons.id ... FROM person WHERE equals(person.team_id, ...) ... GROUP BY person.id HAVING ...)). The subquery aggregates the team's wholepersontable withargMaxover all versions and applies only the property filter. The outergreaterOrEquals(events.timestamp, ...)cannot prune it, because ClickHouse evaluates anINsubquery into a set independently of the outerWHERE. - Found: the new query adds two more unbounded scans around that set build. It reads
eventsfor the team from the cutoff, and itLEFT OUTER JOINs a fullperson_distinct_id_overridesaggregation for the team (GROUP BY person_distinct_id_overrides.distinct_id, no time bound), because HogQL resolvesperson_idoneventsthrough the overrides table. - Found: all of that runs inside one statement under
max_execution_time=60withmax_bytes_before_external_group_by=0, so the personGROUP BYcannot spill to disk. The snapshot lists both settings. - Found: it runs on the default online workload.
recently_active_matched_persons_countcallsexecute_hogql_query(query=query, team=team)with noworkloadargument, while the group path deliberately passesworkload=Workload.OFFLINEwith the comment "groups queries can be massive" (products/feature_flags/backend/user_blast_radius.py:251-256). The codebase already treats this class of query as too heavy for the online cluster. - Found: the endpoint is interactive, not a batch job.
calculateBlastRadiusfans out one POST per condition set (frontend/src/scenes/feature-flags/featureFlagReleaseConditionsLogic.ts:828-836), and each POST issues two ClickHouse queries because_get_person_blast_radiusalways callsrecently_active_persons_countbefore the matched query. Property edits are debounced byawait breakpoint(1000)at line 740, but opening the editor is not. - Found: the person-side scan is pre-existing, so the regression is additive rather than new. The old snapshot for the same test ran that identical
personssubquery as the entire query withcount(DISTINCT persons.id). The PR wraps that same work in a 30-day events scan plus the overrides join, under the same 60s cap. - Found: the suggested alternative is implementable in this codebase. HogQL exposes person properties through the
eventstable, soproperty_to_exprin event scope can push the filter into the events scan instead of a separate person aggregation, which is how insight queries filter events by person property. - Impact: for a team with a large person table and a broad person-property filter, the numerator query must aggregate every historical matching person into a set, join the full overrides table, and scan 30 days of events, all within 60 seconds on the online cluster, once per condition set each time someone opens a flag's release conditions. On timeout the frontend degrades visibly rather than showing wrong data —
setBlastRadiusErrordrives the "Couldn't estimate how many … match." state with a Retry button (frontend/src/scenes/feature-flags/FeatureFlagReleaseConditions.tsx:460-482) — so the confirmed consequence is lost estimates and wasted online-cluster capacity, not corrupt results.
Issue description
The IN subquery selects every historical person that matches the filter. ClickHouse must build this set before it applies the recent event window. Broad filters can materialize millions of person IDs and cause high memory use or query timeouts. Anonymous churn makes this risk larger.
Suggested fix
Restrict the person side to active IDs before ClickHouse materializes the match set. Prefer one query that scans recent events and joins the related person data. Confirm the query plan with a broad filter and a large historical person population.
Prompt to fix with AI (copy-paste)
## Context
@products/feature_flags/backend/user_blast_radius.py#L195-228
<issue_description>
The `IN` subquery selects every historical person that matches the filter. ClickHouse must build this set before it applies the recent event window. Broad filters can materialize millions of person IDs and cause high memory use or query timeouts. Anonymous churn makes this risk larger.
</issue_description>
<issue_validation>
- **Checked:** the generated ClickHouse SQL the PR itself regenerated in `products/feature_flags/backend/api/test/__snapshots__/test_feature_flag.ambr`, the pre-PR query shapes, the query settings applied, the workload each path uses, and how often the endpoint fires from the flag editor.
- **Found:** the snapshot confirms the `IN` right-hand side carries no time bound. `TestBlastRadius.test_user_blast_radius.1` shows `in(if(not(empty(events__override.distinct_id)), events__override.person_id, events.person_id), (SELECT persons.id ... FROM person WHERE equals(person.team_id, ...) ... GROUP BY person.id HAVING ...))`. The subquery aggregates the team's whole `person` table with `argMax` over all versions and applies only the property filter. The outer `greaterOrEquals(events.timestamp, ...)` cannot prune it, because ClickHouse evaluates an `IN` subquery into a set independently of the outer `WHERE`.
- **Found:** the new query adds two more unbounded scans around that set build. It reads `events` for the team from the cutoff, and it `LEFT OUTER JOIN`s a full `person_distinct_id_overrides` aggregation for the team (`GROUP BY person_distinct_id_overrides.distinct_id`, no time bound), because HogQL resolves `person_id` on `events` through the overrides table.
- **Found:** all of that runs inside one statement under `max_execution_time=60` with `max_bytes_before_external_group_by=0`, so the person `GROUP BY` cannot spill to disk. The snapshot lists both settings.
- **Found:** it runs on the default online workload. `recently_active_matched_persons_count` calls `execute_hogql_query(query=query, team=team)` with no `workload` argument, while the group path deliberately passes `workload=Workload.OFFLINE` with the comment "groups queries can be massive" (`products/feature_flags/backend/user_blast_radius.py:251-256`). The codebase already treats this class of query as too heavy for the online cluster.
- **Found:** the endpoint is interactive, not a batch job. `calculateBlastRadius` fans out one POST per condition set (`frontend/src/scenes/feature-flags/featureFlagReleaseConditionsLogic.ts:828-836`), and each POST issues two ClickHouse queries because `_get_person_blast_radius` always calls `recently_active_persons_count` before the matched query. Property edits are debounced by `await breakpoint(1000)` at line 740, but opening the editor is not.
- **Found:** the person-side scan is pre-existing, so the regression is additive rather than new. The old snapshot for the same test ran that identical `persons` subquery as the entire query with `count(DISTINCT persons.id)`. The PR wraps that same work in a 30-day events scan plus the overrides join, under the same 60s cap.
- **Found:** the suggested alternative is implementable in this codebase. HogQL exposes person properties through the `events` table, so `property_to_expr` in event scope can push the filter into the events scan instead of a separate person aggregation, which is how insight queries filter events by person property.
- **Impact:** for a team with a large person table and a broad person-property filter, the numerator query must aggregate every historical matching person into a set, join the full overrides table, and scan 30 days of events, all within 60 seconds on the online cluster, once per condition set each time someone opens a flag's release conditions. On timeout the frontend degrades visibly rather than showing wrong data — `setBlastRadiusError` drives the "Couldn't estimate how many … match." state with a Retry button (`frontend/src/scenes/feature-flags/FeatureFlagReleaseConditions.tsx:460-482`) — so the confirmed consequence is lost estimates and wasted online-cluster capacity, not corrupt results.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Restrict the person side to active IDs before ClickHouse materializes the match set. Prefer one query that scans recent events and joins the related person data. Confirm the query plan with a broad filter and a large historical person population.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Confirmed the concern and leaving it for a human to decide direction rather than changing it blind. The numerator query for a filtered person condition builds the matching-person set as an unbounded IN (SELECT ... FROM persons WHERE <property filter>) with no time bound, so ClickHouse materializes every historically matching person (plus the person-overrides join) independently of the outer 30-day event window, and it runs on the online workload — unlike the group path, which uses the offline workload precisely because that class of query can be large. This is interactive (one request per condition set when the release-conditions editor opens), so on a large team it can time out and degrade to the 'Couldn't estimate…' retry state.
Two directions, both of which need a human call:
- Rewrite the numerator to push the person-property filter into the events scan (event-scope
property_to_expr, the way insight queries filter events by person property) so no all-time person set is built. This changes query semantics and which filter shapes are accepted (behavioral filters, cohorts, and flag dependencies validate/resolve differently in event scope than person scope), so it needs validation against real large-team data — not just the small-fixture tests. - Or route the person-side sizing queries to the offline workload like the group path. That's a cluster-capacity vs interactive-latency tradeoff, and it relocates the heavy query rather than avoiding the materialization.
Neither is a safe unattended change: the payoff and the semantic equivalence are only observable against production-scale ClickHouse, so I'm escalating for a maintainer to pick the approach.
The blast radius endpoint now sizes person-based flags by persons active in the last 30 days, while group-based flags stay all-time. Update the UserBlastRadiusResponseSerializer help_text for `affected` and `total` to describe that split and regenerate the frontend OpenAPI types so the generated client comment matches the response. Generated-By: PostHog Desktop Task-Id: 1798cef1-90e5-4b25-9f2a-6bcec9130a38
The person-specific sizing tooltip (which now says the count is estimated from persons active in the last 30 days) was gated on the flag-level aggregation index. When a flag targets users but one condition is switched to a group type, the shown count is an all-time group count, yet the tooltip still appeared and described persons. Gate it on the resolved condition-level index instead, matching the sibling collapsible component. Generated-By: PostHog Desktop Task-Id: 1798cef1-90e5-4b25-9f2a-6bcec9130a38
haacked
changed the title
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
1 task
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
Contributor
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Team.persons_seen_so_far, a barecount(1)over the full person history with no date or activity bound.persistence: "memory") build a long tail of one-shot persons that never return, inflating that count several times over the real audience.Changes
recently_active_persons_count(team)counts distinctperson_idfrom recent events, so old one-shot anonymous persons drop out of the denominator.min(matched, total)clamp to that active count.Team.persons_seen_so_farunchanged, so the workflows batch-audience caller keeps its behavior.freeze_timein tests and does not depend on ClickHouse server time.Note on scope: the person-deletion staleness and the
persistence: "memory"behavior itself are correct by design and live inPostHog/posthog-js; those are a docs and support answer, not part of this change.How did you test this code?
TestBlastRadiusso its fixture persons emit a recent event (a person with no recent activity is not reachable audience, which is the point of the fix), via a small_create_active_personwrapper.TestBlastRadius(48 tests) andproducts/feature_flags/backend/test/test_user_blast_radius.py(4 tests) locally; all passed. CI reports the authoritative counts.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Fully autonomous
Team.persons_seen_so_far(posthog/models/team/team.py), an all-time person count, surfaced throughget_user_blast_radiusinproducts/feature_flags/backend/user_blast_radius.py.persons_seen_so_farso the unrelated workflows caller is unaffected.is_identifiedorcreated_atfilter: only recent activity excludes the accumulated anonymous tail without also dropping real, long-standing users./writing-user-facing-copy,/simplify,/writing-pr-descriptions.Created with PostHog Desktop from this inbox report.