From c961486ad1b004dd91932efa9ced8acb1ab4c54a Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 2 Jun 2026 16:09:00 +0530 Subject: [PATCH 1/9] feat(experimentation): environment-scoped metrics & experiment-metric attachment Add a reusable, environment-scoped Metric and the ExperimentMetric join that attaches metrics to experiments. - Models: Metric (numeric; count/sum/mean/occurrence aggregations + JSON definition), ExperimentMetric (expected_direction; one attach per experiment+metric); Experiment gains exposure_event ($flag_exposure) and control_variant. - Metric library CRUD under environments/{key}/experiment-metrics/, gated on EXPERIMENT_FLAG + environment admin. Metrics are immutable for now (no update); deletion blocked while attached to an active experiment. - Attach/detach metrics under an experiment, with same-environment and unique-attach validation. Results computation (ClickHouse query builder + statistics) is intentionally kept on a separate branch; this branch is models + API only. --- api/audit/related_object_type.py | 1 + api/environments/urls.py | 4 + api/experimentation/experiment_urls.py | 13 +- api/experimentation/metric_urls.py | 10 + .../migrations/0005_metrics.py | 124 ++++++++ api/experimentation/models.py | 60 ++++ api/experimentation/permissions.py | 16 + api/experimentation/serializers.py | 80 +++++ api/experimentation/services.py | 18 +- api/experimentation/views.py | 91 ++++++ .../test_experiment_metric_views.py | 208 ++++++++++++ .../experimentation/test_metric_models.py | 98 ++++++ .../unit/experimentation/test_metric_views.py | 295 ++++++++++++++++++ 13 files changed, 1013 insertions(+), 5 deletions(-) create mode 100644 api/experimentation/metric_urls.py create mode 100644 api/experimentation/migrations/0005_metrics.py create mode 100644 api/tests/unit/experimentation/test_experiment_metric_views.py create mode 100644 api/tests/unit/experimentation/test_metric_models.py create mode 100644 api/tests/unit/experimentation/test_metric_views.py diff --git a/api/audit/related_object_type.py b/api/audit/related_object_type.py index 853011eb38f4..53f6d0a9fbe3 100644 --- a/api/audit/related_object_type.py +++ b/api/audit/related_object_type.py @@ -14,3 +14,4 @@ class RelatedObjectType(enum.Enum): RELEASE_PIPELINE = "Release pipeline" WAREHOUSE_CONNECTION = "Warehouse connection" EXPERIMENT = "Experiment" + METRIC = "Metric" diff --git a/api/environments/urls.py b/api/environments/urls.py index 348594936a45..be71c2179840 100644 --- a/api/environments/urls.py +++ b/api/environments/urls.py @@ -181,4 +181,8 @@ "/experiments/", include("experimentation.experiment_urls"), ), + path( + "/experiment-metrics/", + include("experimentation.metric_urls"), + ), ] diff --git a/api/experimentation/experiment_urls.py b/api/experimentation/experiment_urls.py index 0022909c55dd..f3c9814ff64a 100644 --- a/api/experimentation/experiment_urls.py +++ b/api/experimentation/experiment_urls.py @@ -1,10 +1,15 @@ -from rest_framework.routers import DefaultRouter +from rest_framework_nested import routers # type: ignore[import-untyped] -from experimentation.views import ExperimentViewSet +from experimentation.views import ExperimentMetricViewSet, ExperimentViewSet app_name = "experiments" -router = DefaultRouter() +router = routers.DefaultRouter() router.register(r"", ExperimentViewSet, basename="experiments") -urlpatterns = router.urls +experiments_router = routers.NestedSimpleRouter(router, r"", lookup="experiment") +experiments_router.register( + r"metrics", ExperimentMetricViewSet, basename="experiment-metrics" +) + +urlpatterns = router.urls + experiments_router.urls diff --git a/api/experimentation/metric_urls.py b/api/experimentation/metric_urls.py new file mode 100644 index 000000000000..e2c6eec3b212 --- /dev/null +++ b/api/experimentation/metric_urls.py @@ -0,0 +1,10 @@ +from rest_framework.routers import DefaultRouter + +from experimentation.views import MetricViewSet + +app_name = "experiment_metrics" + +router = DefaultRouter() +router.register(r"", MetricViewSet, basename="metrics") + +urlpatterns = router.urls diff --git a/api/experimentation/migrations/0005_metrics.py b/api/experimentation/migrations/0005_metrics.py new file mode 100644 index 000000000000..aefebe75b7db --- /dev/null +++ b/api/experimentation/migrations/0005_metrics.py @@ -0,0 +1,124 @@ +# Generated by Django 5.2.14 on 2026-06-02 10:47 + +import django.db.models.deletion +import uuid +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("environments", "0037_add_uuid_field"), + ("experimentation", "0004_experiment"), + ] + + operations = [ + migrations.CreateModel( + name="Metric", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "deleted_at", + models.DateTimeField( + blank=True, + db_index=True, + default=None, + editable=False, + null=True, + ), + ), + ( + "uuid", + models.UUIDField(default=uuid.uuid4, editable=False, unique=True), + ), + ("name", models.CharField(max_length=255)), + ("description", models.TextField(blank=True, default="")), + ( + "aggregation", + models.CharField( + choices=[ + ("count", "Count"), + ("sum", "Sum"), + ("mean", "Mean"), + ("occurrence", "Occurrence (event happened at least once)"), + ], + default="mean", + max_length=20, + ), + ), + ("definition", models.JSONField()), + ("created_at", models.DateTimeField(auto_now_add=True)), + ("updated_at", models.DateTimeField(auto_now=True)), + ( + "environment", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="metrics", + to="environments.environment", + ), + ), + ], + options={ + "abstract": False, + }, + ), + migrations.CreateModel( + name="ExperimentMetric", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "expected_direction", + models.CharField( + choices=[ + ("increase", "Increase"), + ("decrease", "Decrease"), + ("not_increase", "Should not increase"), + ("not_decrease", "Should not decrease"), + ], + max_length=20, + ), + ), + ("created_at", models.DateTimeField(auto_now_add=True)), + ( + "experiment", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="experiment_metrics", + to="experimentation.experiment", + ), + ), + ( + "metric", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="experiment_metrics", + to="experimentation.metric", + ), + ), + ], + options={ + "constraints": [ + models.UniqueConstraint( + fields=("experiment", "metric"), + name="metric_attached_once_per_experiment", + ) + ], + }, + ), + ] diff --git a/api/experimentation/models.py b/api/experimentation/models.py index c2c1f9712b1e..59975c07cc3e 100644 --- a/api/experimentation/models.py +++ b/api/experimentation/models.py @@ -117,3 +117,63 @@ class Meta: name="unique_active_experiment_per_feature_env", ), ] + + +class MetricAggregation(models.TextChoices): + COUNT = "count", "Count" + SUM = "sum", "Sum" + MEAN = "mean", "Mean" + OCCURRENCE = "occurrence", "Occurrence (event happened at least once)" + + +class ExpectedDirection(models.TextChoices): + INCREASE = "increase", "Increase" + DECREASE = "decrease", "Decrease" + NOT_INCREASE = "not_increase", "Should not increase" + NOT_DECREASE = "not_decrease", "Should not decrease" + + +class Metric(SoftDeleteExportableModel): + environment = models.ForeignKey( + Environment, + on_delete=models.CASCADE, + related_name="metrics", + ) + name = models.CharField(max_length=255) + description = models.TextField(blank=True, default="") + aggregation = models.CharField( + max_length=20, + choices=MetricAggregation.choices, + default=MetricAggregation.MEAN, + ) + definition: models.JSONField[dict[str, object], dict[str, object]] = ( + models.JSONField() + ) + created_at = models.DateTimeField(auto_now_add=True) + updated_at = models.DateTimeField(auto_now=True) + + +class ExperimentMetric(models.Model): + experiment = models.ForeignKey( + Experiment, + on_delete=models.CASCADE, + related_name="experiment_metrics", + ) + metric = models.ForeignKey( + Metric, + on_delete=models.CASCADE, + related_name="experiment_metrics", + ) + expected_direction = models.CharField( + max_length=20, + choices=ExpectedDirection.choices, + ) + created_at = models.DateTimeField(auto_now_add=True) + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["experiment", "metric"], + name="metric_attached_once_per_experiment", + ), + ] diff --git a/api/experimentation/permissions.py b/api/experimentation/permissions.py index e1fdc8035101..d7599021ffbd 100644 --- a/api/experimentation/permissions.py +++ b/api/experimentation/permissions.py @@ -40,3 +40,19 @@ def has_permission(self, request: Request, view: APIView) -> bool: user: FFAdminUser = request.user # type: ignore[assignment] return user.is_environment_admin(environment) + + +class MetricPermission(BasePermission): + def has_permission(self, request: Request, view: APIView) -> bool: + try: + environment = Environment.objects.get( + api_key=view.kwargs.get("environment_api_key") + ) + except Environment.DoesNotExist: + return False + + if not is_experiment_feature_enabled(environment.project.organisation): + return False + + user: FFAdminUser = request.user # type: ignore[assignment] + return user.is_environment_admin(environment) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index f0fefecf0a92..c55cee19961b 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -5,6 +5,8 @@ from environments.models import Environment from experimentation.models import ( Experiment, + ExperimentMetric, + Metric, WarehouseConnection, WarehouseType, ) @@ -76,6 +78,84 @@ def _validate_snowflake_config(config: dict[str, Any]) -> SnowflakeConfig: return merged +class MetricSerializer(serializers.ModelSerializer): # type: ignore[type-arg] + class Meta: + model = Metric + fields = ( + "id", + "name", + "description", + "aggregation", + "definition", + "created_at", + "updated_at", + ) + read_only_fields = ( + "id", + "created_at", + "updated_at", + ) + + def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: + definition: Any = attrs.get( + "definition", getattr(self.instance, "definition", None) + ) + error = self._validate_definition(definition) + if error: + raise serializers.ValidationError({"definition": error}) + return attrs + + @staticmethod + def _validate_definition(definition: Any) -> str | None: + if not isinstance(definition, dict): + return "Definition must be an object." + + event = definition.get("event") + if not event or not isinstance(event, str): + return "Definition must specify a non-empty 'event'." + + return None + + +class ExperimentMetricSerializer(serializers.ModelSerializer): # type: ignore[type-arg] + metric = serializers.PrimaryKeyRelatedField( # type: ignore[var-annotated] + queryset=Metric.objects.all(), + ) + metric_name = serializers.CharField(source="metric.name", read_only=True) + aggregation = serializers.CharField(source="metric.aggregation", read_only=True) + + class Meta: + model = ExperimentMetric + fields = ( + "id", + "metric", + "metric_name", + "aggregation", + "expected_direction", + "created_at", + ) + read_only_fields = ("id", "created_at") + + def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: + experiment: Experiment = self.context["experiment"] + metric: Metric = attrs.get("metric", getattr(self.instance, "metric", None)) + + if metric.environment_id != experiment.environment_id: + raise serializers.ValidationError( + {"metric": "Metric must belong to the experiment's environment."} + ) + + attached = experiment.experiment_metrics.all() + if isinstance(self.instance, ExperimentMetric): + attached = attached.exclude(pk=self.instance.pk) + + if "metric" in attrs and attached.filter(metric=metric).exists(): + raise serializers.ValidationError( + {"metric": "Metric is already attached to this experiment."} + ) + return attrs + + class ExperimentSerializer(serializers.ModelSerializer): # type: ignore[type-arg] class Meta: model = Experiment diff --git a/api/experimentation/services.py b/api/experimentation/services.py index b7137658269a..50ae079a9557 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -13,7 +13,7 @@ from integrations.flagsmith.client import get_openfeature_client if typing.TYPE_CHECKING: - from experimentation.models import Experiment, WarehouseConnection + from experimentation.models import Experiment, Metric, WarehouseConnection from organisations.models import Organisation from users.models import FFAdminUser @@ -83,6 +83,22 @@ def create_warehouse_audit_log( ) +def create_metric_audit_log( + metric: Metric, + user: FFAdminUser, + *, + action: str, +) -> None: + AuditLog.objects.create( + environment=metric.environment, + project=metric.environment.project, + **_resolve_audit_log_author(user), + related_object_id=metric.id, + related_object_type=RelatedObjectType.METRIC.name, + log=f"Metric '{metric.name}' {action}", + ) + + def create_experiment_audit_log( experiment: Experiment, user: FFAdminUser, diff --git a/api/experimentation/views.py b/api/experimentation/views.py index c24b812effc4..81a35e026c7b 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -9,25 +9,32 @@ from rest_framework.request import Request from rest_framework.response import Response from rest_framework.serializers import BaseSerializer +from rest_framework.viewsets import GenericViewSet from app.pagination import CustomPagination from environments.views import NestedEnvironmentViewSet from experimentation.models import ( Experiment, + ExperimentMetric, ExperimentStatus, + Metric, WarehouseConnection, ) from experimentation.permissions import ( ExperimentPermission, + MetricPermission, WarehouseConnectionPermission, ) from experimentation.serializers import ( ExperimentListSerializer, + ExperimentMetricSerializer, ExperimentSerializer, + MetricSerializer, WarehouseConnectionSerializer, ) from experimentation.services import ( create_experiment_audit_log, + create_metric_audit_log, create_warehouse_audit_log, transition_experiment_status, ) @@ -251,3 +258,87 @@ def _transition_status(self, target_status: str) -> Response: @staticmethod def _get_user(request: Request) -> FFAdminUser: return request.user # type: ignore[return-value] + + +class ExperimentMetricViewSet( + mixins.ListModelMixin, + mixins.CreateModelMixin, + mixins.RetrieveModelMixin, + mixins.UpdateModelMixin, + mixins.DestroyModelMixin, + GenericViewSet[ExperimentMetric], +): + serializer_class = ExperimentMetricSerializer + pagination_class = None + permission_classes = [IsAuthenticated, ExperimentPermission] + lookup_field = "id" + lookup_url_kwarg = "pk" + + # The nested router derives this kwarg from the parent's lookup_url_kwarg + # ("experiment_id") plus its "experiment_" nest prefix. + experiment_url_kwarg = "experiment_experiment_id" + + def get_queryset(self) -> "QuerySet[ExperimentMetric]": + return ExperimentMetric.objects.filter( + experiment_id=self.kwargs[self.experiment_url_kwarg], + experiment__environment__api_key=self.kwargs["environment_api_key"], + ).select_related("metric") + + def get_serializer_context(self) -> dict[str, Any]: + context = super().get_serializer_context() + context["experiment"] = self._get_experiment() + return context + + def _get_experiment(self) -> Experiment: + experiment: Experiment = Experiment.objects.get( + id=self.kwargs[self.experiment_url_kwarg], + environment__api_key=self.kwargs["environment_api_key"], + ) + return experiment + + def perform_create(self, serializer: BaseSerializer[ExperimentMetric]) -> None: + serializer.save(experiment=self._get_experiment()) + + +class MetricViewSet( + NestedEnvironmentViewSet[Metric], + mixins.ListModelMixin, + mixins.CreateModelMixin, + mixins.RetrieveModelMixin, + mixins.DestroyModelMixin, +): + # Metrics are environment-scoped and immutable for now (no update action). + serializer_class = MetricSerializer + pagination_class = CustomPagination + permission_classes = [IsAuthenticated, MetricPermission] + model_class = Metric + lookup_field = "id" + lookup_url_kwarg = "pk" + + def perform_create(self, serializer: BaseSerializer[Metric]) -> None: + metric: Metric = serializer.save(environment=self._get_environment()) + create_metric_audit_log(metric, self._get_user(self.request), action="created") + + def destroy(self, request: Request, *args: object, **kwargs: object) -> Response: + instance: Metric = self.get_object() + if ( + ExperimentMetric.objects.filter(metric=instance) + .exclude(experiment__status=ExperimentStatus.COMPLETED) + .exists() + ): + return Response( + { + "detail": ( + "Cannot delete a metric attached to an active experiment. " + "Detach it or complete the experiment first." + ) + }, + status=status.HTTP_409_CONFLICT, + ) + create_metric_audit_log(instance, self._get_user(request), action="deleted") + instance.delete() + return Response(status=status.HTTP_204_NO_CONTENT) + + @staticmethod + def _get_user(request: Request) -> FFAdminUser: + return request.user # type: ignore[return-value] diff --git a/api/tests/unit/experimentation/test_experiment_metric_views.py b/api/tests/unit/experimentation/test_experiment_metric_views.py new file mode 100644 index 000000000000..bb493da36181 --- /dev/null +++ b/api/tests/unit/experimentation/test_experiment_metric_views.py @@ -0,0 +1,208 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient + +from environments.models import Environment +from experimentation.constants import EXPERIMENT_FLAG +from experimentation.models import ( + ExpectedDirection, + Experiment, + ExperimentMetric, + Metric, +) + +if TYPE_CHECKING: + from projects.models import Project + from tests.types import EnableFeaturesFixture + +pytestmark = pytest.mark.django_db + + +def _list_url(environment: Environment, experiment: Experiment) -> str: + return reverse( + "api-v1:environments:experiments:experiment-metrics-list", + args=[environment.api_key, experiment.id], + ) + + +def _detail_url( + environment: Environment, experiment: Experiment, em: ExperimentMetric +) -> str: + return reverse( + "api-v1:environments:experiments:experiment-metrics-detail", + args=[environment.api_key, experiment.id, em.id], + ) + + +@pytest.fixture() +def metric(environment: Environment) -> Metric: + metric: Metric = Metric.objects.create( + environment=environment, + name="Sessions per User", + definition={"version": 1, "event": "session_started"}, + ) + return metric + + +def test_attach_metric__valid__returns_201( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + metric: Metric, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client_new.post( + _list_url(environment, experiment), + data={ + "metric": metric.id, + "expected_direction": ExpectedDirection.INCREASE, + }, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + data = response.json() + assert data["metric_name"] == "Sessions per User" + assert data["expected_direction"] == "increase" + assert ExperimentMetric.objects.filter( + experiment=experiment, metric=metric + ).exists() + + +def test_attach_metric__same_metric_twice__returns_400( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + metric: Metric, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + + # When + response = admin_client_new.post( + _list_url(environment, experiment), + data={"metric": metric.id, "expected_direction": ExpectedDirection.INCREASE}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_attach_metric__from_other_environment__returns_400( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + project: "Project", + enable_features: "EnableFeaturesFixture", +) -> None: + # Given a metric defined in a sibling environment + enable_features(EXPERIMENT_FLAG) + other_env = Environment.objects.create(name="Other", project=project) + foreign = Metric.objects.create( + environment=other_env, + name="foreign", + definition={"version": 1, "event": "x"}, + ) + + # When + response = admin_client_new.post( + _list_url(environment, experiment), + data={"metric": foreign.id, "expected_direction": ExpectedDirection.INCREASE}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_list_metrics__with_attachment__returns_attached( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + metric: Metric, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + + # When + response = admin_client_new.get(_list_url(environment, experiment)) + + # Then + assert response.status_code == status.HTTP_200_OK + body = response.json() + assert len(body) == 1 + assert body[0]["metric"] == metric.id + + +def test_detach_metric__attached__returns_204( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + metric: Metric, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + em = ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + + # When + response = admin_client_new.delete(_detail_url(environment, experiment, em)) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + assert not ExperimentMetric.objects.filter(id=em.id).exists() + + +def test_update_attachment__new_direction__returns_200( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + metric: Metric, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given an attached metric + enable_features(EXPERIMENT_FLAG) + em = ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + + # When its expected direction is changed + response = admin_client_new.patch( + _detail_url(environment, experiment, em), + data={"expected_direction": ExpectedDirection.DECREASE}, + format="json", + ) + + # Then the change is persisted + assert response.status_code == status.HTTP_200_OK + em.refresh_from_db() + assert em.expected_direction == ExpectedDirection.DECREASE diff --git a/api/tests/unit/experimentation/test_metric_models.py b/api/tests/unit/experimentation/test_metric_models.py new file mode 100644 index 000000000000..c07290c39adb --- /dev/null +++ b/api/tests/unit/experimentation/test_metric_models.py @@ -0,0 +1,98 @@ +from __future__ import annotations + +import pytest +from django.db.utils import IntegrityError + +from environments.models import Environment +from experimentation.models import ( + ExpectedDirection, + Experiment, + ExperimentMetric, + Metric, + MetricAggregation, +) + +pytestmark = pytest.mark.django_db + + +def _numeric_metric( + environment: Environment, name: str = "Sessions per User" +) -> Metric: + metric: Metric = Metric.objects.create( + environment=environment, + name=name, + aggregation=MetricAggregation.COUNT, + definition={"version": 1, "event": "session_started"}, + ) + return metric + + +def test_metric__defaults__mean_aggregation(environment: Environment) -> None: + # Given a metric created without an explicit aggregation or description + + # When it is created + metric = Metric.objects.create( + environment=environment, + name="Avg Duration", + definition={"version": 1, "event": "session_ended"}, + ) + + # Then it defaults to mean aggregation and an empty description + assert metric.aggregation == MetricAggregation.MEAN + assert metric.description == "" + + +def test_experiment_metric__attach__persists_direction( + experiment: Experiment, + environment: Environment, +) -> None: + # Given + metric = _numeric_metric(environment) + + # When + em = ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + + # Then + assert em in experiment.experiment_metrics.all() + assert em.expected_direction == ExpectedDirection.INCREASE + + +def test_experiment_metric__same_metric_twice__raises_integrity_error( + experiment: Experiment, + environment: Environment, +) -> None: + # Given + metric = _numeric_metric(environment) + ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + + # When / Then + with pytest.raises(IntegrityError): + ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.DECREASE, + ) + + +def test_metric__delete__soft_deletes_and_excludes_from_default_manager( + environment: Environment, +) -> None: + # Given + metric = _numeric_metric(environment) + + # When + metric.delete() + + # Then — default manager excludes soft-deleted (filtering by a non-pk field; + # the soft-delete manager intentionally includes soft-deleted rows on pk lookups) + assert not Metric.objects.filter(name=metric.name).exists() + metric.refresh_from_db() + assert metric.deleted_at is not None diff --git a/api/tests/unit/experimentation/test_metric_views.py b/api/tests/unit/experimentation/test_metric_views.py new file mode 100644 index 000000000000..b506fd8c9cb6 --- /dev/null +++ b/api/tests/unit/experimentation/test_metric_views.py @@ -0,0 +1,295 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient + +from audit.models import AuditLog +from audit.related_object_type import RelatedObjectType +from environments.models import Environment +from experimentation.constants import EXPERIMENT_FLAG +from experimentation.models import ( + ExpectedDirection, + Experiment, + ExperimentMetric, + Metric, + MetricAggregation, +) + +if TYPE_CHECKING: + from projects.models import Project + from tests.types import EnableFeaturesFixture + +pytestmark = pytest.mark.django_db + + +def _list_url(environment: Environment) -> str: + return reverse( + "api-v1:environments:experiment_metrics:metrics-list", + args=[environment.api_key], + ) + + +def _detail_url(environment: Environment, metric: Metric) -> str: + return reverse( + "api-v1:environments:experiment_metrics:metrics-detail", + args=[environment.api_key, metric.id], + ) + + +def _numeric_payload(name: str = "Sessions per User") -> dict[str, object]: + return { + "name": name, + "aggregation": "count", + "definition": {"version": 1, "event": "session_started"}, + } + + +def _metric(environment: Environment, name: str) -> Metric: + metric: Metric = Metric.objects.create( + environment=environment, + name=name, + aggregation=MetricAggregation.COUNT, + definition={"version": 1, "event": "purchase"}, + ) + return metric + + +def test_create_metric__admin_with_flag__returns_201( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client_new.post( + _list_url(environment), data=_numeric_payload(), format="json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + data = response.json() + assert data["name"] == "Sessions per User" + metric = Metric.objects.get(id=data["id"]) + assert metric.environment_id == environment.id + assert AuditLog.objects.filter( + related_object_type=RelatedObjectType.METRIC.name, + related_object_id=metric.id, + ).exists() + + +def test_create_metric__without_flag__returns_403( + admin_client_new: APIClient, + environment: Environment, +) -> None: + # Given the experiment feature flag is not enabled + + # When an admin tries to create a metric + response = admin_client_new.post( + _list_url(environment), data=_numeric_payload(), format="json" + ) + + # Then it is forbidden + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_create_metric__staff_user__returns_403( + staff_client: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When a non-admin tries to create a metric + response = staff_client.post( + _list_url(environment), data=_numeric_payload(), format="json" + ) + + # Then it is forbidden + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_create_metric__mean_without_value__returns_201( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given a mean metric with no explicit value expression + enable_features(EXPERIMENT_FLAG) + payload = { + "name": "Avg Duration", + "aggregation": "mean", + "definition": {"version": 1, "event": "session_ended"}, + } + + # When it is created + response = admin_client_new.post( + _list_url(environment), data=payload, format="json" + ) + + # Then it is accepted (defaults to the event's value column) + assert response.status_code == status.HTTP_201_CREATED + + +def test_create_metric__occurrence_without_value__returns_201( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given — "event happened at least once" needs no value column + enable_features(EXPERIMENT_FLAG) + payload = { + "name": "Activated", + "aggregation": "occurrence", + "definition": {"version": 1, "event": "activated"}, + } + + # When + response = admin_client_new.post( + _list_url(environment), data=payload, format="json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + +def test_create_metric__missing_event__returns_400( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given a definition with no event + enable_features(EXPERIMENT_FLAG) + payload = { + "name": "Bad", + "aggregation": "count", + "definition": {"version": 1}, + } + + # When it is created + response = admin_client_new.post( + _list_url(environment), data=payload, format="json" + ) + + # Then it is rejected + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_list_metrics__other_environment__excluded( + admin_client_new: APIClient, + environment: Environment, + project: "Project", + enable_features: "EnableFeaturesFixture", +) -> None: + # Given a metric in this environment and one in a sibling environment + enable_features(EXPERIMENT_FLAG) + other_env = Environment.objects.create(name="Other", project=project) + _metric(environment, "mine") + _metric(other_env, "theirs") + + # When + response = admin_client_new.get(_list_url(environment)) + + # Then + assert response.status_code == status.HTTP_200_OK + names = [m["name"] for m in response.json()["results"]] + assert names == ["mine"] + + +def test_update_metric__patch__returns_405( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + metric = _metric(environment, "immutable") + + # When + response = admin_client_new.patch( + _detail_url(environment, metric), data={"name": "changed"}, format="json" + ) + + # Then — metrics are immutable for now + assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED + + +def test_delete_metric__attached_to_active_experiment__returns_409( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + metric = _metric(environment, "attached") + ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + + # When + response = admin_client_new.delete(_detail_url(environment, metric)) + + # Then + assert response.status_code == status.HTTP_409_CONFLICT + assert Metric.objects.filter(name="attached").exists() + + +def test_delete_metric__unattached__returns_204( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given a metric not attached to any experiment + enable_features(EXPERIMENT_FLAG) + metric = _metric(environment, "orphan") + + # When it is deleted + response = admin_client_new.delete(_detail_url(environment, metric)) + + # Then it is removed + assert response.status_code == status.HTTP_204_NO_CONTENT + assert not Metric.objects.filter(name="orphan").exists() + + +def test_create_metric__non_object_definition__returns_400( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given a definition that is not an object + enable_features(EXPERIMENT_FLAG) + payload = {"name": "Bad", "aggregation": "count", "definition": "not-an-object"} + + # When it is created + response = admin_client_new.post( + _list_url(environment), data=payload, format="json" + ) + + # Then it is rejected + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "definition" in response.json() + + +def test_list_metrics__unknown_environment__returns_403( + admin_client_new: APIClient, +) -> None: + # Given a non-existent environment api key + url = reverse( + "api-v1:environments:experiment_metrics:metrics-list", + args=["does-not-exist"], + ) + + # When the library is listed + response = admin_client_new.get(url) + + # Then access is denied + assert response.status_code == status.HTTP_403_FORBIDDEN From dd1f915cffdce28eca1209a0bc12ea240af9a303 Mon Sep 17 00:00:00 2001 From: wadii Date: Fri, 5 Jun 2026 10:35:03 +0200 Subject: [PATCH 2/9] feat(experimentation): metric direction and versioned definition --- api/experimentation/metric_definitions.py | 43 +++++++++ .../migrations/0005_metrics.py | 12 +++ api/experimentation/models.py | 21 ++++- api/experimentation/serializers.py | 15 +--- api/experimentation/types.py | 12 +++ .../unit/experimentation/test_metric_views.py | 90 +++++++++++++++++++ 6 files changed, 180 insertions(+), 13 deletions(-) create mode 100644 api/experimentation/metric_definitions.py diff --git a/api/experimentation/metric_definitions.py b/api/experimentation/metric_definitions.py new file mode 100644 index 000000000000..e7437ca130a6 --- /dev/null +++ b/api/experimentation/metric_definitions.py @@ -0,0 +1,43 @@ +"""Versioned schemas for ``Metric.definition``. + +``definition`` is a schema-less JSON column whose shape is versioned so it can +evolve without breaking stored rows. Each supported version has a validator +here; the client sends the version it built the definition with (sourced from +remote config on the frontend). To introduce a new shape, add an entry to +``METRIC_DEFINITION_VALIDATORS`` — the stats engine will key its SQL +construction on the same versions. +""" + +from collections.abc import Callable + +DefinitionValidator = Callable[[dict[str, object]], "str | None"] + + +def _validate_v1(definition: dict[str, object]) -> str | None: + event = definition.get("event") + if not event or not isinstance(event, str): + return "Definition must specify a non-empty 'event'." + return None + + +# Supported definition versions -> their validator. Add an entry per new shape. +METRIC_DEFINITION_VALIDATORS: dict[int, DefinitionValidator] = { + 1: _validate_v1, +} + + +def validate_metric_definition(definition: object) -> str | None: + """Return an error message if ``definition`` is invalid, else ``None``.""" + if not isinstance(definition, dict): + return "Definition must be an object." + + version = definition.get("version") + if ( + not isinstance(version, int) + or isinstance(version, bool) + or version not in METRIC_DEFINITION_VALIDATORS + ): + supported = ", ".join(str(v) for v in sorted(METRIC_DEFINITION_VALIDATORS)) + return f"Definition must specify a supported 'version' ({supported})." + + return METRIC_DEFINITION_VALIDATORS[version](definition) diff --git a/api/experimentation/migrations/0005_metrics.py b/api/experimentation/migrations/0005_metrics.py index aefebe75b7db..8a20db63b888 100644 --- a/api/experimentation/migrations/0005_metrics.py +++ b/api/experimentation/migrations/0005_metrics.py @@ -54,6 +54,18 @@ class Migration(migrations.Migration): max_length=20, ), ), + ( + "direction", + models.CharField( + choices=[ + ("up", "Higher is better"), + ("down", "Lower is better"), + ("informational", "Informational only"), + ], + default="up", + max_length=20, + ), + ), ("definition", models.JSONField()), ("created_at", models.DateTimeField(auto_now_add=True)), ("updated_at", models.DateTimeField(auto_now=True)), diff --git a/api/experimentation/models.py b/api/experimentation/models.py index 59975c07cc3e..9c71c6fe66fc 100644 --- a/api/experimentation/models.py +++ b/api/experimentation/models.py @@ -13,6 +13,7 @@ add_environment_key_to_ingestion, delete_environment_key_from_ingestion, ) +from experimentation.types import MetricDefinition class WarehouseType(models.TextChoices): @@ -126,7 +127,17 @@ class MetricAggregation(models.TextChoices): OCCURRENCE = "occurrence", "Occurrence (event happened at least once)" +class MetricDirection(models.TextChoices): + """A metric's inherent polarity — which way is "better".""" + + UP = "up", "Higher is better" + DOWN = "down", "Lower is better" + INFORMATIONAL = "informational", "Informational only" + + class ExpectedDirection(models.TextChoices): + """The guardrail direction expected of a metric within an experiment.""" + INCREASE = "increase", "Increase" DECREASE = "decrease", "Decrease" NOT_INCREASE = "not_increase", "Should not increase" @@ -146,7 +157,15 @@ class Metric(SoftDeleteExportableModel): choices=MetricAggregation.choices, default=MetricAggregation.MEAN, ) - definition: models.JSONField[dict[str, object], dict[str, object]] = ( + # Polarity of the metric itself — whether a higher or lower value is + # "better". Distinct from ExperimentMetric.expected_direction, which is the + # guardrail direction expected of a specific experiment. + direction = models.CharField( + max_length=20, + choices=MetricDirection.choices, + default=MetricDirection.UP, + ) + definition: models.JSONField[MetricDefinition, MetricDefinition] = ( models.JSONField() ) created_at = models.DateTimeField(auto_now_add=True) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index c55cee19961b..365157f57a98 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -10,6 +10,7 @@ WarehouseConnection, WarehouseType, ) +from experimentation.metric_definitions import validate_metric_definition from experimentation.types import SNOWFLAKE_DEFAULTS, SnowflakeConfig from features.feature_types import MULTIVARIATE from features.models import Feature @@ -86,6 +87,7 @@ class Meta: "name", "description", "aggregation", + "direction", "definition", "created_at", "updated_at", @@ -100,22 +102,11 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: definition: Any = attrs.get( "definition", getattr(self.instance, "definition", None) ) - error = self._validate_definition(definition) + error = validate_metric_definition(definition) if error: raise serializers.ValidationError({"definition": error}) return attrs - @staticmethod - def _validate_definition(definition: Any) -> str | None: - if not isinstance(definition, dict): - return "Definition must be an object." - - event = definition.get("event") - if not event or not isinstance(event, str): - return "Definition must specify a non-empty 'event'." - - return None - class ExperimentMetricSerializer(serializers.ModelSerializer): # type: ignore[type-arg] metric = serializers.PrimaryKeyRelatedField( # type: ignore[var-annotated] diff --git a/api/experimentation/types.py b/api/experimentation/types.py index 1c756c4112f3..1f771e3a632d 100644 --- a/api/experimentation/types.py +++ b/api/experimentation/types.py @@ -1,6 +1,18 @@ from typing import TypedDict +class MetricDefinition(TypedDict): + """The recipe a metric is computed from (v1). + + Versioned so the shape can evolve; see ``metric_definitions`` for the + registry of supported versions and their validators. v1 captures the + warehouse event whose occurrences/values the metric aggregates. + """ + + version: int + event: str + + class SnowflakeConfig(TypedDict): account_identifier: str warehouse: str diff --git a/api/tests/unit/experimentation/test_metric_views.py b/api/tests/unit/experimentation/test_metric_views.py index b506fd8c9cb6..89b7a701d6c8 100644 --- a/api/tests/unit/experimentation/test_metric_views.py +++ b/api/tests/unit/experimentation/test_metric_views.py @@ -17,6 +17,7 @@ ExperimentMetric, Metric, MetricAggregation, + MetricDirection, ) if TYPE_CHECKING: @@ -181,6 +182,95 @@ def test_create_metric__missing_event__returns_400( assert response.status_code == status.HTTP_400_BAD_REQUEST +def test_create_metric__missing_version__returns_400( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + payload = { + "name": "Bad", + "aggregation": "count", + "definition": {"event": "session_started"}, + } + + # When + response = admin_client_new.post( + _list_url(environment), data=payload, format="json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "definition" in response.json() + + +def test_create_metric__unsupported_version__returns_400( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + payload = { + "name": "Bad", + "aggregation": "count", + "definition": {"version": 99, "event": "session_started"}, + } + + # When + response = admin_client_new.post( + _list_url(environment), data=payload, format="json" + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "definition" in response.json() + + +def test_create_metric__with_direction__persists_direction( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + payload = { + **_numeric_payload("Time to Activation"), + "direction": MetricDirection.DOWN, + } + + # When + response = admin_client_new.post( + _list_url(environment), data=payload, format="json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + data = response.json() + assert data["direction"] == MetricDirection.DOWN + metric = Metric.objects.get(id=data["id"]) + assert metric.direction == MetricDirection.DOWN + + +def test_create_metric__without_direction__defaults_to_increase( + admin_client_new: APIClient, + environment: Environment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + + # When + response = admin_client_new.post( + _list_url(environment), data=_numeric_payload(), format="json" + ) + + # Then + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["direction"] == MetricDirection.UP + + def test_list_metrics__other_environment__excluded( admin_client_new: APIClient, environment: Environment, From 620f7b4fd0974bcfc848d1ab522f10fd0849cb07 Mon Sep 17 00:00:00 2001 From: wadii Date: Fri, 5 Jun 2026 11:10:46 +0200 Subject: [PATCH 3/9] fix(experimentation): harden experiment-metric attach/detach and metric deletion --- api/experimentation/serializers.py | 7 ++ api/experimentation/views.py | 19 ++++- .../test_experiment_metric_views.py | 73 +++++++++++++++++++ .../unit/experimentation/test_metric_views.py | 23 ++++++ 4 files changed, 119 insertions(+), 3 deletions(-) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index 365157f57a98..5a8467edc645 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -6,6 +6,7 @@ from experimentation.models import ( Experiment, ExperimentMetric, + ExperimentStatus, Metric, WarehouseConnection, WarehouseType, @@ -129,6 +130,12 @@ class Meta: def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: experiment: Experiment = self.context["experiment"] + + if experiment.status == ExperimentStatus.COMPLETED: + raise serializers.ValidationError( + "Cannot modify metrics of a completed experiment." + ) + metric: Metric = attrs.get("metric", getattr(self.instance, "metric", None)) if metric.environment_id != experiment.environment_id: diff --git a/api/experimentation/views.py b/api/experimentation/views.py index 81a35e026c7b..3dd5748eca7c 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -3,6 +3,7 @@ from django.db import IntegrityError from django.db.models import Count, Q, QuerySet +from django.shortcuts import get_object_or_404 from rest_framework import mixins, serializers, status from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated @@ -290,15 +291,24 @@ def get_serializer_context(self) -> dict[str, Any]: return context def _get_experiment(self) -> Experiment: - experiment: Experiment = Experiment.objects.get( + return get_object_or_404( + Experiment, id=self.kwargs[self.experiment_url_kwarg], environment__api_key=self.kwargs["environment_api_key"], + deleted_at__isnull=True, ) - return experiment def perform_create(self, serializer: BaseSerializer[ExperimentMetric]) -> None: serializer.save(experiment=self._get_experiment()) + def destroy(self, request: Request, *args: object, **kwargs: object) -> Response: + if self._get_experiment().status == ExperimentStatus.COMPLETED: + return Response( + {"detail": "Cannot detach metrics from a completed experiment."}, + status=status.HTTP_400_BAD_REQUEST, + ) + return super().destroy(request, *args, **kwargs) + class MetricViewSet( NestedEnvironmentViewSet[Metric], @@ -322,7 +332,10 @@ def perform_create(self, serializer: BaseSerializer[Metric]) -> None: def destroy(self, request: Request, *args: object, **kwargs: object) -> Response: instance: Metric = self.get_object() if ( - ExperimentMetric.objects.filter(metric=instance) + ExperimentMetric.objects.filter( + metric=instance, + experiment__deleted_at__isnull=True, + ) .exclude(experiment__status=ExperimentStatus.COMPLETED) .exists() ): diff --git a/api/tests/unit/experimentation/test_experiment_metric_views.py b/api/tests/unit/experimentation/test_experiment_metric_views.py index bb493da36181..7d3091401c1c 100644 --- a/api/tests/unit/experimentation/test_experiment_metric_views.py +++ b/api/tests/unit/experimentation/test_experiment_metric_views.py @@ -13,6 +13,7 @@ ExpectedDirection, Experiment, ExperimentMetric, + ExperimentStatus, Metric, ) @@ -132,6 +133,78 @@ def test_attach_metric__from_other_environment__returns_400( assert response.status_code == status.HTTP_400_BAD_REQUEST +def test_attach_metric__unknown_experiment__returns_404( + admin_client_new: APIClient, + environment: Environment, + metric: Metric, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + url = reverse( + "api-v1:environments:experiments:experiment-metrics-list", + args=[environment.api_key, 999_999_999], + ) + + # When + response = admin_client_new.post( + url, + data={"metric": metric.id, "expected_direction": ExpectedDirection.INCREASE}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_attach_metric__completed_experiment__returns_400( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + metric: Metric, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + experiment.status = ExperimentStatus.COMPLETED + experiment.save() + + # When + response = admin_client_new.post( + _list_url(environment, experiment), + data={"metric": metric.id, "expected_direction": ExpectedDirection.INCREASE}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_detach_metric__completed_experiment__returns_400( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + metric: Metric, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + em = ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + experiment.status = ExperimentStatus.COMPLETED + experiment.save() + + # When + response = admin_client_new.delete(_detail_url(environment, experiment, em)) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert ExperimentMetric.objects.filter(pk=em.pk).exists() + + def test_list_metrics__with_attachment__returns_attached( admin_client_new: APIClient, environment: Environment, diff --git a/api/tests/unit/experimentation/test_metric_views.py b/api/tests/unit/experimentation/test_metric_views.py index 89b7a701d6c8..451c7ea6919b 100644 --- a/api/tests/unit/experimentation/test_metric_views.py +++ b/api/tests/unit/experimentation/test_metric_views.py @@ -333,6 +333,29 @@ def test_delete_metric__attached_to_active_experiment__returns_409( assert Metric.objects.filter(name="attached").exists() +def test_delete_metric__attached_to_soft_deleted_experiment__returns_204( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given a metric whose only attachment is to a soft-deleted experiment + enable_features(EXPERIMENT_FLAG) + metric = _metric(environment, "ghost") + ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + experiment.delete() # soft-delete leaves the ExperimentMetric row behind + + # When + response = admin_client_new.delete(_detail_url(environment, metric)) + + # Then the ghost attachment does not block deletion + assert response.status_code == status.HTTP_204_NO_CONTENT + + def test_delete_metric__unattached__returns_204( admin_client_new: APIClient, environment: Environment, From a48959c47f6a2a23b7065b305654d2fe07345643 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 5 Jun 2026 09:12:32 +0000 Subject: [PATCH 4/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/experimentation/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index 5a8467edc645..3ba53284b8a8 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -3,6 +3,7 @@ from rest_framework import serializers from environments.models import Environment +from experimentation.metric_definitions import validate_metric_definition from experimentation.models import ( Experiment, ExperimentMetric, @@ -11,7 +12,6 @@ WarehouseConnection, WarehouseType, ) -from experimentation.metric_definitions import validate_metric_definition from experimentation.types import SNOWFLAKE_DEFAULTS, SnowflakeConfig from features.feature_types import MULTIVARIATE from features.models import Feature From cb1dbe8f879d27a6dfabb7322b36278ea650b858 Mon Sep 17 00:00:00 2001 From: wadii Date: Fri, 5 Jun 2026 11:32:17 +0200 Subject: [PATCH 5/9] feat(experimentation): cache experiment object to avoid redundant lookups --- api/experimentation/views.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/api/experimentation/views.py b/api/experimentation/views.py index 3dd5748eca7c..a75924782b04 100644 --- a/api/experimentation/views.py +++ b/api/experimentation/views.py @@ -291,12 +291,14 @@ def get_serializer_context(self) -> dict[str, Any]: return context def _get_experiment(self) -> Experiment: - return get_object_or_404( - Experiment, - id=self.kwargs[self.experiment_url_kwarg], - environment__api_key=self.kwargs["environment_api_key"], - deleted_at__isnull=True, - ) + if not hasattr(self, "_experiment"): + self._experiment = get_object_or_404( + Experiment, + id=self.kwargs[self.experiment_url_kwarg], + environment__api_key=self.kwargs["environment_api_key"], + deleted_at__isnull=True, + ) + return self._experiment def perform_create(self, serializer: BaseSerializer[ExperimentMetric]) -> None: serializer.save(experiment=self._get_experiment()) From b46b209e550234d728ae4907cc444ea97197e9bd Mon Sep 17 00:00:00 2001 From: wadii Date: Fri, 5 Jun 2026 11:49:09 +0200 Subject: [PATCH 6/9] feat(experimentation): versioned metrics definition typed dict --- api/experimentation/metric_definitions.py | 23 +++++++++-------------- api/experimentation/models.py | 3 --- api/experimentation/types.py | 7 +++++-- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/api/experimentation/metric_definitions.py b/api/experimentation/metric_definitions.py index e7437ca130a6..d3a52d141955 100644 --- a/api/experimentation/metric_definitions.py +++ b/api/experimentation/metric_definitions.py @@ -2,10 +2,8 @@ ``definition`` is a schema-less JSON column whose shape is versioned so it can evolve without breaking stored rows. Each supported version has a validator -here; the client sends the version it built the definition with (sourced from -remote config on the frontend). To introduce a new shape, add an entry to -``METRIC_DEFINITION_VALIDATORS`` — the stats engine will key its SQL -construction on the same versions. +here; the client sends the version it built the definition with. To introduce a +new shape, add an entry to ``METRIC_DEFINITION_VALIDATORS``. """ from collections.abc import Callable @@ -20,7 +18,6 @@ def _validate_v1(definition: dict[str, object]) -> str | None: return None -# Supported definition versions -> their validator. Add an entry per new shape. METRIC_DEFINITION_VALIDATORS: dict[int, DefinitionValidator] = { 1: _validate_v1, } @@ -32,12 +29,10 @@ def validate_metric_definition(definition: object) -> str | None: return "Definition must be an object." version = definition.get("version") - if ( - not isinstance(version, int) - or isinstance(version, bool) - or version not in METRIC_DEFINITION_VALIDATORS - ): - supported = ", ".join(str(v) for v in sorted(METRIC_DEFINITION_VALIDATORS)) - return f"Definition must specify a supported 'version' ({supported})." - - return METRIC_DEFINITION_VALIDATORS[version](definition) + validator = ( + METRIC_DEFINITION_VALIDATORS.get(version) if isinstance(version, int) else None + ) + if validator is None: + return "Definition must specify a supported 'version'." + + return validator(definition) diff --git a/api/experimentation/models.py b/api/experimentation/models.py index 9c71c6fe66fc..630d077afa47 100644 --- a/api/experimentation/models.py +++ b/api/experimentation/models.py @@ -157,9 +157,6 @@ class Metric(SoftDeleteExportableModel): choices=MetricAggregation.choices, default=MetricAggregation.MEAN, ) - # Polarity of the metric itself — whether a higher or lower value is - # "better". Distinct from ExperimentMetric.expected_direction, which is the - # guardrail direction expected of a specific experiment. direction = models.CharField( max_length=20, choices=MetricDirection.choices, diff --git a/api/experimentation/types.py b/api/experimentation/types.py index 1f771e3a632d..74c2114d98e6 100644 --- a/api/experimentation/types.py +++ b/api/experimentation/types.py @@ -1,8 +1,8 @@ from typing import TypedDict -class MetricDefinition(TypedDict): - """The recipe a metric is computed from (v1). +class MetricDefinitionV1(TypedDict): + """The recipe a metric is computed from. Versioned so the shape can evolve; see ``metric_definitions`` for the registry of supported versions and their validators. v1 captures the @@ -13,6 +13,9 @@ class MetricDefinition(TypedDict): event: str +MetricDefinition = MetricDefinitionV1 + + class SnowflakeConfig(TypedDict): account_identifier: str warehouse: str From 1fd7efb4f44dbe9461410574181cf71fedd388cb Mon Sep 17 00:00:00 2001 From: wadii Date: Fri, 5 Jun 2026 11:59:34 +0200 Subject: [PATCH 7/9] feat(experimentation): deduped metrics permissions --- api/experimentation/permissions.py | 16 ++-------------- api/experimentation/serializers.py | 5 +---- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/api/experimentation/permissions.py b/api/experimentation/permissions.py index d7599021ffbd..5af127ca66e0 100644 --- a/api/experimentation/permissions.py +++ b/api/experimentation/permissions.py @@ -42,17 +42,5 @@ def has_permission(self, request: Request, view: APIView) -> bool: return user.is_environment_admin(environment) -class MetricPermission(BasePermission): - def has_permission(self, request: Request, view: APIView) -> bool: - try: - environment = Environment.objects.get( - api_key=view.kwargs.get("environment_api_key") - ) - except Environment.DoesNotExist: - return False - - if not is_experiment_feature_enabled(environment.project.organisation): - return False - - user: FFAdminUser = request.user # type: ignore[assignment] - return user.is_environment_admin(environment) +# Metrics are gated identically to experiments; aliased until the rules diverge. +MetricPermission = ExperimentPermission diff --git a/api/experimentation/serializers.py b/api/experimentation/serializers.py index 3ba53284b8a8..b522712d3641 100644 --- a/api/experimentation/serializers.py +++ b/api/experimentation/serializers.py @@ -100,10 +100,7 @@ class Meta: ) def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: - definition: Any = attrs.get( - "definition", getattr(self.instance, "definition", None) - ) - error = validate_metric_definition(definition) + error = validate_metric_definition(attrs["definition"]) if error: raise serializers.ValidationError({"definition": error}) return attrs From de3e8b94c06e0291731feada453adf75e80d9ea5 Mon Sep 17 00:00:00 2001 From: wadii Date: Fri, 5 Jun 2026 13:45:20 +0200 Subject: [PATCH 8/9] feat(experimentation): added deletion based on experiment status tests --- .../unit/experimentation/test_metric_views.py | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/api/tests/unit/experimentation/test_metric_views.py b/api/tests/unit/experimentation/test_metric_views.py index 451c7ea6919b..9ad5482b9ba9 100644 --- a/api/tests/unit/experimentation/test_metric_views.py +++ b/api/tests/unit/experimentation/test_metric_views.py @@ -15,6 +15,7 @@ ExpectedDirection, Experiment, ExperimentMetric, + ExperimentStatus, Metric, MetricAggregation, MetricDirection, @@ -310,14 +311,25 @@ def test_update_metric__patch__returns_405( assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED +@pytest.mark.parametrize( + "experiment_status", + [ + ExperimentStatus.CREATED, + ExperimentStatus.RUNNING, + ExperimentStatus.PAUSED, + ], +) def test_delete_metric__attached_to_active_experiment__returns_409( admin_client_new: APIClient, environment: Environment, experiment: Experiment, + experiment_status: str, enable_features: "EnableFeaturesFixture", ) -> None: # Given enable_features(EXPERIMENT_FLAG) + experiment.status = experiment_status + experiment.save() metric = _metric(environment, "attached") ExperimentMetric.objects.create( experiment=experiment, @@ -333,6 +345,31 @@ def test_delete_metric__attached_to_active_experiment__returns_409( assert Metric.objects.filter(name="attached").exists() +def test_delete_metric__attached_to_completed_experiment__returns_204( + admin_client_new: APIClient, + environment: Environment, + experiment: Experiment, + enable_features: "EnableFeaturesFixture", +) -> None: + # Given + enable_features(EXPERIMENT_FLAG) + experiment.status = ExperimentStatus.COMPLETED + experiment.save() + metric = _metric(environment, "done") + ExperimentMetric.objects.create( + experiment=experiment, + metric=metric, + expected_direction=ExpectedDirection.INCREASE, + ) + + # When + response = admin_client_new.delete(_detail_url(environment, metric)) + + # Then + assert response.status_code == status.HTTP_204_NO_CONTENT + assert not Metric.objects.filter(name="done").exists() + + def test_delete_metric__attached_to_soft_deleted_experiment__returns_204( admin_client_new: APIClient, environment: Environment, From 282c32f086a82f73219a24e4f635e1ceb549f2ee Mon Sep 17 00:00:00 2001 From: wadii Date: Mon, 8 Jun 2026 10:08:12 +0200 Subject: [PATCH 9/9] chore: drop-stray-frontend-utils-reorder-from-backend-branch --- frontend/common/utils/utils.tsx | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/frontend/common/utils/utils.tsx b/frontend/common/utils/utils.tsx index 85b21201903e..3bbc8e8c32ed 100644 --- a/frontend/common/utils/utils.tsx +++ b/frontend/common/utils/utils.tsx @@ -145,6 +145,16 @@ const Utils = Object.assign({}, BaseUtils, { return res }, + getContrastColour(backgroundColor: string | null | undefined): string { + if (!backgroundColor) return 'white' + + try { + return Color(backgroundColor).luminosity() > 0.179 ? 'black' : 'white' + } catch { + return 'white' + } + }, + copyToClipboard: async ( value: string, successMessage?: string, @@ -158,7 +168,6 @@ const Utils = Object.assign({}, BaseUtils, { throw error } }, - displayLimitAlert(type: string, percentage: number | undefined) { const envOrProject = type === 'segment overrides' ? 'environment' : 'project' @@ -201,6 +210,7 @@ const Utils = Object.assign({}, BaseUtils, { return featureState.string_value } }, + findOperator( operator: SegmentCondition['operator'], value: string, @@ -217,14 +227,12 @@ const Utils = Object.assign({}, BaseUtils, { return conditions.find((v) => v.value === operator) }, - /** Checks whether the specified flag exists, which is different from the flag being enabled or not. This is used to * only add behaviour to Flagsmith-on-Flagsmith flags that have been explicitly created by customers. */ flagsmithFeatureExists(flag: string) { return Object.prototype.hasOwnProperty.call(flagsmith.getAllFlags(), flag) }, - getContentType( contentTypes: ContentType[] | undefined, model: string, @@ -232,15 +240,6 @@ const Utils = Object.assign({}, BaseUtils, { ) { return contentTypes?.find((c: ContentType) => c[model] === type) || null }, - getContrastColour(backgroundColor: string | null | undefined): string { - if (!backgroundColor) return 'white' - - try { - return Color(backgroundColor).luminosity() > 0.179 ? 'black' : 'white' - } catch { - return 'white' - } - }, getCreateProjectPermission(organisation: Organisation) { if (organisation?.restrict_project_create_to_admin) { return OrganisationPermission.ADMIN