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

Allow rolling feature flags out to multiple groups #3030

Merged
merged 8 commits into from
Jan 25, 2021
Merged

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Jan 21, 2021

Changes

This makes it possible to roll out feature flags in several groups. #3007

Since feature flags are used by /decide endpoint, just doing a data migration would mean dropped requests while new servers are spinning up => I added application-level logic for handling that. This logic can be removed/migrated at a later date.

2021-01-21_13-01
2021-01-21_13-00

Commits

  • Support multiple groups in feature flags model
  • Update analytics_metadata method
  • Allow defining multiple groups in frontend
  • Allow sorting
  • Update tests, allow submit without filters.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

This will be part of #3007

I did not do a migration since it would cause errors for decide endpoint
once migrations have run but new schema not up.
@macobo macobo temporarily deployed to posthog-feature-flags-g-pyuqv7 January 21, 2021 11:02 Inactive
@macobo macobo temporarily deployed to posthog-feature-flags-g-pyuqv7 January 21, 2021 12:20 Inactive
@macobo macobo requested a review from EDsCODE January 21, 2021 13:24
@macobo macobo temporarily deployed to posthog-feature-flags-g-pyuqv7 January 21, 2021 13:24 Inactive
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

A small UI consideration would be how these groups are represented in the table. The first feature flag is a 2 group feature flag but the rollout percentages aren't shown and the filters aren't clearly defined with an OR relationship. The second feature flag is just a single group with filters AND'd together

Screen Shot 2021-01-21 at 9 35 00 AM

posthog/api/feature_flag.py Show resolved Hide resolved
@macobo
Copy link
Contributor Author

macobo commented Jan 21, 2021

Good point re UX - not sure how to best represent it and release percentage within the table without getting too busy.

Suggestions/thoughts? :)

@EDsCODE
Copy link
Member

EDsCODE commented Jan 21, 2021

Two options come to mind:

  1. low hanging fruit is just to add the or/and tag in between filters and then iterate in order the percentages in the percentage column. This would cause some annoying cognitive load because you'd have to match up the group to the percentage
  2. filters and rollout percentages should just be a single column called filters. Each filter is accompanied by a rollout percentage tag. Then you put the or/and between the filters

@macobo macobo temporarily deployed to posthog-feature-flags-g-pyuqv7 January 22, 2021 07:50 Inactive
@macobo
Copy link
Contributor Author

macobo commented Jan 22, 2021

I tried. Getting all the filters displaying in the table just made the information density too high, went a different route:

2021-01-22_09-51

@macobo macobo temporarily deployed to posthog-feature-flags-g-pyuqv7 January 22, 2021 08:01 Inactive
@EDsCODE
Copy link
Member

EDsCODE commented Jan 22, 2021

this is a lot better than the previous columns. What was immediately still confusing was that the percentage is shown in the first row when there's a single filter but it disappears when there are more filters such as in row 3 even though i'm assuming those are and'd together in the same group?

@macobo
Copy link
Contributor Author

macobo commented Jan 25, 2021

this is a lot better than the previous columns. What was immediately still confusing was that the percentage is shown in the first row when there's a single filter but it disappears when there are more filters such as in row 3 even though i'm assuming those are and'd together in the same group?

Row 3 has no rollout percentage defined, which is why we don't show it.

Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

Got it. I'm good with this!

@macobo macobo merged commit da92f0b into master Jan 25, 2021
@macobo macobo deleted the feature-flags-groups branch January 25, 2021 23:18
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

Oops, seems like I forgot to submit my review yesterday. Anyways think these comments might be useful for a subsequent PR.

Commenting only on the UI/UX, here are my 2c,

  • This UI seems quite cluttered and hard to understand even on the best of circumstances.
    Screenshot 2021-01-25 at 6 25 42 PM
    I would suggest trying something like this, where we can still provide very useful information in the same page while making it clear what each group is comprised of. But this might entail updating all similar filtering related components,

  • The add group button is hard to find, and it's not use is not very intuitive. In addition, it's not standardized with how we add match groups elsewhere in the app. Would suggest taking a look at actions or cohorts.

  • I would definitely add the "OR" tag (as we do on actions) to make it extra clear how the match groups are referenced. Additionally the close button is also a bit hard to see (particularly the first times) and doesn't match the actions UI either. To be fair it does match the cohorts UI a bit more, but I think it might be worth leaning towards the actions approach.

Let me know if you want me to take on the UI improvements. I would need a couple of days to carve out some time for this but happy to do it.

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.

None yet

3 participants