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

fix(flags): handle deleted flags and some property matching edge cases #28308

Merged
merged 6 commits into from
Feb 4, 2025

Conversation

dmarticus
Copy link
Contributor

@dmarticus dmarticus commented Feb 4, 2025

Context

I wrote a test harness for new flags that went like this:

  • pull a list of flag filters from production that cover what "real flag" filter conditions look like
  • use those filters + the PostHog API to create a bunch of flags locally.
  • then, create a bunch of people in my local PostHog that have properties that explicitly match to true for all of the conditions
  • loop through these flag keys and call is_feature_enabled on each flag key with the corresponding person that has matching properties (I used the python SDK because it's easiest to run locally)
  • change the python SDK to use /flags instead of /decide.
  • run the same tests and diff the results
  • finally, to test flag evaluation for arbitrary distinct_ids, diff the results of the following API requests
curl -v -L --header "Content-Type: application/json" -d '  {
    "api_key": "phc_OVSXQcVMyrNTwErxmLNT21Hb1nzTBKnaMDpbPZkgYyE",
    "distinct_id": "imported_flag_20250131141135_421_person_user",
    "groups" : {}
}' "http://localhost:8010/decide?v=3" | jq '.featureFlags'
curl -v -L --header "Content-Type: application/json" -d '  {
    "api_key": "phc_OVSXQcVMyrNTwErxmLNT21Hb1nzTBKnaMDpbPZkgYyE",
    "distinct_id": "imported_flag_20250131141135_421_person_user",
    "groups" : {}
}' "http://localhost:8010/flags?v=3" | jq '.featureFlags'

And confirm that the responses are the same for each.

In doing this, I found the following issues with new flags:

  • if there was a good key with an unparseable value in redis, the request short-circuited. I handled bad values as if the keys didn't exist, so fell back to postgres
  • for all flag properties with exclusive conditions (e.g. not_icontains, is_not, and not_regex) for a given property (e.g. email), I was evaluating people that were missing the property altogether as false, rather than true (which is what /decide does).

@posthog-bot
Copy link
Contributor

Hey @dmarticus! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR improves feature flag handling by focusing on deleted flags, property matching edge cases, and enhanced logging for better debugging capabilities.

  • Added debug-level logging in feature-flags service and Redis operations to improve observability during development and troubleshooting
  • Fixed critical bug in Redis client where pickle deserialization is performed twice unnecessarily
  • Modified SQL query in flag_operations.rs to filter out deleted/inactive flags at database level with deleted = false AND active = true conditions
  • Made filters field optional in Cohort struct to properly handle null/undefined cases in cohort definitions
  • Enhanced property matching logic for inverted operators (IsNot, NotIcontains, NotRegex) to return true when properties don't exist, making behavior more predictable

10 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

rust/feature-flags/src/client/redis.rs Outdated Show resolved Hide resolved
rust/feature-flags/src/client/redis.rs Outdated Show resolved Hide resolved
rust/feature-flags/src/client/redis.rs Outdated Show resolved Hide resolved
rust/feature-flags/src/cohort/cohort_operations.rs Outdated Show resolved Hide resolved
"properties": [
{
"key": "email",
"value": ".*@posthog\\.com$",
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The regex pattern '.@posthog.com$' could be simplified to '@posthog.com$' since the . at the start is redundant

Suggested change
"value": ".*@posthog\\.com$",
"value": "@posthog\\.com$",

@dmarticus dmarticus merged commit cf528df into master Feb 4, 2025
84 checks passed
@dmarticus dmarticus deleted the fix/beating-the-shit-out-of-flags branch February 4, 2025 23:57
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.

3 participants