-
Notifications
You must be signed in to change notification settings - Fork 517
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(flags): add Unleash feature flagging integration #3888
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #3888 +/- ##
==========================================
+ Coverage 79.83% 79.87% +0.03%
==========================================
Files 139 140 +1
Lines 15419 15451 +32
Branches 2623 2624 +1
==========================================
+ Hits 12310 12341 +31
- Misses 2235 2237 +2
+ Partials 874 873 -1
|
…eeded. Ref tests back to 1 file
Hey @aliu39 ! Thanks for the integration. This will be reviewed after the holiday season. |
Todo:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mutating class attributes in the __init__
method violates encapsulation. There are a whole category of problems which come from this. I've removed it and replaced with by mutating the UnleashClient class directly. I noticed in the tests you were testing for isolation between instances. While that's interesting it isn't the goal. We want to inject ourselves into every instance or none. This follows the behavior of our other integrations.
Also I've reverted the changes in conftest.py
. There was nothing wrong with them. They are just out of scope for this unit of work. Feel free to do those in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking this. We're going to talk about how we handle this going forward and may close this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay we're all good. We're going to keep this setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Nice integrations, and GREAT test suite! Thanks for that!
Follow-up to #3888 The original PR patched 2 methods used for evaluating feature flags, `is_enabled` (simple toggle on/off) and `get_variant` (returns a dict of metadata, see https://docs.getunleash.io/reference/sdks/python#getting-a-variant). We want to remove all `get_variant` code since we only support boolean flag evals atm. It seems like the main usecase for variants is reading payloads (non-bool) for A/B/multivariate testing. This could lead to a lot of extraneous flags, so until it is requested and/or we support non-bool values, let's not patch this method.
Adds an integration for tracking flag evaluations from Unleash customers.
Implementation
Unleash has no native support for evaluation hooks/listeners, unless the user opts in for each flag. Therefore we decided to patch the
is_enabled
andget_variant
methods on theUnleashClient
class. The methods are wrapped and the only side effect is writing to Sentry scope, so users shouldn't see any change in behavior.We patch one
UnleashClient
instance instead of the whole class. The reasons for this are described inIt's also safer to not modify the unleash import.
References
Documentation PR