From a1392de7b574fb4d46ef2db7e481b15b1daf8742 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 10 Jan 2024 11:38:47 +0000 Subject: [PATCH 1/5] fix(flags): Don't override existing props when adding flags --- posthog/client.py | 12 +++-- posthog/test/test_client.py | 88 +++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/posthog/client.py b/posthog/client.py index 113663d4..2595c48a 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -206,6 +206,7 @@ def capture( require("groups", groups, dict) msg["properties"]["$groups"] = groups + extra_properties = {} if send_feature_flags: try: feature_variants = self._get_active_feature_variants(distinct_id, groups, disable_geoip=disable_geoip) @@ -213,19 +214,22 @@ def capture( self.log.exception(f"[FEATURE FLAGS] Unable to get feature variants: {e}") else: for feature, variant in feature_variants.items(): - msg["properties"][f"$feature/{feature}"] = variant - msg["properties"]["$active_feature_flags"] = list(feature_variants.keys()) + extra_properties[f"$feature/{feature}"] = variant + extra_properties["$active_feature_flags"] = list(feature_variants.keys()) elif self.feature_flags: # Local evaluation is enabled, flags are loaded, so try and get all flags we can without going to the server feature_variants = self.get_all_flags( distinct_id, groups=(groups or {}), disable_geoip=disable_geoip, only_evaluate_locally=True ) for feature, variant in feature_variants.items(): - msg["properties"][f"$feature/{feature}"] = variant + extra_properties[f"$feature/{feature}"] = variant active_feature_flags = [key for (key, value) in feature_variants.items() if value is not False] if active_feature_flags: - msg["properties"]["$active_feature_flags"] = active_feature_flags + extra_properties["$active_feature_flags"] = active_feature_flags + + if extra_properties: + msg["properties"] = {**extra_properties, **msg["properties"]} return self._enqueue(msg, disable_geoip) diff --git a/posthog/test/test_client.py b/posthog/test/test_client.py index a81f7965..7131a1e2 100644 --- a/posthog/test/test_client.py +++ b/posthog/test/test_client.py @@ -208,6 +208,94 @@ def test_basic_capture_with_locally_evaluated_feature_flags(self, patch_decide): assert "$feature/beta-feature-local" not in msg["properties"] assert "$feature/false-flag" not in msg["properties"] assert "$active_feature_flags" not in msg["properties"] + + @mock.patch("posthog.client.decide") + def test_dont_override_capture_with_local_flags(self, patch_decide): + patch_decide.return_value = {"featureFlags": {"beta-feature": "random-variant"}} + client = Client(FAKE_TEST_API_KEY, on_error=self.set_fail, personal_api_key=FAKE_TEST_API_KEY) + + multivariate_flag = { + "id": 1, + "name": "Beta Feature", + "key": "beta-feature-local", + "is_simple_flag": False, + "active": True, + "rollout_percentage": 100, + "filters": { + "groups": [ + { + "properties": [ + {"key": "email", "type": "person", "value": "test@posthog.com", "operator": "exact"} + ], + "rollout_percentage": 100, + }, + { + "rollout_percentage": 50, + }, + ], + "multivariate": { + "variants": [ + {"key": "first-variant", "name": "First Variant", "rollout_percentage": 50}, + {"key": "second-variant", "name": "Second Variant", "rollout_percentage": 25}, + {"key": "third-variant", "name": "Third Variant", "rollout_percentage": 25}, + ] + }, + "payloads": {"first-variant": "some-payload", "third-variant": {"a": "json"}}, + }, + } + basic_flag = { + "id": 1, + "name": "Beta Feature", + "key": "person-flag", + "is_simple_flag": True, + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "region", + "operator": "exact", + "value": ["USA"], + "type": "person", + } + ], + "rollout_percentage": 100, + } + ], + "payloads": {"true": 300}, + }, + } + client.feature_flags = [multivariate_flag, basic_flag] + + success, msg = client.capture("distinct_id", "python test event", {"$feature/beta-feature-local": "my-custom-variant"}) + client.flush() + self.assertTrue(success) + self.assertFalse(self.failed) + + self.assertEqual(msg["event"], "python test event") + self.assertTrue(isinstance(msg["timestamp"], str)) + self.assertIsNone(msg.get("uuid")) + self.assertEqual(msg["distinct_id"], "distinct_id") + self.assertEqual(msg["properties"]["$lib"], "posthog-python") + self.assertEqual(msg["properties"]["$lib_version"], VERSION) + self.assertEqual(msg["properties"]["$feature/beta-feature-local"], "my-custom-variant") + self.assertEqual(msg["properties"]["$active_feature_flags"], ["beta-feature-local"]) + assert "$feature/beta-feature" not in msg["properties"] + assert "$feature/person-flag" not in msg["properties"] + + self.assertEqual(patch_decide.call_count, 0) + + # test that flags are not evaluated without local evaluation + client.feature_flags = [] + success, msg = client.capture("distinct_id", "python test event") + client.flush() + self.assertTrue(success) + self.assertFalse(self.failed) + assert "$feature/beta-feature" not in msg["properties"] + assert "$feature/beta-feature-local" not in msg["properties"] + assert "$feature/false-flag" not in msg["properties"] + assert "$active_feature_flags" not in msg["properties"] @mock.patch("posthog.client.decide") def test_get_active_feature_flags(self, patch_decide): From 4486d078c3924ab09060329cb15acca9b88c6ad9 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 10 Jan 2024 11:40:08 +0000 Subject: [PATCH 2/5] fix(flags): Don't override existing props when adding flags --- posthog/test/test_client.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/posthog/test/test_client.py b/posthog/test/test_client.py index 7131a1e2..526ac477 100644 --- a/posthog/test/test_client.py +++ b/posthog/test/test_client.py @@ -286,17 +286,6 @@ def test_dont_override_capture_with_local_flags(self, patch_decide): self.assertEqual(patch_decide.call_count, 0) - # test that flags are not evaluated without local evaluation - client.feature_flags = [] - success, msg = client.capture("distinct_id", "python test event") - client.flush() - self.assertTrue(success) - self.assertFalse(self.failed) - assert "$feature/beta-feature" not in msg["properties"] - assert "$feature/beta-feature-local" not in msg["properties"] - assert "$feature/false-flag" not in msg["properties"] - assert "$active_feature_flags" not in msg["properties"] - @mock.patch("posthog.client.decide") def test_get_active_feature_flags(self, patch_decide): patch_decide.return_value = { From 1e363efb12bcdacd234994e93dbf217c3e05b861 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 10 Jan 2024 11:40:28 +0000 Subject: [PATCH 3/5] fix(flags): Don't override existing props when adding flags --- posthog/client.py | 2 +- posthog/test/test_client.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/posthog/client.py b/posthog/client.py index 2595c48a..07fefc4d 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -227,7 +227,7 @@ def capture( active_feature_flags = [key for (key, value) in feature_variants.items() if value is not False] if active_feature_flags: extra_properties["$active_feature_flags"] = active_feature_flags - + if extra_properties: msg["properties"] = {**extra_properties, **msg["properties"]} diff --git a/posthog/test/test_client.py b/posthog/test/test_client.py index 526ac477..f0c8a8df 100644 --- a/posthog/test/test_client.py +++ b/posthog/test/test_client.py @@ -208,7 +208,7 @@ def test_basic_capture_with_locally_evaluated_feature_flags(self, patch_decide): assert "$feature/beta-feature-local" not in msg["properties"] assert "$feature/false-flag" not in msg["properties"] assert "$active_feature_flags" not in msg["properties"] - + @mock.patch("posthog.client.decide") def test_dont_override_capture_with_local_flags(self, patch_decide): patch_decide.return_value = {"featureFlags": {"beta-feature": "random-variant"}} @@ -268,7 +268,9 @@ def test_dont_override_capture_with_local_flags(self, patch_decide): } client.feature_flags = [multivariate_flag, basic_flag] - success, msg = client.capture("distinct_id", "python test event", {"$feature/beta-feature-local": "my-custom-variant"}) + success, msg = client.capture( + "distinct_id", "python test event", {"$feature/beta-feature-local": "my-custom-variant"} + ) client.flush() self.assertTrue(success) self.assertFalse(self.failed) From 2e40ae92c319e663c1bd2c9dd75e792b346e84bf Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 10 Jan 2024 11:42:41 +0000 Subject: [PATCH 4/5] version update --- CHANGELOG.md | 4 ++++ posthog/version.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b92416f7..c6247904 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 3.3.1 - 2024-01-10 + +1. Make sure we don't override any existing feature flag properties when adding locally evaluated feature flag properties. + ## 3.3.0 - 2024-01-09 1. When local evaluation is enabled, we automatically add flag information to all events sent to PostHog, whenever possible. This makes it easier to use these events in experiments. diff --git a/posthog/version.py b/posthog/version.py index 8c067eda..f4f2a613 100644 --- a/posthog/version.py +++ b/posthog/version.py @@ -1,4 +1,4 @@ -VERSION = "3.3.0" +VERSION = "3.3.1" if __name__ == "__main__": print(VERSION, end="") # noqa: T201 From 6e35124ff590d5e6470872088260f200cd038a52 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 10 Jan 2024 13:38:26 +0000 Subject: [PATCH 5/5] simplify --- posthog/client.py | 29 +++++++++-------------------- posthog/test/test_client.py | 20 -------------------- 2 files changed, 9 insertions(+), 40 deletions(-) diff --git a/posthog/client.py b/posthog/client.py index 07fefc4d..afbfb1ad 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -137,16 +137,6 @@ def get_feature_variants( resp_data = self.get_decide(distinct_id, groups, person_properties, group_properties, disable_geoip) return resp_data["featureFlags"] - def _get_active_feature_variants( - self, distinct_id, groups=None, person_properties=None, group_properties=None, disable_geoip=None - ): - feature_variants = self.get_feature_variants( - distinct_id, groups, person_properties, group_properties, disable_geoip - ) - return { - k: v for (k, v) in feature_variants.items() if v is not False - } # explicitly test for false to account for values that may seem falsy (ex: 0) - def get_feature_payloads( self, distinct_id, groups=None, person_properties=None, group_properties=None, disable_geoip=None ): @@ -207,26 +197,25 @@ def capture( msg["properties"]["$groups"] = groups extra_properties = {} + feature_variants = {} if send_feature_flags: try: - feature_variants = self._get_active_feature_variants(distinct_id, groups, disable_geoip=disable_geoip) + feature_variants = self.get_feature_variants(distinct_id, groups, disable_geoip=disable_geoip) except Exception as e: self.log.exception(f"[FEATURE FLAGS] Unable to get feature variants: {e}") - else: - for feature, variant in feature_variants.items(): - extra_properties[f"$feature/{feature}"] = variant - extra_properties["$active_feature_flags"] = list(feature_variants.keys()) + elif self.feature_flags: # Local evaluation is enabled, flags are loaded, so try and get all flags we can without going to the server feature_variants = self.get_all_flags( distinct_id, groups=(groups or {}), disable_geoip=disable_geoip, only_evaluate_locally=True ) - for feature, variant in feature_variants.items(): - extra_properties[f"$feature/{feature}"] = variant - active_feature_flags = [key for (key, value) in feature_variants.items() if value is not False] - if active_feature_flags: - extra_properties["$active_feature_flags"] = active_feature_flags + for feature, variant in feature_variants.items(): + extra_properties[f"$feature/{feature}"] = variant + + active_feature_flags = [key for (key, value) in feature_variants.items() if value is not False] + if active_feature_flags: + extra_properties["$active_feature_flags"] = active_feature_flags if extra_properties: msg["properties"] = {**extra_properties, **msg["properties"]} diff --git a/posthog/test/test_client.py b/posthog/test/test_client.py index f0c8a8df..91d3f13d 100644 --- a/posthog/test/test_client.py +++ b/posthog/test/test_client.py @@ -288,26 +288,6 @@ def test_dont_override_capture_with_local_flags(self, patch_decide): self.assertEqual(patch_decide.call_count, 0) - @mock.patch("posthog.client.decide") - def test_get_active_feature_flags(self, patch_decide): - patch_decide.return_value = { - "featureFlags": {"beta-feature": "random-variant", "alpha-feature": True, "off-feature": False} - } - - client = Client(FAKE_TEST_API_KEY, on_error=self.set_fail, personal_api_key=FAKE_TEST_API_KEY) - variants = client._get_active_feature_variants("some_id", None, None, None, False) - self.assertEqual(variants, {"beta-feature": "random-variant", "alpha-feature": True}) - patch_decide.assert_called_with( - "random_key", - None, - timeout=10, - distinct_id="some_id", - groups={}, - person_properties=None, - group_properties=None, - disable_geoip=False, - ) - @mock.patch("posthog.client.decide") def test_basic_capture_with_feature_flags_returns_active_only(self, patch_decide): patch_decide.return_value = {