Skip to content

fix: include group context in $feature_flag_called dedupe key #625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gustavohstrassburger wants to merge 2 commits into
base: main
Choose a base branch
from
+118 −7
Open

fix: include group context in $feature_flag_called dedupe key #625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7759e53
fix: include group context in $feature_flag_called dedupe key
gustavohstrassburger May 25, 2026
d422f99
chore: add changeset
gustavohstrassburger May 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .sampo/changesets/include-group-context-in-flag-called-dedupe.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
pypi/posthog: patch
---

Include group context in the `$feature_flag_called` dedupe key so group-scoped flags fire a separate event for each group a user is evaluated under, instead of being dedup-ed against the first group context the same `(distinct_id, flag, response)` was seen under.
19 changes: 12 additions & 7 deletions posthog/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2214,14 +2214,19 @@ def _capture_feature_flag_called_if_needed(
groups: Optional[Dict[str, str]] = None,
disable_geoip: Optional[bool] = None,
) -> None:
"""Fire a ``$feature_flag_called`` event if the (distinct_id, flag, response)
triple hasn't already been reported on this client. Shared by the single-flag
evaluation path and ``FeatureFlagEvaluations.is_enabled() / get_flag()`` so
both paths dedupe identically.
"""Fire a ``$feature_flag_called`` event if the (distinct_id, flag, response,
groups) tuple hasn't already been reported on this client. Group context is
included so that group-scoped flags fire a separate event for each group a
user is evaluated under. Shared by the single-flag evaluation path and
``FeatureFlagEvaluations.is_enabled() / get_flag()`` so both paths dedupe
identically.
"""
feature_flag_reported_key = (
f"{key}_{'::null::' if response is None else str(response)}"
)
response_repr = "::null::" if response is None else str(response)
if groups:
groups_repr = json.dumps(sorted(groups.items()), separators=(",", ":"))
Copy link
Copy Markdown
Contributor

@dustinbyrne dustinbyrne May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this relies on groups being JSON serializable, which should be fine assuming it's a Dict[str, str] as declared

but if someone passes something like a UUID, this can raise

Suggested change
groups_repr = json.dumps(sorted(groups.items()), separators=(",", ":"))
groups_repr = json.dumps(
sorted((str(k), str(v)) for k, v in groups.items()),
separators=(",", ":"),
)

feature_flag_reported_key = f"{key}_{response_repr}_{groups_repr}"
Copy link
Copy Markdown
Contributor

@dustinbyrne dustinbyrne May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for clarity, we could consider a tuple here (and other assignments)

feature_flag_reported_key = (key, response, groups_key)

else:
feature_flag_reported_key = f"{key}_{response_repr}"

reported_flags = self.distinct_ids_feature_flags_reported.get(distinct_id)
if reported_flags is None:
Expand Down
101 changes: 101 additions & 0 deletions posthog/test/test_feature_flags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5271,6 +5271,107 @@ def test_disable_geoip_get_flag_capture_call(self, patch_flags, patch_capture):
disable_geoip=False,
)

@mock.patch.object(Client, "capture")
@mock.patch("posthog.client.flags")
def test_capture_fires_per_group_context(self, patch_flags, patch_capture):
client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY)
client.feature_flags = [
{
"id": 1,
"name": "Group flag",
"key": "group-scoped-flag",
"active": True,
"filters": {
"groups": [{"properties": [], "rollout_percentage": 100}],
},
}
]

# Same user, same flag, same response — but two different group contexts.
client.get_feature_flag(
"group-scoped-flag", "user-1", groups={"company": "org-a"}
)
client.get_feature_flag(
"group-scoped-flag", "user-1", groups={"company": "org-b"}
)

flag_called_groups = [
call.kwargs.get("groups")
for call in patch_capture.call_args_list
if call.args and call.args[0] == "$feature_flag_called"
]
self.assertEqual(len(flag_called_groups), 2)
self.assertIn({"company": "org-a"}, flag_called_groups)
self.assertIn({"company": "org-b"}, flag_called_groups)

@mock.patch.object(Client, "capture")
@mock.patch("posthog.client.flags")
def test_capture_dedupes_repeated_calls_under_same_group_context(
self, patch_flags, patch_capture
):
client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY)
client.feature_flags = [
{
"id": 1,
"name": "Group flag",
"key": "group-scoped-flag",
"active": True,
"filters": {
"groups": [{"properties": [], "rollout_percentage": 100}],
},
}
]

client.get_feature_flag(
"group-scoped-flag", "user-1", groups={"company": "org-a"}
)
client.get_feature_flag(
"group-scoped-flag", "user-1", groups={"company": "org-a"}
)

flag_called_count = sum(
1
for call in patch_capture.call_args_list
if call.args and call.args[0] == "$feature_flag_called"
)
self.assertEqual(flag_called_count, 1)

@mock.patch.object(Client, "capture")
@mock.patch("posthog.client.flags")
def test_capture_dedupes_when_groups_passed_in_different_key_order(
self, patch_flags, patch_capture
):
client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY)
client.feature_flags = [
{
"id": 1,
"name": "Group flag",
"key": "group-scoped-flag",
"active": True,
"filters": {
"groups": [{"properties": [], "rollout_percentage": 100}],
},
}
]

client.get_feature_flag(
"group-scoped-flag",
"user-1",
groups={"company": "org-a", "team": "red"},
)
client.get_feature_flag(
"group-scoped-flag",
"user-1",
groups={"team": "red", "company": "org-a"},
)

flag_called_count = sum(
1
for call in patch_capture.call_args_list
if call.args and call.args[0] == "$feature_flag_called"
)
self.assertEqual(flag_called_count, 1)
Comment on lines +5339 to +5373
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Prefer parameterised tests for the two dedupe cases

test_capture_dedupes_repeated_calls_under_same_group_context and test_capture_dedupes_when_groups_passed_in_different_key_order have identical structure — they both assert flag_called_count == 1 and differ only in the groups supplied. The codebase already uses @parameterized.expand (e.g. test_mixed_targeting above), so these two cases should be collapsed into a single @parameterized.expand test instead of two separate methods.

Context Used: Do not attempt to comment on incorrect alphabetica... (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 5339-5373

Comment:
**Prefer parameterised tests for the two dedupe cases**

`test_capture_dedupes_repeated_calls_under_same_group_context` and `test_capture_dedupes_when_groups_passed_in_different_key_order` have identical structure — they both assert `flag_called_count == 1` and differ only in the groups supplied. The codebase already uses `@parameterized.expand` (e.g. `test_mixed_targeting` above), so these two cases should be collapsed into a single `@parameterized.expand` test instead of two separate methods.

**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

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!


@mock.patch.object(Client, "capture")
@mock.patch("posthog.client.flags")
def test_capture_multiple_users_doesnt_out_of_memory(
Expand Down
Loading