feat: add get_feature_flags for bulk flag evaluation with events#532
Closed
feat: add get_feature_flags for bulk flag evaluation with events#532
Conversation
Add a new get_feature_flags(keys, distinct_id, ...) method for evaluating a known subset of feature flags in one bulk pass while still emitting $feature_flag_called events per resolved flag. Locally-evaluated flags reuse the poller's cached definitions; keys that can't be resolved locally fall through to a single remote /flags call with flag_keys_to_evaluate. Event dedup uses the existing distinct_ids_feature_flags_reported cache so the single-flag and bulk paths share one source of truth. Also adds a send_feature_flag_events option to get_all_flags and get_all_flags_and_payloads for opt-in per-flag event emission. Defaults to False to preserve existing behavior. Ports PostHog/posthog-js#3447 to posthog-python. Generated-By: PostHog Code Task-Id: b54693f6-498d-4193-bbc2-b9a97e07e69d
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 6789-6801
Comment:
**Incorrect mock response format**
`patch_flags` simulates the `flags()` function, whose return type is `FlagsResponse` — a dict with a `"flags"` key, not `"featureFlags"`. The adjacent `test_get_all_flags_emits_events_when_send_feature_flag_events_enabled` correctly uses `{"flags": {...}, "requestId": "..."}`. This test passes only because it asserts a negative (`call_count == 0`), so the malformed response goes undetected. Using the correct format keeps the two tests consistent and ensures the mock reflects what the real endpoint returns.
```suggestion
patch_flags.return_value = {
"flags": {
"bulk-a": {
"key": "bulk-a",
"enabled": True,
"variant": None,
"reason": {"description": "Matched"},
"metadata": {"id": 1, "version": 1},
},
"bulk-b": {
"key": "bulk-b",
"enabled": True,
"variant": "b-variant",
"reason": {"description": "Matched"},
"metadata": {"id": 2, "version": 1},
},
},
"requestId": "req-all-default",
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 6531-6540
Comment:
**Prefer parameterised tests**
The codebase's style guide requires parameterised tests. Several pairs within `TestGetFeatureFlagsBulk` share the same scaffolding and differ only in one parameter, making them natural candidates:
- `test_no_events_when_send_feature_flag_events_is_false` / `test_emits_feature_flag_called_per_resolved_flag_with_metadata` → vary `send_feature_flag_events` and expected `call_count`.
- `test_get_all_flags_emits_events_when_send_feature_flag_events_enabled` / `test_get_all_flags_default_does_not_emit_events` → same pattern, differ on `send_feature_flag_events`.
Consider collapsing these (and similar pairs) with `@parameterized.expand` or `subTest`.
**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.Reviews (1): Last reviewed commit: "feat: add get_feature_flags for bulk fla..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+6789
to
+6801
| @mock.patch.object(Client, "capture") | ||
| @mock.patch("posthog.client.flags") | ||
| def test_get_all_flags_default_does_not_emit_events( | ||
| self, patch_flags, patch_capture | ||
| ): | ||
| patch_flags.return_value = { | ||
| "featureFlags": {"bulk-a": True, "bulk-b": "b-variant"}, | ||
| } | ||
| client = Client(FAKE_TEST_API_KEY) | ||
|
|
||
| client.get_all_flags("user-1") | ||
|
|
||
| self.assertEqual(patch_capture.call_count, 0) |
Contributor
There was a problem hiding this comment.
patch_flags simulates the flags() function, whose return type is FlagsResponse — a dict with a "flags" key, not "featureFlags". The adjacent test_get_all_flags_emits_events_when_send_feature_flag_events_enabled correctly uses {"flags": {...}, "requestId": "..."}. This test passes only because it asserts a negative (call_count == 0), so the malformed response goes undetected. Using the correct format keeps the two tests consistent and ensures the mock reflects what the real endpoint returns.
Suggested change
| @mock.patch.object(Client, "capture") | |
| @mock.patch("posthog.client.flags") | |
| def test_get_all_flags_default_does_not_emit_events( | |
| self, patch_flags, patch_capture | |
| ): | |
| patch_flags.return_value = { | |
| "featureFlags": {"bulk-a": True, "bulk-b": "b-variant"}, | |
| } | |
| client = Client(FAKE_TEST_API_KEY) | |
| client.get_all_flags("user-1") | |
| self.assertEqual(patch_capture.call_count, 0) | |
| patch_flags.return_value = { | |
| "flags": { | |
| "bulk-a": { | |
| "key": "bulk-a", | |
| "enabled": True, | |
| "variant": None, | |
| "reason": {"description": "Matched"}, | |
| "metadata": {"id": 1, "version": 1}, | |
| }, | |
| "bulk-b": { | |
| "key": "bulk-b", | |
| "enabled": True, | |
| "variant": "b-variant", | |
| "reason": {"description": "Matched"}, | |
| "metadata": {"id": 2, "version": 1}, | |
| }, | |
| }, | |
| "requestId": "req-all-default", | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 6789-6801
Comment:
**Incorrect mock response format**
`patch_flags` simulates the `flags()` function, whose return type is `FlagsResponse` — a dict with a `"flags"` key, not `"featureFlags"`. The adjacent `test_get_all_flags_emits_events_when_send_feature_flag_events_enabled` correctly uses `{"flags": {...}, "requestId": "..."}`. This test passes only because it asserts a negative (`call_count == 0`), so the malformed response goes undetected. Using the correct format keeps the two tests consistent and ensures the mock reflects what the real endpoint returns.
```suggestion
patch_flags.return_value = {
"flags": {
"bulk-a": {
"key": "bulk-a",
"enabled": True,
"variant": None,
"reason": {"description": "Matched"},
"metadata": {"id": 1, "version": 1},
},
"bulk-b": {
"key": "bulk-b",
"enabled": True,
"variant": "b-variant",
"reason": {"description": "Matched"},
"metadata": {"id": 2, "version": 1},
},
},
"requestId": "req-all-default",
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
6531
to
+6540
| self.assertNotIn("flag2", result) | ||
|
|
||
|
|
||
| class TestGetFeatureFlagsBulk(unittest.TestCase): | ||
| """Tests for the bulk `get_feature_flags` method and the `send_feature_flag_events` | ||
| option on `get_all_flags` / `get_all_flags_and_payloads`. | ||
| """ | ||
|
|
||
| def _remote_flags_response(self): | ||
| return { |
Contributor
There was a problem hiding this comment.
The codebase's style guide requires parameterised tests. Several pairs within TestGetFeatureFlagsBulk share the same scaffolding and differ only in one parameter, making them natural candidates:
test_no_events_when_send_feature_flag_events_is_false/test_emits_feature_flag_called_per_resolved_flag_with_metadata→ varysend_feature_flag_eventsand expectedcall_count.test_get_all_flags_emits_events_when_send_feature_flag_events_enabled/test_get_all_flags_default_does_not_emit_events→ same pattern, differ onsend_feature_flag_events.
Consider collapsing these (and similar pairs) with @parameterized.expand or subTest.
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: 6531-6540
Comment:
**Prefer parameterised tests**
The codebase's style guide requires parameterised tests. Several pairs within `TestGetFeatureFlagsBulk` share the same scaffolding and differ only in one parameter, making them natural candidates:
- `test_no_events_when_send_feature_flag_events_is_false` / `test_emits_feature_flag_called_per_resolved_flag_with_metadata` → vary `send_feature_flag_events` and expected `call_count`.
- `test_get_all_flags_emits_events_when_send_feature_flag_events_enabled` / `test_get_all_flags_default_does_not_emit_events` → same pattern, differ on `send_feature_flag_events`.
Consider collapsing these (and similar pairs) with `@parameterized.expand` or `subTest`.
**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!
Contributor
Author
|
Closing in favor of a fresh branch rebased onto current main (original branch was based on a stale commit before several unsigned release commits on main, which prevented the rebase from being pushed under the repo's signature policy). New PR incoming. |
4 tasks
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
1 participant
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.
Summary
Add a new
get_feature_flags(keys, distinct_id, ...)method for evaluating a known subset of feature flags in one bulk pass while still emitting$feature_flag_calledevents per resolved flag. Also adds asend_feature_flag_eventsoption to the existingget_all_flags/get_all_flags_and_payloadsmethods for opt-in per-flag event emission.The motivation (per the original PR): customers loading a page server-side often need a specific ~10 flags.
get_feature_flagper key is N network calls;get_all_flagsis one call but emits no$feature_flag_calledevents and evaluates flags the caller doesn't care about. The new method is the sweet spot: at most one network round trip, per-flag usage events.Ports PostHog/posthog-js#3447 from
posthog-nodetoposthog-python.Changes
get_feature_flagsmethod (posthog/client.py): evaluates a subset of flags in bulk. Locally-evaluated flags reuse the poller's cached definitions; keys that can't be resolved locally fall through to a single remote/flagscall withflag_keys_to_evaluate.distinct_ids_feature_flags_reportedcache so single-flag and bulk paths share one source of truth.send_feature_flag_eventsoption onget_all_flags/get_all_flags_and_payloads. Defaults toFalseto preserve existing behavior.posthog/__init__.py): addsposthog.get_feature_flags(...)and threadssend_feature_flag_eventsthrough the module-levelget_all_flags/get_all_flags_and_payloadswrappers.posthog/test/test_feature_flags.py): newTestGetFeatureFlagsBulkclass with 9 tests covering remote-only evaluation, local-only evaluation, hybrid scenarios, event deduplication,send_feature_flag_events=Falsesuppression, missing-key handling, and the newget_all_flagsoption.Test plan
uv run pytest posthog/test/test_feature_flags.py::TestGetFeatureFlagsBulk— 9/9 passuv run pytest posthog/test/— 324/324 pass (no regressions;send_feature_flag_events=Falsedefault preserves historical behavior)uv run mypy posthog/client.py posthog/__init__.py— no new errors vs. baselineuvx ruff format— cleanCreated with PostHog Code