From 6f44441551691c8adbc41cadc8d2182d82d33657 Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 20 Nov 2024 13:45:17 -0600 Subject: [PATCH 01/10] this is the fix, needs tests --- posthog/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/client.py b/posthog/client.py index 0f7c833c..371b2e16 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -724,7 +724,7 @@ def get_feature_flag_payload( person_properties=person_properties, group_properties=group_properties, send_feature_flag_events=send_feature_flag_events, - only_evaluate_locally=True, + only_evaluate_locally=only_evaluate_locally, disable_geoip=disable_geoip, ) From fdf8f54770805bc1260573f9c49782eea66ce19e Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 20 Nov 2024 13:52:46 -0600 Subject: [PATCH 02/10] fix test --- posthog/test/test_feature_flags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 90a857e3..140c5d02 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -1648,7 +1648,7 @@ def test_boolean_feature_flag_payload_decide(self, patch_decide): ), 300, ) - self.assertEqual(patch_decide.call_count, 2) + self.assertEqual(patch_decide.call_count, 3) @mock.patch("posthog.client.decide") def test_multivariate_feature_flag_payloads(self, patch_decide): From 2ef214aa917f453756e94f61806de73fe84512b2 Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 20 Nov 2024 17:02:16 -0600 Subject: [PATCH 03/10] tests --- posthog/test/test_feature_flags.py | 108 ++++++++++++++++++++++++++++- 1 file changed, 107 insertions(+), 1 deletion(-) diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 140c5d02..2a37616d 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -1632,8 +1632,9 @@ def test_boolean_feature_flag_payloads_local(self, patch_decide): ) self.assertEqual(patch_decide.call_count, 0) + @mock.patch.object(Client, "capture") @mock.patch("posthog.client.decide") - def test_boolean_feature_flag_payload_decide(self, patch_decide): + def test_boolean_feature_flag_payload_decide(self, patch_decide, patch_capture): patch_decide.return_value = {"featureFlagPayloads": {"person-flag": 300}} self.assertEqual( self.client.get_feature_flag_payload( @@ -1649,6 +1650,20 @@ def test_boolean_feature_flag_payload_decide(self, patch_decide): 300, ) self.assertEqual(patch_decide.call_count, 3) + self.assertEqual(patch_capture.call_count, 1) + # patch_capture.assert_called_with( + # "some-distinct-id", + # "$feature_flag_called", + # { + # "$feature_flag": "person-flag", + # "$feature_flag_response": True, + # "locally_evaluated": False, + # "$feature/complex-flag": True, + # }, + # groups={}, + # disable_geoip=None, + # ) + patch_capture.reset_mock() @mock.patch("posthog.client.decide") def test_multivariate_feature_flag_payloads(self, patch_decide): @@ -2334,6 +2349,97 @@ def test_capture_is_called(self, patch_decide, patch_capture): disable_geoip=None, ) + + @mock.patch.object(Client, "capture") + @mock.patch("posthog.client.decide") + def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch_capture): + # Mock the decide response + patch_decide.return_value = {"featureFlags": {"decide-flag": "decide-value"}, "featureFlagPayloads": {"person-flag": 300}} + + # Initialize the Client with necessary keys + client = Client(api_key=FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + + # Set up feature flags + client.feature_flags = [ + { + "id": 1, + "name": "Beta Feature", + "key": "complex-flag", + "is_simple_flag": False, + "active": True, + "filters": { + "groups": [ + { + "properties": [{"key": "region", "value": "USA"}], + "rollout_percentage": 100, + } + ], + }, + } + ] + + # Call get_feature_flag_payload with match_value=None to trigger get_feature_flag + client.get_feature_flag_payload( + key="complex-flag", + distinct_id="some-distinct-id", + person_properties={"region": "USA", "name": "Aloha"} + ) + + # Assert that capture was called once + self.assertEqual(patch_capture.call_count, 1) + + # Verify the capture was called with the correct parameters + patch_capture.assert_called_with( + "some-distinct-id", + "$feature_flag_called", + { + "$feature_flag": "complex-flag", + "$feature_flag_response": True, + "locally_evaluated": True, + "$feature/complex-flag": True, + }, + groups={}, + disable_geoip=None, + ) + + # Reset mocks for further tests + patch_capture.reset_mock() + patch_decide.reset_mock() + + # Call get_feature_flag_payload again for the same user; capture should not be called again + client.get_feature_flag_payload( + key="complex-flag", + distinct_id="some-distinct-id", + person_properties={"region": "USA", "name": "Aloha"} + ) + + self.assertEqual(patch_capture.call_count, 0) + patch_capture.reset_mock() + + # Call get_feature_flag_payload for a different user; capture should be called + client.get_feature_flag_payload( + key="complex-flag", + distinct_id="some-distinct-id2", + person_properties={"region": "USA", "name": "Aloha"} + ) + + # self.assertIsNotNone(payload) + self.assertEqual(patch_capture.call_count, 1) + patch_capture.assert_called_with( + "some-distinct-id2", + "$feature_flag_called", + { + "$feature_flag": "complex-flag", + "$feature_flag_response": True, + "locally_evaluated": True, + "$feature/complex-flag": True, + }, + groups={}, + disable_geoip=None, + ) + + patch_capture.reset_mock() + @mock.patch.object(Client, "capture") @mock.patch("posthog.client.decide") def test_disable_geoip_get_flag_capture_call(self, patch_decide, patch_capture): From b114a488c5874c6388e3bf34f07a5d08d00d4329 Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 20 Nov 2024 17:08:48 -0600 Subject: [PATCH 04/10] yeah --- posthog/test/test_feature_flags.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 2a37616d..22b90238 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -1651,18 +1651,6 @@ def test_boolean_feature_flag_payload_decide(self, patch_decide, patch_capture): ) self.assertEqual(patch_decide.call_count, 3) self.assertEqual(patch_capture.call_count, 1) - # patch_capture.assert_called_with( - # "some-distinct-id", - # "$feature_flag_called", - # { - # "$feature_flag": "person-flag", - # "$feature_flag_response": True, - # "locally_evaluated": False, - # "$feature/complex-flag": True, - # }, - # groups={}, - # disable_geoip=None, - # ) patch_capture.reset_mock() @mock.patch("posthog.client.decide") From feb9560b6f1218905bb384d57640fa17f1a07200 Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 20 Nov 2024 17:16:41 -0600 Subject: [PATCH 05/10] please work --- posthog/test/test_feature_flags.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 22b90238..77144bb0 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -2341,13 +2341,9 @@ def test_capture_is_called(self, patch_decide, patch_capture): @mock.patch.object(Client, "capture") @mock.patch("posthog.client.decide") def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch_capture): - # Mock the decide response patch_decide.return_value = {"featureFlags": {"decide-flag": "decide-value"}, "featureFlagPayloads": {"person-flag": 300}} - - # Initialize the Client with necessary keys client = Client(api_key=FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) - # Set up feature flags client.feature_flags = [ { "id": 1, @@ -2373,10 +2369,8 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch person_properties={"region": "USA", "name": "Aloha"} ) - # Assert that capture was called once + # Assert that capture was called once, with the correct parameters self.assertEqual(patch_capture.call_count, 1) - - # Verify the capture was called with the correct parameters patch_capture.assert_called_with( "some-distinct-id", "$feature_flag_called", @@ -2394,7 +2388,7 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch patch_capture.reset_mock() patch_decide.reset_mock() - # Call get_feature_flag_payload again for the same user; capture should not be called again + # Call get_feature_flag_payload again for the same user; capture should not be called again because we've already reported an event for this distinct_id + flag client.get_feature_flag_payload( key="complex-flag", distinct_id="some-distinct-id", @@ -2411,7 +2405,6 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch person_properties={"region": "USA", "name": "Aloha"} ) - # self.assertIsNotNone(payload) self.assertEqual(patch_capture.call_count, 1) patch_capture.assert_called_with( "some-distinct-id2", From d271880608cb703cafedd9c653851f145d1f9f70 Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 20 Nov 2024 17:18:40 -0600 Subject: [PATCH 06/10] ran the formatter --- posthog/test/test_feature_flags.py | 38 +++++++++++++----------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 77144bb0..949ddc18 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -2337,13 +2337,15 @@ def test_capture_is_called(self, patch_decide, patch_capture): disable_geoip=None, ) - @mock.patch.object(Client, "capture") @mock.patch("posthog.client.decide") def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch_capture): - patch_decide.return_value = {"featureFlags": {"decide-flag": "decide-value"}, "featureFlagPayloads": {"person-flag": 300}} + patch_decide.return_value = { + "featureFlags": {"decide-flag": "decide-value"}, + "featureFlagPayloads": {"person-flag": 300}, + } client = Client(api_key=FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) - + client.feature_flags = [ { "id": 1, @@ -2361,14 +2363,12 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch }, } ] - + # Call get_feature_flag_payload with match_value=None to trigger get_feature_flag client.get_feature_flag_payload( - key="complex-flag", - distinct_id="some-distinct-id", - person_properties={"region": "USA", "name": "Aloha"} + key="complex-flag", distinct_id="some-distinct-id", person_properties={"region": "USA", "name": "Aloha"} ) - + # Assert that capture was called once, with the correct parameters self.assertEqual(patch_capture.call_count, 1) patch_capture.assert_called_with( @@ -2383,28 +2383,24 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch groups={}, disable_geoip=None, ) - + # Reset mocks for further tests patch_capture.reset_mock() patch_decide.reset_mock() - + # Call get_feature_flag_payload again for the same user; capture should not be called again because we've already reported an event for this distinct_id + flag client.get_feature_flag_payload( - key="complex-flag", - distinct_id="some-distinct-id", - person_properties={"region": "USA", "name": "Aloha"} + key="complex-flag", distinct_id="some-distinct-id", person_properties={"region": "USA", "name": "Aloha"} ) - + self.assertEqual(patch_capture.call_count, 0) patch_capture.reset_mock() - + # Call get_feature_flag_payload for a different user; capture should be called client.get_feature_flag_payload( - key="complex-flag", - distinct_id="some-distinct-id2", - person_properties={"region": "USA", "name": "Aloha"} + key="complex-flag", distinct_id="some-distinct-id2", person_properties={"region": "USA", "name": "Aloha"} ) - + self.assertEqual(patch_capture.call_count, 1) patch_capture.assert_called_with( "some-distinct-id2", @@ -2418,9 +2414,9 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch groups={}, disable_geoip=None, ) - + patch_capture.reset_mock() - + @mock.patch.object(Client, "capture") @mock.patch("posthog.client.decide") def test_disable_geoip_get_flag_capture_call(self, patch_decide, patch_capture): From 63aeaa39ca2907bece89794a89a969cea79ca8d3 Mon Sep 17 00:00:00 2001 From: dylan Date: Thu, 21 Nov 2024 10:59:23 -0600 Subject: [PATCH 07/10] code review feedback --- posthog/client.py | 55 +++++++++++++++++++++++++----- posthog/test/test_feature_flags.py | 28 ++++++++------- 2 files changed, 62 insertions(+), 21 deletions(-) diff --git a/posthog/client.py b/posthog/client.py index 371b2e16..70bb8030 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -1,5 +1,6 @@ import atexit import logging +from mailbox import NotEmptyError import numbers import sys from datetime import datetime, timedelta @@ -161,6 +162,15 @@ def get_feature_payloads( resp_data = self.get_decide(distinct_id, groups, person_properties, group_properties, disable_geoip) return resp_data["featureFlagPayloads"] + def get_feature_flags_and_payloads( + self, distinct_id, groups=None, person_properties=None, group_properties=None, disable_geoip=None + ): + resp_data = self.get_decide(distinct_id, groups, person_properties, group_properties, disable_geoip) + return { + "featureFlags": resp_data["featureFlags"], + "featureFlagPayloads": resp_data["featureFlagPayloads"], + } + def get_decide(self, distinct_id, groups=None, person_properties=None, group_properties=None, disable_geoip=None): require("distinct_id", distinct_id, ID_TYPES) @@ -723,23 +733,52 @@ def get_feature_flag_payload( groups=groups, person_properties=person_properties, group_properties=group_properties, - send_feature_flag_events=send_feature_flag_events, - only_evaluate_locally=only_evaluate_locally, + send_feature_flag_events=False, + # Disable automatic sending of feature flag events because we're manually handling event dispatch. + # This prevents sending events with empty data when `get_feature_flag` cannot be evaluated locally. + only_evaluate_locally=True, # Enable local evaluation of feature flags to avoid making multiple requests to `/decide`. disable_geoip=disable_geoip, ) response = None + payload = None if match_value is not None: - response = self._compute_payload_locally(key, match_value) + payload = self._compute_payload_locally(key, match_value) + + flag_was_locally_evaluated = payload is not None + if not flag_was_locally_evaluated and not only_evaluate_locally: + try: + responses_and_payloads = self.get_feature_flags_and_payloads( + distinct_id, groups, person_properties, group_properties, disable_geoip + ) + response = responses_and_payloads["featureFlags"].get(key, None) + payload = responses_and_payloads["featureFlagPayloads"].get(str(key).lower(), None) + except Exception as e: + self.log.exception(f"[FEATURE FLAGS] Unable to get feature flags and payloads: {e}") - if response is None and not only_evaluate_locally: - decide_payloads = self.get_feature_payloads( - distinct_id, groups, person_properties, group_properties, disable_geoip + feature_flag_reported_key = f"{key}_{str(response)}" + + if ( + feature_flag_reported_key not in self.distinct_ids_feature_flags_reported[distinct_id] + and send_feature_flag_events # noqa: W503 + ): + self.capture( + distinct_id, + "$feature_flag_called", + { + "$feature_flag": key, + "$feature_flag_response": response, + "$feature_flag_payload": payload, + "locally_evaluated": flag_was_locally_evaluated, + f"$feature/{key}": response, + }, + groups=groups, + disable_geoip=disable_geoip, ) - response = decide_payloads.get(str(key).lower(), None) + self.distinct_ids_feature_flags_reported[distinct_id].add(feature_flag_reported_key) - return response + return payload def _compute_payload_locally(self, key, match_value): payload = None diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 949ddc18..1cb5b20a 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -1635,7 +1635,7 @@ def test_boolean_feature_flag_payloads_local(self, patch_decide): @mock.patch.object(Client, "capture") @mock.patch("posthog.client.decide") def test_boolean_feature_flag_payload_decide(self, patch_decide, patch_capture): - patch_decide.return_value = {"featureFlagPayloads": {"person-flag": 300}} + patch_decide.return_value = {"featureFlags": {"person-flag": True}, "featureFlagPayloads": {"person-flag": 300}} self.assertEqual( self.client.get_feature_flag_payload( "person-flag", "some-distinct-id", person_properties={"region": "USA"} @@ -1649,7 +1649,7 @@ def test_boolean_feature_flag_payload_decide(self, patch_decide, patch_capture): ), 300, ) - self.assertEqual(patch_decide.call_count, 3) + self.assertEqual(patch_decide.call_count, 2) self.assertEqual(patch_capture.call_count, 1) patch_capture.reset_mock() @@ -2341,7 +2341,7 @@ def test_capture_is_called(self, patch_decide, patch_capture): @mock.patch("posthog.client.decide") def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch_capture): patch_decide.return_value = { - "featureFlags": {"decide-flag": "decide-value"}, + "featureFlags": {"person-flag": True}, "featureFlagPayloads": {"person-flag": 300}, } client = Client(api_key=FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) @@ -2350,7 +2350,7 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch { "id": 1, "name": "Beta Feature", - "key": "complex-flag", + "key": "person-flag", "is_simple_flag": False, "active": True, "filters": { @@ -2366,7 +2366,7 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch # Call get_feature_flag_payload with match_value=None to trigger get_feature_flag client.get_feature_flag_payload( - key="complex-flag", distinct_id="some-distinct-id", person_properties={"region": "USA", "name": "Aloha"} + key="person-flag", distinct_id="some-distinct-id", person_properties={"region": "USA", "name": "Aloha"} ) # Assert that capture was called once, with the correct parameters @@ -2375,10 +2375,11 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch "some-distinct-id", "$feature_flag_called", { - "$feature_flag": "complex-flag", + "$feature_flag": "person-flag", "$feature_flag_response": True, - "locally_evaluated": True, - "$feature/complex-flag": True, + "$feature_flag_payload": 300, + "locally_evaluated": False, + "$feature/person-flag": True, }, groups={}, disable_geoip=None, @@ -2390,7 +2391,7 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch # Call get_feature_flag_payload again for the same user; capture should not be called again because we've already reported an event for this distinct_id + flag client.get_feature_flag_payload( - key="complex-flag", distinct_id="some-distinct-id", person_properties={"region": "USA", "name": "Aloha"} + key="person-flag", distinct_id="some-distinct-id", person_properties={"region": "USA", "name": "Aloha"} ) self.assertEqual(patch_capture.call_count, 0) @@ -2398,7 +2399,7 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch # Call get_feature_flag_payload for a different user; capture should be called client.get_feature_flag_payload( - key="complex-flag", distinct_id="some-distinct-id2", person_properties={"region": "USA", "name": "Aloha"} + key="person-flag", distinct_id="some-distinct-id2", person_properties={"region": "USA", "name": "Aloha"} ) self.assertEqual(patch_capture.call_count, 1) @@ -2406,10 +2407,11 @@ def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch "some-distinct-id2", "$feature_flag_called", { - "$feature_flag": "complex-flag", + "$feature_flag": "person-flag", "$feature_flag_response": True, - "locally_evaluated": True, - "$feature/complex-flag": True, + "$feature_flag_payload": 300, + "locally_evaluated": False, + "$feature/person-flag": True, }, groups={}, disable_geoip=None, From c30d17bb796fcf26856a8de0fbf23decae1259b6 Mon Sep 17 00:00:00 2001 From: dylan Date: Thu, 21 Nov 2024 11:18:44 -0600 Subject: [PATCH 08/10] how'd this get here --- posthog/client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/posthog/client.py b/posthog/client.py index 70bb8030..f7669d5f 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -1,6 +1,5 @@ import atexit import logging -from mailbox import NotEmptyError import numbers import sys from datetime import datetime, timedelta From c544d643b37bfcaa3182b0c674f5d06ad876af72 Mon Sep 17 00:00:00 2001 From: dylan Date: Thu, 21 Nov 2024 18:39:29 -0600 Subject: [PATCH 09/10] bump version add changelog --- CHANGELOG.md | 4 ++++ posthog/version.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5008e64..3b3147cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 3.7.3 - 2024-11-22 + +Fix bug where this SDK incorrectly sent feature flag events with null values when calling `get_feature_flag_payload`, even though the actual payload is successfully retrieved and returned to the user. + ## 3.7.2 - 2024-11-19 1. Add `type` property to exception stacks. diff --git a/posthog/version.py b/posthog/version.py index f0f5a9e2..74b62d61 100644 --- a/posthog/version.py +++ b/posthog/version.py @@ -1,4 +1,4 @@ -VERSION = "3.7.2" +VERSION = "3.7.3" if __name__ == "__main__": print(VERSION, end="") # noqa: T201 From 54ae9755f999710ee5461b4c4bebca534dba347e Mon Sep 17 00:00:00 2001 From: Dylan Martin Date: Mon, 25 Nov 2024 14:39:44 -0500 Subject: [PATCH 10/10] Update CHANGELOG.md Co-authored-by: David Newell --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b3147cb..63c8063c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## 3.7.3 - 2024-11-22 -Fix bug where this SDK incorrectly sent feature flag events with null values when calling `get_feature_flag_payload`, even though the actual payload is successfully retrieved and returned to the user. +1. Fix bug where this SDK incorrectly sent feature flag events with null values when calling `get_feature_flag_payload`. ## 3.7.2 - 2024-11-19