Skip to content

fix(flags): correctly emit feature flag events with the FF response on get_feature_flag_payload calls #143

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

Merged
dmarticus merged 13 commits into from
Nov 25, 2024
Merged

fix(flags): correctly emit feature flag events with the FF response on get_feature_flag_payload calls #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6f44441
this is the fix, needs tests
dmarticus Nov 20, 2024
fdf8f54
fix test
dmarticus Nov 20, 2024
2ef214a
tests
dmarticus Nov 20, 2024
b114a48
yeah
dmarticus Nov 20, 2024
feb9560
please work
dmarticus Nov 20, 2024
d271880
ran the formatter
dmarticus Nov 20, 2024
63aeaa3
code review feedback
dmarticus Nov 21, 2024
c30d17b
how'd this get here
dmarticus Nov 21, 2024
af2f83e
Merge branch 'master' into fix/feature-flag-payload-events
dmarticus Nov 22, 2024
c544d64
bump version add changelog
dmarticus Nov 22, 2024
54ae975
Update CHANGELOG.md
dmarticus Nov 25, 2024
85c020b
update changelog
dmarticus Nov 25, 2024
2694fd1
merge conflicts
dmarticus Nov 25, 2024
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 3.7.4 - 2024-11-25

1. Fix bug where this SDK incorrectly sent feature flag events with null values when calling `get_feature_flag_payload`.

## 3.7.3 - 2024-11-25

1. Use personless mode when sending an exception without a provided `distinct_id`.
Expand Down
54 changes: 46 additions & 8 deletions posthog/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,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)

Expand Down Expand Up @@ -746,23 +755,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=True,
Copy link
Contributor Author

@dmarticus dmarticus Nov 20, 2024

Choose a reason for hiding this comment

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

@EDsCODE you wrote this originally; given that this pattern feels to intentionally deviate from the other function arguments, is there any reason why you did this originally? I want to make sure I'm not regressing some other behavior.

Copy link
Contributor

@neilkakkar neilkakkar Nov 21, 2024

Choose a reason for hiding this comment

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

the problem now is you're making two requests to /decide in some cases to fetch the payload (once to get the match value, 2nd time to get the payload).

I can see both cases being not ideal though! I think I'd recommend setting send_feature_flag_events to False here, and add a comment about this, and then manually handle sending the event in the right case in the end.

Also, I expect all SDKs for payloads to follow this same format, so might have the same problem, worth a quick check in posthog-node atleast (the other popular SDK)

Copy link
Contributor Author

@dmarticus dmarticus Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I like that idea! I feel like I want to be a bit more clever than that, though, because I don't want the event to contain the feature flag payload as the feature_flag_response, I still want to track whatever response the feature flag returned. A few things I could do on that front:

Add a new method to return both the response and payload, and use that method to get both subsets of data

        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.get("featureFlags", []),
                "featureFlagPayloads": resp_data.get("featureFlagPayloads", {})
            }

And then do something like this in get_feature_flag_payloads

    def get_feature_flag_payload(
        self,
        key,
        distinct_id,
        *,
        match_value=None,
        groups={},
        person_properties={},
        group_properties={},
        only_evaluate_locally=False,
        send_feature_flag_events=True,
        disable_geoip=None,
    ):
        if self.disabled:
            return None

        if match_value is None:
            match_value = self.get_feature_flag(
                key,
                distinct_id,
                groups=groups,
                person_properties=person_properties,
                group_properties=group_properties,
                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

        if match_value is not None:
            response = self._compute_payload_locally(key, match_value)

        if response is None and not only_evaluate_locally:
            decide_responses, decide_payloads = self.get_feature_flags_and_payloads(
                distinct_id, groups, person_properties, group_properties, disable_geoip
            )
            response = decide_payloads.get(str(key).lower(), None)
            payload = decide_payloads.get(str(key).lower(), None)

        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,
                    "locally_evaluated": flag_was_locally_evaluated,
                    f"$feature/{key}": response,
                },
                groups=groups,
                disable_geoip=disable_geoip,
            )
            self.distinct_ids_feature_flags_reported[distinct_id].add(feature_flag_reported_key)

        return payload

Another thought is to include both the feature_flag_response and feature_flag_payload as properties on the event (basically do what I did above, but add the payload to the capture call (the event would like this)

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,
            )

I guess it's up to me to decide what to do here, since it seems like we haven't been doing this with our SDKs, so just brainstorming a bit. Seems easier to not include the payload in the event (since we don't have a nice way of showing it in the flags UI), but I like the idea of tracking both things (more changes, but it gives the users a more holistic view of their events overall, because then they'll be able to differentiate between feature_flag_called events with and without payloads).

Copy link
Contributor Author

@dmarticus dmarticus Nov 21, 2024

Choose a reason for hiding this comment

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

Slack thread on this: https://posthog.slack.com/archives/C07Q2U4BH4L/p1732202609363039?thread_ts=1732144656.105669&cid=C07Q2U4BH4L

Copy link
Contributor Author

@dmarticus dmarticus Nov 25, 2024

Choose a reason for hiding this comment

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

FYI, closing the loop: I made the decision to include the payload response on the event; couldn't hurt to have more things to break out by!

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
Expand Down
89 changes: 87 additions & 2 deletions posthog/test/test_feature_flags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1632,9 +1632,10 @@ 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):
patch_decide.return_value = {"featureFlagPayloads": {"person-flag": 300}}
def test_boolean_feature_flag_payload_decide(self, patch_decide, patch_capture):
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"}
Expand All @@ -1649,6 +1650,8 @@ def test_boolean_feature_flag_payload_decide(self, patch_decide):
300,
)
self.assertEqual(patch_decide.call_count, 2)
self.assertEqual(patch_capture.call_count, 1)
Copy link
Contributor Author

@dmarticus dmarticus Nov 20, 2024

Choose a reason for hiding this comment

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

testing that we call capture when calling decide with match_value as None.

patch_capture.reset_mock()

@mock.patch("posthog.client.decide")
def test_multivariate_feature_flag_payloads(self, patch_decide):
Expand Down Expand Up @@ -2334,6 +2337,88 @@ 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):
Comment on lines +2340 to +2342
Copy link
Contributor Author

@dmarticus dmarticus Nov 20, 2024

Choose a reason for hiding this comment

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

full suite of tests confirming that get_feature_flag_payload sends events with actual data in the responses.

patch_decide.return_value = {
"featureFlags": {"person-flag": True},
"featureFlagPayloads": {"person-flag": 300},
}
client = Client(api_key=FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY)

client.feature_flags = [
{
"id": 1,
"name": "Beta Feature",
"key": "person-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="person-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(
"some-distinct-id",
"$feature_flag_called",
{
"$feature_flag": "person-flag",
"$feature_flag_response": True,
"$feature_flag_payload": 300,
"locally_evaluated": False,
"$feature/person-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 because we've already reported an event for this distinct_id + flag
client.get_feature_flag_payload(
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="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": "person-flag",
"$feature_flag_response": True,
"$feature_flag_payload": 300,
"locally_evaluated": False,
"$feature/person-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):
Expand Down
Loading