Unleash / unleash-client-python

Unleash client SDK for Python 💡💡💡
http://unleash.github.io/unleash-client-python
MIT License
84 stars 60 forks source link

fix: avoid double call to Strategy.execute() to find out if a feature is enabled #288

Closed ClementDelgrange closed 11 months ago

ClementDelgrange commented 11 months ago

Description

This commit https://github.com/Unleash/unleash-client-python/commit/0e7b894851c5a9bec69e9926fcc07a87250fb958 introduced a new method: Feature._get_evaluation_result(context) -> EvaluationResult. This method is used for instance to determine whether a feature is enabled or not. If there are strategies associated to the feature, this method makes a call to strategy.execute(context) and another to enabled_strategy.get_result(context) that itself calls self.execute(context). In the end, Strategy.execute() is called two times which is an issue in the case of a GradualRolloutRandom because a new random integer is compared each time to the rollout percentage.

Steps to follow to highlight the issue:

api_token = "..." client = UnleashClient( url="https://app.unleash-hosted.com/demo/api/", app_name="my_python_app", custom_headers={"Authorization": api_token} ) client.initialize_client()

total_enabled = sum(client.is_enabled("f-random-stickiness") for i in range(1000))

print(total_enabled) # 228



The change in this pull request proposes to remove the double call to `Strategy.execute()`.

## Type of change

Please delete options that are not relevant.

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

- [x] Unit tests
- [ ] Spec Tests
- [x] Integration tests / Manual Tests  (manual test with the above snippet)

# Checklist:

- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged and published in downstream modules
coveralls commented 11 months ago

Coverage Status

coverage: 96.898% (+0.006%) from 96.892% when pulling 8b9c274f60749cdd4b38bcffb5cf04404eeff913 on ClementDelgrange:main into 0cc36b0ccf20857b9681058d0bb44c5f7847a0bd on Unleash:main.

ClementDelgrange commented 11 months ago

Hello @kwasniew @sighphyre , I saw you were active recently on other pull requests. Is there any chance you could take a look at this one? Please let me know if you need more information or if you'd like me to open an issue to discuss the problem I'm facing.

kwasniew commented 11 months ago

Hey @ClementDelgrange we'll try to handle this PR soon.