From d9973c7e5d886f9b6f93e9c772df6951629c9e45 Mon Sep 17 00:00:00 2001 From: Alex Gyujin Kim Date: Mon, 19 Jul 2021 12:00:59 -0700 Subject: [PATCH] Add all steps option to time conversion funnel (#5142) * add all steps option * all steps working; add total and mean time to convert * change display type checks to use enum * kea types * dangling console log * Add average conversion time to time to convert results * respond to feedabck * responsive histogram sizes * merged @Twixes backend changes; adjust data shape on frontend; add responsiveness to histogram * add tooltip label * adjust copy and tooltip * minor tweaks * respond to general feedback * kea auto * better empty state: * error handling null time bins * fix tests Co-authored-by: Michael Matloka Co-authored-by: Paolo D'Amico --- .../queries/funnels/funnel_time_to_convert.py | 26 +-- .../test/test_funnel_time_to_convert.py | 123 +++++++------- .../views/test/test_clickhouse_insights.py | 24 ++- frontend/src/lib/constants.tsx | 1 - frontend/src/lib/utils.tsx | 4 +- .../src/scenes/funnels/FunnelCanvasLabel.tsx | 32 ++-- .../src/scenes/funnels/FunnelHistogram.scss | 8 +- .../src/scenes/funnels/FunnelHistogram.tsx | 37 +++-- frontend/src/scenes/funnels/FunnelViz.tsx | 2 +- frontend/src/scenes/funnels/funnelLogic.ts | 151 ++++++++++++------ frontend/src/scenes/funnels/funnelUtils.ts | 22 ++- .../scenes/insights/Histogram/Histogram.scss | 13 +- .../scenes/insights/Histogram/Histogram.tsx | 34 +++- .../insights/Histogram/histogramLogic.ts | 18 +++ .../insights/Histogram/histogramUtils.ts | 84 ++++++---- .../InsightTabs/InsightDisplayConfig.tsx | 3 +- frontend/src/types.ts | 43 ++++- package.json | 1 + yarn.lock | 50 +++++- 19 files changed, 455 insertions(+), 221 deletions(-) create mode 100644 frontend/src/scenes/insights/Histogram/histogramLogic.ts diff --git a/ee/clickhouse/queries/funnels/funnel_time_to_convert.py b/ee/clickhouse/queries/funnels/funnel_time_to_convert.py index 2ec31ae73d8..a08d5f93dc0 100644 --- a/ee/clickhouse/queries/funnels/funnel_time_to_convert.py +++ b/ee/clickhouse/queries/funnels/funnel_time_to_convert.py @@ -16,8 +16,11 @@ class ClickhouseFunnelTimeToConvert(ClickhouseFunnelBase): super().__init__(filter, team) self.funnel_order = funnel_order_class(filter, team) - def _format_results(self, results: list) -> list: - return results + def _format_results(self, results: list) -> dict: + return { + "bins": [(bin_from_seconds, person_count) for bin_from_seconds, person_count, _ in results], + "average_conversion_time": results[0][2], + } def get_query(self) -> str: steps_per_person_query = self.funnel_order.get_step_counts_query() @@ -68,6 +71,7 @@ class ClickhouseFunnelTimeToConvert(ClickhouseFunnelBase): SELECT floor(min({steps_average_conversion_time_expression_sum})) AS from_seconds, ceil(max({steps_average_conversion_time_expression_sum})) AS to_seconds, + round(avg({steps_average_conversion_time_expression_sum}), 2) AS average_conversion_time, {bin_count_expression or ""} ceil((to_seconds - from_seconds) / {bin_count_identifier}) AS bin_width_seconds_raw, -- Use 60 seconds as fallback bin width in case of only one sample @@ -82,27 +86,29 @@ class ClickhouseFunnelTimeToConvert(ClickhouseFunnelBase): if bin_count_expression else "" } ( SELECT from_seconds FROM histogram_params ) AS histogram_from_seconds, - ( SELECT to_seconds FROM histogram_params ) AS histogram_to_seconds + ( SELECT to_seconds FROM histogram_params ) AS histogram_to_seconds, + ( SELECT average_conversion_time FROM histogram_params ) AS histogram_average_conversion_time SELECT - bin_to_seconds, - person_count + bin_from_seconds, + person_count, + histogram_average_conversion_time AS average_conversion_time FROM ( -- Calculating bins from step runs SELECT - histogram_from_seconds + floor(({steps_average_conversion_time_expression_sum} - histogram_from_seconds) / bin_width_seconds) * bin_width_seconds AS bin_to_seconds, + histogram_from_seconds + floor(({steps_average_conversion_time_expression_sum} - histogram_from_seconds) / bin_width_seconds) * bin_width_seconds AS bin_from_seconds, count() AS person_count FROM step_runs -- We only need to check step to_step here, because it depends on all the other ones being NOT NULL too WHERE step_{to_step}_average_conversion_time IS NOT NULL - GROUP BY bin_to_seconds + GROUP BY bin_from_seconds ) results FULL OUTER JOIN ( -- Making sure bin_count bins are returned -- Those not present in the results query due to lack of data simply get person_count 0 - SELECT histogram_from_seconds + number * bin_width_seconds AS bin_to_seconds FROM system.numbers LIMIT {bin_count_identifier} + 1 + SELECT histogram_from_seconds + number * bin_width_seconds AS bin_from_seconds FROM system.numbers LIMIT {bin_count_identifier} + 1 ) fill - USING (bin_to_seconds) - ORDER BY bin_to_seconds + USING (bin_from_seconds) + ORDER BY bin_from_seconds SETTINGS allow_experimental_window_functions = 1""" return query diff --git a/ee/clickhouse/queries/funnels/test/test_funnel_time_to_convert.py b/ee/clickhouse/queries/funnels/test/test_funnel_time_to_convert.py index a9bebb04a55..f14605bdd1e 100644 --- a/ee/clickhouse/queries/funnels/test/test_funnel_time_to_convert.py +++ b/ee/clickhouse/queries/funnels/test/test_funnel_time_to_convert.py @@ -67,12 +67,15 @@ class TestFunnelTrends(ClickhouseTestMixin, APIBaseTest): # Autobinned using the minimum time to convert, maximum time to convert, and sample count self.assertEqual( results, - [ - (2220.0, 2), # Reached step 1 from step 0 in at least 2200 s but less than 29_080 s - users A and B - (29080.0, 0), # Analogous to above, just an interval (in this case 26_880 s) up - no users - (55940.0, 0), # Same as above - (82800.0, 1), # Reached step 1 from step 0 in at least 82_800 s but less than 109_680 s - user C - ], + { + "bins": [ + (2220.0, 2), # Reached step 1 from step 0 in at least 2200 s but less than 29_080 s - users A and B + (29080.0, 0), # Analogous to above, just an interval (in this case 26_880 s) up - no users + (55940.0, 0), # Same as above + (82800.0, 1), # Reached step 1 from step 0 in at least 82_800 s but less than 109_680 s - user C + ], + "average_conversion_time": 29_540, + }, ) def test_custom_bin_count_single_step(self): @@ -117,16 +120,19 @@ class TestFunnelTrends(ClickhouseTestMixin, APIBaseTest): # 7 bins, autoscaled to work best with minimum time to convert and maximum time to convert at hand self.assertEqual( results, - [ - (2220.0, 2), # Reached step 1 from step 0 in at least 2200 s but less than 13_732 s - users A and B - (13732.0, 0), # Analogous to above, just an interval (in this case 13_732 s) up - no users - (25244.0, 0), # And so on - (36756.0, 0), - (48268.0, 0), - (59780.0, 0), - (71292.0, 1), # Reached step 1 from step 0 in at least 71_292 s but less than 82_804 s - user C - (82804.0, 0), - ], + { + "bins": [ + (2220.0, 2), # Reached step 1 from step 0 in at least 2200 s but less than 13_732 s - users A and B + (13732.0, 0), # Analogous to above, just an interval (in this case 13_732 s) up - no users + (25244.0, 0), # And so on + (36756.0, 0), + (48268.0, 0), + (59780.0, 0), + (71292.0, 1), # Reached step 1 from step 0 in at least 71_292 s but less than 82_804 s - user C + (82804.0, 0), + ], + "average_conversion_time": 29_540, + }, ) def test_auto_bin_count_total(self): @@ -151,8 +157,6 @@ class TestFunnelTrends(ClickhouseTestMixin, APIBaseTest): "interval": "day", "date_from": "2021-06-07 00:00:00", "date_to": "2021-06-13 23:59:59", - "funnel_from_step": 0, - "funnel_to_step": 2, "funnel_window_days": 7, "events": [ {"id": "step one", "order": 0}, @@ -167,42 +171,24 @@ class TestFunnelTrends(ClickhouseTestMixin, APIBaseTest): self.assertEqual( results, - [ - (10800.0, 1), # Reached step 2 from step 0 in at least 10_800 s but less than 10_860 s - user A - (10860.0, 0), # Analogous to above, just an interval (in this case 60 s) up - no users - (10920.0, 0), # And so on - (10980.0, 0), - ], - ) - - # check with no params - filter = Filter( - data={ - "insight": INSIGHT_FUNNELS, - "interval": "day", - "date_from": "2021-06-07 00:00:00", - "date_to": "2021-06-13 23:59:59", - "funnel_window_days": 7, - "events": [ - {"id": "step one", "order": 0}, - {"id": "step two", "order": 1}, - {"id": "step three", "order": 2}, + { + "bins": [ + (10800.0, 1), # Reached step 2 from step 0 in at least 10_800 s but less than 10_860 s - user A + (10860.0, 0), # Analogous to above, just an interval (in this case 60 s) up - no users + (10920.0, 0), # And so on + (10980.0, 0), ], - } + "average_conversion_time": 10_800, + }, ) - funnel_trends = ClickhouseFunnelTimeToConvert(filter, self.team, ClickhouseFunnel) - results = funnel_trends.run() - - self.assertEqual( - results, - [ - (10800.0, 1), # Reached step 2 from step 0 in at least 10_800 s but less than 10_860 s - user A - (10860.0, 0), # Analogous to above, just an interval (in this case 60 s) up - no users - (10920.0, 0), # And so on - (10980.0, 0), - ], + # Let's verify that behavior with steps unspecified is the same as when first and last steps specified + funnel_trends_steps_specified = ClickhouseFunnelTimeToConvert( + Filter(data={**filter._data, "funnel_from_step": 0, "funnel_to_step": 2,}), self.team, ClickhouseFunnel ) + results_steps_specified = funnel_trends_steps_specified.run() + + self.assertEqual(results, results_steps_specified) def test_basic_unordered(self): _create_person(distinct_ids=["user a"], team=self.team) @@ -212,12 +198,15 @@ class TestFunnelTrends(ClickhouseTestMixin, APIBaseTest): _create_event(event="step three", distinct_id="user a", team=self.team, timestamp="2021-06-08 18:00:00") _create_event(event="step one", distinct_id="user a", team=self.team, timestamp="2021-06-08 19:00:00") _create_event(event="step two", distinct_id="user a", team=self.team, timestamp="2021-06-08 21:00:00") + # Converted from 0 to 1 in 7200 s _create_event(event="step one", distinct_id="user b", team=self.team, timestamp="2021-06-09 13:00:00") _create_event(event="step two", distinct_id="user b", team=self.team, timestamp="2021-06-09 13:37:00") + # Converted from 0 to 1 in 2200 s _create_event(event="step two", distinct_id="user c", team=self.team, timestamp="2021-06-11 07:00:00") _create_event(event="step one", distinct_id="user c", team=self.team, timestamp="2021-06-12 06:00:00") + # Converted from 0 to 1 in 82_800 s filter = Filter( data={ @@ -243,12 +232,15 @@ class TestFunnelTrends(ClickhouseTestMixin, APIBaseTest): # Autobinned using the minimum time to convert, maximum time to convert, and sample count self.assertEqual( results, - [ - (2220.0, 2), # Reached step 1 from step 0 in at least 2200 s but less than 29_080 s - users A and B - (29080.0, 0), # Analogous to above, just an interval (in this case 26_880 s) up - no users - (55940.0, 0), # Same as above - (82800.0, 1), # Reached step 1 from step 0 in at least 82_800 s but less than 109_680 s - user C - ], + { + "bins": [ + (2220.0, 2), # Reached step 1 from step 0 in at least 2200 s but less than 29_080 s - users A and B + (29080.0, 0), # Analogous to above, just an interval (in this case 26_880 s) up - no users + (55940.0, 0), # Same as above + (82800.0, 1), # Reached step 1 from step 0 in at least 82_800 s but less than 109_680 s - user C + ], + "average_conversion_time": 29540, + }, ) def test_basic_strict(self): @@ -259,18 +251,22 @@ class TestFunnelTrends(ClickhouseTestMixin, APIBaseTest): _create_event(event="step one", distinct_id="user a", team=self.team, timestamp="2021-06-08 18:00:00") _create_event(event="step two", distinct_id="user a", team=self.team, timestamp="2021-06-08 19:00:00") + # Converted from 0 to 1 in 3600 s _create_event(event="step three", distinct_id="user a", team=self.team, timestamp="2021-06-08 21:00:00") _create_event(event="step one", distinct_id="user b", team=self.team, timestamp="2021-06-09 13:00:00") _create_event(event="step two", distinct_id="user b", team=self.team, timestamp="2021-06-09 13:37:00") + # Converted from 0 to 1 in 2200 s _create_event(event="blah", distinct_id="user b", team=self.team, timestamp="2021-06-09 13:38:00") _create_event(event="step three", distinct_id="user b", team=self.team, timestamp="2021-06-09 13:39:00") _create_event(event="step one", distinct_id="user c", team=self.team, timestamp="2021-06-11 07:00:00") _create_event(event="step two", distinct_id="user c", team=self.team, timestamp="2021-06-12 06:00:00") + # Converted from 0 to 1 in 82_800 s _create_event(event="step one", distinct_id="user d", team=self.team, timestamp="2021-06-11 07:00:00") _create_event(event="blah", distinct_id="user d", team=self.team, timestamp="2021-06-12 07:00:00") + # Blah cancels conversion _create_event(event="step two", distinct_id="user d", team=self.team, timestamp="2021-06-12 09:00:00") filter = Filter( @@ -297,10 +293,13 @@ class TestFunnelTrends(ClickhouseTestMixin, APIBaseTest): # Autobinned using the minimum time to convert, maximum time to convert, and sample count self.assertEqual( results, - [ - (2220.0, 2), # Reached step 1 from step 0 in at least 2200 s but less than 29_080 s - users A and B - (29080.0, 0), # Analogous to above, just an interval (in this case 26_880 s) up - no users - (55940.0, 0), # Same as above - (82800.0, 1), # Reached step 1 from step 0 in at least 82_800 s but less than 109_680 s - user C - ], + { + "bins": [ + (2220.0, 2), # Reached step 1 from step 0 in at least 2200 s but less than 29_080 s - users A and B + (29080.0, 0), # Analogous to above, just an interval (in this case 26_880 s) up - no users + (55940.0, 0), # Same as above + (82800.0, 1), # Reached step 1 from step 0 in at least 82_800 s but less than 109_680 s - user C + ], + "average_conversion_time": 29540, + }, ) diff --git a/ee/clickhouse/views/test/test_clickhouse_insights.py b/ee/clickhouse/views/test/test_clickhouse_insights.py index 35f316a7c56..ddcb6ff1be4 100644 --- a/ee/clickhouse/views/test/test_clickhouse_insights.py +++ b/ee/clickhouse/views/test/test_clickhouse_insights.py @@ -260,7 +260,13 @@ class ClickhouseTestFunnelTypes(ClickhouseTestMixin, APIBaseTest): self.assertEqual(response.status_code, 200) self.assertEqual( - response.json(), {"result": [[2220.0, 2], [29080.0, 0], [55940.0, 0], [82800.0, 1],]}, + response.json(), + { + "result": { + "bins": [[2220.0, 2], [29080.0, 0], [55940.0, 0], [82800.0, 1]], + "average_conversion_time": 29540.0, + } + }, ) def test_funnel_time_to_convert_auto_bins_strict(self): @@ -302,7 +308,13 @@ class ClickhouseTestFunnelTypes(ClickhouseTestMixin, APIBaseTest): self.assertEqual(response.status_code, 200) self.assertEqual( - response.json(), {"result": [[2220.0, 2], [29080.0, 0], [55940.0, 0], [82800.0, 1],]}, + response.json(), + { + "result": { + "bins": [[2220.0, 2], [29080.0, 0], [55940.0, 0], [82800.0, 1]], + "average_conversion_time": 29540.0, + } + }, ) def test_funnel_time_to_convert_auto_bins_unordered(self): @@ -344,7 +356,13 @@ class ClickhouseTestFunnelTypes(ClickhouseTestMixin, APIBaseTest): self.assertEqual(response.status_code, 200) self.assertEqual( - response.json(), {"result": [[2220.0, 2], [29080.0, 0], [55940.0, 0], [82800.0, 1],]}, + response.json(), + { + "result": { + "bins": [[2220.0, 2], [29080.0, 0], [55940.0, 0], [82800.0, 1]], + "average_conversion_time": 29540.0, + } + }, ) def test_funnel_invalid_action_handled(self): diff --git a/frontend/src/lib/constants.tsx b/frontend/src/lib/constants.tsx index 26e0561e93e..38634d51a58 100644 --- a/frontend/src/lib/constants.tsx +++ b/frontend/src/lib/constants.tsx @@ -11,7 +11,6 @@ export const ACTIONS_BAR_CHART = 'ActionsBar' export const ACTIONS_BAR_CHART_VALUE = 'ActionsBarValue' export const PATHS_VIZ = 'PathsViz' export const FUNNEL_VIZ = 'FunnelViz' -export const FUNNELS_TIME_TO_CONVERT = 'FunnelsTimeToConvert' export enum OrganizationMembershipLevel { Member = 1, diff --git a/frontend/src/lib/utils.tsx b/frontend/src/lib/utils.tsx index ec13ac0317d..641b04b0920 100644 --- a/frontend/src/lib/utils.tsx +++ b/frontend/src/lib/utils.tsx @@ -414,10 +414,10 @@ export function slugify(text: string): string { .replace(/--+/g, '-') } -export function humanFriendlyDuration(d: string | number | null, maxUnits?: number): string { +export function humanFriendlyDuration(d: string | number | null | undefined, maxUnits?: number): string { // Convert `d` (seconds) to a human-readable duration string. // Example: `1d 10hrs 9mins 8s` - if (d === '' || d === null) { + if (d === '' || d === null || d === undefined) { return '' } d = Number(d) diff --git a/frontend/src/scenes/funnels/FunnelCanvasLabel.tsx b/frontend/src/scenes/funnels/FunnelCanvasLabel.tsx index 38071c5137a..3703d012125 100644 --- a/frontend/src/scenes/funnels/FunnelCanvasLabel.tsx +++ b/frontend/src/scenes/funnels/FunnelCanvasLabel.tsx @@ -1,17 +1,17 @@ // This file contains funnel-related components that are used in the general insights scope import { useActions, useValues } from 'kea' -import { FUNNELS_TIME_TO_CONVERT, FUNNEL_VIZ } from 'lib/constants' import { humanFriendlyDuration } from 'lib/utils' import React from 'react' -import { Button } from 'antd' +import { Button, Tooltip } from 'antd' import { insightLogic } from 'scenes/insights/insightLogic' import { funnelLogic } from './funnelLogic' import './FunnelCanvasLabel.scss' import { chartFilterLogic } from 'lib/components/ChartFilter/chartFilterLogic' import { ChartDisplayType } from '~/types' +import { InfoCircleOutlined } from '@ant-design/icons' export function FunnelCanvasLabel(): JSX.Element | null { - const { stepsWithCount, histogramStep, totalConversionRate } = useValues(funnelLogic) + const { stepsWithCount, histogramStep, conversionMetrics } = useValues(funnelLogic) const { allFilters } = useValues(insightLogic) const { setChartFilter } = useActions(chartFilterLogic) @@ -21,25 +21,35 @@ export function FunnelCanvasLabel(): JSX.Element | null { return (
- {allFilters.display === FUNNEL_VIZ && ( + {allFilters.display === ChartDisplayType.FunnelViz && ( <> - Total conversion rate: - {totalConversionRate}% + + + + + Total conversion rate:{' '} + + {conversionMetrics.totalRate}% )} - {stepsWithCount[histogramStep]?.average_conversion_time !== null ? ( + {stepsWithCount[histogramStep.from_step]?.average_conversion_time !== null && ( <> - Average time to convert: + + + + + Average time to convert:{' '} + - ) : null} + )}
) } diff --git a/frontend/src/scenes/funnels/FunnelHistogram.scss b/frontend/src/scenes/funnels/FunnelHistogram.scss index b3ee6a61210..6d8e803720c 100644 --- a/frontend/src/scenes/funnels/FunnelHistogram.scss +++ b/frontend/src/scenes/funnels/FunnelHistogram.scss @@ -1,12 +1,8 @@ -.funnel__header__info { - margin-right: 1rem; -} - -.funnel__header__steps { +.funnel-header-steps { display: flex; align-items: center; - .funnel__header__steps__label { + .funnel-header-steps-label { margin-right: 0.5rem; } } diff --git a/frontend/src/scenes/funnels/FunnelHistogram.tsx b/frontend/src/scenes/funnels/FunnelHistogram.tsx index cffb17a49d2..fdf836bf359 100644 --- a/frontend/src/scenes/funnels/FunnelHistogram.tsx +++ b/frontend/src/scenes/funnels/FunnelHistogram.tsx @@ -1,48 +1,54 @@ -import React from 'react' +import React, { useRef } from 'react' import { Col, Row, Select } from 'antd' import { useActions, useValues } from 'kea' -import { humanFriendlyDuration, humanizeNumber } from 'lib/utils' +import useSize from '@react-hook/size' +import { ANTD_TOOLTIP_PLACEMENTS, humanFriendlyDuration, humanizeNumber } from 'lib/utils' import { calcPercentage, getReferenceStep } from './funnelUtils' import { funnelLogic } from './funnelLogic' import { Histogram } from 'scenes/insights/Histogram' import { insightLogic } from 'scenes/insights/insightLogic' +import { ChartDisplayType } from '~/types' + import './FunnelHistogram.scss' -import { FUNNELS_TIME_TO_CONVERT } from 'lib/constants' export function FunnelHistogramHeader(): JSX.Element | null { const { stepsWithCount, stepReference, histogramStepsDropdown } = useValues(funnelLogic) const { changeHistogramStep } = useActions(funnelLogic) const { allFilters } = useValues(insightLogic) - if (allFilters.display !== FUNNELS_TIME_TO_CONVERT) { + if (allFilters.display !== ChartDisplayType.FunnelsTimeToConvert) { return null } return ( -
- Steps +
+ Steps {histogramStepsDropdown.length > 0 && stepsWithCount.length > 0 && (