code review feedback

Author: dmarticus

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

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,26 +2391,27 @@ 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)
         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="person-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",
             "$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,