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

danielbachhuber · 2025-01-29T00:57:54Z

Previously #27938

Changes

Allows an existing feature flag to be assigned to a new experiment:

The feature flag has to match some criteria:

It has to have at least two variants.
The first variant must be called control.
It must not be associated with any other experiments.

The modal shows a tooltip if you try to select a feature flag that isn't eligible:

If you manually specify an ineligible feature flag, you'll see an error in the field:

Also removes the Holdout generally, for consistency between UX.

How did you test this code?

Happy path:

Created a new feature flag with multiple variants and control as the first variant.
Navigated to New experiment.
Clicked Choose under the Feature flag key field and selected the feature flag I previously created.
Created the experiment and verified the feature flag was assigned to it.

github-actions · 2025-01-29T01:14:34Z

Size Change: +330 B (+0.03%)

Total Size: 1.17 MB

ℹ️ View Unchanged

Filename	Size	Change
`frontend/dist/toolbar.js`	1.17 MB	+330 B (+0.03%)

_{compressed-size-action}

danielbachhuber · 2025-01-31T14:15:44Z

ee/clickhouse/views/experiments.py

@@ -252,6 +253,23 @@ def validate_parameters(self, value):

        return value

+    def validate_existing_feature_flag_for_experiment(self, feature_flag: FeatureFlag):


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

Looking good to me 👍

danielbachhuber · 2025-01-31T14:16:10Z

frontend/src/scenes/experiments/ExperimentForm.tsx

+                    <div className="my-10">
+                        <LemonBanner type="info">
+                            Existing feature flag configuration will be applied to the experiment.
+                        </LemonBanner>


I don't love this — open to other ideas.

I think Variants is the most appropriate header to have before this banner.

danielbachhuber · 2025-01-31T14:25:34Z

frontend/src/scenes/experiments/experimentLogic.tsx

+            actions.setExperimentManualErrors({
+                ...existingErrors,
+                feature_flag_key: validateFeatureFlagKey(featureFlagKey),
+            })


One problem here: when a new field is focused, the manual errors state is cleared and the error state is reset to the previous value of experimentErrors. As such, it's not possible to assign the value to a reducer and then reference the reducer in errors: ({ because the reducer will contain the value that existed before validateFeatureFlag() fired.

Workaround ideas?

Branch

Master

One problem here: when a new field is focused, the manual errors state is cleared and the error state is reset to the previous value of experimentErrors.

It actually happens onBlur for the field:

https://github.com/keajs/kea-forms/blob/7b7028dae22df4fa624331a713968be6bd8ee2a7/src/components.tsx#L119-L120

https://github.com/keajs/kea-forms/blob/839f6f71b72554ccaf404c5066da5c911b214934/src/builder.ts#L99-L106

962bd18 (and a302b2a) seem to work better:

greptile-apps

PR Summary

This PR adds functionality to create experiments using existing feature flags, with robust validation and UI improvements. Here's a concise summary of the key changes:

Added feature flag selection modal in ExperimentForm.tsx with validation tooltips and error states
Implemented featureFlagEligibleForExperiment validation requiring multiple variants with 'control' first
Added async feature flag validation in experimentLogic.tsx with proper state management
Modified backend validation in experiments.py to support and validate existing feature flag usage
Added analytics tracking for feature flag modal interactions and selections

Key implementation points:

Feature flags must have multiple variants with 'control' as first variant
Flags cannot be deleted or associated with other experiments
UI provides clear feedback through tooltips and field errors
Backend validation ensures data integrity
Analytics track feature flag selection workflow

The changes look well-structured with good error handling and user feedback throughout the feature flag selection process.

_{7 file(s) reviewed, 9 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-01-31T14:28:05Z

ee/clickhouse/views/experiments.py

+
+        variants = feature_flag.filters.get("multivariate", {}).get("variants", [])
+
+        if len(variants) and len(variants) > 1:


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:

greptile-apps · 2025-01-31T14:28:16Z

frontend/src/scenes/experiments/ExperimentForm.tsx

+                                let disabledReason: string | undefined = undefined
+                                try {
+                                    featureFlagEligibleForExperiment(flag)
+                                } catch (error) {
+                                    disabledReason = (error as Error).message
+                                }


style: error is implicitly typed as unknown here - should explicitly catch Error type

Suggested change

let disabledReason: string | undefined = undefined

try {

featureFlagEligibleForExperiment(flag)

} catch (error) {

disabledReason = (error as Error).message

}

let disabledReason: string | undefined = undefined

try {

featureFlagEligibleForExperiment(flag)

} catch (error: unknown) {

disabledReason = (error instanceof Error) ? error.message : String(error)

}

greptile-apps · 2025-01-31T14:28:17Z

frontend/src/scenes/experiments/ExperimentForm.tsx

+                                        to={urls.featureFlag(flag.id as number)}
+                                        target="_blank"


logic: unsafe type assertion with 'as number' - should validate id exists first

greptile-apps · 2025-01-31T14:28:40Z

frontend/src/scenes/experiments/utils.test.ts

+        const featureFlag = { ...baseFeatureFlag, is_remote_configuration: true }
+        expect(() => featureFlagEligibleForExperiment(featureFlag)).toThrow(
+            'Feature flag must use multiple variants with control as the first variant.'
+        )


logic: Error message doesn't match the actual issue. Should be about remote configuration not being supported rather than about variants.

Suggested change

const featureFlag = { ...baseFeatureFlag, is_remote_configuration: true }

expect(() => featureFlagEligibleForExperiment(featureFlag)).toThrow(

'Feature flag must use multiple variants with control as the first variant.'

)

const featureFlag = { ...baseFeatureFlag, is_remote_configuration: true }

expect(() => featureFlagEligibleForExperiment(featureFlag)).toThrow(

'Remote configuration feature flags cannot be used in experiments.'

)

frontend/src/scenes/experiments/experimentLogic.tsx

greptile-apps · 2025-01-31T14:28:45Z

frontend/src/scenes/experiments/experimentLogic.tsx

+                    let isValid
+                    try {
+                        isValid = featureFlagEligibleForExperiment(matchingFlag)
+                    } catch (error) {
+                        isValid = false
+                    }
+                    actions.setIsExistingFeatureFlag(true)


logic: The try-catch block swallows the error details. Consider logging the error or handling specific error cases differently.

frontend/src/scenes/experiments/experimentLogic.tsx

greptile-apps · 2025-01-31T14:29:16Z

frontend/src/scenes/experiments/utils.ts

+export function featureFlagEligibleForExperiment(featureFlag: FeatureFlagType): true {
+    if (featureFlag.deleted) {
+        throw new Error('Feature flag must not be deleted.')
+    }
+
+    if (featureFlag.experiment_set && featureFlag.experiment_set.length > 0) {
+        throw new Error('Feature flag is already associated with an experiment.')
+    }
+
+    if (featureFlag.filters.multivariate?.variants?.length && featureFlag.filters.multivariate.variants.length > 1) {
+        if (featureFlag.filters.multivariate.variants[0].key !== 'control') {
+            throw new Error('Feature flag must have control as the first variant.')
+        }
+        return true
+    }
+
+    throw new Error('Feature flag must use multiple variants with control as the first variant.')
+}


style: The function returns true but never returns false - it throws errors instead. Consider either changing return type to void or making it return a boolean.

greptile-apps · 2025-01-31T14:29:17Z

frontend/src/scenes/experiments/utils.ts

+        throw new Error('Feature flag is already associated with an experiment.')
+    }
+
+    if (featureFlag.filters.multivariate?.variants?.length && featureFlag.filters.multivariate.variants.length > 1) {


style: Optional chaining on variants.length could be simplified to variants?.length > 1 since you're already checking for truthiness

Suggested change

if (featureFlag.filters.multivariate?.variants?.length && featureFlag.filters.multivariate.variants.length > 1) {

if (featureFlag.filters.multivariate?.variants?.length > 1) {

jurajmajerik · 2025-02-03T14:59:36Z

I'm getting a form error even though everything though the flag seems to have been added correctly:

jurajmajerik · 2025-02-03T15:01:13Z

I think I would prefer the Variants header before this banner. Otherwise it looks as if the banner belongs to the Experiment type section.

Also:

Worth adding the link to the flag to the banner?
The Holdouts input should remain - it is assigned to the experiment, not to the flag.

danielbachhuber · 2025-02-03T15:26:43Z

I'm getting a form error even though everything though the flag seems to have been added correctly:

@jurajmajerik You probably started reviewing before a302b2a was applied.

danielbachhuber · 2025-02-03T15:46:34Z

I think I would prefer the Variants header before this banner. Otherwise it looks as if the banner belongs to the Experiment type section.
Worth adding the link to the flag to the banner?

@jurajmajerik Good suggestions, thank you:

The Holdouts input should remain - it is assigned to the experiment, not to the flag.

Kinda. It's actually mostly stored on the feature flag:

posthog/ee/clickhouse/views/experiments.py

Lines 304 to 309 in 7f13464

    
           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, 
        
           }

I thought about including it on the form, but I feel like it's more tied to the feature flag configuration conceptually. Another example:

Is this something you're strongly opinionated about?

jurajmajerik · 2025-02-04T13:47:10Z

Kinda. It's actually mostly stored on the feature flag:

Yes, but the selector in the form only sets the holdout_id field on an experiment. It doesn't modify the flag directly. The experiment serializer then takes care of propagating it onto the feature flag.

The Holdout group selector is available when creating an experiment with a new feature flag. Why hide it if the feature flag is already existing?

jurajmajerik · 2025-02-04T13:50:21Z

@jurajmajerik You probably started reviewing before a302b2a was applied.

All good now!

danielbachhuber · 2025-02-04T21:46:12Z

The Holdout group selector is available when creating an experiment with a new feature flag. Why hide it if the feature flag is already existing?

Per our conversation in Slack, I removed the Holdout selector from the creation form entirely.

jurajmajerik · 2025-02-05T09:06:02Z

This is a very useful feature, thanks for doing this!

First pass at attaching to existing feature flag

0d5382d

danielbachhuber mentioned this pull request Jan 29, 2025

feat(flags): Create an experiment from a feature flag #27938

Closed

Update test

44411d5

github-actions bot and others added 11 commits January 29, 2025 01:14

Update query snapshots

7331356

Merge branch 'master' into experiments/attach-existing-ff

7050098

Restore <LemonBanner>

bce1143

Fix merge conflict

e71389c

UX tweaks

817e552

Send event when used

5579d81

Tweaks

6766efd

Validation

b35101c

Move feature flag validation entirely async

fc65a5a

Merge branch 'master' into experiments/attach-existing-ff

2a68769

Drop stale test

216ccb3

danielbachhuber commented Jan 31, 2025

View reviewed changes

danielbachhuber marked this pull request as ready for review January 31, 2025 14:26

danielbachhuber requested review from a team January 31, 2025 14:26

greptile-apps bot reviewed Jan 31, 2025

View reviewed changes

danielbachhuber added 5 commits January 31, 2025 06:45

Allow experiments to be created with soft-deleted feature flags

915cfbf

Remnant from 915cfbf

f37aeff

Merge branch 'master' into experiments/attach-existing-ff

5b148dc

Persist manual validation when the field is blurred

962bd18

Expects undefined for no error, not empty string

a302b2a

Add a heading and a link to the banner

7f13464

danielbachhuber added 2 commits February 3, 2025 07:56

Fix type issue

800b047

Merge branch 'master' into experiments/attach-existing-ff

e433afa

Remove Holdout field from the experiment form

05b38a4

danielbachhuber requested a review from jurajmajerik February 4, 2025 21:03

danielbachhuber enabled auto-merge (squash) February 5, 2025 00:10

jurajmajerik approved these changes Feb 5, 2025

View reviewed changes

danielbachhuber merged commit 6a7387f into master Feb 5, 2025
105 checks passed

danielbachhuber deleted the experiments/attach-existing-ff branch February 5, 2025 09:05

jurajmajerik mentioned this pull request Feb 5, 2025

Sprint - Feb 10 - Feb 21 #28311

Open

danielbachhuber mentioned this pull request Feb 5, 2025

Creating an experiment now allows existing feature flags PostHog/posthog.com#10581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danielbachhuber commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

danielbachhuber Jan 31, 2025

jurajmajerik Feb 3, 2025

danielbachhuber Jan 31, 2025 •

edited

Loading

jurajmajerik Feb 3, 2025 •

edited

Loading

danielbachhuber Jan 31, 2025

danielbachhuber Feb 3, 2025

danielbachhuber Feb 3, 2025

greptile-apps bot left a comment

greptile-apps bot Jan 31, 2025

greptile-apps bot Jan 31, 2025

greptile-apps bot Jan 31, 2025

greptile-apps bot Jan 31, 2025

greptile-apps bot Jan 31, 2025

greptile-apps bot Jan 31, 2025

greptile-apps bot Jan 31, 2025

jurajmajerik commented Feb 3, 2025

jurajmajerik commented Feb 3, 2025 •

edited

Loading

danielbachhuber commented Feb 3, 2025

danielbachhuber commented Feb 3, 2025

jurajmajerik commented Feb 4, 2025

jurajmajerik commented Feb 4, 2025

danielbachhuber commented Feb 4, 2025

jurajmajerik commented Feb 5, 2025

		@@ -252,6 +253,23 @@ def validate_parameters(self, value):

		return value

		def validate_existing_feature_flag_for_experiment(self, feature_flag: FeatureFlag):


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

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

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

	if (featureFlag.filters.multivariate?.variants?.length && featureFlag.filters.multivariate.variants.length > 1) {
	if (featureFlag.filters.multivariate?.variants?.length > 1) {

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

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

Conversation

danielbachhuber commented Jan 29, 2025 • edited Loading

Changes

How did you test this code?

github-actions bot commented Jan 29, 2025 • edited Loading

danielbachhuber Jan 31, 2025

Choose a reason for hiding this comment

jurajmajerik Feb 3, 2025

Choose a reason for hiding this comment

danielbachhuber Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

jurajmajerik Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

danielbachhuber Jan 31, 2025

Choose a reason for hiding this comment

danielbachhuber Feb 3, 2025

Choose a reason for hiding this comment

danielbachhuber Feb 3, 2025

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 31, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 31, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 31, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 31, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 31, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 31, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 31, 2025

Choose a reason for hiding this comment

jurajmajerik commented Feb 3, 2025

jurajmajerik commented Feb 3, 2025 • edited Loading

danielbachhuber commented Feb 3, 2025

danielbachhuber commented Feb 3, 2025

jurajmajerik commented Feb 4, 2025

jurajmajerik commented Feb 4, 2025

danielbachhuber commented Feb 4, 2025

jurajmajerik commented Feb 5, 2025

danielbachhuber commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

danielbachhuber Jan 31, 2025 •

edited

Loading

jurajmajerik Feb 3, 2025 •

edited

Loading

jurajmajerik commented Feb 3, 2025 •

edited

Loading