feat(experiments): Add custom exposures to new query runner #29015

danielbachhuber · 2025-02-20T16:48:16Z

Changes

Adds a new "Exposure criteria" UI element where the customer can:

Choose between Default and Custom as their exposure criteria.
Specify an event and event property filters for their custom exposure.
Enable or disable the test and internal accounts filter.

I didn't actually display anything in the UI element because I didn't have any great ideas for it yet.

Applies the exposure criteria to ExperimentQueryRunner and ExperimentExposureQueryRunner.

CleanShot.2025-02-20.at.16.52.16.mp4

How did you test this code?

Tests should pass.

I also created a new experiment, sent some test events, and verified I could use custom exposure criteria as expected.

github-actions · 2025-02-20T17:44:07Z

Size Change: 0 B

Total Size: 9.71 MB

ℹ️ View Unchanged

Filename	Size
`frontend/dist/toolbar.js`	9.71 MB

_{compressed-size-action}

greptile-apps

PR Summary

Based on the provided files, I'll summarize the key changes in this PR that adds custom exposure criteria to experiments:

Introduces a new "Exposure criteria" UI component allowing users to choose between Default (using $feature_flag_called event) and Custom exposure tracking modes.
Adds exposure_criteria JSONField to the Experiment model with corresponding migration, storing custom event and property filters for experiment exposure tracking.
Refactors experiment query runners (ExperimentQueryRunner and ExperimentExposureQueryRunner) to support both default and custom exposure configurations.
Moves filterTestAccounts from individual metrics to experiment-level exposure_criteria, consolidating test account filtering configuration.
Adds new schema types ExperimentEventExposureConfig and ExperimentExposureCriteria to support structured exposure configuration.

The implementation appears well-structured with:

Comprehensive test coverage for both default and custom exposure scenarios
Proper error handling and timezone support in query runners
Backward compatibility by defaulting to original behavior when no custom config is present
Clean UI integration with existing experiment management interface

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

greptile-apps · 2025-02-21T00:56:59Z

frontend/src/scenes/experiments/ExperimentMetricForm.tsx

@@ -37,9 +37,11 @@ const dataWarehousePopoverFields: DataWarehousePopoverField[] = [
 export function ExperimentMetricForm({
    metric,
    handleSetMetric,
+    filterTestAccounts,
 }: {
    metric: ExperimentMetric
    handleSetMetric: any


style: handleSetMetric is typed as 'any' which could lead to type safety issues

Suggested change

handleSetMetric: any

handleSetMetric: (params: { newMetric: ExperimentMetric }) => void

greptile-apps · 2025-02-21T00:57:29Z

frontend/src/scenes/experiments/ExperimentView/ExposureCriteria.tsx

+            <LemonButton className="mt-2" size="xsmall" type="secondary" onClick={() => openExposureCriteriaModal()}>
+                Edit
+            </LemonButton>
+            <ExposureCriteriaModal />


style: Modal is rendered unconditionally which could cause unnecessary re-renders. Consider moving ExposureCriteriaModal to a separate component and only rendering when isExposureCriteriaModalOpen is true.

greptile-apps · 2025-02-21T00:57:30Z

frontend/src/scenes/experiments/ExperimentView/ExposureCriteria.tsx

+                checked={(() => {
+                    const val = experiment.exposure_criteria?.filterTestAccounts
+                    return hasFilters ? !!val : false
+                })()}


style: IIFE is unnecessary here and adds complexity. Can be simplified to: checked={hasFilters && !!experiment.exposure_criteria?.filterTestAccounts}

Suggested change

checked={(() => {

const val = experiment.exposure_criteria?.filterTestAccounts

return hasFilters ? !!val : false

})()}

checked={hasFilters && !!experiment.exposure_criteria?.filterTestAccounts}

greptile-apps · 2025-02-21T00:57:31Z

frontend/src/scenes/experiments/ExperimentView/ExposureCriteria.tsx

+                                event: '$feature_flag_called',
+                                properties: [],


logic: Custom mode initializes with $feature_flag_called event which could be confusing since that's the default mode's behavior. Consider initializing with null or empty string.

greptile-apps · 2025-02-21T00:57:31Z

frontend/src/scenes/experiments/ExperimentView/ExposureCriteria.tsx

+                        form="edit-experiment-exposure-form"
+                        type="secondary"


logic: form attribute is specified but no <form> element exists in the modal. This could cause unexpected behavior with form submission.

greptile-apps · 2025-02-21T00:58:48Z

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

        }

-        const query = metricToQuery(metric)
+        const query = metricToQuery(metric, false)


style: filterTestAccounts parameter is now required but not documented in function signature

greptile-apps · 2025-02-21T00:59:00Z

frontend/src/scenes/experiments/utils.ts

+export function exposureConfigToFilter(exposure_config: ExperimentEventExposureConfig): FilterType {
+    if (exposure_config.kind === NodeKind.ExperimentEventExposureConfig) {
+        return {
+            events: [
+                {
+                    id: exposure_config.event,
+                    name: exposure_config.event,
+                    kind: NodeKind.EventsNode,
+                    type: 'events',
+                    properties: exposure_config.properties,
+                } as EventsNode,
+            ],
+            actions: [],
+            data_warehouse: [],
+        }
+    }
+
+    return {}
+}


logic: This function returns an empty object if the exposure config kind doesn't match ExperimentEventExposureConfig. Should throw an error or handle other kinds explicitly.

Suggested change

export function exposureConfigToFilter(exposure_config: ExperimentEventExposureConfig): FilterType {

if (exposure_config.kind === NodeKind.ExperimentEventExposureConfig) {

return {

events: [

{

id: exposure_config.event,

name: exposure_config.event,

kind: NodeKind.EventsNode,

type: 'events',

properties: exposure_config.properties,

} as EventsNode,

],

actions: [],

data_warehouse: [],

}

}

return {}

}

export function exposureConfigToFilter(exposure_config: ExperimentEventExposureConfig): FilterType {

if (exposure_config.kind === NodeKind.ExperimentEventExposureConfig) {

return {

events: [

{

id: exposure_config.event,

name: exposure_config.event,

kind: NodeKind.EventsNode,

type: 'events',

properties: exposure_config.properties,

} as EventsNode,

],

actions: [],

data_warehouse: [],

}

}

throw new Error(`Unsupported exposure config kind: ${exposure_config.kind}`)

}

greptile-apps · 2025-02-21T00:59:01Z

frontend/src/scenes/experiments/utils.ts

+    if (entity.kind === NodeKind.EventsNode) {
+        if (entity.type === 'events') {
+            return {
+                kind: NodeKind.ExperimentEventExposureConfig,
+                event: entity.id,
+                properties: entity.properties,
+            }
+        }
+    }


style: Consider supporting other node kinds like ActionsNode for exposure config, similar to how metricConfigToFilter handles multiple types.

greptile-apps · 2025-02-21T00:59:50Z

posthog/hogql_queries/experiments/experiment_exposures_query_runner.py

+        if exposure_config and exposure_config.get("kind") == "ExperimentEventExposureConfig":
+            event_name = exposure_config.get("event")


logic: Should validate that event_name is not empty/None when using custom exposure config

Validated on the way in: 795dceb

greptile-apps · 2025-02-21T01:00:37Z

posthog/hogql_queries/experiments/test/test_experiment_exposures_query_runner.py

+        self.assertEqual(response.total_exposures["control"], 3)
+        self.assertEqual(response.total_exposures["test"], 5)


logic: Test case doesn't verify that control user with plan=free was correctly excluded from exposure count

Invalid:

diff --git a/posthog/hogql_queries/experiments/test/test_experiment_exposures_query_runner.py b/posthog/hogql_queries/experiments/test/test_experiment_exposures_query_runner.py index c85b5d3a79..c12fc78598 100644 --- a/posthog/hogql_queries/experiments/test/test_experiment_exposures_query_runner.py +++ b/posthog/hogql_queries/experiments/test/test_experiment_exposures_query_runner.py @@ -559,7 +559,7 @@ class TestExperimentExposuresQueryRunner(ClickhouseTestMixin, APIBaseTest): { "event": "$pageview", "timestamp": "2024-01-03", - "properties": {ff_property: "control", "plan": "free"}, + "properties": {ff_property: "control"}, }, ], "user_test_1": [

FAILED posthog/hogql_queries/experiments/test/test_experiment_exposures_query_runner.py::TestExperimentExposuresQueryRunner::test_exposure_query_with_custom_exposure - AssertionError: 4.0 != 3

jurajmajerik

Looks great! I'm getting the following error though:

"/Users/jurajmajerik/posthog/posthog/hogql_queries/experiments/experiment_query_runner.py", line 454, in <lambda>
    sorted_results = sorted(response.results, key=lambda x: self.variants.index(x[0]))
                                                            ^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: '' is not in list

I think the issue is:

Some $feature_flag_called events are missing the variant value.
These events are still included in the query response with an empty string ("") as the breakdown.
The sorting line fails because "" is not in self.variants:
sorted_results = sorted(response.results, key=lambda x: self.variants.index(x[0]))

To fix this, you could:

Make sure the breakdown only includes valid variants
Handle the sorting more gracefully

Please also add a test, as $feature_flag_called events without variant data are common in production!

danielbachhuber · 2025-02-21T13:04:20Z

To fix this, you could:
* Make sure the breakdown only includes valid variants
* Handle the sorting more gracefully
Please also add a test, as $feature_flag_called events without variant data are common in production!

@jurajmajerik Fixed up with ea3c873

I think the issue is:
* Some $feature_flag_called events are missing the variant value.
* These events are still included in the query response with an empty string ("") as the breakdown.
* The sorting line fails because "" is not in self.variants:
sorted_results = sorted(response.results, key=lambda x: self.variants.index(x[0]))

Funny enough, I spent an hour pulling my hair out in the car debugging this issue 🙃 I sent bad data into the system too.

I decided not to fix at the time because I figured it was better for the query to error if you have invalid data vs. silently ignore it. I guess we can incorporate it in some form of health check later, though.

ee/clickhouse/views/experiments.py

jurajmajerik

Great! 🙌

danielbachhuber and others added 5 commits February 20, 2025 08:47

Move "Filter test accounts" to new exposure criteria modal

303bc1b

Update query snapshots

673f08b

Update query snapshots

efe8461

Apply experiment setting to exposure query

59b2f1b

Fix type issues

e73371e

danielbachhuber added 6 commits February 20, 2025 10:20

Frontend UI for managing custom exposure

11733c0

Fix another type issue

f3d2c74

Apply custom exposure to query runner

89e0ccd

Update snapshots

6dcf633

Apply property filters

d91b130

Update exposure queries runner to support custom queries

14deb09

danielbachhuber changed the title ~~feat(experiments): First pass at exposure criteria modal~~ feat(experiments): Adds custom exposures to new query runner Feb 21, 2025

Merge branch 'master' into experiments/exposure-criteria

3f5cf7c

danielbachhuber marked this pull request as ready for review February 21, 2025 00:54

danielbachhuber requested a review from a team February 21, 2025 00:54

danielbachhuber changed the title ~~feat(experiments): Adds custom exposures to new query runner~~ feat(experiments): Add custom exposures to new query runner Feb 21, 2025

greptile-apps bot reviewed Feb 21, 2025

View reviewed changes

jurajmajerik reviewed Feb 21, 2025

View reviewed changes

danielbachhuber added 2 commits February 21, 2025 04:37

Ensure $feature_flag_called has the data we need

ea3c873

Validate exposure_criteria

795dceb

danielbachhuber requested a review from jurajmajerik February 21, 2025 13:04

github-advanced-security bot found potential problems Feb 21, 2025

View reviewed changes

ee/clickhouse/views/experiments.py Fixed Show resolved Hide resolved

danielbachhuber added 4 commits February 21, 2025 05:09

Bump cache version

f6e6191

Drop the exception

6fcae72

Only allow events as exposure criteria right now

b84a070

Merge branch 'master' into experiments/exposure-criteria

a655f3c

jurajmajerik approved these changes Feb 21, 2025

View reviewed changes

danielbachhuber merged commit 5ced32e into master Feb 21, 2025
97 checks passed

danielbachhuber deleted the experiments/exposure-criteria branch February 21, 2025 14:28

danielbachhuber mentioned this pull request Feb 21, 2025

fix(experiments): Handle None value for exposure_criteria #29066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(experiments): Add custom exposures to new query runner #29015

feat(experiments): Add custom exposures to new query runner #29015

danielbachhuber commented Feb 20, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

greptile-apps bot Feb 21, 2025

danielbachhuber Feb 21, 2025

greptile-apps bot Feb 21, 2025

danielbachhuber Feb 21, 2025

jurajmajerik left a comment

danielbachhuber commented Feb 21, 2025

jurajmajerik left a comment

	handleSetMetric: any
	handleSetMetric: (params: { newMetric: ExperimentMetric }) => void

		if exposure_config and exposure_config.get("kind") == "ExperimentEventExposureConfig":
		event_name = exposure_config.get("event")

		self.assertEqual(response.total_exposures["control"], 3)
		self.assertEqual(response.total_exposures["test"], 5)

feat(experiments): Add custom exposures to new query runner #29015

feat(experiments): Add custom exposures to new query runner #29015

Conversation

danielbachhuber commented Feb 20, 2025 • edited Loading

Changes

How did you test this code?

github-actions bot commented Feb 20, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

danielbachhuber Feb 21, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 21, 2025

Choose a reason for hiding this comment

danielbachhuber Feb 21, 2025

Choose a reason for hiding this comment

jurajmajerik left a comment

Choose a reason for hiding this comment

danielbachhuber commented Feb 21, 2025

jurajmajerik left a comment

Choose a reason for hiding this comment

danielbachhuber commented Feb 20, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025 •

edited

Loading