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

Monkeypatch Flipper to provide a consistent control group #3434

Merged
merged 1 commit into from Mar 25, 2017

Conversation

@nilbus
Copy link

@nilbus nilbus commented Mar 25, 2017

This is for the exercism/discussions#123 experiment.

Override to consider only the actor value (GitHub username), not the feature name, to provide a consistent control group across features, rather than a different 50% for each feature.

This new test fails without the monkey patch. Considering the boolean output and the random-like nature of Zlib.crc32 output (the hashing algorithm used), 250 iterations should more than cover it. The test completes in 40ms on my machine.

Override to consider only the actor value (GitHub username), not the
feature name, to provide a consistent control group across features,
rather than a different 50% for each feature.

This new test fails without the monkey patch. Considering the boolean
output and the random-like nature of Zlib.crc32 output ([the hashing
algorithm used][]), 250 iterations should more than cover it. The test
finishes in 40ms on my machine.

[the hashing algorithm used]: https://github.com/jnunemaker/flipper/blob/v0.10.2/lib/flipper/gates/percentage_of_actors.rb#L33
@nilbus
Copy link
Author

@nilbus nilbus commented Mar 25, 2017

This could be removed after the experiment completes. Keeping it would enable future wider-scale control groups across feature flags.

@kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented Mar 25, 2017

Have you considered submitting a patch to the flipper library? It seems like it could be useful to have a "suite" of features that are all enabled together.

@kytrinyx kytrinyx merged commit 3a8b078 into exercism:master Mar 25, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 85.464%
Details
@kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented Mar 25, 2017

This looks great, btw 🌻 - I was just waiting for CI to run.

@nilbus
Copy link
Author

@nilbus nilbus commented Mar 25, 2017

Good idea. I'll give this some thought and talk with the author.

@nilbus nilbus deleted the consistent-control-group branch Mar 25, 2017
@nilbus
Copy link
Author

@nilbus nilbus commented Mar 25, 2017

BTW, because of this issue, I enabled the advertise_cli_update feature for all users. It won't hurt the control group to update.

@kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented Mar 25, 2017

Yeah, we definitely want everyone to have the newest version.

@kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented Mar 25, 2017

I'll give this some thought and talk with the author.

The author is one of my colleagues, so if you want me to chat with them about it casually, I could totally do that.

@nilbus
Copy link
Author

@nilbus nilbus commented Mar 25, 2017

Cool, thanks. I totally didn't realize that @jnunemaker is a Githubber too. He just merged one of my PRs today, so I know he's responsive. Maybe we can just pull him in here. 😄

@jnunemaker We're performing an experimental study on exercism.io with control and experimental groups, and we're using flipper's Percentage of Actors gate to separate the two groups. This would work great out-of-the-box if we used a single flipper feature to manage the entire experiment, but we wanted the ability to turn off exercism features individually, in case of problems in production with some new exercism feature, without disabling all of the experimental features. Since the key/id used in each flipper feature's percentage of actors gate contains the feature name, the group selected for each feature will differ. In order to have a consistent 50% control group across all flipper features, this PR monkeypatches that key/id to instead contain only the actor value.

@kytrinyx and I wanted to brainstorm about how flipper might be able to have a "suite" of features that select a common percentage of actors within that suite. This could take the form of grouping features, but since the Percentage of Actors gate is really the only one that this applies to, it could could take the form of a new gate that is similar to the Percentage of Actors gate—Consistent Percentage of Actors vs Random Percentage of Actors? Or better yet, a configuration option on the Percentage of Actors gate that excludes the feature name from affecting the actors selected? I'd love to hear your thoughts on this.

@nilbus
Copy link
Author

@nilbus nilbus commented Mar 25, 2017

…or perhaps a custom string that overrides or replaces the portion of the key/id that is currently the feature name. Then it could be set arbitrarily to form multiple named suites of features.

@jnunemaker
Copy link

@jnunemaker jnunemaker commented Mar 31, 2017

👋 Thanks for the ping. So neat to see flipper being used here. Always exciting to see new uses that push flipper to the limit.

I would not recommend overriding Flipper's default behavior as that could be confusing for others down the road. I created an example for you of one way to achieve the same result without monkey patching flipper. What do you think of that? Happy to hear your thoughts either way.

Have you considered submitting a patch to the flipper library? It seems like it could be useful to have a "suite" of features that are all enabled together.

This is an interesting idea. I don't have any good ideas off hand for something baked into flipper, but the experimental group thing I added above would be one way to do it currently. For each suite, you could have a group.

@nilbus
Copy link
Author

@nilbus nilbus commented Mar 31, 2017

@kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented Apr 1, 2017

Rad! The experimental group is exactly what we need. No need for the "suite" feature. Thanks @jnunemaker

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

Successfully merging this pull request may close these issues.

None yet

3 participants