fix: Exposure events sent without a variant and duplicated within a flush interval (#233)

Author: khvn26

flagsmith/analytics.py

@@ -83,6 +83,12 @@ def track_feature(self, feature_name: str) -> None:
             self.flush()
 
 
+class ExposureKey(typing.NamedTuple):
+    feature_name: typing.Optional[str]
+    identifier: typing.Optional[str]
+    value: typing.Optional[str]
+
+
 @dataclass
 class EventProcessorConfig:
     events_api_url: str = DEFAULT_EVENT_API_URL
@@ -111,6 +117,7 @@ def __init__(
         self._flush_interval_seconds = config.flush_interval_seconds
 
         self._buffer: typing.List[typing.Dict[str, typing.Any]] = []
+        self._buffered_exposure_keys: typing.Set[ExposureKey] = set()
         self._lock = threading.Lock()
         self._timer: typing.Optional[threading.Timer] = None
 
@@ -159,6 +166,24 @@ def _buffer_event(
     ) -> None:
         should_flush = False
         with self._lock:
+            if event == FLAG_EXPOSURE_EVENT:
+                # An exposure is defined by who saw which variant of which
+                # feature; equal exposures within one flush interval add no
+                # information, so only the first is buffered.
+                exposure_key = ExposureKey(
+                    feature_name=feature_name,
+                    identifier=identifier,
+                    value=str(value) if value is not None else None,
+                )
+                if exposure_key in self._buffered_exposure_keys:
+                    logger.debug(
+                        "Skipping duplicate %s event for feature %s, identifier %s",
+                        FLAG_EXPOSURE_EVENT,
+                        feature_name,
+                        identifier,
+                    )
+                    return
+                self._buffered_exposure_keys.add(exposure_key)
             self._buffer.append(
                 {
                     "event": event,
@@ -182,6 +207,7 @@ def flush(self) -> None:
                 return
             events = self._buffer
             self._buffer = []
+            self._buffered_exposure_keys.clear()
 
         payload = json.dumps({"events": events})
         try:

flagsmith/flagsmith.py

@@ -368,19 +368,38 @@ def get_experiment_flag(
         """
         Resolve a flag for an identity and record an exposure event.
 
-        Skips the exposure event when the resolved flag is a `DefaultFlag`
-        (i.e. the feature was not present and was served via the
-        `default_flag_handler`) or when the feature is disabled, to keep
-        experimentation data clean.
+        The exposure event's ``value`` is the flag's variant key. It is only
+        sent when the flag exists, is enabled and carries a variant; any
+        other outcome is logged and skipped to keep experimentation data
+        clean. A `DefaultFlag` served via the `default_flag_handler` counts
+        as the feature not existing.
         """
         if not self._event_processor:
             raise ValueError("Events must be enabled to use experiment flags.")
         flag = self.get_identity_flags(identifier, traits).get_flag(feature_name)
-        if isinstance(flag, Flag) and flag.enabled:
+        if not isinstance(flag, Flag):
+            logger.debug(
+                "Not sending %s for feature %s: feature not found.",
+                FLAG_EXPOSURE_EVENT,
+                feature_name,
+            )
+        elif not flag.enabled:
+            logger.debug(
+                "Not sending %s for feature %s: feature is disabled.",
+                FLAG_EXPOSURE_EVENT,
+                feature_name,
+            )
+        elif flag.variant is None:
+            logger.debug(
+                "Not sending %s for feature %s: flag has no variant.",
+                FLAG_EXPOSURE_EVENT,
+                feature_name,
+            )
+        else:
             self.track_exposure_event(
                 feature_name=feature_name,
                 identifier=identifier,
-                value=flag.variant if flag.variant is not None else flag.value,
+                value=flag.variant,
                 traits=traits,
             )
         return flag
@@ -418,6 +437,14 @@ def track_exposure_event(
     ) -> None:
         if not self._event_processor:
             raise ValueError("Events must be enabled to track exposure events.")
+        if not identifier:
+            logger.warning(
+                "Not sending %s for feature %s: an exposure requires an"
+                " identifier to reconcile with conversion events.",
+                FLAG_EXPOSURE_EVENT,
+                feature_name,
+            )
+            return
         self._event_processor.track_exposure_event(
             feature_name=feature_name,
             identifier=identifier,

tests/data/identities.json

@@ -20,6 +20,7 @@
                 "project": 1
             },
             "feature_state_value": "some-value",
+            "variant": "treatment",
             "enabled": true,
             "environment": 1,
             "identity": null,

tests/test_event_processor.py

@@ -82,6 +82,72 @@ def test_track_exposure_event_buffers_with_flag_exposure_event_name(
     assert "sdk_version" in event["metadata"]
 
 
+def test_track_exposure_event__equal_events_in_flush_interval__deduplicated(
+    event_processor: EventProcessor,
+) -> None:
+    # Given / When
+    for _ in range(3):
+        event_processor.track_exposure_event(
+            feature_name="checkout_v2",
+            identifier="user1",
+            value="variant_b",
+        )
+
+    # Then
+    assert len(event_processor._buffer) == 1
+
+
+def test_track_exposure_event__differing_events__not_deduplicated(
+    event_processor: EventProcessor,
+) -> None:
+    # Given / When
+    event_processor.track_exposure_event(
+        feature_name="checkout_v2", identifier="user1", value="variant_b"
+    )
+    event_processor.track_exposure_event(
+        feature_name="checkout_v2", identifier="user2", value="variant_b"
+    )
+    event_processor.track_exposure_event(
+        feature_name="checkout_v2", identifier="user1", value="variant_a"
+    )
+    event_processor.track_exposure_event(
+        feature_name="banner_test", identifier="user1", value="variant_b"
+    )
+
+    # Then
+    assert len(event_processor._buffer) == 4
+
+
+def test_track_exposure_event__equal_event_after_flush__buffered_again(
+    event_processor: EventProcessor,
+) -> None:
+    # Given
+    with mock.patch("flagsmith.analytics.session"):
+        event_processor.track_exposure_event(
+            feature_name="checkout_v2", identifier="user1", value="variant_b"
+        )
+        event_processor.flush()
+
+        # When
+        event_processor.track_exposure_event(
+            feature_name="checkout_v2", identifier="user1", value="variant_b"
+        )
+
+    # Then
+    assert len(event_processor._buffer) == 1
+
+
+def test_track_event__equal_events__not_deduplicated(
+    event_processor: EventProcessor,
+) -> None:
+    # Given / When
+    event_processor.track_event(event="purchase", identifier="user1", value="99.5")
+    event_processor.track_event(event="purchase", identifier="user1", value="99.5")
+
+    # Then
+    assert len(event_processor._buffer) == 2
+
+
 def test_auto_flush_on_buffer_full() -> None:
     config = EventProcessorConfig(events_api_url="http://test/", max_buffer_items=5)
     processor = EventProcessor(config=config, environment_key="key")

tests/test_flagsmith.py

@@ -1,4 +1,5 @@
 import json
+import logging
 import time
 import typing
 
@@ -1020,6 +1021,27 @@ def test_track_exposure_event_raises_without_config(api_key: str) -> None:
         flagsmith.track_exposure_event("checkout_v2")
 
 
+def test_track_exposure_event__no_identifier__exposure_not_sent(
+    mocker: MockerFixture,
+    api_key: str,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    # Given
+    config = EventProcessorConfig(events_api_url="http://test/")
+    flagsmith = Flagsmith(
+        environment_key=api_key, enable_events=True, event_processor_config=config
+    )
+
+    mock_track = mocker.patch.object(flagsmith._event_processor, "track_exposure_event")
+
+    # When
+    flagsmith.track_exposure_event("checkout_v2", value="variant_b")
+
+    # Then
+    mock_track.assert_not_called()
+    assert "an exposure requires an identifier" in caplog.text
+
+
 def test_track_exposure_event_delegates_to_event_processor(
     mocker: MockerFixture, api_key: str
 ) -> None:
@@ -1054,9 +1076,10 @@ def test_get_experiment_flag_raises_without_events_enabled(api_key: str) -> None
 
 
 @responses.activate()
-def test_get_experiment_flag_returns_flag_and_tracks_exposure(
+def test_get_experiment_flag__variant__returns_flag_and_tracks_exposure(
     mocker: MockerFixture, api_key: str, identities_json: str
 ) -> None:
+    # Given
     config = EventProcessorConfig(events_api_url="http://test/")
     flagsmith = Flagsmith(
         environment_key=api_key, enable_events=True, event_processor_config=config
@@ -1065,29 +1088,33 @@ def test_get_experiment_flag_returns_flag_and_tracks_exposure(
     mock_track = mocker.patch.object(flagsmith._event_processor, "track_exposure_event")
     responses.add(method="POST", url=flagsmith.identities_url, body=identities_json)
 
+    # When
     result = flagsmith.get_experiment_flag(
         feature_name="some_feature",
         identifier="user1",
         traits={"plan": "premium"},
     )
 
+    # Then
     assert isinstance(result, Flag)
     assert result.is_default is False
     assert result.feature_name == "some_feature"
     assert result.value == "some-value"
+    assert result.variant == "treatment"
     mock_track.assert_called_once_with(
         feature_name="some_feature",
         identifier="user1",
-        value="some-value",
+        value="treatment",
         traits={"plan": "premium"},
         metadata=None,
     )
 
 
 @responses.activate()
-def test_get_experiment_flag_skips_exposure_for_default_flag(
-    mocker: MockerFixture, api_key: str
+def test_get_experiment_flag__default_flag__skips_exposure(
+    mocker: MockerFixture, api_key: str, caplog: pytest.LogCaptureFixture
 ) -> None:
+    # Given
     config = EventProcessorConfig(events_api_url="http://test/")
 
     def default_flag_handler(feature_name: str) -> DefaultFlag:
@@ -1106,20 +1133,27 @@ def default_flag_handler(feature_name: str) -> DefaultFlag:
         body=json.dumps({"flags": [], "traits": []}),
     )
 
-    result = flagsmith.get_experiment_flag(
-        feature_name="missing_feature", identifier="user1"
-    )
+    # When
+    with caplog.at_level(logging.DEBUG, logger="flagsmith.flagsmith"):
+        result = flagsmith.get_experiment_flag(
+            feature_name="missing_feature", identifier="user1"
+        )
 
+    # Then
     assert isinstance(result, DefaultFlag)
     assert result.is_default is True
     assert result.value == "default-variant"
     mock_track.assert_not_called()
+    assert (
+        "Not sending $flag_exposure for feature missing_feature: feature not found."
+        in caplog.messages
+    )
 
 
-def test_get_experiment_flag_uses_variant_as_exposure_value(
+def test_get_experiment_flag__variant__used_as_exposure_value(
     mocker: MockerFixture, api_key: str
 ) -> None:
-    # Given - a resolved flag carrying a variant
+    # Given
     config = EventProcessorConfig(events_api_url="http://test/")
     flagsmith = Flagsmith(
         environment_key=api_key, enable_events=True, event_processor_config=config
@@ -1141,7 +1175,7 @@ def test_get_experiment_flag_uses_variant_as_exposure_value(
     # When
     flagsmith.get_experiment_flag(feature_name="checkout_v2", identifier="user1")
 
-    # Then - the exposure value is the variant, not the flag value
+    # Then
     mock_track.assert_called_once_with(
         feature_name="checkout_v2",
         identifier="user1",
@@ -1151,10 +1185,10 @@ def test_get_experiment_flag_uses_variant_as_exposure_value(
     )
 
 
-def test_get_experiment_flag_falls_back_to_value_without_variant(
-    mocker: MockerFixture, api_key: str
+def test_get_experiment_flag__no_variant__skips_exposure(
+    mocker: MockerFixture, api_key: str, caplog: pytest.LogCaptureFixture
 ) -> None:
-    # Given - a resolved flag with no variant
+    # Given
     config = EventProcessorConfig(events_api_url="http://test/")
     flagsmith = Flagsmith(
         environment_key=api_key, enable_events=True, event_processor_config=config
@@ -1174,15 +1208,17 @@ def test_get_experiment_flag_falls_back_to_value_without_variant(
     mock_track = mocker.patch.object(flagsmith._event_processor, "track_exposure_event")
 
     # When
-    flagsmith.get_experiment_flag(feature_name="checkout_v2", identifier="user1")
+    with caplog.at_level(logging.DEBUG, logger="flagsmith.flagsmith"):
+        result = flagsmith.get_experiment_flag(
+            feature_name="checkout_v2", identifier="user1"
+        )
 
-    # Then - the exposure value falls back to the flag value
-    mock_track.assert_called_once_with(
-        feature_name="checkout_v2",
-        identifier="user1",
-        value="blue",
-        traits=None,
-        metadata=None,
+    # Then
+    assert result is flag
+    mock_track.assert_not_called()
+    assert (
+        "Not sending $flag_exposure for feature checkout_v2: flag has no variant."
+        in caplog.messages
     )
 
 
@@ -1260,10 +1296,10 @@ def test_flagsmith_posts_analytics_to_api_url_when_analytics_url_unset(
     assert len(analytics_posts) == 1
 
 
-def test_get_experiment_flag_skips_exposure_for_disabled_feature(
-    mocker: MockerFixture, api_key: str
+def test_get_experiment_flag__disabled_feature__skips_exposure(
+    mocker: MockerFixture, api_key: str, caplog: pytest.LogCaptureFixture
 ) -> None:
-    # Given - a resolved flag for a disabled feature
+    # Given
     config = EventProcessorConfig(events_api_url="http://test/")
     flagsmith = Flagsmith(
         environment_key=api_key, enable_events=True, event_processor_config=config
@@ -1283,10 +1319,15 @@ def test_get_experiment_flag_skips_exposure_for_disabled_feature(
     mock_track = mocker.patch.object(flagsmith._event_processor, "track_exposure_event")
 
     # When
-    result = flagsmith.get_experiment_flag(
-        feature_name="checkout_v2", identifier="user1"
-    )
+    with caplog.at_level(logging.DEBUG, logger="flagsmith.flagsmith"):
+        result = flagsmith.get_experiment_flag(
+            feature_name="checkout_v2", identifier="user1"
+        )
 
-    # Then - the flag is returned but no exposure event is tracked
+    # Then
     assert result is flag
     mock_track.assert_not_called()
+    assert (
+        "Not sending $flag_exposure for feature checkout_v2: feature is disabled."
+        in caplog.messages
+    )