Skip to content

Parked: fix(experimentation): test SRM against the rollout override's split #8798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Zaimwa9 wants to merge 3 commits into
base: main
Choose a base branch
from
+134 −62
Draft

Parked: fix(experimentation): test SRM against the rollout override's split #8798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b3e9cc
fix(experimentation): test SRM against the rollout override's split
Zaimwa9 Oct 15, 2026
a956289
Merge remote-tracking branch 'origin/main' into fix/experiment-srm-ro…
Zaimwa9 Oct 16, 2026
bb65e7e
chore: Update documentation artefacts
flagsmith-engineering[bot] Oct 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions api/experimentation/services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,14 +355,19 @@ def _experiment_metric_specs(experiment: "Experiment") -> list[MetricSpec]:

def _expected_variant_shares(experiment: "Experiment") -> dict[str, float]:
"""The traffic split SRM tests against: each multivariate option's
environment allocation, with ``control`` taking the unallocated remainder.
Empty when the feature has no usable allocations, skipping the SRM check."""
# TODO: read the split from the percentage-split segment override feature
# state once that's implemented, rather than the environment default.
allocation on the rollout override, with ``control`` taking the unallocated
remainder. Only the rollout cohort records exposures, so the environment
default (zeroed once the rollout exists) doesn't count. Empty when there is
no usable split, skipping the SRM check."""
if experiment.rollout_segment_id is None:
return {}
feature_state = (
FeatureState.objects.get_live_feature_states(
environment=experiment.environment,
additional_filters=Q(feature_segment__isnull=True, identity__isnull=True),
additional_filters=Q(
feature_segment__segment_id=experiment.rollout_segment_id,
identity__isnull=True,
),
Comment on lines +367 to +370

@coderabbitai coderabbitai Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Account for split changes within the results window.

If a running experiment changes its variant weights, this query returns only the current split. build_results_summary compares that split with exposures aggregated across the results window. Exposures assigned under the earlier split can therefore produce a false SRM alert. Use allocation history for the exposure window, or restrict the check to exposures collected under one stable split.

feature_id=experiment.feature_id,
)
.prefetch_related(
Expand All @@ -388,8 +393,10 @@ def _expected_variant_shares(experiment: "Experiment") -> dict[str, float]:
feature__id=experiment.feature_id,
)
return {}
shares[key] = mv_value.percentage_allocation / 100
allocated += mv_value.percentage_allocation
if mv_value.percentage_allocation > 0:
# A zero share has no expected traffic to test against.
shares[key] = mv_value.percentage_allocation / 100
Comment on lines +397 to +399

@coderabbitai coderabbitai Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

rg -n 'expected_shares|srm_p_value' api/experimentation/services.py
git diff b6d69889652df199a21dfa9f64a2c8a7550936f0 bb65e7e3e3d7e565e70dcf30058e3e5cffd3647d -- api/experimentation/services.py

Repository: Flagsmith/flagsmith

Length of output: 3006

🏁 Script executed:

set -eu
printf '%s\n' '--- current services.py relevant block ---'
nl -ba api/experimentation/services.py | sed -n '250,420p'
printf '%s\n' '--- merge-base services.py relevant block ---'
git show b6d69889652df199a21dfa9f64a2c8a7550936f0:api/experimentation/services.py | nl -ba | sed -n '250,420p'
printf '%s\n' '--- srm_p_value declaration and implementation references ---'
rg -n -F --glob '*.py' -e 'def srm_p_value' -e 'srm_p_value(' .
printf '%s\n' '--- exposure_counts construction and relevant tests ---'
rg -n -F --glob '*.py' -e 'exposure_counts' -e 'expected_shares' -e 'Sample ratio mismatch' tests api

Repository: Flagsmith/flagsmith

Length of output: 21276

🏁 Script executed:

set -eu
printf '%s\n' '--- srm_p_value implementation ---'
nl -ba api/experimentation/stats.py | sed -n '80,115p'
printf '%s\n' '--- focused build_results_summary tests ---'
nl -ba api/tests/unit/experimentation/test_services.py | sed -n '1160,1355p'
printf '%s\n' '--- exposure aggregation implementation ---'
nl -ba api/experimentation/results_query.py | sed -n '285,325p'

Repository: Flagsmith/flagsmith

Length of output: 11975

Flag exposures for variants outside expected_shares.

When a zero-weight variant has exposures, _expected_variant_shares excludes it. build_results_summary then omits its count from srm_p_value, which can hide an SRM mismatch.

Do not restore a zero share directly. srm_p_value returns None for non-positive expected shares. Detect positive exposure counts for keys absent from expected_shares and report a mismatch instead.

Suggested fix 
     if expected_shares and total >= SRM_MIN_TOTAL_IDENTITIES:
-        srm = srm_p_value(
-            [exposure_counts.get(variant, 0) for variant in expected_shares],
-            list(expected_shares.values()),
-        )
+        has_unexpected_exposures = any(
+            count > 0
+            for variant, count in exposure_counts.items()
+            if variant not in expected_shares
+        )
+        if has_unexpected_exposures:
+            srm = 0.0
+        else:
+            srm = srm_p_value(
+                [exposure_counts.get(variant, 0) for variant in expected_shares],
+                list(expected_shares.values()),
+            )

if not shares:
return {}
if allocated > 100:
Expand All @@ -402,7 +409,8 @@ def _expected_variant_shares(experiment: "Experiment") -> dict[str, float]:
feature__id=experiment.feature_id,
)
return {}
shares[CONTROL_VARIANT_KEY] = (100 - allocated) / 100
if allocated < 100:
shares[CONTROL_VARIANT_KEY] = (100 - allocated) / 100
return shares


Expand Down
158 changes: 111 additions & 47 deletions api/tests/unit/experimentation/test_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@
from features.feature_types import MULTIVARIATE
from features.future.services import update_flag
from features.models import Feature, FeatureState
from features.multivariate.models import MultivariateFeatureOption
from features.multivariate.models import (
MultivariateFeatureOption,
MultivariateFeatureStateValue,
)
from features.value_types import STRING
from organisations.models import Organisation
from projects.models import Project
Expand Down Expand Up @@ -1569,51 +1572,102 @@ def _multivariate_feature(
return feature


@pytest.mark.django_db
def test_expected_variant_shares__keyed_options__control_takes_remainder(
def _split_experiment(
environment: Environment,
) -> None:
# Given a multivariate feature whose options are allocated 30% and 20%
feature = _multivariate_feature(environment, {"variant_a": 30, "variant_b": 20})
experiment = Experiment.objects.create(
allocations: dict[str | None, int],
author: FFAdminUser,
*,
rollout_percentage: float = 100.0,
) -> Experiment:
"""An experiment on a fresh multivariate feature whose rollout splits its
cohort by ``allocations``, as do the options' environment defaults."""
feature = _multivariate_feature(environment, allocations)
experiment: Experiment = Experiment.objects.create(
environment=environment,
feature=feature,
name="exp",
hypothesis="h",
status=ExperimentStatus.RUNNING,
)
services.apply_experiment_rollout(
experiment,
RolloutSpec(
enabled=True,
rollout_percentage=rollout_percentage,
feature_state_value="control",
value_type="string",
variants=[
{"id": option.id, "weight": option.default_percentage_allocation}
for option in feature.multivariate_options.all()
],
author=AuthorData(user=author),
),
)
return experiment


@pytest.mark.parametrize(
"allocations, rollout_percentage, expected_shares",
[
({"treatment": 50}, 100.0, {"treatment": 0.5, "control": 0.5}),
(
{"variant_a": 30, "variant_b": 20},
20.0,
{"variant_a": 0.3, "variant_b": 0.2, "control": 0.5},
),
(
{"variant_a": 50, "variant_b": 50},
100.0,
{"variant_a": 0.5, "variant_b": 0.5},
),
(
{"variant_a": 50, "variant_b": 0},
100.0,
{"variant_a": 0.5, "control": 0.5},
),
],
ids=["control_remainder", "partial_rollout", "fully_allocated", "zero_weight"],
)
@pytest.mark.django_db
def test_expected_variant_shares__rollout_applied__returns_rollout_split(
environment: Environment,
admin_user: FFAdminUser,
allocations: dict[str | None, int],
rollout_percentage: float,
expected_shares: dict[str, float],
) -> None:
# Given an experiment whose rollout has zeroed the environment default's
# allocations, leaving the split on the rollout override alone
experiment = _split_experiment(
environment, allocations, admin_user, rollout_percentage=rollout_percentage
)

# When
shares = services._expected_variant_shares(experiment)

# Then control takes the unallocated remainder
assert shares == pytest.approx({"variant_a": 0.3, "variant_b": 0.2, "control": 0.5})
# Then the rollout cohort's split is tested, with control taking any
# remainder and zero-weight variants left out
assert shares == pytest.approx(expected_shares)


@pytest.mark.django_db
def test_expected_variant_shares__null_option_keys__returns_empty(
experiment: Experiment,
experiment_with_rollout: Experiment,
) -> None:
# Given the experiment's multivariate options carry no variant keys

# When / Then the split can't be described, so SRM is skipped
assert services._expected_variant_shares(experiment) == {}
assert services._expected_variant_shares(experiment_with_rollout) == {}


@pytest.mark.django_db
def test_expected_variant_shares__mixed_keyed_and_null_options__returns_empty(
environment: Environment,
admin_user: FFAdminUser,
log: StructuredLogCapture,
) -> None:
# Given a multivariate feature with one keyed and one unkeyed option
feature = _multivariate_feature(environment, {"variant_a": 30, None: 30})
experiment = Experiment.objects.create(
environment=environment,
feature=feature,
name="exp",
hypothesis="h",
status=ExperimentStatus.RUNNING,
)
# Given a rollout splitting between one keyed and one unkeyed option
experiment = _split_experiment(environment, {"variant_a": 30, None: 30}, admin_user)

# When / Then the unkeyed option's share can't be attributed, so rather than
# folding it into control SRM is skipped entirely and the gap is logged
Expand All @@ -1630,17 +1684,17 @@ def test_expected_variant_shares__mixed_keyed_and_null_options__returns_empty(
@pytest.mark.django_db
def test_expected_variant_shares__overallocated_options__returns_empty(
environment: Environment,
admin_user: FFAdminUser,
log: StructuredLogCapture,
) -> None:
# Given a misconfigured feature whose options allocate more than 100%
feature = _multivariate_feature(environment, {"variant_a": 70, "variant_b": 60})
experiment = Experiment.objects.create(
environment=environment,
feature=feature,
name="exp",
hypothesis="h",
status=ExperimentStatus.RUNNING,
# Given a rollout override whose options were pushed past 100% in total
experiment = _split_experiment(
environment, {"variant_a": 50, "variant_b": 50}, admin_user
)
MultivariateFeatureStateValue.objects.filter(
feature_state__feature_segment__segment_id=experiment.rollout_segment_id,
multivariate_feature_option__key="variant_a",
).update(percentage_allocation=70)

# When / Then control's share would be negative, so SRM is skipped and logged
assert services._expected_variant_shares(experiment) == {}
Expand All @@ -1654,11 +1708,26 @@ def test_expected_variant_shares__overallocated_options__returns_empty(


@pytest.mark.django_db
def test_expected_variant_shares__no_multivariate_options__returns_empty(
def test_expected_variant_shares__all_variants_zero_weight__returns_empty(
environment: Environment,
admin_user: FFAdminUser,
) -> None:
# Given a rollout that sends its whole cohort to control
experiment = _split_experiment(
environment, {"variant_a": 0, "variant_b": 0}, admin_user
)

# When / Then there is no split to test
assert services._expected_variant_shares(experiment) == {}


@pytest.mark.django_db
def test_expected_variant_shares__no_rollout__returns_empty(
environment: Environment,
feature: Feature,
) -> None:
# Given a standard feature with no multivariate allocations
# Given an experiment whose feature splits traffic by default, but which
# has no rollout, so no identity is in the experiment
feature = _multivariate_feature(environment, {"variant_a": 50})
experiment = Experiment.objects.create(
environment=environment,
feature=feature,
Expand All @@ -1667,22 +1736,21 @@ def test_expected_variant_shares__no_multivariate_options__returns_empty(
status=ExperimentStatus.RUNNING,
)

# When / Then there is no split to test
# When / Then the environment default's split isn't tested
assert services._expected_variant_shares(experiment) == {}


@pytest.mark.django_db
def test_expected_variant_shares__no_live_feature_state__returns_empty(
experiment: Experiment,
def test_expected_variant_shares__no_live_rollout_override__returns_empty(
experiment_with_rollout: Experiment,
) -> None:
# Given the feature has no live state in the environment
# Given the rollout segment's override is gone
FeatureState.objects.filter(
feature=experiment.feature,
environment=experiment.environment,
feature_segment__segment_id=experiment_with_rollout.rollout_segment_id,
).delete()

# When / Then
assert services._expected_variant_shares(experiment) == {}
assert services._expected_variant_shares(experiment_with_rollout) == {}


COMPUTE_SUMMARIES = pytest.mark.parametrize(
Expand Down Expand Up @@ -1765,17 +1833,13 @@ def test_compute_summary__deleted_connection__reads_managed_store(
@pytest.mark.django_db
def test_compute_results_summary__experiment__queries_warehouse_and_builds(
environment: Environment,
admin_user: FFAdminUser,
mocker: MockerFixture,
) -> None:
# Given a running experiment with one keyed variant and one attached metric
feature = _multivariate_feature(environment, {"variant_a": 50})
experiment = Experiment.objects.create(
environment=environment,
feature=feature,
name="exp",
hypothesis="h",
status=ExperimentStatus.RUNNING,
)
# Given a running experiment rolled out to one keyed variant, with one
# attached metric
experiment = _split_experiment(environment, {"variant_a": 50}, admin_user)
feature = experiment.feature
metric = Metric.objects.create(
environment=environment,
name="Purchases",
Expand Down
16 changes: 8 additions & 8 deletions docs/docs/deployment-self-hosting/observability/_events-catalogue.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ Attributes:
### `experimentation.rollout.applied`

Logged at `info` from:
- `api/experimentation/services.py:1018`
- `api/experimentation/services.py:1026`

Attributes:
- `audience.match`
Expand All @@ -256,7 +256,7 @@ Attributes:
### `experimentation.rollout.superseded`

Logged at `info` from:
- `api/experimentation/services.py:609`
- `api/experimentation/services.py:617`

Attributes:
- `environment.id`
Expand Down Expand Up @@ -906,7 +906,7 @@ Attributes:
### `warehouse.connection.connected`

Logged at `info` from:
- `api/experimentation/services.py:1194`
- `api/experimentation/services.py:1202`

Attributes:
- `environment.id`
Expand Down Expand Up @@ -935,7 +935,7 @@ Attributes:
### `warehouse.connection.test_event_sent`

Logged at `info` from:
- `api/experimentation/services.py:1137`
- `api/experimentation/services.py:1145`

Attributes:
- `environment.id`
Expand All @@ -944,7 +944,7 @@ Attributes:
### `warehouse.connection.verification_failed`

Logged at `warning` from:
- `api/experimentation/services.py:1169`
- `api/experimentation/services.py:1177`

Attributes:
- `environment.id`
Expand All @@ -954,7 +954,7 @@ Attributes:
### `warehouse.connection.verification_succeeded`

Logged at `info` from:
- `api/experimentation/services.py:1179`
- `api/experimentation/services.py:1187`

Attributes:
- `environment.id`
Expand All @@ -963,7 +963,7 @@ Attributes:
### `warehouse.srm.overallocated`

Logged at `error` from:
- `api/experimentation/services.py:398`
- `api/experimentation/services.py:405`

Attributes:
- `environment.id`
Expand All @@ -973,7 +973,7 @@ Attributes:
### `warehouse.srm.unkeyed_variant`

Logged at `error` from:
- `api/experimentation/services.py:384`
- `api/experimentation/services.py:389`

Attributes:
- `environment.id`
Expand Down
Loading