Skip to content

Commit

Permalink
fix(experiments): Remove most property math operations (#27133)
Browse files Browse the repository at this point in the history
  • Loading branch information
@danielbachhuber
danielbachhuber authored Dec 23, 2024
1 parent 1323e66 commit feaf60e
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 234 deletions.
3 changes: 2 additions & 1 deletion frontend/src/scenes/experiments/Metrics/TrendsMetricForm.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { actionsAndEventsToSeries } from '~/queries/nodes/InsightQuery/utils/fil
import { queryNodeToFilter } from '~/queries/nodes/InsightQuery/utils/queryNodeToFilter'
import { Query } from '~/queries/Query/Query'
import { ExperimentTrendsQuery, InsightQueryNode, NodeKind } from '~/queries/schema'
import { BaseMathType, ChartDisplayType, FilterType } from '~/types'
import { BaseMathType, ChartDisplayType, FilterType, PropertyMathType } from '~/types'

import { experimentLogic } from '../experimentLogic'
import { commonActionFilterProps } from './Selectors'
Expand Down Expand Up @@ -81,6 +81,7 @@ export function TrendsMetricForm({ isSecondary = false }: { isSecondary?: boolea
showSeriesIndicator={true}
entitiesLimit={1}
showNumericalPropsOnly={true}
onlyPropertyMathDefinitions={[PropertyMathType.Average]}
{...commonActionFilterProps}
/>
<div className="mt-4 space-y-4">
Expand Down
4 changes: 4 additions & 0 deletions frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ export interface ActionFilterProps {
deleteButton,
orLabel,
}: Record<string, JSX.Element | string | undefined>) => JSX.Element
/** Only show these property math definitions */
onlyPropertyMathDefinitions?: Array<string>
}

export const ActionFilter = React.forwardRef<HTMLDivElement, ActionFilterProps>(function ActionFilter(
Expand Down Expand Up @@ -116,6 +118,7 @@ export const ActionFilter = React.forwardRef<HTMLDivElement, ActionFilterProps>(
buttonType = 'tertiary',
readOnly = false,
bordered = false,
onlyPropertyMathDefinitions,
},
ref
): JSX.Element {
Expand Down Expand Up @@ -174,6 +177,7 @@ export const ActionFilter = React.forwardRef<HTMLDivElement, ActionFilterProps>(
onRenameClick: showModal,
sortable,
showNumericalPropsOnly,
onlyPropertyMathDefinitions,
}

const reachedLimit: boolean = Boolean(entitiesLimit && localFilters.length >= entitiesLimit)
Expand Down
26 changes: 20 additions & 6 deletions frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ export interface ActionFilterRowProps {
trendsDisplayCategory: ChartDisplayCategory | null
/** Whether properties shown should be limited to just numerical types */
showNumericalPropsOnly?: boolean
/** Only show these property math definitions */
onlyPropertyMathDefinitions?: Array<string>
}

export function ActionFilterRow({
Expand Down Expand Up @@ -155,6 +157,7 @@ export function ActionFilterRow({
renderRow,
trendsDisplayCategory,
showNumericalPropsOnly,
onlyPropertyMathDefinitions,
}: ActionFilterRowProps): JSX.Element {
const { entityFilterVisible } = useValues(logic)
const {
Expand Down Expand Up @@ -425,6 +428,7 @@ export function ActionFilterRow({
style={{ maxWidth: '100%', width: 'initial' }}
mathAvailability={mathAvailability}
trendsDisplayCategory={trendsDisplayCategory}
onlyPropertyMathDefinitions={onlyPropertyMathDefinitions}
/>
{mathDefinitions[math || BaseMathType.TotalCount]?.category ===
MathCategory.PropertyValue && (
Expand Down Expand Up @@ -642,6 +646,8 @@ interface MathSelectorProps {
onMathSelect: (index: number, value: any) => any
trendsDisplayCategory: ChartDisplayCategory | null
style?: React.CSSProperties
/** Only show these property math definitions */
onlyPropertyMathDefinitions?: Array<string>
}

function isPropertyValueMath(math: string | undefined): math is PropertyMathType {
Expand All @@ -660,6 +666,7 @@ function useMathSelectorOptions({
mathAvailability,
onMathSelect,
trendsDisplayCategory,
onlyPropertyMathDefinitions,
}: MathSelectorProps): LemonSelectOptions<string> {
const mountedInsightDataLogic = insightDataLogic.findMounted()
const query = mountedInsightDataLogic?.values?.query
Expand Down Expand Up @@ -758,12 +765,19 @@ function useMathSelectorOptions({
setPropertyMathTypeShown(value as PropertyMathType)
onMathSelect(index, value)
}}
options={Object.entries(PROPERTY_MATH_DEFINITIONS).map(([key, definition]) => ({
value: key,
label: definition.shortName,
tooltip: definition.description,
'data-attr': `math-${key}-${index}`,
}))}
options={Object.entries(PROPERTY_MATH_DEFINITIONS)
.filter(([key]) => {
if (undefined === onlyPropertyMathDefinitions) {
return true
}
return onlyPropertyMathDefinitions.includes(key)
})
.map(([key, definition]) => ({
value: key,
label: definition.shortName,
tooltip: definition.description,
'data-attr': `math-${key}-${index}`,
}))}
onClick={(e) => e.stopPropagation()}
size="small"
dropdownMatchSelectWidth={false}
Expand Down
10 changes: 0 additions & 10 deletions posthog/hogql_queries/experiments/experiment_trends_query_runner.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
ExperimentTrendsQueryResponse,
ExperimentVariantTrendsBaseStats,
DateRange,
PropertyMathType,
PropertyOperator,
TrendsFilter,
TrendsQuery,
Expand Down Expand Up @@ -128,15 +127,6 @@ def _prepare_count_query(self) -> TrendsQuery:
"""
prepared_count_query = TrendsQuery(**self.query.count_query.model_dump())

uses_math_aggregation = self._uses_math_aggregation_by_user_or_property_value(prepared_count_query)

# :TRICKY: for `avg` aggregation, use `sum` data as an approximation
if prepared_count_query.series[0].math == PropertyMathType.AVG:
prepared_count_query.series[0].math = PropertyMathType.SUM
# TODO: revisit this; using the count data for the remaining aggregation types is likely wrong
elif uses_math_aggregation:
prepared_count_query.series[0].math = None

prepared_count_query.trendsFilter = TrendsFilter(display=ChartDisplayType.ACTIONS_LINE_GRAPH_CUMULATIVE)
prepared_count_query.dateRange = self._get_date_range()
if self._is_data_warehouse_query(prepared_count_query):
Expand Down
219 changes: 2 additions & 217 deletions posthog/hogql_queries/experiments/test/test_experiment_trends_query_runner.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1250,7 +1250,7 @@ def test_query_runner_with_avg_math(self):
flush_persons_and_events()

prepared_count_query = query_runner.prepared_count_query
self.assertEqual(prepared_count_query.series[0].math, "sum")
self.assertEqual(prepared_count_query.series[0].math, "avg")

result = query_runner.calculate()
trend_result = cast(ExperimentTrendsQueryResponse, result)
Expand Down Expand Up @@ -1363,7 +1363,7 @@ def test_query_runner_with_avg_math_v2_stats(self):
flush_persons_and_events()

prepared_count_query = query_runner.prepared_count_query
self.assertEqual(prepared_count_query.series[0].math, "sum")
self.assertEqual(prepared_count_query.series[0].math, "avg")

result = query_runner.calculate()
trend_result = cast(ExperimentTrendsQueryResponse, result)
Expand Down Expand Up @@ -1640,221 +1640,6 @@ def test_query_runner_standard_flow_v2_stats(self):
self.assertEqual(test_variant.count, 5.0)
self.assertEqual(test_variant.exposure, 1.0)

@freeze_time("2020-01-01T12:00:00Z")
def test_query_runner_property_math_sum(self):
self._test_query_runner_property_math(
math="sum",
expected_control={
"count": 10,
"absolute_exposure": 5,
"data": [0.0, 0.0, 1.0, 3.0, 6.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0],
},
expected_test={
"count": 90,
"absolute_exposure": 10,
"data": [0.0, 0.0, 2.0, 6.0, 12.0, 20.0, 30.0, 42.0, 56.0, 72.0, 90.0, 90.0, 90.0, 90.0, 90.0],
},
)

@freeze_time("2020-01-01T12:00:00Z")
def test_query_runner_property_math_avg(self):
self._test_query_runner_property_math(
math="avg",
expected_control={
"count": 10,
"absolute_exposure": 5,
"data": [0.0, 0.0, 1.0, 3.0, 6.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0],
},
expected_test={
"count": 90,
"absolute_exposure": 10,
"data": [0.0, 0.0, 2.0, 6.0, 12.0, 20.0, 30.0, 42.0, 56.0, 72.0, 90.0, 90.0, 90.0, 90.0, 90.0],
},
)

@freeze_time("2020-01-01T12:00:00Z")
def test_query_runner_property_math_min(self):
self._test_query_runner_property_math(
math="min",
expected_control={
"count": 5,
"absolute_exposure": 5,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0],
},
expected_test={
"count": 10,
"absolute_exposure": 10,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 10.0, 10.0, 10.0, 10.0],
},
)

@freeze_time("2020-01-01T12:00:00Z")
def test_query_runner_property_math_max(self):
self._test_query_runner_property_math(
math="max",
expected_control={
"count": 5,
"absolute_exposure": 5,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0],
},
expected_test={
"count": 10,
"absolute_exposure": 10,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 10.0, 10.0, 10.0, 10.0],
},
)

@freeze_time("2020-01-01T12:00:00Z")
def test_query_runner_property_math_median(self):
self._test_query_runner_property_math(
math="median",
expected_control={
"count": 5,
"absolute_exposure": 5,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0],
},
expected_test={
"count": 10,
"absolute_exposure": 10,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 10.0, 10.0, 10.0, 10.0],
},
)

@freeze_time("2020-01-01T12:00:00Z")
def test_query_runner_property_math_p90(self):
self._test_query_runner_property_math(
math="p90",
expected_control={
"count": 5,
"absolute_exposure": 5,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0],
},
expected_test={
"count": 10,
"absolute_exposure": 10,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 10.0, 10.0, 10.0, 10.0],
},
)

@freeze_time("2020-01-01T12:00:00Z")
def test_query_runner_property_math_p95(self):
self._test_query_runner_property_math(
math="p95",
expected_control={
"count": 5,
"absolute_exposure": 5,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0],
},
expected_test={
"count": 10,
"absolute_exposure": 10,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 10.0, 10.0, 10.0, 10.0],
},
)

@freeze_time("2020-01-01T12:00:00Z")
def test_query_runner_property_math_p99(self):
self._test_query_runner_property_math(
math="p99",
expected_control={
"count": 5,
"absolute_exposure": 5,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0, 5.0],
},
expected_test={
"count": 10,
"absolute_exposure": 10,
"data": [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 10.0, 10.0, 10.0, 10.0],
},
)

def _test_query_runner_property_math(self, math, expected_control, expected_test):
feature_flag = self.create_feature_flag()
experiment = self.create_experiment(feature_flag=feature_flag, start_date=datetime(2020, 1, 1))

feature_flag_property = f"$feature/{feature_flag.key}"

# control values are 0, 1, 2, 3, 4
# test values are 0, 2, 4, 6, 8, 10, 12, 14, 16, 18

# Populate metric + exposure events
for variant, count in [("control", 5), ("test", 10)]:
for i in range(count):
_create_event(
team=self.team,
event="$feature_flag_called",
distinct_id=f"user_{variant}_{i}",
properties={
"$feature_flag_response": variant,
feature_flag_property: variant,
"$feature_flag": feature_flag.key,
},
timestamp=datetime(2020, 1, i + 1),
)
_create_event(
team=self.team,
event="purchase",
distinct_id=f"user_{variant}_{i}",
properties={
feature_flag_property: variant,
"amount": i * (1 if variant == "control" else 2),
},
timestamp=datetime(2020, 1, i + 2),
)

count_query = TrendsQuery(
series=[
EventsNode(
event="purchase",
math=math,
math_property="amount",
math_property_type="event_properties",
)
]
)
exposure_query = TrendsQuery(series=[EventsNode(event="$feature_flag_called")])
experiment_query = ExperimentTrendsQuery(
experiment_id=experiment.id,
kind="ExperimentTrendsQuery",
count_query=count_query,
exposure_query=exposure_query,
)

experiment.metrics = [{"type": "primary", "query": experiment_query.model_dump()}]
experiment.save()

query_runner = ExperimentTrendsQueryRunner(
query=ExperimentTrendsQuery(**experiment.metrics[0]["query"]), team=self.team
)

flush_persons_and_events()

result = query_runner.calculate()

trend_result = cast(ExperimentTrendsQueryResponse, result)

self.assertEqual(len(result.variants), 2)

control_result = next(variant for variant in trend_result.variants if variant.key == "control")
test_result = next(variant for variant in trend_result.variants if variant.key == "test")

control_insight = next(variant for variant in trend_result.insight if variant["breakdown_value"] == "control")
test_insight = next(variant for variant in trend_result.insight if variant["breakdown_value"] == "test")

self.assertEqual(control_result.count, expected_control["count"])
self.assertEqual(test_result.count, expected_test["count"])
self.assertEqual(control_result.absolute_exposure, expected_control["absolute_exposure"])
self.assertEqual(test_result.absolute_exposure, expected_test["absolute_exposure"])

self.assertEqual(
control_insight["data"],
expected_control["data"],
)
self.assertEqual(
test_insight["data"],
expected_test["data"],
)

@freeze_time("2020-01-01T12:00:00Z")
def test_validate_event_variants_no_events(self):
feature_flag = self.create_feature_flag()
Expand Down

0 comments on commit feaf60e

Please sign in to comment.