Skip to content
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.

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

feat: add decide field based on env var for rollout of identified_only as default #25662

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

raquelmsmith
Copy link
Member

@raquelmsmith raquelmsmith commented Oct 17, 2024

Problem

We want to roll out identified_only as the default person_processing mode for our stateful SDKs. See: https://github.com/PostHog/product-internal/pull/637

posthog-js changes that consume this field here: PostHog/posthog-js#1468

Changes

Uses an env var to add a boolean to the decide response. If a team ID is greater than the var (I think this is safer to do by using newer team IDs / higher numbers first, since many of them will be using identified_only already), set defaultIdentifiedOnly: true in the response.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

Shouldn't impact self-hosted.

How did you test this code?

Will update tests shortly

Copy link

sentry-io bot commented Oct 17, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: posthog/api/decide.py

Function Unhandled Issue
get_decide TypeError: 'str' object is not a mapping /decide/...
Event Count: 3
get_decide AttributeError: 'NoneType' object has no attribute 'get' /deci...
Event Count: 2

Did you find this useful? React with a 👍 or 👎

@raquelmsmith
Copy link
Member Author

How did this not break any tests? 😰

@dmarticus
Copy link
Contributor

dmarticus commented Oct 17, 2024

How did this not break any tests? 😰

You're not gonna believe this, but in our 5400+ line test_decide.py file, we never actually verify the entire /decide response, we only ever test that keys match specific values (e.g. stuff like this)

self.assertEqual(
    response.json()["featureFlags"],
    {"groups-flag": True, "default-no-prop-group-flag": True},
)

You added a new key to the payload response, but since we never check the full response body anywhere, our tests don't care. I don't love it, but that's why.

@raquelmsmith
Copy link
Member Author

Do you want me to add a test for that? Or just :disappear:

@dmarticus
Copy link
Contributor

dmarticus commented Oct 17, 2024

Do you want me to add a test for that? Or just :disappear:

How long is this personless state intended to exist? I think there's limited value in adding a test for this payload if you're just gonna delete it in a few days anyway. Up to you, though.

My only thought is to consider not setting the defaultIdentifiedOnly payload at all if it doesn't exist in the settings; it's already an optional value, so why bother including it in every response if it's not relevant in most cases. Most consumers of /decide don't really care about that field.

@raquelmsmith
Copy link
Member Author

I'd like it to be true/false explicitly so I'll leave it in for everyone. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants