From aa2ff3d1fcec207a58915ec64c53304e59a88be8 Mon Sep 17 00:00:00 2001 From: Anders Asheim Hennum Date: Fri, 10 Jan 2025 10:26:50 +0100 Subject: [PATCH] Revert "fix(experiments): Remove most property math operations (#27133)" This reverts commit feaf60e3c2882a5b2ba781341a21df44d635ca93. --- .../experiments/Metrics/TrendsMetricForm.tsx | 3 +- .../filters/ActionFilter/ActionFilter.tsx | 4 - .../ActionFilterRow/ActionFilterRow.tsx | 26 +-- .../experiment_trends_query_runner.py | 10 + .../test_experiment_trends_query_runner.py | 219 +++++++++++++++++- 5 files changed, 234 insertions(+), 28 deletions(-) diff --git a/frontend/src/scenes/experiments/Metrics/TrendsMetricForm.tsx b/frontend/src/scenes/experiments/Metrics/TrendsMetricForm.tsx index bfe5cb88ec168..f76d69664f008 100644 --- a/frontend/src/scenes/experiments/Metrics/TrendsMetricForm.tsx +++ b/frontend/src/scenes/experiments/Metrics/TrendsMetricForm.tsx @@ -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, PropertyMathType } from '~/types' +import { BaseMathType, ChartDisplayType, FilterType } from '~/types' import { experimentLogic } from '../experimentLogic' import { commonActionFilterProps } from './Selectors' @@ -81,7 +81,6 @@ export function TrendsMetricForm({ isSecondary = false }: { isSecondary?: boolea showSeriesIndicator={true} entitiesLimit={1} showNumericalPropsOnly={true} - onlyPropertyMathDefinitions={[PropertyMathType.Average]} {...commonActionFilterProps} />
diff --git a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx index 28c1e19d8419c..91d3c1fcf260d 100644 --- a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx +++ b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx @@ -86,8 +86,6 @@ export interface ActionFilterProps { deleteButton, orLabel, }: Record) => JSX.Element - /** Only show these property math definitions */ - onlyPropertyMathDefinitions?: Array } export const ActionFilter = React.forwardRef(function ActionFilter( @@ -118,7 +116,6 @@ export const ActionFilter = React.forwardRef( buttonType = 'tertiary', readOnly = false, bordered = false, - onlyPropertyMathDefinitions, }, ref ): JSX.Element { @@ -177,7 +174,6 @@ export const ActionFilter = React.forwardRef( onRenameClick: showModal, sortable, showNumericalPropsOnly, - onlyPropertyMathDefinitions, } const reachedLimit: boolean = Boolean(entitiesLimit && localFilters.length >= entitiesLimit) diff --git a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx index 026f8bdda9de7..15586e766a310 100644 --- a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx +++ b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx @@ -126,8 +126,6 @@ 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 } export function ActionFilterRow({ @@ -157,7 +155,6 @@ export function ActionFilterRow({ renderRow, trendsDisplayCategory, showNumericalPropsOnly, - onlyPropertyMathDefinitions, }: ActionFilterRowProps): JSX.Element { const { entityFilterVisible } = useValues(logic) const { @@ -428,7 +425,6 @@ export function ActionFilterRow({ style={{ maxWidth: '100%', width: 'initial' }} mathAvailability={mathAvailability} trendsDisplayCategory={trendsDisplayCategory} - onlyPropertyMathDefinitions={onlyPropertyMathDefinitions} /> {mathDefinitions[math || BaseMathType.TotalCount]?.category === MathCategory.PropertyValue && ( @@ -646,8 +642,6 @@ interface MathSelectorProps { onMathSelect: (index: number, value: any) => any trendsDisplayCategory: ChartDisplayCategory | null style?: React.CSSProperties - /** Only show these property math definitions */ - onlyPropertyMathDefinitions?: Array } function isPropertyValueMath(math: string | undefined): math is PropertyMathType { @@ -666,7 +660,6 @@ function useMathSelectorOptions({ mathAvailability, onMathSelect, trendsDisplayCategory, - onlyPropertyMathDefinitions, }: MathSelectorProps): LemonSelectOptions { const mountedInsightDataLogic = insightDataLogic.findMounted() const query = mountedInsightDataLogic?.values?.query @@ -765,19 +758,12 @@ function useMathSelectorOptions({ setPropertyMathTypeShown(value as PropertyMathType) onMathSelect(index, value) }} - 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}`, - }))} + options={Object.entries(PROPERTY_MATH_DEFINITIONS).map(([key, definition]) => ({ + value: key, + label: definition.shortName, + tooltip: definition.description, + 'data-attr': `math-${key}-${index}`, + }))} onClick={(e) => e.stopPropagation()} size="small" dropdownMatchSelectWidth={false} diff --git a/posthog/hogql_queries/experiments/experiment_trends_query_runner.py b/posthog/hogql_queries/experiments/experiment_trends_query_runner.py index b09187c4453aa..b08ba1306c8f7 100644 --- a/posthog/hogql_queries/experiments/experiment_trends_query_runner.py +++ b/posthog/hogql_queries/experiments/experiment_trends_query_runner.py @@ -38,6 +38,7 @@ ExperimentTrendsQueryResponse, ExperimentVariantTrendsBaseStats, DateRange, + PropertyMathType, PropertyOperator, TrendsFilter, TrendsQuery, @@ -128,6 +129,15 @@ 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): diff --git a/posthog/hogql_queries/experiments/test/test_experiment_trends_query_runner.py b/posthog/hogql_queries/experiments/test/test_experiment_trends_query_runner.py index 58aef9b8c05c7..adc42d82fff00 100644 --- a/posthog/hogql_queries/experiments/test/test_experiment_trends_query_runner.py +++ b/posthog/hogql_queries/experiments/test/test_experiment_trends_query_runner.py @@ -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, "avg") + self.assertEqual(prepared_count_query.series[0].math, "sum") result = query_runner.calculate() trend_result = cast(ExperimentTrendsQueryResponse, result) @@ -1364,7 +1364,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, "avg") + self.assertEqual(prepared_count_query.series[0].math, "sum") result = query_runner.calculate() trend_result = cast(ExperimentTrendsQueryResponse, result) @@ -1642,6 +1642,221 @@ 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()