diff --git a/flagsmith/analytics.py b/flagsmith/analytics.py index 634dd27..a26a9b4 100644 --- a/flagsmith/analytics.py +++ b/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: diff --git a/flagsmith/flagsmith.py b/flagsmith/flagsmith.py index f10350e..cd060ca 100644 --- a/flagsmith/flagsmith.py +++ b/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 diff --git a/tests/data/identities.json b/tests/data/identities.json index c829bfd..bec5461 100644 --- a/tests/data/identities.json +++ b/tests/data/identities.json @@ -20,6 +20,7 @@ "project": 1 }, "feature_state_value": "some-value", + "variant": "treatment", "enabled": true, "environment": 1, "identity": null, diff --git a/tests/test_event_processor.py b/tests/test_event_processor.py index 5e2c80f..6a1bc2a 100644 --- a/tests/test_event_processor.py +++ b/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") diff --git a/tests/test_flagsmith.py b/tests/test_flagsmith.py index 285a96d..6cdd6cd 100644 --- a/tests/test_flagsmith.py +++ b/tests/test_flagsmith.py @@ -1,4 +1,5 @@ import json +import logging import time import typing @@ -1054,9 +1055,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 +1067,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 +1112,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 +1154,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 +1164,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 +1187,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 +1275,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 +1298,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 + )