Repository navigation
fix(insights): group-flag, null-property, and dashboard filter fixes - #65069
Charan-place wants to merge 3 commits into
Conversation
Feature flags that target groups (aggregation_group_type_index set) had their default usage insights (Total Volume, Unique Users) count all $feature_flag_called events, including ones fired before a group was resolved. Those events have no $group_<n> property set and don't belong to any group, which skews rollout-percentage charts. Add a $group_<n> is_set filter to the default insights' queries when the flag is group-targeted. Person-targeted flags are unaffected. Fixes PostHog#39135
…lter
prop_filter_json_extract() used a bare JSONHas() check for the
is_set/is_not_set operators. JSONHas() only checks key presence, so a
property explicitly set to JSON null (e.g. {"email": null}) was
treated as set. This is most visible for static/precalculated cohorts
built on person properties, where a person with email: null was
incorrectly included in an 'email is set' cohort.
Require the extracted value to also be non-empty and not the literal
string 'null' for is_set, and mirror the same check for is_not_set.
Fixes PostHog#29916
|
Thanks for taking the time to contribute to PostHog! We get a lot of pull requests — to help get this PR reviewed faster, we have some checks in place to help you:
Push a change or update the description and we'll automatically re-review the PR. Once all checks are green, the team will be notified. |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
ee/clickhouse/models/test/test_property.py:1854-1866
**Denormalized path left untested**
The new test passes `allow_denormalized_props=False`, so it only exercises the JSON-extract branch (`JSONHas(...) AND {left} != 'null'`). The denormalized/materialized-column branch — which was also changed in `prop_filter_json_extract` — is never reached by this test. If a materialized column stores a JSON-null property as the literal string `'null'` (rather than empty string), the denormalized fix matters; but because no test instantiates a materialized column for the `{"email": null}` event, a regression in that branch would pass silently. The existing `test_prop_filter_json_extract_nullable_materialized_is_set_uses_json` skips non-nullable materialized columns, so the gap remains. Adding a second loop iteration with `allow_denormalized_props=True` (after materializing the `email` column) would give the denormalized branch the same coverage as the non-denormalized branch.
Reviews (1): Last reviewed commit: "feat: add filter to hide insights alread..." | Re-trigger Greptile |
| for property, expected_event_indexes in cases: | ||
| query, params = prop_filter_json_extract(property, 0, allow_denormalized_props=False) | ||
| uuids = sorted( | ||
| [ | ||
| str(uuid) | ||
| for (uuid,) in sync_execute( | ||
| f"SELECT uuid FROM events WHERE team_id = %(team_id)s {query}", | ||
| {"team_id": team.pk, **params}, | ||
| ) | ||
| ] | ||
| ) | ||
| expected = sorted([events[index] for index in expected_event_indexes]) | ||
| assert uuids == expected, f"operator={property.operator}" |
There was a problem hiding this comment.
Denormalized path left untested
The new test passes allow_denormalized_props=False, so it only exercises the JSON-extract branch (JSONHas(...) AND {left} != 'null'). The denormalized/materialized-column branch — which was also changed in prop_filter_json_extract — is never reached by this test. If a materialized column stores a JSON-null property as the literal string 'null' (rather than empty string), the denormalized fix matters; but because no test instantiates a materialized column for the {"email": null} event, a regression in that branch would pass silently. The existing test_prop_filter_json_extract_nullable_materialized_is_set_uses_json skips non-nullable materialized columns, so the gap remains. Adding a second loop iteration with allow_denormalized_props=True (after materializing the email column) would give the denormalized branch the same coverage as the non-denormalized branch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ee/clickhouse/models/test/test_property.py
Line: 1854-1866
Comment:
**Denormalized path left untested**
The new test passes `allow_denormalized_props=False`, so it only exercises the JSON-extract branch (`JSONHas(...) AND {left} != 'null'`). The denormalized/materialized-column branch — which was also changed in `prop_filter_json_extract` — is never reached by this test. If a materialized column stores a JSON-null property as the literal string `'null'` (rather than empty string), the denormalized fix matters; but because no test instantiates a materialized column for the `{"email": null}` event, a regression in that branch would pass silently. The existing `test_prop_filter_json_extract_nullable_materialized_is_set_uses_json` skips non-nullable materialized columns, so the gap remains. Adding a second loop iteration with `allow_denormalized_props=True` (after materializing the `email` column) would give the denormalized branch the same coverage as the non-denormalized branch.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks for taking the time to contribute to PostHog! We get a lot of pull requests — to help get this PR reviewed faster, we have some checks in place to help you:
Push a change or update the description and we'll automatically re-review the PR. Once all checks are green, the team will be notified. |
Add a not_on_any_dashboard query param to the insights list API, plus a matching quick-filter toggle on the Saved Insights page, so people can find insights that haven't been added to a dashboard yet instead of scrolling through ones that already are. Fixes PostHog#26621
23a4c34 to
661beac
Compare
|
Thanks for taking the time to contribute to PostHog! We get a lot of pull requests — to help get this PR reviewed faster, we have some checks in place to help you:
Push a change or update the description and we'll automatically re-review the PR. Once all checks are green, the team will be notified. |
|
Thanks for taking the time to contribute to PostHog! ✅ All automated hygiene checks pass:
|
|
Thanks for the contributions. But its our preference to have a single PR per issue. That way each PR can be reviewed and deployed independently. |
|
Yeah Ok Sure sir |
Problem
Three small but distinct bugs in PostHog's insights/cohorts flow:
$feature_flag_calledevents even when no group was resolved, skewing rollout percentages.
is_set/is_not_setfilter treats an explicit JSONnullvalue as "set," so a person with
email: nullshows up in an "email is set" cohort.any dashboard yet.
Closes #39135
Closes #29916
Closes #26621
Changes
$group_<n>is_set filter to default flag insights when the flag is group-targeted.'null'for is_set/is_not_set in
prop_filter_json_extract.not_on_any_dashboardAPI filter and a matching toggle on the Saved Insights page.How did you test this code?
I'm an agent (Claude) and didn't have a Django/ClickHouse/Postgres environment available, so I couldn't run the suite myself. I added unit tests alongside each fix:
products/feature_flags/backend/api/test/test_feature_flag.py::test_create_feature_flag_usage_dashboard_for_group_targeted_flagee/clickhouse/models/test/test_property.py::test_prop_filter_json_extract_is_set_excludes_explicit_json_nullproducts/product_analytics/backend/api/test/test_insight.py::test_not_on_any_dashboard_filter@Charan-place ran these locally before submitting:
<paste your pytest output here>🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude (Cowork session). I located the three issues, implemented each fix, and wrote the tests above; @Charan-place reviewed the changes, ran the tests, and is pushing/owning this PR. No skills/slash-commands were invoked — this was direct code editing.