Repository: Unleash/unleash-client-python
Pull request #253 (2023-05-24 12:39:24)

feat: add variant metrics
## Description

This change adds most variant metrics functionality to the Python SDK.
Variant metrics should be counted whenever get_variant is called:

- if a feature doesn't have variants, register the disabled variant
- if a feature is disabled, register the disabled variant
- otherwise, register the variant that was returned

There is one piece of functionality that isn't included in this PR:

- if a feature doesn't exist, it should still register the disabled variant.

As far as I can tell, this SDK doesn't count metrics for toggles that don't exist, so there was no obvious way to add variant metrics for these features. Both of those cases will be implemented in a follow-up PR.

## Discussion points

### Serialization of emtpy dicts

When serializing metrics, I have done the simple thing and added the variant counts dict, regardless of whether it is empty or not. It might be preferable to not include that key at all if the feature doesn't have any variant metrics. I'd be happy to implement that if we think that's better.

### Tests

I extended the `test_aggregate_and_send_metrics` test with variant data instead of adding a new test. Because variant metrics now are a part of metrics, that felt reasonable. However, we can create a separate test if that's preferable.

### Counting variants on the feature level

The variant counting is currently handled on the feature (`Feature` class) level instead of on the variant (`Variant` class) level.
This was done for a few reasons:
- most importantly: we want to count stats for the disabled variant. Because the disabled variant isn't added as part of the `Feature`'s variants, we'd have to count this separately if we placed counts on the `Variant` level.
- `yes_count` and `no_count` are already tracked on the feature, so it felt natural to count variants there too
- it simplifies serialization and the `reset_stats` method

### Casting `DISABLED_VARIANT["name"]`

Described in the [inline comment](https://github.com/Unleash/unleash-client-python/pull/253#discussion_r1205484173):



This cast appears to be the only way to get the linter to accept that we get a string here. As far as I can tell, "name" should always exist and will always be a string. However, the disabled variant isn't typed in any particular way, so it just shows up as a Dict[str, object].

I'd rather avoid this cast if I can, but I don't know if it's possible without re-architecting significant portions of the code and introducing breaking changes. Input would be much appreciated.



## Type of change

Please delete options that are not relevant.

- [X] New feature (non-breaking change which adds functionality)

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

- [X] Unit tests
- [ ] Spec Tests
- [ ] 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
- [ ] 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 (the ETAG test is failing, but it fails on main too)
- [ ] Any dependent changes have been merged and published in downstream modules (I don't think there are any downstream modules?)

thomasheartman: For sure! It's much appreciated. Been a while since I touched python, so I'm a little out of touch 😅  
ivanklee86: lgtm and thanks for the clear explanation!  Let me look into those unit tests failures, something is odd. 🤔 

Patch:
diff --git a/UnleashClient/features/Feature.py b/UnleashClient/features/Feature.py
index bd52287..968e0e2 100644
--- a/UnleashClient/features/Feature.py
+++ b/UnleashClient/features/Feature.py
@@ -1,5 +1,5 @@
 # pylint: disable=invalid-name
-from typing import Optional
+from typing import Dict, Optional, cast
 
 from UnleashClient.constants import DISABLED_VARIATION
 from UnleashClient.utils import LOGGER
@@ -36,6 +36,8 @@ class Feature:
         # Stats tracking
         self.yes_count = 0
         self.no_count = 0
+        ## { [ variant name ]: number }
+        self.variant_counts: Dict[str, int] = {}
 
     def reset_stats(self) -> None:
         """
@@ -45,6 +47,7 @@ class Feature:
         """
         self.yes_count = 0
         self.no_count = 0
+        self.variant_counts = {}
 
     def increment_stats(self, result: bool) -> None:
         """
@@ -58,6 +61,15 @@ class Feature:
         else:
             self.no_count += 1
 
+    def _count_variant(self, variant_name: str) -> None:
+        """
+        Count a specific variant.
+
+        :param variant_name: The name of the variant to count.
+        :return:
+        """
+        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
@@ -105,4 +117,5 @@ class Feature:
             except Exception as variant_exception:
                 LOGGER.warning("Error selecting variant: %s", variant_exception)
 
+        self._count_variant(cast(str, variant["name"]))
         return variant
diff --git a/UnleashClient/periodic_tasks/send_metrics.py b/UnleashClient/periodic_tasks/send_metrics.py
index 9403bce..5734dc2 100644
--- a/UnleashClient/periodic_tasks/send_metrics.py
+++ b/UnleashClient/periodic_tasks/send_metrics.py
@@ -26,6 +26,7 @@ def aggregate_and_send_metrics(
             features[feature_name].name: {
                 "yes": features[feature_name].yes_count,
                 "no": features[feature_name].no_count,
+                "variants": features[feature_name].variant_counts,
             }
         }
 


Test patch:
diff --git a/tests/unit_tests/periodic/test_aggregate_and_send_metrics.py b/tests/unit_tests/periodic/test_aggregate_and_send_metrics.py
index 979fd49..9bf223f 100644
--- a/tests/unit_tests/periodic/test_aggregate_and_send_metrics.py
+++ b/tests/unit_tests/periodic/test_aggregate_and_send_metrics.py
@@ -3,6 +3,7 @@ from datetime import datetime, timedelta, timezone
 
 import responses
 
+from tests.utilities.mocks.mock_variants import VARIANTS
 from tests.utilities.testing_constants import (
     APP_NAME,
     CUSTOM_HEADERS,
@@ -16,6 +17,7 @@ from UnleashClient.constants import METRIC_LAST_SENT_TIME, METRICS_URL
 from UnleashClient.features import Feature
 from UnleashClient.periodic_tasks import aggregate_and_send_metrics
 from UnleashClient.strategies import Default, RemoteAddress
+from UnleashClient.variants import Variants
 
 FULL_METRICS_URL = URL + METRICS_URL
 print(FULL_METRICS_URL)
@@ -33,10 +35,19 @@ def test_aggregate_and_send_metrics():
     my_feature1.yes_count = 1
     my_feature1.no_count = 1
 
-    my_feature2 = Feature("My Feature2", True, strategies)
+    my_feature2 = Feature(
+        "My Feature2", True, strategies, variants=Variants(VARIANTS, "My Feature2")
+    )
     my_feature2.yes_count = 2
     my_feature2.no_count = 2
 
+    feature2_variant_counts = {
+        "VarA": 56,
+        "VarB": 0,
+        "VarC": 4,
+    }
+    my_feature2.variant_counts = feature2_variant_counts
+
     my_feature3 = Feature("My Feature3", True, strategies)
     my_feature3.yes_count = 0
     my_feature3.no_count = 0
@@ -53,6 +64,10 @@ def test_aggregate_and_send_metrics():
     assert len(request["bucket"]["toggles"].keys()) == 2
     assert request["bucket"]["toggles"]["My Feature1"]["yes"] == 1
     assert request["bucket"]["toggles"]["My Feature1"]["no"] == 1
+    assert (
+        request["bucket"]["toggles"]["My Feature2"]["variants"]
+        == feature2_variant_counts
+    )
     assert "My Feature3" not in request["bucket"]["toggles"].keys()
     assert cache.get(METRIC_LAST_SENT_TIME) > start_time
 
diff --git a/tests/unit_tests/test_features.py b/tests/unit_tests/test_features.py
index 6c44e75..5e0160d 100644
--- a/tests/unit_tests/test_features.py
+++ b/tests/unit_tests/test_features.py
@@ -76,3 +76,31 @@ def test_select_variation_variation(test_feature_variants):
     selected_variant = test_feature_variants.get_variant({"userId": "2"})
     assert selected_variant["enabled"]
     assert selected_variant["name"] == "VarB"
+
+
+def test_variant_metrics_are_reset(test_feature_variants):
+    test_feature_variants.get_variant({"userId": "2"})
+    assert test_feature_variants.variant_counts["VarB"] == 1
+
+    test_feature_variants.reset_stats()
+    assert not test_feature_variants.variant_counts
+
+
+def test_variant_metrics_with_existing_variant(test_feature_variants):
+    for iteration in range(1, 7):
+        test_feature_variants.get_variant({"userId": "2"})
+        assert test_feature_variants.variant_counts["VarB"] == iteration
+
+
+def test_variant_metrics_with_disabled_feature(test_feature_variants):
+    test_feature_variants.enabled = False
+    assert not test_feature_variants.is_enabled()
+    for iteration in range(1, 7):
+        test_feature_variants.get_variant({})
+        assert test_feature_variants.variant_counts["disabled"] == iteration
+
+
+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