Repository: Unleash/unleash-client-python
Pull request #124 (2021-01-20 11:06:09)

Log strategy failures once
# Description

* If strategy creation fails, log exception and add to a failed strategy list in fcache.  On future refreshes, if strategy is in the failed strategy list don't log it.
* Add Python 3.9. 

## Type of change

Please delete options that are not relevant.

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] 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
- [x] Spec Tests
- [x] Integration tests / Manual Tests

# Checklist:

- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] 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


Patch:
diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml
index 9cf75c6..6d441af 100644
--- a/.github/workflows/pull_request.yml
+++ b/.github/workflows/pull_request.yml
@@ -47,7 +47,7 @@ jobs:
     strategy:
       matrix:
         os: [ubuntu-latest, macos-latest, windows-latest]
-        python: [3.5, 3.6, 3.7, 3.8]
+        python: [3.5, 3.6, 3.7, 3.8, 3.9]
     steps:
       - uses: actions/checkout@v1
       - name: Setup Python
diff --git a/UnleashClient/api/metrics.py b/UnleashClient/api/metrics.py
index d70c44a..f0e65a6 100644
--- a/UnleashClient/api/metrics.py
+++ b/UnleashClient/api/metrics.py
@@ -39,6 +39,6 @@ def send_metrics(url: str,
 
         return True
     except requests.RequestException as exc:
-        LOGGER.exception("Unleash Client metrics submission failed due to exception: %s", exc)
+        LOGGER.warning("Unleash Client metrics submission failed due to exception: %s", exc)
 
     return False
diff --git a/UnleashClient/constants.py b/UnleashClient/constants.py
index dd6125e..afd7092 100644
--- a/UnleashClient/constants.py
+++ b/UnleashClient/constants.py
@@ -15,3 +15,6 @@ DISABLED_VARIATION = {
 REGISTER_URL = "/client/register"
 FEATURES_URL = "/client/features"
 METRICS_URL = "/client/metrics"
+
+# Cache keys
+FAILED_STRATEGIES = 'failed_strategies'
diff --git a/UnleashClient/loader.py b/UnleashClient/loader.py
index 7eed4bc..eecbf70 100644
--- a/UnleashClient/loader.py
+++ b/UnleashClient/loader.py
@@ -1,13 +1,14 @@
 from fcache.cache import FileCache
 from UnleashClient.features.Feature import Feature
 from UnleashClient.variants.Variants import Variants
-from UnleashClient.constants import FEATURES_URL
+from UnleashClient.constants import FEATURES_URL, FAILED_STRATEGIES
 from UnleashClient.utils import LOGGER
 
 
 # pylint: disable=broad-except
 def _create_strategies(provisioning: dict,
-                       strategy_mapping: dict) -> list:
+                       strategy_mapping: dict,
+                       cache: FileCache) -> list:
     feature_strategies = []
 
     for strategy in provisioning["strategies"]:
@@ -26,16 +27,21 @@ def _create_strategies(provisioning: dict,
                 constraints=constraint_provisioning, parameters=strategy_provisioning
             ))
         except Exception as excep:
-            LOGGER.warning("Failed to load strategy. This may be a problem with a custom strategy. Exception: %s",
-                           excep)
+            if FAILED_STRATEGIES not in cache.keys():
+                cache[FAILED_STRATEGIES] = []  # Initialize cache key only if failures exist.
+
+            if strategy['name'] not in cache[FAILED_STRATEGIES]:
+                LOGGER.warning("Failed to load strategy. This may be a problem with a custom strategy. Exception: %s", excep)
+                cache[FAILED_STRATEGIES].append(strategy['name'])
 
     return feature_strategies
 
 
 def _create_feature(provisioning: dict,
-                    strategy_mapping: dict) -> Feature:
+                    strategy_mapping: dict,
+                    cache: FileCache) -> Feature:
     if "strategies" in provisioning.keys():
-        parsed_strategies = _create_strategies(provisioning, strategy_mapping)
+        parsed_strategies = _create_strategies(provisioning, strategy_mapping, cache)
     else:
         parsed_strategies = []
 
@@ -85,7 +91,7 @@ def load_features(cache: FileCache,
 
             feature_for_update.enabled = parsed_features[feature]["enabled"]
             if strategies:
-                parsed_strategies = _create_strategies(parsed_features[feature], strategy_mapping)
+                parsed_strategies = _create_strategies(parsed_features[feature], strategy_mapping, cache)
                 feature_for_update.strategies = parsed_strategies
 
             if 'variants' in parsed_features[feature]:
@@ -98,7 +104,7 @@ def load_features(cache: FileCache,
         new_features = list(set(feature_names) - set(feature_toggles.keys()))
 
         for feature in new_features:
-            feature_toggles[feature] = _create_feature(parsed_features[feature], strategy_mapping)
+            feature_toggles[feature] = _create_feature(parsed_features[feature], strategy_mapping, cache)
     except KeyError as cache_exception:
         LOGGER.warning("Cache Exception: %s", cache_exception)
         LOGGER.warning("Unleash client does not have cached features. "
diff --git a/docs/changelog.md b/docs/changelog.md
index 8bbd2e9..b0e17f0 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -1,4 +1,6 @@
 ## Next version
+* (Minor) Errors submitting metrics will be logged as warnings and not exceptions.
+* (Minor) Update apscheduler version to 3.7.0
 
 ## v3.5.1
 * (Minor) Better error handling and typo fixes.  Thanks @vgerak!
diff --git a/setup.py b/setup.py
index be51b19..9e44b02 100644
--- a/setup.py
+++ b/setup.py
@@ -32,6 +32,7 @@ setup(
         "Programming Language :: Python :: 3.5",
         "Programming Language :: Python :: 3.6",
         "Programming Language :: Python :: 3.7",
-        "Programming Language :: Python :: 3.8"
+        "Programming Language :: Python :: 3.8",
+        "Programming Language :: Python :: 3.9",
     ]
 )
diff --git a/tox.ini b/tox.ini
index fae6eed..2c4a31f 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,6 +1,6 @@
 
 [tox]
-envlist = py35,py36,py37,py38
+envlist = py35,py36,py37,py38,py39
 
 [testenv]
 deps = -rrequirements.txt


Test patch:
diff --git a/tests/unit_tests/test_loader.py b/tests/unit_tests/test_loader.py
index dbbc3e2..752456e 100644
--- a/tests/unit_tests/test_loader.py
+++ b/tests/unit_tests/test_loader.py
@@ -3,7 +3,7 @@ from UnleashClient.loader import load_features
 from UnleashClient.features import Feature
 from UnleashClient.strategies import GradualRolloutUserId, FlexibleRollout, UserWithId
 from UnleashClient.variants import Variants
-from UnleashClient.constants import FEATURES_URL
+from UnleashClient.constants import FEATURES_URL, FAILED_STRATEGIES
 from tests.utilities.mocks import MOCK_ALL_FEATURES
 from tests.utilities.testing_constants import DEFAULT_STRATEGY_MAPPING
 from tests.utilities.decorators import cache_full, cache_custom  # noqa: F401
@@ -23,6 +23,9 @@ def test_loader_initialization(cache_full):  # noqa: F811
     assert isinstance(in_memory_features["GradualRolloutUserID"].strategies[0], GradualRolloutUserId)
 
     for feature_name in in_memory_features.keys():
+        if feature_name == 'Garbage':  # Don't check purposely invalid strategy.
+            break
+
         feature = in_memory_features[feature_name]
         assert len(feature.strategies) > 0
         strategy = feature.strategies[0]
@@ -52,6 +55,7 @@ def test_loader_refresh(cache_full):  # noqa: F811
     load_features(temp_cache, in_memory_features, DEFAULT_STRATEGY_MAPPING)
 
     assert in_memory_features["GradualRolloutUserID"].strategies[0].parameters["percentage"] == 60
+    assert len(temp_cache[FAILED_STRATEGIES]) == 1
 
 
 def test_loader_initialization_failure(cache_custom):  # noqa: F811
diff --git a/tests/utilities/mocks/mock_all_features.py b/tests/utilities/mocks/mock_all_features.py
index 4edcf83..1ad2b7f 100644
--- a/tests/utilities/mocks/mock_all_features.py
+++ b/tests/utilities/mocks/mock_all_features.py
@@ -197,6 +197,18 @@ MOCK_ALL_FEATURES = \
           }
       ],
       "createdAt": "2019-10-25T13:22:02.035Z"
-  }
+  },
+  {
+    "name": "Garbage",
+    "description": "Invalid strategy",
+    "enabled": True,
+    "strategies": [
+      {
+        "name": "blargwatisdis",
+        "parameters": {}
+      }
+    ],
+    "createdAt": "2018-10-09T06:04:05.667Z"
+  },
   ]
 }