From 4a6f2ce8c91440a8d113a85d7d766dbf125e6263 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 7 Mar 2024 07:55:44 +0100 Subject: [PATCH 1/9] fix: correctly count metrics when a flag with dependencies has disabled parents --- UnleashClient/__init__.py | 12 +++++++++--- tests/unit_tests/test_client.py | 34 ++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/UnleashClient/__init__.py b/UnleashClient/__init__.py index 89bf9fb2..e0e1e74e 100644 --- a/UnleashClient/__init__.py +++ b/UnleashClient/__init__.py @@ -364,9 +364,13 @@ def is_enabled( if self.unleash_bootstrapped or self.is_initialized: try: feature = self.features[feature_name] - feature_check = feature.is_enabled( - context - ) and self._dependencies_are_satisfied(feature_name, context) + dependency_check = self._dependencies_are_satisfied(feature_name, context) + + if dependency_check: + feature_check = feature.is_enabled(context) + else: + feature.increment_stats(False) + feature_check = False if feature.only_for_metrics: return self._get_fallback_value( @@ -449,6 +453,8 @@ def get_variant(self, feature_name: str, context: Optional[dict] = None) -> dict feature = self.features[feature_name] if not self._dependencies_are_satisfied(feature_name, context): + feature.increment_stats(False) + feature._count_variant("disabled") return DISABLED_VARIATION variant_check = feature.get_variant(context) diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index cb34c5a1..18149800 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -572,6 +572,7 @@ def test_uc_doesnt_count_metrics_for_dependency_parents(unleash_client): time.sleep(1) child = "ChildWithVariant" + parent = "Parent" # Check a flag that depends on a parent unleash_client.is_enabled(child) unleash_client.get_variant(child) @@ -581,7 +582,38 @@ def test_uc_doesnt_count_metrics_for_dependency_parents(unleash_client): request = json.loads(responses.calls[-1].request.body) assert request["bucket"]["toggles"][child]["yes"] == 2 assert request["bucket"]["toggles"][child]["variants"]["childVariant"] == 1 - assert "Parent" not in request["bucket"]["toggles"] + assert parent not in request["bucket"]["toggles"] + + +@responses.activate +def test_uc_counts_metrics_for_child_even_if_parent_is_disabled(unleash_client): + # Set up API + responses.add(responses.POST, URL + REGISTER_URL, json={}, status=202) + responses.add( + responses.GET, + URL + FEATURES_URL, + json=MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE, + status=200, + ) + responses.add(responses.POST, URL + METRICS_URL, json={}, status=202) + + # Create Unleash client and check initial load + unleash_client.initialize_client() + time.sleep(1) + + child = "WithDisabledDependency" + parent = "Disabled" + # Check a flag that depends on a parent + enabled = unleash_client.is_enabled(child) + variant = unleash_client.get_variant(child) + + # Verify that the parent doesn't have any metrics registered + time.sleep(12) + request = json.loads(responses.calls[-1].request.body) + print(enabled, variant, request) + assert request["bucket"]["toggles"][child]["no"] == 2 + assert request["bucket"]["toggles"][child]["variants"]["disabled"] == 1 + assert parent not in request["bucket"]["toggles"] @responses.activate From 576216f34483dec3308231e9b281264f22b2bedb Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 7 Mar 2024 07:01:01 +0000 Subject: [PATCH 2/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- UnleashClient/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/UnleashClient/__init__.py b/UnleashClient/__init__.py index e0e1e74e..f04cdbad 100644 --- a/UnleashClient/__init__.py +++ b/UnleashClient/__init__.py @@ -364,7 +364,9 @@ def is_enabled( if self.unleash_bootstrapped or self.is_initialized: try: feature = self.features[feature_name] - dependency_check = self._dependencies_are_satisfied(feature_name, context) + dependency_check = self._dependencies_are_satisfied( + feature_name, context + ) if dependency_check: feature_check = feature.is_enabled(context) From fb3db22af17fb655b3370c32d6e4a9898ba21421 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 7 Mar 2024 08:39:03 +0100 Subject: [PATCH 3/9] fix: remove print --- tests/unit_tests/test_client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index 18149800..015bf05b 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -610,7 +610,6 @@ def test_uc_counts_metrics_for_child_even_if_parent_is_disabled(unleash_client): # Verify that the parent doesn't have any metrics registered time.sleep(12) request = json.loads(responses.calls[-1].request.body) - print(enabled, variant, request) assert request["bucket"]["toggles"][child]["no"] == 2 assert request["bucket"]["toggles"][child]["variants"]["disabled"] == 1 assert parent not in request["bucket"]["toggles"] From 2f4a2ffd826dac8137bce8ba5b0c51ac59525b1b Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 7 Mar 2024 08:48:56 +0100 Subject: [PATCH 4/9] fix: remove unused variable declarations --- tests/unit_tests/test_client.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index 015bf05b..e0a78d67 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -603,9 +603,9 @@ def test_uc_counts_metrics_for_child_even_if_parent_is_disabled(unleash_client): child = "WithDisabledDependency" parent = "Disabled" - # Check a flag that depends on a parent - enabled = unleash_client.is_enabled(child) - variant = unleash_client.get_variant(child) + # Check a flag that depends on a disabled parent + unleash_client.is_enabled(child) + unleash_client.get_variant(child) # Verify that the parent doesn't have any metrics registered time.sleep(12) From 1abe74e3a107431203b3fdd8ad7db40f36fcc58e Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 7 Mar 2024 09:30:51 +0100 Subject: [PATCH 5/9] Chore: improve test execution time: lower refresh and metrics intervals This commit lowers refresh and metrics intervals in tests, thereby cutting about two minutes or so of the test suite execution time. I'm worried, though, that it might make some tests more flaky. --- tests/unit_tests/test_client.py | 24 ++++++++++++------------ tests/utilities/testing_constants.py | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index e0a78d67..48e3597c 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -210,7 +210,7 @@ def test_uc_lifecycle(unleash_client): status=304, headers={"etag": ETAG_VALUE}, ) - time.sleep(16) + time.sleep(REFRESH_INTERVAL + 1) # Simulate server provisioning change responses.add( @@ -220,7 +220,7 @@ def test_uc_lifecycle(unleash_client): status=200, headers={"etag": "W/somethingelse"}, ) - time.sleep(30) + time.sleep(REFRESH_INTERVAL * 2) assert len(unleash_client.features) >= 9 @@ -329,13 +329,13 @@ def test_uc_dirty_cache(unleash_client_nodestroy): # Create Unleash client and check initial load unleash_client.initialize_client() - time.sleep(5) + time.sleep(1) assert unleash_client.is_enabled("testFlag") unleash_client.unleash_scheduler.shutdown() # Check that everything works if previous cache exists. unleash_client.initialize_client() - time.sleep(5) + time.sleep(1) assert unleash_client.is_enabled("testFlag") @@ -484,7 +484,7 @@ def test_uc_metrics(unleash_client): time.sleep(1) assert unleash_client.is_enabled("testFlag") - time.sleep(12) + time.sleep(METRICS_INTERVAL) request = json.loads(responses.calls[-1].request.body) assert request["bucket"]["toggles"]["testFlag"]["yes"] == 1 @@ -506,7 +506,7 @@ def test_uc_registers_metrics_for_nonexistent_features(unleash_client): unleash_client.is_enabled("nonexistent-flag") # Verify that the metrics are serialized - time.sleep(12) + time.sleep(METRICS_INTERVAL) request = json.loads(responses.calls[-1].request.body) assert request["bucket"]["toggles"]["nonexistent-flag"]["no"] == 1 @@ -526,7 +526,7 @@ def test_uc_metrics_dependencies(unleash_client): time.sleep(1) assert unleash_client.is_enabled("Child") - time.sleep(12) + time.sleep(METRICS_INTERVAL) request = json.loads(responses.calls[-1].request.body) assert request["bucket"]["toggles"]["Child"]["yes"] == 1 assert "Parent" not in request["bucket"]["toggles"] @@ -549,7 +549,7 @@ def test_uc_registers_variant_metrics_for_nonexistent_features(unleash_client): unleash_client.get_variant("nonexistent-flag") # Verify that the metrics are serialized - time.sleep(12) + time.sleep(METRICS_INTERVAL) request = json.loads(responses.calls[-1].request.body) assert request["bucket"]["toggles"]["nonexistent-flag"]["no"] == 1 assert request["bucket"]["toggles"]["nonexistent-flag"]["variants"]["disabled"] == 1 @@ -578,7 +578,7 @@ def test_uc_doesnt_count_metrics_for_dependency_parents(unleash_client): unleash_client.get_variant(child) # Verify that the parent doesn't have any metrics registered - time.sleep(12) + time.sleep(METRICS_INTERVAL) request = json.loads(responses.calls[-1].request.body) assert request["bucket"]["toggles"][child]["yes"] == 2 assert request["bucket"]["toggles"][child]["variants"]["childVariant"] == 1 @@ -608,7 +608,7 @@ def test_uc_counts_metrics_for_child_even_if_parent_is_disabled(unleash_client): unleash_client.get_variant(child) # Verify that the parent doesn't have any metrics registered - time.sleep(12) + time.sleep(METRICS_INTERVAL) request = json.loads(responses.calls[-1].request.body) assert request["bucket"]["toggles"][child]["no"] == 2 assert request["bucket"]["toggles"][child]["variants"]["disabled"] == 1 @@ -627,7 +627,7 @@ def test_uc_disabled_registration(unleash_client_toggle_only): unleash_client.initialize_client() unleash_client.is_enabled("testFlag") - time.sleep(20) + time.sleep(REFRESH_INTERVAL * 2) assert unleash_client.is_enabled("testFlag") for api_call in responses.calls: @@ -650,7 +650,7 @@ def test_uc_server_error(unleash_client): responses.add( responses.GET, URL + FEATURES_URL, json=MOCK_FEATURE_RESPONSE, status=200 ) - time.sleep(20) + time.sleep(REFRESH_INTERVAL * 2) assert unleash_client.is_enabled("testFlag") diff --git a/tests/utilities/testing_constants.py b/tests/utilities/testing_constants.py index ef32d0da..7f0311c2 100644 --- a/tests/utilities/testing_constants.py +++ b/tests/utilities/testing_constants.py @@ -14,9 +14,9 @@ APP_NAME = "pytest" ENVIRONMENT = "unit" INSTANCE_ID = "123" -REFRESH_INTERVAL = 15 +REFRESH_INTERVAL = 3 REFRESH_JITTER = None -METRICS_INTERVAL = 10 +METRICS_INTERVAL = 2 METRICS_JITTER = None DISABLE_METRICS = True DISABLE_REGISTRATION = True From 16254933032f3b4ec84e5e90ad8c86de9a38b6d8 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 7 Mar 2024 11:16:50 +0100 Subject: [PATCH 6/9] fix: extract aggregate_metrics into own method to test that --- UnleashClient/periodic_tasks/__init__.py | 2 +- UnleashClient/periodic_tasks/send_metrics.py | 34 +++++++++------ tests/unit_tests/test_client.py | 44 +++++++++----------- 3 files changed, 41 insertions(+), 39 deletions(-) diff --git a/UnleashClient/periodic_tasks/__init__.py b/UnleashClient/periodic_tasks/__init__.py index a4391c50..99dca01d 100644 --- a/UnleashClient/periodic_tasks/__init__.py +++ b/UnleashClient/periodic_tasks/__init__.py @@ -1,3 +1,3 @@ # ruff: noqa: F401 from .fetch_and_load import fetch_and_load_features -from .send_metrics import aggregate_and_send_metrics +from .send_metrics import (aggregate_and_send_metrics, aggregate_metrics) diff --git a/UnleashClient/periodic_tasks/send_metrics.py b/UnleashClient/periodic_tasks/send_metrics.py index 3df7ec7d..326baf63 100644 --- a/UnleashClient/periodic_tasks/send_metrics.py +++ b/UnleashClient/periodic_tasks/send_metrics.py @@ -6,17 +6,9 @@ from UnleashClient.constants import METRIC_LAST_SENT_TIME from UnleashClient.utils import LOGGER - -def aggregate_and_send_metrics( - url: str, - app_name: str, - instance_id: str, - custom_headers: dict, - custom_options: dict, +def aggregate_metrics( features: dict, - cache: BaseCache, - request_timeout: int, -) -> None: +) -> dict: feature_stats_list = [] for feature_name in features.keys(): @@ -31,20 +23,36 @@ def aggregate_and_send_metrics( } } - features[feature_name].reset_stats() feature_stats_list.append(feature_stats) + return dict(ChainMap(*feature_stats_list)) + +def aggregate_and_send_metrics( + url: str, + app_name: str, + instance_id: str, + custom_headers: dict, + custom_options: dict, + features: dict, + cache: BaseCache, + request_timeout: int, +) -> None: + feature_stats_dict = aggregate_metrics(features) + + for feature_name in features.keys(): + features[feature_name].reset_stats() + metrics_request = { "appName": app_name, "instanceId": instance_id, "bucket": { "start": cache.get(METRIC_LAST_SENT_TIME).isoformat(), "stop": datetime.now(timezone.utc).isoformat(), - "toggles": dict(ChainMap(*feature_stats_list)), + "toggles": feature_stats_dict, }, } - if feature_stats_list: + if feature_stats_dict: send_metrics( url, metrics_request, custom_headers, custom_options, request_timeout ) diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index 48e3597c..2795f5c0 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -41,6 +41,7 @@ from UnleashClient.events import UnleashEvent, UnleashEventType from UnleashClient.strategies import Strategy from UnleashClient.utils import InstanceAllowType +from UnleashClient.periodic_tasks import aggregate_metrics class EnvironmentStrategy(Strategy): @@ -484,9 +485,8 @@ def test_uc_metrics(unleash_client): time.sleep(1) assert unleash_client.is_enabled("testFlag") - time.sleep(METRICS_INTERVAL) - request = json.loads(responses.calls[-1].request.body) - assert request["bucket"]["toggles"]["testFlag"]["yes"] == 1 + metrics = aggregate_metrics(unleash_client.features) + assert metrics["testFlag"]["yes"] == 1 @responses.activate @@ -506,9 +506,8 @@ def test_uc_registers_metrics_for_nonexistent_features(unleash_client): unleash_client.is_enabled("nonexistent-flag") # Verify that the metrics are serialized - time.sleep(METRICS_INTERVAL) - request = json.loads(responses.calls[-1].request.body) - assert request["bucket"]["toggles"]["nonexistent-flag"]["no"] == 1 + metrics = aggregate_metrics(unleash_client.features) + assert metrics["nonexistent-flag"]["no"] == 1 @responses.activate @@ -526,10 +525,9 @@ def test_uc_metrics_dependencies(unleash_client): time.sleep(1) assert unleash_client.is_enabled("Child") - time.sleep(METRICS_INTERVAL) - request = json.loads(responses.calls[-1].request.body) - assert request["bucket"]["toggles"]["Child"]["yes"] == 1 - assert "Parent" not in request["bucket"]["toggles"] + metrics = aggregate_metrics(unleash_client.features) + assert metrics["Child"]["yes"] == 1 + assert "Parent" not in metrics @responses.activate @@ -548,11 +546,9 @@ def test_uc_registers_variant_metrics_for_nonexistent_features(unleash_client): # Check a flag that doesn't exist unleash_client.get_variant("nonexistent-flag") - # Verify that the metrics are serialized - time.sleep(METRICS_INTERVAL) - request = json.loads(responses.calls[-1].request.body) - assert request["bucket"]["toggles"]["nonexistent-flag"]["no"] == 1 - assert request["bucket"]["toggles"]["nonexistent-flag"]["variants"]["disabled"] == 1 + metrics = aggregate_metrics(unleash_client.features) + assert metrics["nonexistent-flag"]["no"] == 1 + assert metrics["nonexistent-flag"]["variants"]["disabled"] == 1 @responses.activate @@ -578,11 +574,10 @@ def test_uc_doesnt_count_metrics_for_dependency_parents(unleash_client): unleash_client.get_variant(child) # Verify that the parent doesn't have any metrics registered - time.sleep(METRICS_INTERVAL) - request = json.loads(responses.calls[-1].request.body) - assert request["bucket"]["toggles"][child]["yes"] == 2 - assert request["bucket"]["toggles"][child]["variants"]["childVariant"] == 1 - assert parent not in request["bucket"]["toggles"] + metrics = aggregate_metrics(unleash_client.features) + assert metrics[child]["yes"] == 2 + assert metrics[child]["variants"]["childVariant"] == 1 + assert parent not in metrics @responses.activate @@ -608,11 +603,10 @@ def test_uc_counts_metrics_for_child_even_if_parent_is_disabled(unleash_client): unleash_client.get_variant(child) # Verify that the parent doesn't have any metrics registered - time.sleep(METRICS_INTERVAL) - request = json.loads(responses.calls[-1].request.body) - assert request["bucket"]["toggles"][child]["no"] == 2 - assert request["bucket"]["toggles"][child]["variants"]["disabled"] == 1 - assert parent not in request["bucket"]["toggles"] + metrics = aggregate_metrics(unleash_client.features) + assert metrics[child]["no"] == 2 + assert metrics[child]["variants"]["disabled"] == 1 + assert parent not in metrics @responses.activate From 59595f752710d496f907bac9ee0c03a32248c350 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 7 Mar 2024 10:17:11 +0000 Subject: [PATCH 7/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- UnleashClient/periodic_tasks/__init__.py | 2 +- UnleashClient/periodic_tasks/send_metrics.py | 2 ++ tests/unit_tests/test_client.py | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/UnleashClient/periodic_tasks/__init__.py b/UnleashClient/periodic_tasks/__init__.py index 99dca01d..13b71529 100644 --- a/UnleashClient/periodic_tasks/__init__.py +++ b/UnleashClient/periodic_tasks/__init__.py @@ -1,3 +1,3 @@ # ruff: noqa: F401 from .fetch_and_load import fetch_and_load_features -from .send_metrics import (aggregate_and_send_metrics, aggregate_metrics) +from .send_metrics import aggregate_and_send_metrics, aggregate_metrics diff --git a/UnleashClient/periodic_tasks/send_metrics.py b/UnleashClient/periodic_tasks/send_metrics.py index 326baf63..b1634e79 100644 --- a/UnleashClient/periodic_tasks/send_metrics.py +++ b/UnleashClient/periodic_tasks/send_metrics.py @@ -6,6 +6,7 @@ from UnleashClient.constants import METRIC_LAST_SENT_TIME from UnleashClient.utils import LOGGER + def aggregate_metrics( features: dict, ) -> dict: @@ -27,6 +28,7 @@ def aggregate_metrics( return dict(ChainMap(*feature_stats_list)) + def aggregate_and_send_metrics( url: str, app_name: str, diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index 2795f5c0..cc4e8fbc 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -39,9 +39,9 @@ from UnleashClient.cache import FileCache from UnleashClient.constants import FEATURES_URL, METRICS_URL, REGISTER_URL from UnleashClient.events import UnleashEvent, UnleashEventType +from UnleashClient.periodic_tasks import aggregate_metrics from UnleashClient.strategies import Strategy from UnleashClient.utils import InstanceAllowType -from UnleashClient.periodic_tasks import aggregate_metrics class EnvironmentStrategy(Strategy): From 410c5f82211a74022ccacd26d3f0315220763efe Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 7 Mar 2024 11:19:58 +0100 Subject: [PATCH 8/9] fix: remove json import --- tests/unit_tests/test_client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index cc4e8fbc..610029ae 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -1,4 +1,3 @@ -import json import time import warnings from pathlib import Path From adee672bad73fae3d6701df1c743c24ba8b731c7 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 7 Mar 2024 11:48:04 +0100 Subject: [PATCH 9/9] refactor: remove redundant response config --- tests/unit_tests/test_client.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index 610029ae..50a7b981 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -495,7 +495,6 @@ def test_uc_registers_metrics_for_nonexistent_features(unleash_client): responses.add( responses.GET, URL + FEATURES_URL, json=MOCK_FEATURE_RESPONSE, status=200 ) - responses.add(responses.POST, URL + METRICS_URL, json={}, status=202) # Create Unleash client and check initial load unleash_client.initialize_client() @@ -518,7 +517,6 @@ def test_uc_metrics_dependencies(unleash_client): json=MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE, status=200, ) - responses.add(responses.POST, URL + METRICS_URL, json={}, status=202) unleash_client.initialize_client() time.sleep(1) @@ -536,7 +534,6 @@ def test_uc_registers_variant_metrics_for_nonexistent_features(unleash_client): responses.add( responses.GET, URL + FEATURES_URL, json=MOCK_FEATURE_RESPONSE, status=200 ) - responses.add(responses.POST, URL + METRICS_URL, json={}, status=202) # Create Unleash client and check initial load unleash_client.initialize_client() @@ -560,7 +557,6 @@ def test_uc_doesnt_count_metrics_for_dependency_parents(unleash_client): json=MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE, status=200, ) - responses.add(responses.POST, URL + METRICS_URL, json={}, status=202) # Create Unleash client and check initial load unleash_client.initialize_client() @@ -589,7 +585,6 @@ def test_uc_counts_metrics_for_child_even_if_parent_is_disabled(unleash_client): json=MOCK_FEATURE_WITH_DEPENDENCIES_RESPONSE, status=200, ) - responses.add(responses.POST, URL + METRICS_URL, json={}, status=202) # Create Unleash client and check initial load unleash_client.initialize_client()