Skip to content
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.

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

feat(experiments): Create experiment with existing feature flag #28004

Merged
merged 22 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
0d5382d
First pass at attaching to existing feature flag
danielbachhuber Jan 29, 2025
44411d5
Update test
danielbachhuber Jan 29, 2025
7331356
Update query snapshots
github-actions[bot] Jan 29, 2025
7050098
Merge branch 'master' into experiments/attach-existing-ff
danielbachhuber Jan 30, 2025
bce1143
Restore `<LemonBanner>`
danielbachhuber Jan 30, 2025
e71389c
Fix merge conflict
danielbachhuber Jan 30, 2025
817e552
UX tweaks
danielbachhuber Jan 30, 2025
5579d81
Send event when used
danielbachhuber Jan 31, 2025
6766efd
Tweaks
danielbachhuber Jan 31, 2025
b35101c
Validation
danielbachhuber Jan 31, 2025
fc65a5a
Move feature flag validation entirely async
danielbachhuber Jan 31, 2025
2a68769
Merge branch 'master' into experiments/attach-existing-ff
danielbachhuber Jan 31, 2025
216ccb3
Drop stale test
danielbachhuber Jan 31, 2025
915cfbf
Allow experiments to be created with soft-deleted feature flags
danielbachhuber Jan 31, 2025
f37aeff
Remnant from 915cfbf22379494f77158c7e65d78ccf645712a0
danielbachhuber Jan 31, 2025
5b148dc
Merge branch 'master' into experiments/attach-existing-ff
danielbachhuber Feb 3, 2025
962bd18
Persist manual validation when the field is blurred
danielbachhuber Feb 3, 2025
a302b2a
Expects `undefined` for no error, not empty string
danielbachhuber Feb 3, 2025
7f13464
Add a heading and a link to the banner
danielbachhuber Feb 3, 2025
800b047
Fix type issue
danielbachhuber Feb 3, 2025
e433afa
Merge branch 'master' into experiments/attach-existing-ff
danielbachhuber Feb 3, 2025
05b38a4
Remove Holdout field from the experiment form
danielbachhuber Feb 4, 2025
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
75 changes: 48 additions & 27 deletions ee/clickhouse/views/experiments.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
from posthog.clickhouse.query_tagging import tag_queries
from posthog.constants import INSIGHT_TRENDS
from posthog.models.experiment import Experiment, ExperimentHoldout, ExperimentSavedMetric
from posthog.models.feature_flag.feature_flag import FeatureFlag
from posthog.models.filters.filter import Filter
from posthog.utils import generate_cache_key, get_safe_cache

Expand Down Expand Up @@ -252,6 +253,19 @@ def validate_parameters(self, value):

return value

def validate_existing_feature_flag_for_experiment(self, feature_flag: FeatureFlag):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel reasonably confident in this validation logic but would love a second pair of eyes on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking good to me 👍

if feature_flag.experiment_set.exists():
raise ValidationError("Feature flag is already associated with an experiment.")

variants = feature_flag.filters.get("multivariate", {}).get("variants", [])

if len(variants) and len(variants) > 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The condition len(variants) and len(variants) > 1 can be simplified to just len(variants) > 1 since that covers both cases

Suggested change
if len(variants) and len(variants) > 1:
if len(variants) > 1:

if variants[0].get("key") != "control":
raise ValidationError("Feature flag must have control as the first variant.")
return True

raise ValidationError("Feature flag is not eligible for experiments.")

def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment:
is_draft = "start_date" not in validated_data or validated_data["start_date"] is None

Expand All @@ -271,35 +285,42 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Experiment:

feature_flag_key = validated_data.pop("get_feature_flag_key")

holdout_groups = None
if validated_data.get("holdout"):
holdout_groups = validated_data["holdout"].filters

default_variants = [
{"key": "control", "name": "Control Group", "rollout_percentage": 50},
{"key": "test", "name": "Test Variant", "rollout_percentage": 50},
]

feature_flag_filters = {
"groups": [{"properties": [], "rollout_percentage": 100}],
"multivariate": {"variants": variants or default_variants},
"aggregation_group_type_index": aggregation_group_type_index,
"holdout_groups": holdout_groups,
}
existing_feature_flag = FeatureFlag.objects.filter(
key=feature_flag_key, team_id=self.context["team_id"], deleted=False
).first()
if existing_feature_flag:
self.validate_existing_feature_flag_for_experiment(existing_feature_flag)
feature_flag = existing_feature_flag
else:
holdout_groups = None
if validated_data.get("holdout"):
holdout_groups = validated_data["holdout"].filters

default_variants = [
{"key": "control", "name": "Control Group", "rollout_percentage": 50},
{"key": "test", "name": "Test Variant", "rollout_percentage": 50},
]

feature_flag_filters = {
"groups": [{"properties": [], "rollout_percentage": 100}],
"multivariate": {"variants": variants or default_variants},
"aggregation_group_type_index": aggregation_group_type_index,
"holdout_groups": holdout_groups,
}

feature_flag_serializer = FeatureFlagSerializer(
data={
"key": feature_flag_key,
"name": f'Feature Flag for Experiment {validated_data["name"]}',
"filters": feature_flag_filters,
"active": not is_draft,
"creation_context": "experiments",
},
context=self.context,
)
feature_flag_serializer = FeatureFlagSerializer(
data={
"key": feature_flag_key,
"name": f'Feature Flag for Experiment {validated_data["name"]}',
"filters": feature_flag_filters,
"active": not is_draft,
"creation_context": "experiments",
},
context=self.context,
)

feature_flag_serializer.is_valid(raise_exception=True)
feature_flag = feature_flag_serializer.save()
feature_flag_serializer.is_valid(raise_exception=True)
feature_flag = feature_flag_serializer.save()

if not validated_data.get("stats_config"):
validated_data["stats_config"] = {"version": 2}
Expand Down
80 changes: 55 additions & 25 deletions ee/clickhouse/views/test/test_clickhouse_experiments.py
Original file line number Diff line number Diff line change
Expand Up @@ -744,31 +744,6 @@ def test_invalid_update(self):
"Can't update keys: get_feature_flag_key on Experiment",
)

def test_cant_reuse_existing_feature_flag(self):
ff_key = "a-b-test"
FeatureFlag.objects.create(
team=self.team,
rollout_percentage=50,
name="Beta feature",
key=ff_key,
created_by=self.user,
)
response = self.client.post(
f"/api/projects/{self.team.id}/experiments/",
{
"name": "Test Experiment",
"description": "",
"start_date": "2021-12-01T10:23",
"end_date": None,
"feature_flag_key": ff_key,
"parameters": None,
"filters": {"events": []},
},
)

self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertEqual(response.json()["detail"], "There is already a feature flag with this key.")

def test_draft_experiment_doesnt_have_FF_active(self):
# Draft experiment
ff_key = "a-b-tests"
Expand Down Expand Up @@ -1776,6 +1751,61 @@ def test_create_draft_experiment_without_filters(self) -> None:
self.assertEqual(response.json()["name"], "Test Experiment")
self.assertEqual(response.json()["feature_flag_key"], ff_key)

def test_create_experiment_with_feature_flag_missing_control(self):
feature_flag = FeatureFlag.objects.create(
team=self.team,
name="Beta feature",
key="beta-feature",
filters={
"multivariate": {
"variants": [
{"key": "test-1", "rollout_percentage": 50},
{"key": "test-2", "rollout_percentage": 50},
]
}
},
created_by=self.user,
)

response = self.client.post(
f"/api/projects/{self.team.id}/experiments/",
{
"name": "Beta experiment",
"feature_flag_key": feature_flag.key,
"parameters": {},
},
)

self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertEqual(response.json()["detail"], "Feature flag must have control as the first variant.")

def test_create_experiment_with_valid_existing_feature_flag(self):
feature_flag = FeatureFlag.objects.create(
team=self.team,
name="Beta feature",
key="beta-feature",
filters={
"multivariate": {
"variants": [
{"key": "control", "rollout_percentage": 50},
{"key": "test", "rollout_percentage": 50},
]
}
},
created_by=self.user,
)

response = self.client.post(
f"/api/projects/{self.team.id}/experiments/",
{
"name": "Beta experiment",
"feature_flag_key": feature_flag.key,
"parameters": {},
},
)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
self.assertEqual(response.json()["feature_flag"]["id"], feature_flag.id)

def test_feature_flag_and_experiment_sync(self):
# Create an experiment with control and test variants
response = self.client.post(
Expand Down
8 changes: 8 additions & 0 deletions frontend/src/lib/utils/eventUsageLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,8 @@ export const eventUsageLogic = kea<eventUsageLogicType>([
experimentId,
metric,
}),
reportExperimentFeatureFlagModalOpened: () => ({}),
reportExperimentFeatureFlagSelected: (featureFlagKey: string) => ({ featureFlagKey }),
// Definition Popover
reportDataManagementDefinitionHovered: (type: TaxonomicFilterGroupType) => ({ type }),
reportDataManagementDefinitionClickView: (type: TaxonomicFilterGroupType) => ({ type }),
Expand Down Expand Up @@ -1039,6 +1041,12 @@ export const eventUsageLogic = kea<eventUsageLogicType>([
reportExperimentMetricTimeout: ({ experimentId, metric }) => {
posthog.capture('experiment metric timeout', { experiment_id: experimentId, metric })
},
reportExperimentFeatureFlagModalOpened: () => {
posthog.capture('experiment feature flag modal opened')
},
reportExperimentFeatureFlagSelected: ({ featureFlagKey }: { featureFlagKey: string }) => {
posthog.capture('experiment feature flag selected', { feature_flag_key: featureFlagKey })
},
reportPropertyGroupFilterAdded: () => {
posthog.capture('property group filter added')
},
Expand Down
Loading
Loading