From feaf60e3c2882a5b2ba781341a21df44d635ca93 Mon Sep 17 00:00:00 2001 From: Daniel Bachhuber Date: Mon, 23 Dec 2024 09:29:05 -0800 Subject: [PATCH] fix(experiments): Remove most property math operations (#27133) --- .../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, 28 insertions(+), 234 deletions(-) diff --git a/frontend/src/scenes/experiments/Metrics/TrendsMetricForm.tsx b/frontend/src/scenes/experiments/Metrics/TrendsMetricForm.tsx index f76d69664f008..bfe5cb88ec168 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 } from '~/types' +import { BaseMathType, ChartDisplayType, FilterType, PropertyMathType } from '~/types' import { experimentLogic } from '../experimentLogic' import { commonActionFilterProps } from './Selectors' @@ -81,6 +81,7 @@ 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 91d3c1fcf260d..28c1e19d8419c 100644 --- a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx +++ b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx @@ -86,6 +86,8 @@ export interface ActionFilterProps { deleteButton, orLabel, }: Record) => JSX.Element + /** Only show these property math definitions */ + onlyPropertyMathDefinitions?: Array } export const ActionFilter = React.forwardRef(function ActionFilter( @@ -116,6 +118,7 @@ export const ActionFilter = React.forwardRef( buttonType = 'tertiary', readOnly = false, bordered = false, + onlyPropertyMathDefinitions, }, ref ): JSX.Element { @@ -174,6 +177,7 @@ 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 15586e766a310..026f8bdda9de7 100644 --- a/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx +++ b/frontend/src/scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow.tsx @@ -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 } export function ActionFilterRow({ @@ -155,6 +157,7 @@ export function ActionFilterRow({ renderRow, trendsDisplayCategory, showNumericalPropsOnly, + onlyPropertyMathDefinitions, }: ActionFilterRowProps): JSX.Element { const { entityFilterVisible } = useValues(logic) const { @@ -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 && ( @@ -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 } function isPropertyValueMath(math: string | undefined): math is PropertyMathType { @@ -660,6 +666,7 @@ function useMathSelectorOptions({ mathAvailability, onMathSelect, trendsDisplayCategory, + onlyPropertyMathDefinitions, }: MathSelectorProps): LemonSelectOptions { const mountedInsightDataLogic = insightDataLogic.findMounted() const query = mountedInsightDataLogic?.values?.query @@ -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} diff --git a/posthog/hogql_queries/experiments/experiment_trends_query_runner.py b/posthog/hogql_queries/experiments/experiment_trends_query_runner.py index f43c7c1998a82..6cf76b4c5b4e7 100644 --- a/posthog/hogql_queries/experiments/experiment_trends_query_runner.py +++ b/posthog/hogql_queries/experiments/experiment_trends_query_runner.py @@ -38,7 +38,6 @@ ExperimentTrendsQueryResponse, ExperimentVariantTrendsBaseStats, DateRange, - PropertyMathType, PropertyOperator, TrendsFilter, TrendsQuery, @@ -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): 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 69cfc5720e009..0d3b37eeedc77 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, "sum") + self.assertEqual(prepared_count_query.series[0].math, "avg") result = query_runner.calculate() trend_result = cast(ExperimentTrendsQueryResponse, result) @@ -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) @@ -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()