feat(feature-flags): Enable local evaluation of flags #68
Conversation
|
Update:
|
|
Thanks! I was planning to not have any deprecated options in the library, since we control the API which returns flag definitions, and we can transform all deprecated ones into new ones. Effectively, I want to deprecate |
|
That would probs imply a major version update, so self-hosted know not to upgrade unless they're on PostHog version 1.38, but I think that's a good trade-off to make, since it means a little less code in every client library, and a clean cut w.r.t the API endpoints used to support local eval. (see main PostHog PR, very much WIP: PostHog/posthog#10957 ) |
posthog/client.py
Outdated
| flag_filters = feature_flag.get("filters") or {} | ||
| aggregation_group_type_index = flag_filters.get("aggregation_group_type_index") | ||
| if aggregation_group_type_index is not None: | ||
| focused_group_properties = group_properties[ |
There was a problem hiding this comment.
This is tricky. We should hash out how we expect users to add properties here.
I think what you're doing makes sense, specially when different groups can have the same name property. However at the same time, feature flags support a single group at a time, so we'd expect only properties from a single group per call..
This is more future proof, but also a bit annoying for current user needs.
There was a problem hiding this comment.
EIther works for me. When only one group is in focus, how does this function know which group is being used?
There was a problem hiding this comment.
aggregation_group_type_index ? Not sure I get the question
There was a problem hiding this comment.
Oh, so upon calling posthog.get_feature_flag() you're assumed to already know what group type the flag in question is, so you only pass in the group properties related to that group?
There was a problem hiding this comment.
Yeah pretty much, since they're the ones who defined the flag
There was a problem hiding this comment.
Ah, thought about this a bit more, and I think your format makes sense, since we'll want to send these properties to /decide as well, when we do fallback, so that these props can be overridden.
There was a problem hiding this comment.
And /decide evaluates multiple flags, and we don't know which of those have which groups
| for consumer in client.consumers: | ||
| self.assertEqual(consumer.timeout, 15) | ||
|
|
||
| @mock.patch("posthog.client.Poller") | ||
| @mock.patch("posthog.client.get") | ||
| def test_load_feature_flags(self, patch_get, patch_poll): |
There was a problem hiding this comment.
these moved to test_feature_flags.py
|
A better handle on what the tests are doing: PostHog/posthog#10459 (comment) (scroll to the bottom) - I've added tests we'd need in common between all libraries |
|
|
||
| @mock.patch("posthog.client.decide") | ||
| @mock.patch("posthog.client.get") | ||
| def test_feature_enabled_simple(self, patch_get, patch_decide): |
There was a problem hiding this comment.
this and following tests were copied over from the client, so not necessary to write again in every other lib (these libs should have their own old tests 🤷).
Following spec from PostHog/posthog#10459
The PR isn't really that big, ~3,000 LoC are tests, 2,000 of which are autogenerated consistency tests.
(but still big enough, sigh). Wanted to figure out exactly what all we need for the new library to be feature complete, and use this as a template for other libraries to follow.