diff --git a/api/experimentation/services.py b/api/experimentation/services.py index abddebe6dc86..d1cd4935c44c 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -42,6 +42,7 @@ ) from experimentation.models import ( VALID_STATUS_TRANSITIONS, + Experiment, ExperimentStatus, MetricAggregation, MetricDirection, @@ -57,22 +58,29 @@ ) from features.models import FeatureState from features.value_types import BOOLEAN, INTEGER, STRING -from features.versioning.dataclasses import FlagChangeSet +from features.versioning.dataclasses import FlagChangeSet, MultivariateValueChangeSet from features.versioning.versioning_service import ( + get_environment_flags_list, update_flag, update_multivariate_values, ) from integrations.flagsmith.client import get_openfeature_client from segments.models import Condition, Segment, SegmentRule -_ROLLOUT_VALUE_TYPE = {INTEGER: "integer", STRING: "string", BOOLEAN: "boolean"} +_ROLLOUT_VALUE_TYPE: dict[str, "FeatureValueType"] = { + INTEGER: "integer", + STRING: "string", + BOOLEAN: "boolean", +} if typing.TYPE_CHECKING: from collections.abc import Sequence from datetime import datetime - from experimentation.models import Experiment, Metric, WarehouseConnection + from experimentation.models import Metric, WarehouseConnection from experimentation.types import ExposureGranularity + from features.feature_states.models import FeatureValueType + from features.models import FeatureStateValue from organisations.models import Organisation from users.models import FFAdminUser @@ -579,18 +587,15 @@ def _sync_rollout_segment(experiment: Experiment, rollout_percentage: float) -> def _get_live_rollout_override(experiment: Experiment) -> FeatureState | None: - return ( - FeatureState.objects.get_live_feature_states( - environment=experiment.environment, - additional_filters=Q( - feature_segment__segment_id=experiment.rollout_segment_id, - identity__isnull=True, - ), + flags = get_environment_flags_list( + environment=experiment.environment, + additional_filters=Q( feature_id=experiment.feature_id, - ) - .order_by("-id") - .first() + feature_segment__segment_id=experiment.rollout_segment_id, + identity__isnull=True, + ), ) + return flags[0] if flags else None def _update_live_feature_state( @@ -627,15 +632,60 @@ def _update_rollout_in_place(experiment: Experiment, change_set: FlagChangeSet) update_flag(experiment.environment, experiment.feature, change_set) +def _reset_default_allocations_to_control( + experiment: Experiment, author: AuthorData +) -> None: + """Zero every variant's allocation on the feature's environment-default + feature state, leaving control (the unallocated remainder) at 100%. + + Run once, when the rollout segment is first created: identities outside the + rollout cohort should all receive control while the experiment runs. + """ + (default_state,) = get_environment_flags_list( + environment=experiment.environment, + additional_filters=Q( + feature_id=experiment.feature_id, + feature_segment__isnull=True, + identity__isnull=True, + ), + ) + str_value, value_type = _serialize_feature_state_value( + default_state.feature_state_value + ) + update_flag( + experiment.environment, + experiment.feature, + FlagChangeSet( + author=author, + enabled=default_state.enabled, + feature_state_value=str_value, + type_=value_type, + multivariate_values=[ + MultivariateValueChangeSet( + multivariate_feature_option_id=option_id, + percentage_allocation=0, + ) + for option_id in experiment.feature.multivariate_options.values_list( + "id", flat=True + ) + ], + ), + ) + + def apply_experiment_rollout(experiment: Experiment, spec: RolloutSpec) -> None: - if experiment.status == ExperimentStatus.COMPLETED: - raise ValidationError( - f"Cannot change the rollout of a {experiment.status} experiment." - ) validate_rollout_spec(experiment, spec) environment_id = experiment.environment_id with transaction.atomic(): + experiment.refresh_from_db(from_queryset=Experiment.objects.select_for_update()) + if experiment.status == ExperimentStatus.COMPLETED: + raise ValidationError( + f"Cannot change the rollout of a {experiment.status} experiment." + ) + is_first_rollout = experiment.rollout_segment_id is None segment = _sync_rollout_segment(experiment, spec.rollout_percentage) + if is_first_rollout: + _reset_default_allocations_to_control(experiment, spec.author) _update_rollout_in_place( experiment, FlagChangeSet( @@ -655,6 +705,19 @@ def apply_experiment_rollout(experiment: Experiment, spec: RolloutSpec) -> None: ) +def _serialize_feature_state_value( + value: FeatureStateValue, +) -> tuple[str, FeatureValueType]: + """Render a stored feature state value as the (string, API type) pair that + a `FlagChangeSet` expects.""" + if value.value is None: + return "", "string" + return ( + str(value.value).lower() if value.type == BOOLEAN else str(value.value), + _ROLLOUT_VALUE_TYPE.get(value.type or STRING, "string"), + ) + + def get_experiment_rollout(experiment: Experiment) -> dict[str, typing.Any] | None: segment_id = experiment.rollout_segment_id if segment_id is None: @@ -671,16 +734,13 @@ def get_experiment_rollout(experiment: Experiment) -> dict[str, typing.Any] | No condition = Condition.objects.get( rule__segment_id=segment_id, operator=PERCENTAGE_SPLIT ) - value = feature_state.feature_state_value + str_value, value_type = _serialize_feature_state_value( + feature_state.feature_state_value + ) return { "enabled": feature_state.enabled, "rollout_percentage": float(condition.value or 0), - "feature_state_value": { - "type": _ROLLOUT_VALUE_TYPE.get(value.type or STRING, "string"), - "value": ( - str(value.value).lower() if value.type == BOOLEAN else str(value.value) - ), - }, + "feature_state_value": {"type": value_type, "value": str_value}, "multivariate_feature_state_values": [ { "multivariate_feature_option": mv.multivariate_feature_option_id, diff --git a/api/tests/unit/experimentation/test_experiment_views.py b/api/tests/unit/experimentation/test_experiment_views.py index 2b7ec0fe1b5d..a27762c29823 100644 --- a/api/tests/unit/experimentation/test_experiment_views.py +++ b/api/tests/unit/experimentation/test_experiment_views.py @@ -6,6 +6,7 @@ import pytest from django.db import IntegrityError +from django.db.models import Q from django.urls import reverse from django.utils import timezone from freezegun import freeze_time @@ -1926,6 +1927,135 @@ def test_post__with_experiment_rollout__creates_rollout( assert experiment.rollout_segment.is_system_segment is True +def test_post__with_experiment_rollout__zeroes_default_allocations( + admin_client_new: APIClient, + environment: Environment, + multivariate_feature: Feature, + multivariate_options: list[MultivariateFeatureOption], + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + option_a, option_b, option_c = multivariate_options + + # When + response = admin_client_new.post( + _list_url(environment), + data={ + "feature": multivariate_feature.id, + "name": "Rollout experiment", + "hypothesis": "It will work", + "experiment_rollout": { + "enabled": True, + "rollout_percentage": 30, + "feature_state_value": {"type": "string", "value": "control"}, + "multivariate_feature_state_values": [ + { + "multivariate_feature_option": option_a.id, + "percentage_allocation": 60, + }, + { + "multivariate_feature_option": option_b.id, + "percentage_allocation": 40, + }, + ], + }, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + experiment = Experiment.objects.get(id=response.json()["id"]) + env_default_state = FeatureState.objects.get( + feature=multivariate_feature, + environment=environment, + identity__isnull=True, + feature_segment__isnull=True, + ) + default_allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in env_default_state.multivariate_feature_state_values.all() + } + assert default_allocations == {option_a.id: 0, option_b.id: 0, option_c.id: 0} + + # The rollout segment override keeps the experiment's own split. + override = FeatureState.objects.get( + feature=multivariate_feature, + environment=environment, + feature_segment__segment=experiment.rollout_segment, + ) + override_allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in override.multivariate_feature_state_values.all() + } + assert override_allocations == {option_a.id: 60.0, option_b.id: 40.0} + + +def test_post__with_experiment_rollout_v2_versioning__zeroes_default_allocations( + admin_client_new: APIClient, + environment_v2_versioning: Environment, + multivariate_feature: Feature, + multivariate_options: list[MultivariateFeatureOption], + enable_features: EnableFeaturesFixture, +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + option_a, option_b, option_c = multivariate_options + + # When + response = admin_client_new.post( + _list_url(environment_v2_versioning), + data={ + "feature": multivariate_feature.id, + "name": "Rollout experiment", + "hypothesis": "It will work", + "experiment_rollout": { + "enabled": True, + "rollout_percentage": 30, + "feature_state_value": {"type": "string", "value": "control"}, + "multivariate_feature_state_values": [ + { + "multivariate_feature_option": option_a.id, + "percentage_allocation": 60, + }, + { + "multivariate_feature_option": option_b.id, + "percentage_allocation": 40, + }, + ], + }, + }, + format="json", + ) + + # Then the live environment default has its allocations zeroed + assert response.status_code == status.HTTP_201_CREATED + experiment = Experiment.objects.get(id=response.json()["id"]) + env_default_state = FeatureState.objects.get_live_feature_states( + environment=environment_v2_versioning, + additional_filters=Q(feature_segment__isnull=True, identity__isnull=True), + feature_id=multivariate_feature.id, + ).latest("id") + default_allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in env_default_state.multivariate_feature_state_values.all() + } + assert default_allocations == {option_a.id: 0, option_b.id: 0, option_c.id: 0} + + # and the live rollout segment override keeps the experiment's own split + override = FeatureState.objects.get_live_feature_states( + environment=environment_v2_versioning, + additional_filters=Q(feature_segment__segment=experiment.rollout_segment), + feature_id=multivariate_feature.id, + ).latest("id") + override_allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in override.multivariate_feature_state_values.all() + } + assert override_allocations == {option_a.id: 60.0, option_b.id: 40.0} + + def test_post__rollout_allocations_exceed_100__returns_400( admin_client_new: APIClient, environment: Environment, diff --git a/api/tests/unit/experimentation/test_services.py b/api/tests/unit/experimentation/test_services.py index 2f881ecbfc73..804de33159d3 100644 --- a/api/tests/unit/experimentation/test_services.py +++ b/api/tests/unit/experimentation/test_services.py @@ -1359,6 +1359,150 @@ def test_apply_experiment_rollout__no_segment__creates_segment_and_override( assert allocations == {option_a.id: 60.0, option_b.id: 40.0} +def test_apply_experiment_rollout__null_default_value__serialised_as_empty_string( + experiment: Experiment, + multivariate_options: list[MultivariateFeatureOption], + admin_user: FFAdminUser, +) -> None: + # Given the environment default has no value stored + option_a, option_b, _ = multivariate_options + default_state = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__isnull=True, + identity__isnull=True, + ) + default_state.feature_state_value.string_value = None + default_state.feature_state_value.save() + + # When + services.apply_experiment_rollout( + experiment, + RolloutSpec( + enabled=True, + rollout_percentage=42.0, + feature_state_value="control", + value_type="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 60.0), + MultivariateValueChangeSet(option_b.id, 40.0), + ], + author=AuthorData(user=admin_user), + ), + ) + + # Then the default value is not rewritten as the string "None" + default_state.refresh_from_db() + assert default_state.get_feature_state_value() == "" + + +def test_apply_experiment_rollout__first_rollout__zeroes_default_allocations( + experiment: Experiment, + multivariate_options: list[MultivariateFeatureOption], + admin_user: FFAdminUser, +) -> None: + # Given + option_a, option_b, option_c = multivariate_options + + # When + services.apply_experiment_rollout( + experiment, + RolloutSpec( + enabled=True, + rollout_percentage=42.0, + feature_state_value="control", + value_type="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 60.0), + MultivariateValueChangeSet(option_b.id, 40.0), + ], + author=AuthorData(user=admin_user), + ), + ) + + # Then + experiment.refresh_from_db() + default_state = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__isnull=True, + identity__isnull=True, + ) + default_allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in default_state.multivariate_feature_state_values.all() + } + assert default_allocations == {option_a.id: 0, option_b.id: 0, option_c.id: 0} + + # The rollout segment override keeps the experiment's own split. + override = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__segment=experiment.rollout_segment, + ) + override_allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in override.multivariate_feature_state_values.all() + } + assert override_allocations == {option_a.id: 60.0, option_b.id: 40.0} + + +def test_apply_experiment_rollout__existing_segment__leaves_default_allocations( + experiment_with_rollout: Experiment, + multivariate_options: list[MultivariateFeatureOption], + admin_user: FFAdminUser, +) -> None: + # Given + experiment = experiment_with_rollout + option_a, option_b, _ = multivariate_options + default_state = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__isnull=True, + identity__isnull=True, + ) + # A later manual edit to the default allocations must survive a rollout update. + default_state.multivariate_feature_state_values.filter( + multivariate_feature_option=option_a + ).update(percentage_allocation=25.0) + + # When + services.apply_experiment_rollout( + experiment, + RolloutSpec( + enabled=True, + rollout_percentage=80.0, + feature_state_value="control", + value_type="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 50.0), + MultivariateValueChangeSet(option_b.id, 50.0), + ], + author=AuthorData(user=admin_user), + ), + ) + + # Then the manual edit to the default allocations survives... + allocation = default_state.multivariate_feature_state_values.get( + multivariate_feature_option=option_a + ).percentage_allocation + assert allocation == 25.0 + + # ...while the rollout update itself is applied + condition = Condition.objects.get(rule__segment=experiment.rollout_segment) + assert condition.value == "80.0" + override = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__segment=experiment.rollout_segment, + ) + override_allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in override.multivariate_feature_state_values.all() + } + assert override_allocations == {option_a.id: 50.0, option_b.id: 50.0} + + def test_apply_experiment_rollout__existing_segment__updates_percentage_and_enabled( experiment_with_rollout: Experiment, multivariate_options: list[MultivariateFeatureOption], diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 21ceb55de3a7..398503624dd4 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -532,7 +532,7 @@ Attributes: ### `warehouse.connection.connected` Logged at `info` from: - - `api/experimentation/services.py:742` + - `api/experimentation/services.py:802` Attributes: - `environment.id` @@ -541,7 +541,7 @@ Attributes: ### `warehouse.connection.test_event_sent` Logged at `info` from: - - `api/experimentation/services.py:722` + - `api/experimentation/services.py:782` Attributes: - `environment.id` @@ -550,7 +550,7 @@ Attributes: ### `warehouse.srm.overallocated` Logged at `error` from: - - `api/experimentation/services.py:396` + - `api/experimentation/services.py:404` Attributes: - `environment.id` @@ -560,7 +560,7 @@ Attributes: ### `warehouse.srm.unkeyed_variant` Logged at `error` from: - - `api/experimentation/services.py:382` + - `api/experimentation/services.py:390` Attributes: - `environment.id`