Disable geoip for capture and decide calls by default #98
Conversation
liyiy
changed the title
posthog/test/test_client.py
Outdated
| @@ -112,7 +112,7 @@ def test_get_active_feature_flags(self, patch_decide): | |||
| } | |||
|
|
|||
| 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) | |||
| variants = client._get_active_feature_variants("some_id", None, None, None, True) | |||
There was a problem hiding this comment.
Here, assert decide was called with the 'right' geoip_disable value.
Also add a similar test for capture, where you're not manually calling client._get_active_feature_variants but let capture call it
This makes for an annoying user experience. I don't think we should have to force users to remember to set this setting, to make sure that their properties behave as expected. Rather, we have very clear priority ordering here: if the user inputs anything, use that; if not, use the defaults. My previous comments in code are working towards this ideology. |
Right, but the default you're asking for is that it disables geoip by default.. so users will still have to manually set geoip_disable to |
| return resp_data["featureFlags"] | ||
|
|
||
| def _get_active_feature_variants(self, distinct_id, groups=None, person_properties=None, group_properties=None): | ||
| feature_variants = self.get_feature_variants(distinct_id, groups, person_properties, group_properties) | ||
| def _get_active_feature_variants( |
There was a problem hiding this comment.
Don't think this works for the case when someone calls posthog.capture(send_feature_flags=True, geoip_disable=False), or is tested yet?
There was a problem hiding this comment.
huh? what exactly would that test? If we're checking to see if setting a method geoip_disable param overrides the init one then the test in test_feature_flags should cover that, right?
There was a problem hiding this comment.
What is the problem/issue here of why posthog.capture(send_feature_flags=True, geoip_disable=False) won't work?
There was a problem hiding this comment.
https://github.com/PostHog/posthog-python/blob/geoip-disable/posthog/client.py#L211
This is separate from test_feature_flags, it's for test_capture. In the line above^, we're not passing in geoip_disable coming from capture, which is why anything the user passes in there is discarded when it comes to _get_active_feature_variants
There was a problem hiding this comment.
oh I thought that would've been fine since it's a capture call that still hits the logic in enqueue for any geoip_disable stuff
There was a problem hiding this comment.
but I guess what you're saying here is the feature flags that returns will be incorrect because geoip_disable isn't passed there, yes?
There was a problem hiding this comment.
correct, so it will rightly not add geoip properties to the captured event, but wrongly add feature flags.
There was a problem hiding this comment.
Can we lead with that next time? 😁 I'm better at working backwards from the initial problem than building up to the problem, if that makes sense?
There was a problem hiding this comment.
Not quite sure I get it. Could you give me another example of what this means?
PostHog/posthog#14351
The
$geoipproperty on server libraries tends to default to incorrect locations (mainly based from where the server is located and not the user) and causes issues such as incorrect event/user data for analytics and problems with feature flag rollouts based on location.This fix prevents the
advanced geoip pluginand thedecidecall in https://github.com/PostHog/posthog/blob/master/posthog/api/decide.py#L144 from overriding user properties with incorrect server library geoip properties by default.Users who are manually passing in geoip values will have to set
geoip_disabletoFalseto ensure their values are not discardedrelated PR: PostHog/posthog#15039