From 0e7b894851c5a9bec69e9926fcc07a87250fb958 Mon Sep 17 00:00:00 2001 From: andreas-unleash Date: Thu, 3 Aug 2023 11:56:36 +0300 Subject: [PATCH] feat: strategy variants (#265) * feat: strategy variants * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix linter * add unit tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add unit tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * lint * cleanup * cleanup * cleanup * cleanup * fix linter complaints * fix linter complaints * fix linter complaints * fix logic * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * formatting * improvements * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * bug fixes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove constraint noise from test * fix tests * fix tests * fix ruff complaining * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix PR comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- UnleashClient/__init__.py | 3 +- UnleashClient/constants.py | 2 +- UnleashClient/constraints/Constraint.py | 7 +- UnleashClient/deprecation_warnings.py | 2 +- UnleashClient/features/Feature.py | 64 +++++++++++-------- UnleashClient/loader.py | 6 ++ UnleashClient/strategies/RemoteAddress.py | 7 +- UnleashClient/strategies/Strategy.py | 30 ++++++++- UnleashClient/strategies/__init__.py | 2 +- UnleashClient/variants/Variants.py | 23 ++++--- .../specification_tests/test_client_specs.py | 43 ++++++++----- tests/unit_tests/api/test_feature.py | 2 +- .../strategies/test_strategy_variants.py | 54 ++++++++++++++++ tests/unit_tests/test_client.py | 3 +- tests/unit_tests/test_features.py | 40 +++++++++++- 15 files changed, 221 insertions(+), 67 deletions(-) create mode 100644 tests/unit_tests/strategies/test_strategy_variants.py diff --git a/UnleashClient/__init__.py b/UnleashClient/__init__.py index 8cd53381..19ec8a30 100644 --- a/UnleashClient/__init__.py +++ b/UnleashClient/__init__.py @@ -469,7 +469,8 @@ def get_variant(self, feature_name: str, context: Optional[dict] = None) -> dict self.features[feature_name] = new_feature # Use the feature's get_variant method to count the call - return new_feature.get_variant(context) + variant_check = new_feature.get_variant(context) + return variant_check else: LOGGER.log( self.unleash_verbose_log_level, diff --git a/UnleashClient/constants.py b/UnleashClient/constants.py index 68c7ed43..fc59dbfc 100644 --- a/UnleashClient/constants.py +++ b/UnleashClient/constants.py @@ -6,7 +6,7 @@ REQUEST_TIMEOUT = 30 REQUEST_RETRIES = 3 METRIC_LAST_SENT_TIME = "mlst" -CLIENT_SPEC_VERSION = "4.2.2" +CLIENT_SPEC_VERSION = "4.3.1" # =Unleash= APPLICATION_HEADERS = { diff --git a/UnleashClient/constraints/Constraint.py b/UnleashClient/constraints/Constraint.py index 22fb235e..764fad28 100644 --- a/UnleashClient/constraints/Constraint.py +++ b/UnleashClient/constraints/Constraint.py @@ -226,10 +226,9 @@ def apply(self, context: dict = None) -> bool: constraint_check = self.check_semver_operators( context_value=context_value ) - else: - # This is a special case in the client spec - so it's getting it's own handler here - if self.operator is ConstraintOperators.NOT_IN: # noqa: PLR5501 - constraint_check = True + # This is a special case in the client spec - so it's getting it's own handler here + elif self.operator is ConstraintOperators.NOT_IN: # noqa: PLR5501 + constraint_check = True except Exception as excep: # pylint: disable=broad-except LOGGER.info( diff --git a/UnleashClient/deprecation_warnings.py b/UnleashClient/deprecation_warnings.py index 8b661ec0..1bd3b3cb 100644 --- a/UnleashClient/deprecation_warnings.py +++ b/UnleashClient/deprecation_warnings.py @@ -10,7 +10,7 @@ def strategy_v2xx_deprecation_check(strategies: list) -> None: for strategy in strategies: try: # Check if the __call__() method is overwritten (should only be true for custom strategies in v1.x or v2.x. - if strategy.__call__ != Strategy.__call__: + if strategy.__call__ != Strategy.__call__: # type:ignore warnings.warn( f"unleash-client-python v3.x.x requires overriding the execute() method instead of the __call__() method. Error in: {strategy.__name__}", DeprecationWarning, diff --git a/UnleashClient/features/Feature.py b/UnleashClient/features/Feature.py index e16143d1..f1953bf9 100644 --- a/UnleashClient/features/Feature.py +++ b/UnleashClient/features/Feature.py @@ -1,7 +1,9 @@ # pylint: disable=invalid-name +import copy from typing import Dict, Optional, cast from UnleashClient.constants import DISABLED_VARIATION +from UnleashClient.strategies import EvaluationResult, Strategy from UnleashClient.utils import LOGGER from UnleashClient.variants import Variants @@ -73,33 +75,16 @@ def _count_variant(self, variant_name: str) -> None: """ self.variant_counts[variant_name] = self.variant_counts.get(variant_name, 0) + 1 - def is_enabled( - self, context: dict = None, default_value: bool = False - ) -> bool: # pylint: disable=unused-argument + def is_enabled(self, context: dict = None) -> bool: """ Checks if feature is enabled. :param context: Context information - :param default_value: Deprecated! Users should use the fallback_function on the main is_enabled() method. :return: """ - flag_value = False + evaluation_result = self._get_evaluation_result(context) - if self.enabled: - try: - if self.strategies: - strategy_result = any(x.execute(context) for x in self.strategies) - else: - # If no strategies are present, should default to true. This isn't possible via UI. - strategy_result = True - - flag_value = strategy_result - except Exception as strategy_except: - LOGGER.warning("Error checking feature flag: %s", strategy_except) - - self.increment_stats(flag_value) - - LOGGER.info("Feature toggle status for feature %s: %s", self.name, flag_value) + flag_value = evaluation_result.enabled return flag_value @@ -110,19 +95,46 @@ def get_variant(self, context: dict = None) -> dict: :param context: Context information :return: """ - variant = DISABLED_VARIATION - is_feature_enabled = self.is_enabled(context) - - if is_feature_enabled and self.variants is not None: + evaluation_result = self._get_evaluation_result(context) + is_feature_enabled = evaluation_result.enabled + variant = evaluation_result.variant + if variant is None or (is_feature_enabled and variant == DISABLED_VARIATION): try: - variant = self.variants.get_variant(context) - variant["enabled"] = is_feature_enabled + LOGGER.debug("Getting variant from feature: %s", self.name) + variant = ( + self.variants.get_variant(context, is_feature_enabled) + if is_feature_enabled + else copy.deepcopy(DISABLED_VARIATION) + ) + except Exception as variant_exception: LOGGER.warning("Error selecting variant: %s", variant_exception) self._count_variant(cast(str, variant["name"])) return variant + def _get_evaluation_result(self, context: dict = None) -> EvaluationResult: + strategy_result = EvaluationResult(False, None) + if self.enabled: + try: + if self.strategies: + enabled_strategy: Strategy = next( + (x for x in self.strategies if x.execute(context)), None + ) + if enabled_strategy is not None: + strategy_result = enabled_strategy.get_result(context) + + else: + # If no strategies are present, should default to true. This isn't possible via UI. + strategy_result = EvaluationResult(True, None) + + except Exception as evaluation_except: + LOGGER.warning("Error getting evaluation result: %s", evaluation_except) + + self.increment_stats(strategy_result.enabled) + LOGGER.info("%s evaluation result: %s", self.name, strategy_result) + return strategy_result + @staticmethod def metrics_only_feature(feature_name: str): feature = Feature(feature_name, False, []) diff --git a/UnleashClient/loader.py b/UnleashClient/loader.py index 40711585..187fff91 100644 --- a/UnleashClient/loader.py +++ b/UnleashClient/loader.py @@ -33,9 +33,15 @@ def _create_strategies( else: segment_provisioning = [] + if "variants" in strategy.keys(): + variant_provisioning = strategy["variants"] + else: + variant_provisioning = [] + feature_strategies.append( strategy_mapping[strategy["name"]]( constraints=constraint_provisioning, + variants=variant_provisioning, parameters=strategy_provisioning, global_segments=global_segments, segment_ids=segment_provisioning, diff --git a/UnleashClient/strategies/RemoteAddress.py b/UnleashClient/strategies/RemoteAddress.py index 940c5590..f5099b6c 100644 --- a/UnleashClient/strategies/RemoteAddress.py +++ b/UnleashClient/strategies/RemoteAddress.py @@ -61,9 +61,8 @@ def apply(self, context: dict = None) -> bool: if context_ip == addr_or_range: return_value = True break - else: - if context_ip in addr_or_range: # noqa: PLR5501 - return_value = True - break + elif context_ip in addr_or_range: # noqa: PLR5501 + return_value = True + break return return_value diff --git a/UnleashClient/strategies/Strategy.py b/UnleashClient/strategies/Strategy.py index 83df64ed..eceb3787 100644 --- a/UnleashClient/strategies/Strategy.py +++ b/UnleashClient/strategies/Strategy.py @@ -1,8 +1,16 @@ # pylint: disable=invalid-name,dangerous-default-value import warnings -from typing import Iterator +from dataclasses import dataclass +from typing import Iterator, Optional from UnleashClient.constraints import Constraint +from UnleashClient.variants import Variants + + +@dataclass +class EvaluationResult: + enabled: bool + variant: Optional[dict] class Strategy: @@ -15,6 +23,7 @@ class Strategy: - ``apply()`` - Your feature provisioning :param constraints: List of 'constraints' objects derived from strategy section (...from feature section) of `/api/clients/features` Unleash server response. + :param variants: List of 'variant' objects derived from strategy section (...from feature section) of `/api/clients/features` Unleash server response. :param parameters: The 'parameter' objects from the strategy section (...from feature section) of `/api/clients/features` Unleash server response. """ @@ -24,9 +33,11 @@ def __init__( parameters: dict = {}, segment_ids: list = None, global_segments: dict = None, + variants: list = None, ) -> None: self.parameters = parameters self.constraints = constraints + self.variants = variants or [] self.segment_ids = segment_ids or [] self.global_segments = global_segments or {} self.parsed_provisioning = self.load_provisioning() @@ -56,6 +67,15 @@ def execute(self, context: dict = None) -> bool: return flag_state + def get_result(self, context) -> EvaluationResult: + enabled = self.execute(context) + variant = None + if enabled: + variant = self.parsed_variants.get_variant(context, enabled) + + result = EvaluationResult(enabled, variant) + return result + @property def parsed_constraints(self) -> Iterator[Constraint]: for constraint_dict in self.constraints: @@ -66,6 +86,14 @@ def parsed_constraints(self) -> Iterator[Constraint]: for constraint in segment["constraints"]: yield Constraint(constraint_dict=constraint) + @property + def parsed_variants(self) -> Variants: + return Variants( + variants_list=self.variants, + group_id=self.parameters.get("groupId"), + is_feature_variants=False, + ) + def load_provisioning(self) -> list: # pylint: disable=no-self-use """ Loads strategy provisioning from Unleash feature flag configuration. diff --git a/UnleashClient/strategies/__init__.py b/UnleashClient/strategies/__init__.py index d7db4549..b5f68eff 100644 --- a/UnleashClient/strategies/__init__.py +++ b/UnleashClient/strategies/__init__.py @@ -6,5 +6,5 @@ from .GradualRolloutSessionId import GradualRolloutSessionId from .GradualRolloutUserId import GradualRolloutUserId from .RemoteAddress import RemoteAddress -from .Strategy import Strategy +from .Strategy import EvaluationResult, Strategy from .UserWithId import UserWithId diff --git a/UnleashClient/variants/Variants.py b/UnleashClient/variants/Variants.py index 20eba7d0..de54ef8e 100644 --- a/UnleashClient/variants/Variants.py +++ b/UnleashClient/variants/Variants.py @@ -1,21 +1,24 @@ # pylint: disable=invalid-name, too-few-public-methods import copy import random -from typing import Dict # noqa: F401 +from typing import Dict, Optional # noqa: F401 from UnleashClient import utils from UnleashClient.constants import DISABLED_VARIATION class Variants: - def __init__(self, variants_list: list, feature_name: str) -> None: + def __init__( + self, variants_list: list, group_id: str, is_feature_variants: bool = True + ) -> None: """ Represents an A/B test variants_list = From the strategy document. """ self.variants = variants_list - self.feature_name = feature_name + self.group_id = group_id + self.is_feature_variants = is_feature_variants def _apply_overrides(self, context: dict) -> dict: """ @@ -60,20 +63,23 @@ def _get_seed(context: dict, stickiness_selector: str = "default") -> str: return seed @staticmethod - def _format_variation(variation: dict) -> dict: + def _format_variation(variation: dict, flag_status: Optional[bool] = None) -> dict: formatted_variation = copy.deepcopy(variation) del formatted_variation["weight"] if "overrides" in formatted_variation: del formatted_variation["overrides"] if "stickiness" in formatted_variation: del formatted_variation["stickiness"] + if "enabled" not in formatted_variation and flag_status is not None: + formatted_variation["enabled"] = flag_status return formatted_variation - def get_variant(self, context: dict) -> dict: + def get_variant(self, context: dict, flag_status: Optional[bool] = None) -> dict: """ Determines what variation a user is in. :param context: + :param flag_status: :return: """ fallback_variant = copy.deepcopy(DISABLED_VARIATION) @@ -81,7 +87,7 @@ def get_variant(self, context: dict) -> dict: if self.variants: override_variant = self._apply_overrides(context) if override_variant: - return self._format_variation(override_variant) + return self._format_variation(override_variant, flag_status) total_weight = sum(x["weight"] for x in self.variants) if total_weight <= 0: @@ -92,9 +98,10 @@ def get_variant(self, context: dict) -> dict: if "stickiness" in self.variants[0].keys() else "default" ) + target = utils.normalized_hash( self._get_seed(context, stickiness_selector), - self.feature_name, + self.group_id, total_weight, ) counter = 0 @@ -102,7 +109,7 @@ def get_variant(self, context: dict) -> dict: counter += variation["weight"] if counter >= target: - return self._format_variation(variation) + return self._format_variation(variation, flag_status) # Catch all return. return fallback_variant diff --git a/tests/specification_tests/test_client_specs.py b/tests/specification_tests/test_client_specs.py index 105eba35..8ca0afc2 100644 --- a/tests/specification_tests/test_client_specs.py +++ b/tests/specification_tests/test_client_specs.py @@ -27,29 +27,43 @@ def load_specs(): return json.load(_f) +def get_client(state, test_context=None): + cache = FileCache("MOCK_CACHE") + cache.bootstrap_from_dict(state) + env = "default" + if test_context is not None and "environment" in test_context: + env = test_context["environment"] + + unleash_client = UnleashClient( + url=URL, + app_name=APP_NAME, + instance_id="pytest_%s" % uuid.uuid4(), + disable_metrics=True, + disable_registration=True, + cache=cache, + environment=env, + ) + + unleash_client.initialize_client(fetch_toggles=False) + return unleash_client + + def iter_spec(): for spec in load_specs(): name, state, tests, variant_tests = load_spec(spec) - cache = FileCache("MOCK_CACHE") - cache.bootstrap_from_dict(state) - - unleash_client = UnleashClient( - url=URL, - app_name=APP_NAME, - instance_id="pytest_%s" % uuid.uuid4(), - disable_metrics=True, - disable_registration=True, - cache=cache, - ) - - unleash_client.initialize_client(fetch_toggles=False) + unleash_client = get_client(state=state) for test in tests: yield name, test["description"], unleash_client, test, False for variant_test in variant_tests: - yield name, test["description"], unleash_client, variant_test, True + test_context = {} + if "context" in variant_test: + test_context = variant_test["context"] + + unleash_client = get_client(state, test_context) + yield name, variant_test["description"], unleash_client, variant_test, True unleash_client.destroy() @@ -77,6 +91,5 @@ def test_spec(spec): toggle_name = test_data["toggleName"] expected = test_data["expectedResult"] context = test_data.get("context") - variant = unleash_client.get_variant(toggle_name, context) assert variant == expected diff --git a/tests/unit_tests/api/test_feature.py b/tests/unit_tests/api/test_feature.py index e33b672a..16af187a 100644 --- a/tests/unit_tests/api/test_feature.py +++ b/tests/unit_tests/api/test_feature.py @@ -90,7 +90,7 @@ def test_get_feature_toggle_failed_etag(): @pytest.mark.skipif( - date.today() < date(2023, 7, 1), + date.today() < date(2023, 9, 1), reason="This is currently breaking due to a dependency or the test setup. Skipping this allows us to run tests in CI without this popping up as an error all the time.", ) @responses.activate diff --git a/tests/unit_tests/strategies/test_strategy_variants.py b/tests/unit_tests/strategies/test_strategy_variants.py new file mode 100644 index 00000000..927a4d54 --- /dev/null +++ b/tests/unit_tests/strategies/test_strategy_variants.py @@ -0,0 +1,54 @@ +import pytest + +from tests.utilities.mocks.mock_variants import VARIANTS_WITH_STICKINESS +from UnleashClient.strategies import EvaluationResult, FlexibleRollout + +BASE_FLEXIBLE_ROLLOUT_DICT = { + "name": "flexibleRollout", + "parameters": {"rollout": 50, "stickiness": "userId", "groupId": "AB12A"}, + "variants": VARIANTS_WITH_STICKINESS, + "constraints": [ + {"contextName": "environment", "operator": "IN", "values": ["staging", "prod"]}, + {"contextName": "userId", "operator": "IN", "values": ["122", "155", "9"]}, + {"contextName": "userId", "operator": "NOT_IN", "values": ["4"]}, + {"contextName": "appName", "operator": "IN", "values": ["test"]}, + ], +} + + +@pytest.fixture() +def strategy(): + yield FlexibleRollout( + BASE_FLEXIBLE_ROLLOUT_DICT["constraints"], + BASE_FLEXIBLE_ROLLOUT_DICT["parameters"], + variants=VARIANTS_WITH_STICKINESS, + ) + + +def test_flexiblerollout_satisfies_constraints_returns_variant(strategy): + context = { + "userId": "122", + "appName": "test", + "environment": "prod", + "customField": "1", + } + result: EvaluationResult = strategy.get_result(context) + assert result.enabled + assert result.variant == { + "enabled": True, + "name": "VarB", + "payload": {"type": "string", "value": "Test 2"}, + } + + +def test_flexiblerollout_does_not_satisfy_constraints_returns_default_variant(strategy): + context = { + "userId": "12234", + "appName": "test2", + "environment": "prod2", + "customField": "1", + } + result: EvaluationResult = strategy.get_result(context) + print(result) + assert not result.enabled + assert result.variant is None diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index eb000a9e..a5141613 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -476,8 +476,7 @@ def test_uc_disabled_registration(unleash_client_toggle_only): @responses.activate def test_uc_server_error(unleash_client): # Verify that Unleash Client will still fall back gracefully if SERVER ANGRY RAWR, and then recover gracefully. - - unleash_client = unleash_client + unleash_client = unleash_client # noqa # Set up APIs responses.add(responses.POST, URL + REGISTER_URL, json={}, status=401) responses.add(responses.GET, URL + FEATURES_URL, status=500) diff --git a/tests/unit_tests/test_features.py b/tests/unit_tests/test_features.py index 5e0160d7..a3b55703 100644 --- a/tests/unit_tests/test_features.py +++ b/tests/unit_tests/test_features.py @@ -1,14 +1,21 @@ import pytest from tests.utilities import generate_email_list -from tests.utilities.mocks.mock_variants import VARIANTS +from tests.utilities.mocks.mock_variants import VARIANTS, VARIANTS_WITH_STICKINESS from tests.utilities.testing_constants import IP_LIST from UnleashClient.features import Feature -from UnleashClient.strategies import Default, RemoteAddress, UserWithId +from UnleashClient.strategies import Default, FlexibleRollout, RemoteAddress, UserWithId from UnleashClient.variants import Variants (EMAIL_LIST, CONTEXT) = generate_email_list(20) +BASE_FLEXIBLE_ROLLOUT_DICT = { + "name": "flexibleRollout", + "parameters": {"rollout": 50, "stickiness": "userId", "groupId": "AB12A"}, + "variants": VARIANTS_WITH_STICKINESS, + "constraints": [], +} + @pytest.fixture() def test_feature(): @@ -26,6 +33,19 @@ def test_feature_variants(): yield Feature("My Feature", True, strategies, variants) +@pytest.fixture() +def test_feature_strategy_variants(): + strategies = [ + FlexibleRollout( + BASE_FLEXIBLE_ROLLOUT_DICT["constraints"], + BASE_FLEXIBLE_ROLLOUT_DICT["parameters"], + variants=VARIANTS_WITH_STICKINESS, + ) + ] + variants = Variants(VARIANTS, "My Feature") + yield Feature("My Feature", True, strategies, variants) + + def test_create_feature_true(test_feature): my_feature = test_feature @@ -104,3 +124,19 @@ def test_variant_metrics_feature_has_no_variants(test_feature): for iteration in range(1, 7): test_feature.get_variant({}) assert test_feature.variant_counts["disabled"] == iteration + + +def test_strategy_variant_is_returned(test_feature_strategy_variants): + context = { + "userId": "122", + "appName": "test", + "environment": "prod", + "customField": "1", + } + variant = test_feature_strategy_variants.get_variant(context) + + assert variant == { + "enabled": True, + "name": "VarB", + "payload": {"type": "string", "value": "Test 2"}, + }