From b3cf87ddb628ef587bcdd228cb8fb4521a9238f1 Mon Sep 17 00:00:00 2001 From: Juraj Majerik Date: Fri, 20 Dec 2024 05:52:43 +0100 Subject: [PATCH] chore(experiments): clean up legacy experimentResults (#27064) Co-authored-by: Daniel Bachhuber --- cypress/e2e/experiments.cy.ts | 2 + .../CumulativeExposuresChart.tsx | 6 +- .../ExperimentView/DataCollection.tsx | 4 +- .../ExperimentView/DistributionTable.tsx | 6 +- .../ExperimentView/ExperimentView.tsx | 14 +- .../experiments/ExperimentView/Overview.tsx | 26 +- .../experiments/ExperimentView/Results.tsx | 8 +- .../ExperimentView/SecondaryMetricsTable.tsx | 4 +- .../ExperimentView/SummaryTable.tsx | 41 +- .../experiments/ExperimentView/components.tsx | 102 ++--- .../scenes/experiments/experimentLogic.tsx | 367 ++++++------------ .../Nodes/NotebookNodeExperiment.tsx | 8 +- 12 files changed, 255 insertions(+), 333 deletions(-) diff --git a/cypress/e2e/experiments.cy.ts b/cypress/e2e/experiments.cy.ts index a635cf7841cad..34898f3146536 100644 --- a/cypress/e2e/experiments.cy.ts +++ b/cypress/e2e/experiments.cy.ts @@ -96,6 +96,7 @@ describe('Experiments', () => { cy.get('[data-attr="experiment-creation-date"]').contains('a few seconds ago').should('be.visible') cy.get('[data-attr="experiment-start-date"]').should('not.exist') + cy.wait(1000) cy.get('[data-attr="launch-experiment"]').first().click() cy.get('[data-attr="experiment-creation-date"]').should('not.exist') cy.get('[data-attr="experiment-start-date"]').contains('a few seconds ago').should('be.visible') @@ -114,6 +115,7 @@ describe('Experiments', () => { it('move start date', () => { createExperimentInNewUi() + cy.wait(1000) cy.get('[data-attr="launch-experiment"]').first().click() cy.get('[data-attr="move-experiment-start-date"]').first().click() diff --git a/frontend/src/scenes/experiments/ExperimentView/CumulativeExposuresChart.tsx b/frontend/src/scenes/experiments/ExperimentView/CumulativeExposuresChart.tsx index 7f4378a7ec5ef..6efdd992f5988 100644 --- a/frontend/src/scenes/experiments/ExperimentView/CumulativeExposuresChart.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/CumulativeExposuresChart.tsx @@ -10,11 +10,11 @@ import { BaseMathType, ChartDisplayType, InsightType, PropertyFilterType, Proper import { experimentLogic } from '../experimentLogic' export function CumulativeExposuresChart(): JSX.Element { - const { experiment, experimentResults, getMetricType } = useValues(experimentLogic) + const { experiment, metricResults, getMetricType } = useValues(experimentLogic) const metricIdx = 0 const metricType = getMetricType(metricIdx) - + const result = metricResults?.[metricIdx] const variants = experiment.parameters?.feature_flag_variants?.map((variant) => variant.key) || [] if (experiment.holdout) { variants.push(`holdout-${experiment.holdout.id}`) @@ -25,7 +25,7 @@ export function CumulativeExposuresChart(): JSX.Element { if (metricType === InsightType.TRENDS) { query = { kind: NodeKind.InsightVizNode, - source: (experimentResults as CachedExperimentTrendsQueryResponse)?.exposure_query || { + source: (result as CachedExperimentTrendsQueryResponse)?.exposure_query || { kind: NodeKind.TrendsQuery, series: [], interval: 'day', diff --git a/frontend/src/scenes/experiments/ExperimentView/DataCollection.tsx b/frontend/src/scenes/experiments/ExperimentView/DataCollection.tsx index 2463e1dd791a5..b22eb57b35d4b 100644 --- a/frontend/src/scenes/experiments/ExperimentView/DataCollection.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/DataCollection.tsx @@ -32,7 +32,7 @@ export function DataCollection(): JSX.Element { const experimentProgressPercent = metricType === InsightType.FUNNELS - ? (funnelResultsPersonsTotal / recommendedSampleSize) * 100 + ? (funnelResultsPersonsTotal(0) / recommendedSampleSize) * 100 : (actualRunningTime / recommendedRunningTime) * 100 const hasHighRunningTime = recommendedRunningTime > 62 @@ -109,7 +109,7 @@ export function DataCollection(): JSX.Element { Saw  - {humanFriendlyNumber(funnelResultsPersonsTotal)} of{' '} + {humanFriendlyNumber(funnelResultsPersonsTotal(0))} of{' '} {humanFriendlyNumber(recommendedSampleSize)}{' '} {' '} {formatUnitByQuantity(recommendedSampleSize, 'participant')} diff --git a/frontend/src/scenes/experiments/ExperimentView/DistributionTable.tsx b/frontend/src/scenes/experiments/ExperimentView/DistributionTable.tsx index b3c2962d95c55..caee718efb726 100644 --- a/frontend/src/scenes/experiments/ExperimentView/DistributionTable.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/DistributionTable.tsx @@ -137,9 +137,11 @@ export function DistributionModal({ experimentId }: { experimentId: Experiment[' export function DistributionTable(): JSX.Element { const { openDistributionModal } = useActions(experimentLogic) - const { experimentId, experiment, experimentResults } = useValues(experimentLogic) + const { experimentId, experiment, metricResults } = useValues(experimentLogic) const { reportExperimentReleaseConditionsViewed } = useActions(experimentLogic) + const result = metricResults?.[0] + const onSelectElement = (variant: string): void => { LemonDialog.open({ title: 'Select a domain', @@ -166,7 +168,7 @@ export function DistributionTable(): JSX.Element { key: 'key', title: 'Variant', render: function Key(_, item): JSX.Element { - if (!experimentResults || !experimentResults.insight) { + if (!result || !result.insight) { return {item.key} } return diff --git a/frontend/src/scenes/experiments/ExperimentView/ExperimentView.tsx b/frontend/src/scenes/experiments/ExperimentView/ExperimentView.tsx index 75aa23d2f6284..9a9ad237665f4 100644 --- a/frontend/src/scenes/experiments/ExperimentView/ExperimentView.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/ExperimentView.tsx @@ -25,9 +25,9 @@ import { Results } from './Results' import { SecondaryMetricsTable } from './SecondaryMetricsTable' const ResultsTab = (): JSX.Element => { - const { experiment, experimentResults, featureFlags } = useValues(experimentLogic) - - const hasResultsInsight = experimentResults && experimentResults.insight + const { experiment, metricResults, featureFlags } = useValues(experimentLogic) + const result = metricResults?.[0] + const hasResultsInsight = result && result.insight return (
@@ -69,12 +69,12 @@ const VariantsTab = (): JSX.Element => { } export function ExperimentView(): JSX.Element { - const { experimentLoading, experimentResultsLoading, experimentId, experimentResults, tabKey, featureFlags } = + const { experimentLoading, metricResultsLoading, experimentId, metricResults, tabKey, featureFlags } = useValues(experimentLogic) const { setTabKey } = useActions(experimentLogic) - - const hasResultsInsight = experimentResults && experimentResults.insight + const result = metricResults?.[0] + const hasResultsInsight = result && result.insight return ( <> @@ -85,7 +85,7 @@ export function ExperimentView(): JSX.Element { ) : ( <> - {experimentResultsLoading ? ( + {metricResultsLoading ? ( ) : ( <> diff --git a/frontend/src/scenes/experiments/ExperimentView/Overview.tsx b/frontend/src/scenes/experiments/ExperimentView/Overview.tsx index 2095309364143..c8c44c8ea5c04 100644 --- a/frontend/src/scenes/experiments/ExperimentView/Overview.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/Overview.tsx @@ -1,17 +1,29 @@ import { useValues } from 'kea' +import { CachedExperimentFunnelsQueryResponse, CachedExperimentTrendsQueryResponse } from '~/queries/schema' + import { experimentLogic } from '../experimentLogic' import { VariantTag } from './components' export function Overview(): JSX.Element { - const { experimentId, experimentResults, getIndexForVariant, getHighestProbabilityVariant, areResultsSignificant } = + const { experimentId, metricResults, getIndexForVariant, getHighestProbabilityVariant, areResultsSignificant } = useValues(experimentLogic) + const result = metricResults?.[0] + if (!result) { + return <> + } + function WinningVariantText(): JSX.Element { - const highestProbabilityVariant = getHighestProbabilityVariant(experimentResults) - const index = getIndexForVariant(experimentResults, highestProbabilityVariant || '') - if (highestProbabilityVariant && index !== null && experimentResults) { - const { probability } = experimentResults + const highestProbabilityVariant = getHighestProbabilityVariant( + result as CachedExperimentFunnelsQueryResponse | CachedExperimentTrendsQueryResponse + ) + const index = getIndexForVariant( + result as CachedExperimentFunnelsQueryResponse | CachedExperimentTrendsQueryResponse, + highestProbabilityVariant || '' + ) + if (highestProbabilityVariant && index !== null && result) { + const { probability } = result return (
@@ -32,7 +44,9 @@ export function Overview(): JSX.Element { return (
Your results are  - {`${areResultsSignificant ? 'significant' : 'not significant'}`}. + + {`${areResultsSignificant(0) ? 'significant' : 'not significant'}`}. +
) } diff --git a/frontend/src/scenes/experiments/ExperimentView/Results.tsx b/frontend/src/scenes/experiments/ExperimentView/Results.tsx index c4e7a4b05ed62..61574de1b3966 100644 --- a/frontend/src/scenes/experiments/ExperimentView/Results.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/Results.tsx @@ -5,13 +5,17 @@ import { ResultsHeader, ResultsQuery } from './components' import { SummaryTable } from './SummaryTable' export function Results(): JSX.Element { - const { experimentResults } = useValues(experimentLogic) + const { metricResults } = useValues(experimentLogic) + const result = metricResults?.[0] + if (!result) { + return <> + } return (
- +
) } diff --git a/frontend/src/scenes/experiments/ExperimentView/SecondaryMetricsTable.tsx b/frontend/src/scenes/experiments/ExperimentView/SecondaryMetricsTable.tsx index 8369038f00cbb..7f5bcbabfa3a1 100644 --- a/frontend/src/scenes/experiments/ExperimentView/SecondaryMetricsTable.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/SecondaryMetricsTable.tsx @@ -21,7 +21,7 @@ export function SecondaryMetricsTable({ experimentId }: { experimentId: Experime const [modalMetricIdx, setModalMetricIdx] = useState(null) const { - experimentResults, + metricResults, secondaryMetricResultsLoading, experiment, getSecondaryMetricType, @@ -65,7 +65,7 @@ export function SecondaryMetricsTable({ experimentId }: { experimentId: Experime { title:
Variant
, render: function Key(_, item: TabularSecondaryMetricResults): JSX.Element { - if (!experimentResults || !experimentResults.insight) { + if (!metricResults?.[0] || !metricResults?.[0].insight) { return {item.variant} } return ( diff --git a/frontend/src/scenes/experiments/ExperimentView/SummaryTable.tsx b/frontend/src/scenes/experiments/ExperimentView/SummaryTable.tsx index 6150d4e7b7826..adb1bc9d69616 100644 --- a/frontend/src/scenes/experiments/ExperimentView/SummaryTable.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/SummaryTable.tsx @@ -10,7 +10,6 @@ import { urls } from 'scenes/urls' import { FilterLogicalOperator, - FunnelExperimentVariant, InsightType, PropertyFilterType, PropertyOperator, @@ -27,7 +26,7 @@ export function SummaryTable(): JSX.Element { const { experimentId, experiment, - experimentResults, + metricResults, tabularExperimentResults, getMetricType, exposureCountDataForVariant, @@ -38,14 +37,14 @@ export function SummaryTable(): JSX.Element { credibleIntervalForVariant, } = useValues(experimentLogic) const metricType = getMetricType(0) - - if (!experimentResults) { + const result = metricResults?.[0] + if (!result) { return <> } - const winningVariant = getHighestProbabilityVariant(experimentResults) + const winningVariant = getHighestProbabilityVariant(result) - const columns: LemonTableColumns = [ + const columns: LemonTableColumns = [ { key: 'variants', title: 'Variant', @@ -64,14 +63,14 @@ export function SummaryTable(): JSX.Element { key: 'counts', title: (
- {experimentResults.insight?.[0] && 'action' in experimentResults.insight[0] && ( - + {result.insight?.[0] && 'action' in result.insight[0] && ( + )} {experimentMathAggregationForTrends() ? 'metric' : 'count'}
), render: function Key(_, variant): JSX.Element { - const count = countDataForVariant(experimentResults, variant.key) + const count = countDataForVariant(result, variant.key) if (!count) { return <>— } @@ -83,7 +82,7 @@ export function SummaryTable(): JSX.Element { key: 'exposure', title: 'Exposure', render: function Key(_, variant): JSX.Element { - const exposure = exposureCountDataForVariant(experimentResults, variant.key) + const exposure = exposureCountDataForVariant(result, variant.key) if (!exposure) { return <>— } @@ -120,7 +119,7 @@ export function SummaryTable(): JSX.Element { return Baseline } - const controlVariant = (experimentResults.variants as TrendExperimentVariant[]).find( + const controlVariant = (result.variants as TrendExperimentVariant[]).find( ({ key }) => key === 'control' ) as TrendExperimentVariant @@ -161,7 +160,7 @@ export function SummaryTable(): JSX.Element { return Baseline } - const credibleInterval = credibleIntervalForVariant(experimentResults || null, variant.key, metricType) + const credibleInterval = credibleIntervalForVariant(result || null, variant.key, metricType) if (!credibleInterval) { return <>— } @@ -181,7 +180,7 @@ export function SummaryTable(): JSX.Element { key: 'conversionRate', title: 'Conversion rate', render: function Key(_, item): JSX.Element { - const conversionRate = conversionRateForVariant(experimentResults, item.key) + const conversionRate = conversionRateForVariant(result, item.key) if (!conversionRate) { return <>— } @@ -204,8 +203,8 @@ export function SummaryTable(): JSX.Element { return Baseline } - const controlConversionRate = conversionRateForVariant(experimentResults, 'control') - const variantConversionRate = conversionRateForVariant(experimentResults, item.key) + const controlConversionRate = conversionRateForVariant(result, 'control') + const variantConversionRate = conversionRateForVariant(result, item.key) if (!controlConversionRate || !variantConversionRate) { return <>— @@ -235,7 +234,7 @@ export function SummaryTable(): JSX.Element { return Baseline } - const credibleInterval = credibleIntervalForVariant(experimentResults || null, item.key, metricType) + const credibleInterval = credibleIntervalForVariant(result || null, item.key, metricType) if (!credibleInterval) { return <>— } @@ -254,15 +253,13 @@ export function SummaryTable(): JSX.Element { key: 'winProbability', title: 'Win probability', sorter: (a, b) => { - const aPercentage = (experimentResults?.probability?.[a.key] || 0) * 100 - const bPercentage = (experimentResults?.probability?.[b.key] || 0) * 100 + const aPercentage = (result?.probability?.[a.key] || 0) * 100 + const bPercentage = (result?.probability?.[b.key] || 0) * 100 return aPercentage - bPercentage }, render: function Key(_, item): JSX.Element { const variantKey = item.key - const percentage = - experimentResults?.probability?.[variantKey] != undefined && - experimentResults.probability?.[variantKey] * 100 + const percentage = result?.probability?.[variantKey] != undefined && result.probability?.[variantKey] * 100 const isWinning = variantKey === winningVariant return ( @@ -351,7 +348,7 @@ export function SummaryTable(): JSX.Element { return (
- +
) } diff --git a/frontend/src/scenes/experiments/ExperimentView/components.tsx b/frontend/src/scenes/experiments/ExperimentView/components.tsx index 9e5bfb7c2c4b0..87885a3761b79 100644 --- a/frontend/src/scenes/experiments/ExperimentView/components.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/components.tsx @@ -62,7 +62,11 @@ export function VariantTag({ muted?: boolean fontSize?: number }): JSX.Element { - const { experiment, experimentResults, getIndexForVariant } = useValues(experimentLogic({ experimentId })) + const { experiment, getIndexForVariant, metricResults } = useValues(experimentLogic({ experimentId })) + + if (!metricResults) { + return <> + } if (experiment.holdout && variantKey === `holdout-${experiment.holdout_id}`) { return ( @@ -71,7 +75,7 @@ export function VariantTag({ className="w-2 h-2 rounded-full mr-0.5" // eslint-disable-next-line react/forbid-dom-props style={{ - backgroundColor: getExperimentInsightColour(getIndexForVariant(experimentResults, variantKey)), + backgroundColor: getExperimentInsightColour(getIndexForVariant(metricResults[0], variantKey)), }} /> {experiment.holdout.name} @@ -85,7 +89,7 @@ export function VariantTag({ className="w-2 h-2 rounded-full mr-0.5" // eslint-disable-next-line react/forbid-dom-props style={{ - backgroundColor: getExperimentInsightColour(getIndexForVariant(experimentResults, variantKey)), + backgroundColor: getExperimentInsightColour(getIndexForVariant(metricResults[0], variantKey)), }} /> + {result.label} @@ -205,8 +209,15 @@ export function ResultsQuery({ ) } -export function ExploreButton({ icon = }: { icon?: JSX.Element }): JSX.Element { - const { experimentResults, experiment, featureFlags } = useValues(experimentLogic) +export function ExploreButton({ + icon = , + metricIndex = 0, +}: { + icon?: JSX.Element + metricIndex?: number +}): JSX.Element { + const { metricResults, experiment, featureFlags } = useValues(experimentLogic) + const result = metricResults?.[metricIndex] // keep in sync with https://github.com/PostHog/posthog/blob/master/ee/clickhouse/queries/experiments/funnel_experiment_result.py#L71 // :TRICKY: In the case of no results, we still want users to explore the query, so they can debug further. @@ -223,7 +234,7 @@ export function ExploreButton({ icon = }: { icon?: JSX.Element let query: InsightVizNode if (featureFlags[FEATURE_FLAGS.EXPERIMENTS_HOGQL]) { - const newQueryResults = experimentResults as unknown as + const newQueryResults = result as unknown as | CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse @@ -237,7 +248,7 @@ export function ExploreButton({ icon = }: { icon?: JSX.Element source: source as InsightQueryNode, } } else { - const oldQueryResults = experimentResults as ExperimentResults['result'] + const oldQueryResults = result as unknown as ExperimentResults['result'] if (!oldQueryResults?.filters) { return <> @@ -272,7 +283,9 @@ export function ExploreButton({ icon = }: { icon?: JSX.Element } export function ResultsHeader(): JSX.Element { - const { experimentResults } = useValues(experimentLogic) + const { metricResults } = useValues(experimentLogic) + + const result = metricResults?.[0] return (
@@ -284,16 +297,17 @@ export function ResultsHeader(): JSX.Element {
-
{experimentResults && }
+
{result && }
) } -export function NoResultsEmptyState(): JSX.Element { +export function NoResultsEmptyState({ metricIndex = 0 }: { metricIndex?: number }): JSX.Element { type ErrorCode = 'no-events' | 'no-flag-info' | 'no-control-variant' | 'no-test-variant' - const { experimentResultsLoading, experimentResultCalculationError } = useValues(experimentLogic) + const { metricResultsLoading, primaryMetricsResultErrors } = useValues(experimentLogic) + const metricError = primaryMetricsResultErrors?.[metricIndex] function ChecklistItem({ errorCode, value }: { errorCode: ErrorCode; value: boolean }): JSX.Element { const failureText = { @@ -327,28 +341,25 @@ export function NoResultsEmptyState(): JSX.Element { ) } - if (experimentResultsLoading) { + if (metricResultsLoading) { return <> } // Validation errors return 400 and are rendered as a checklist - if (experimentResultCalculationError?.statusCode === 400) { - let parsedDetail: Record - try { - parsedDetail = JSON.parse(experimentResultCalculationError.detail) - } catch (error) { + if (metricError?.statusCode === 400) { + if (!metricError.hasDiagnostics) { return (
Experiment results could not be calculated
-
{experimentResultCalculationError.detail}
+
{metricError.detail}
) } const checklistItems = [] - for (const [errorCode, value] of Object.entries(parsedDetail)) { + for (const [errorCode, value] of Object.entries(metricError.detail as Record)) { checklistItems.push() } @@ -377,14 +388,14 @@ export function NoResultsEmptyState(): JSX.Element { ) } - if (experimentResultCalculationError?.statusCode === 504) { + if (metricError?.statusCode === 504) { return (

Experiment results timed out

- {!!experimentResultCalculationError && ( + {!!metricError && (
This may occur when the experiment has a large amount of data or is particularly complex. We are actively working on fixing this. In the meantime, please try refreshing @@ -404,11 +415,7 @@ export function NoResultsEmptyState(): JSX.Element {

Experiment results could not be calculated

- {!!experimentResultCalculationError && ( -
- {experimentResultCalculationError.detail} -
- )} + {!!metricError &&
{metricError.detail}
}
@@ -464,7 +471,7 @@ export function PageHeaderCustom(): JSX.Element { launchExperiment, endExperiment, archiveExperiment, - loadExperimentResults, + loadMetricResults, loadSecondaryMetricResults, createExposureCohort, openShipVariantModal, @@ -505,7 +512,7 @@ export function PageHeaderCustom(): JSX.Element { {exposureCohortId ? 'View' : 'Create'} exposure cohort loadExperimentResults(true)} + onClick={() => loadMetricResults(true)} fullWidth data-attr="refresh-experiment" > @@ -588,7 +595,7 @@ export function PageHeaderCustom(): JSX.Element {
)} {featureFlags[FEATURE_FLAGS.EXPERIMENT_MAKE_DECISION] && - areResultsSignificant && + areResultsSignificant(0) && !isSingleVariantShipped && ( <> @@ -615,7 +622,7 @@ export function ShipVariantModal({ experimentId }: { experimentId: Experiment['i const { aggregationLabel } = useValues(groupsModel) const [selectedVariantKey, setSelectedVariantKey] = useState() - useEffect(() => setSelectedVariantKey(sortedWinProbabilities[0]?.key), [sortedWinProbabilities]) + useEffect(() => setSelectedVariantKey(sortedWinProbabilities(0)[0]?.key), [sortedWinProbabilities(0)]) const aggregationTargetName = experiment.filters.aggregation_group_type_index != null @@ -656,12 +663,12 @@ export function ShipVariantModal({ experimentId }: { experimentId: Experiment['i data-attr="metrics-selector" value={selectedVariantKey} onChange={(variantKey) => setSelectedVariantKey(variantKey)} - options={sortedWinProbabilities.map(({ key }) => ({ + options={sortedWinProbabilities(0).map(({ key }) => ({ value: key, label: (
- {key === sortedWinProbabilities[0]?.key && ( + {key === sortedWinProbabilities(0)[0]?.key && ( Winning @@ -693,9 +700,9 @@ export function ActionBanner(): JSX.Element { const { experiment, getMetricType, - experimentResults, + metricResults, experimentLoading, - experimentResultsLoading, + metricResultsLoading, isExperimentRunning, areResultsSignificant, isExperimentStopped, @@ -706,6 +713,7 @@ export function ActionBanner(): JSX.Element { featureFlags, } = useValues(experimentLogic) + const result = metricResults?.[0] const { archiveExperiment } = useActions(experimentLogic) const { aggregationLabel } = useValues(groupsModel) @@ -720,7 +728,7 @@ export function ActionBanner(): JSX.Element { const recommendedRunningTime = experiment?.parameters?.recommended_running_time || 1 const recommendedSampleSize = experiment?.parameters?.recommended_sample_size || 100 - if (!experiment || experimentLoading || experimentResultsLoading) { + if (!experiment || experimentLoading || metricResultsLoading) { return <> } @@ -766,12 +774,12 @@ export function ActionBanner(): JSX.Element { } // Running, results present, not significant - if (isExperimentRunning && experimentResults && !isExperimentStopped && !areResultsSignificant) { + if (isExperimentRunning && result && !isExperimentStopped && !areResultsSignificant(0)) { // Results insignificant, but a large enough sample/running time has been achieved // Further collection unlikely to change the result -> recommmend cutting the losses if ( metricType === InsightType.FUNNELS && - funnelResultsPersonsTotal > Math.max(recommendedSampleSize, 500) && + funnelResultsPersonsTotal(0) > Math.max(recommendedSampleSize, 500) && dayjs().diff(experiment.start_date, 'day') > 2 // at least 2 days running ) { return ( @@ -800,9 +808,9 @@ export function ActionBanner(): JSX.Element { } // Running, results significant - if (isExperimentRunning && !isExperimentStopped && areResultsSignificant && experimentResults) { - const { probability } = experimentResults - const winningVariant = getHighestProbabilityVariant(experimentResults) + if (isExperimentRunning && !isExperimentStopped && areResultsSignificant(0) && result) { + const { probability } = result + const winningVariant = getHighestProbabilityVariant(result) if (!winningVariant) { return <> } @@ -812,7 +820,7 @@ export function ActionBanner(): JSX.Element { // Win probability only slightly over 0.9 and the recommended sample/time just met -> proceed with caution if ( metricType === InsightType.FUNNELS && - funnelResultsPersonsTotal < recommendedSampleSize + 50 && + funnelResultsPersonsTotal(0) < recommendedSampleSize + 50 && winProbability < 0.93 ) { return ( @@ -848,7 +856,7 @@ export function ActionBanner(): JSX.Element { } // Stopped, results significant - if (isExperimentStopped && areResultsSignificant) { + if (isExperimentStopped && areResultsSignificant(0)) { return ( You have stopped this experiment, and it is no longer collecting data. With significant results in hand, @@ -866,7 +874,7 @@ export function ActionBanner(): JSX.Element { } // Stopped, results not significant - if (isExperimentStopped && experimentResults && !areResultsSignificant) { + if (isExperimentStopped && result && !areResultsSignificant(0)) { return ( You have stopped this experiment, and it is no longer collecting data. Because your results are not diff --git a/frontend/src/scenes/experiments/experimentLogic.tsx b/frontend/src/scenes/experiments/experimentLogic.tsx index 6094de7721879..42f6b2cd37652 100644 --- a/frontend/src/scenes/experiments/experimentLogic.tsx +++ b/frontend/src/scenes/experiments/experimentLogic.tsx @@ -1,4 +1,3 @@ -import { IconInfo } from '@posthog/icons' import { actions, connect, kea, key, listeners, path, props, reducers, selectors } from 'kea' import { forms } from 'kea-forms' import { loaders } from 'kea-loaders' @@ -8,11 +7,9 @@ import { EXPERIMENT_DEFAULT_DURATION, FunnelLayout } from 'lib/constants' import { FEATURE_FLAGS } from 'lib/constants' import { dayjs } from 'lib/dayjs' import { lemonToast } from 'lib/lemon-ui/LemonToast/LemonToast' -import { Tooltip } from 'lib/lemon-ui/Tooltip' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { hasFormErrors, toParams } from 'lib/utils' import { eventUsageLogic } from 'lib/utils/eventUsageLogic' -import { ReactElement } from 'react' import { validateFeatureFlagKey } from 'scenes/feature-flags/featureFlagLogic' import { funnelDataLogic } from 'scenes/funnels/funnelDataLogic' import { insightDataLogic } from 'scenes/insights/insightDataLogic' @@ -31,6 +28,7 @@ import { CachedExperimentFunnelsQueryResponse, CachedExperimentTrendsQueryResponse, ExperimentFunnelsQuery, + ExperimentSignificanceCode, ExperimentTrendsQuery, NodeKind, } from '~/queries/schema' @@ -55,7 +53,6 @@ import { ProductKey, PropertyMathType, SecondaryMetricResults, - SignificanceCode, TrendExperimentVariant, TrendResult, TrendsFilterType, @@ -177,7 +174,6 @@ export const experimentLogic = kea([ setExperimentType: (type?: string) => ({ type }), removeExperimentGroup: (idx: number) => ({ idx }), setEditExperiment: (editing: boolean) => ({ editing }), - setExperimentResultCalculationError: (error: ExperimentResultCalculationError) => ({ error }), setFlagImplementationWarning: (warning: boolean) => ({ warning }), setExposureAndSampleSize: (exposure: number, sampleSize: number) => ({ exposure, sampleSize }), updateExperimentGoal: (filters: Partial) => ({ filters }), @@ -420,12 +416,6 @@ export const experimentLogic = kea([ setEditExperiment: (_, { editing }) => editing, }, ], - experimentResultCalculationError: [ - null as ExperimentResultCalculationError | null, - { - setExperimentResultCalculationError: (_, { error }) => error, - }, - ], flagImplementationWarning: [ false as boolean, { @@ -597,10 +587,7 @@ export const experimentLogic = kea([ experiment && actions.reportExperimentViewed(experiment) if (experiment?.start_date) { - actions.loadExperimentResults() - if (values.featureFlags[FEATURE_FLAGS.EXPERIMENTS_MULTIPLE_METRICS]) { - actions.loadMetricResults() - } + actions.loadMetricResults() actions.loadSecondaryMetricResults() } }, @@ -618,7 +605,7 @@ export const experimentLogic = kea([ actions.updateExperiment({ end_date: endDate.toISOString() }) const duration = endDate.diff(values.experiment?.start_date, 'second') values.experiment && - actions.reportExperimentCompleted(values.experiment, endDate, duration, values.areResultsSignificant) + actions.reportExperimentCompleted(values.experiment, endDate, duration, values.areResultsSignificant(0)) }, archiveExperiment: async () => { actions.updateExperiment({ archived: true }) @@ -683,13 +670,11 @@ export const experimentLogic = kea([ resetRunningExperiment: async () => { actions.updateExperiment({ start_date: null, end_date: null, archived: false }) values.experiment && actions.reportExperimentReset(values.experiment) - - actions.loadExperimentResultsSuccess(null) actions.loadSecondaryMetricResultsSuccess([]) }, updateExperimentSuccess: async ({ experiment }) => { actions.updateExperiments(experiment) - actions.loadExperimentResults() + actions.loadMetricResults() actions.loadSecondaryMetricResults() }, setExperiment: async ({ experiment }) => { @@ -838,66 +823,6 @@ export const experimentLogic = kea([ return response }, }, - experimentResults: [ - null as - | ExperimentResults['result'] - | CachedExperimentTrendsQueryResponse - | CachedExperimentFunnelsQueryResponse - | null, - { - loadExperimentResults: async ( - refresh?: boolean - ): Promise< - | ExperimentResults['result'] - | CachedExperimentTrendsQueryResponse - | CachedExperimentFunnelsQueryResponse - | null - > => { - try { - // :FLAG: CLEAN UP AFTER MIGRATION - if (values.featureFlags[FEATURE_FLAGS.EXPERIMENTS_HOGQL]) { - // Queries are shareable, so we need to set the experiment_id for the backend to correctly associate the query with the experiment - const queryWithExperimentId = { - ...values.experiment.metrics[0], - experiment_id: values.experimentId, - } - if (values.featureFlags[FEATURE_FLAGS.EXPERIMENT_STATS_V2]) { - queryWithExperimentId.stats_version = 2 - } - - const response = await performQuery(queryWithExperimentId, undefined, refresh) - - return { - ...response, - fakeInsightId: Math.random().toString(36).substring(2, 15), - } as unknown as CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse - } - - const refreshParam = refresh ? '?refresh=true' : '' - const response: ExperimentResults = await api.get( - `api/projects/${values.currentProjectId}/experiments/${values.experimentId}/results${refreshParam}` - ) - return { - ...response.result, - fakeInsightId: Math.random().toString(36).substring(2, 15), - last_refresh: response.last_refresh, - } - } catch (error: any) { - let errorDetail = error.detail - // :HANDLE FLAG: CLEAN UP AFTER MIGRATION - if (values.featureFlags[FEATURE_FLAGS.EXPERIMENTS_HOGQL]) { - const errorDetailMatch = error.detail.match(/\{.*\}/) - errorDetail = errorDetailMatch ? errorDetailMatch[0] : error.detail - } - actions.setExperimentResultCalculationError({ detail: errorDetail, statusCode: error.status }) - if (error.status === 504) { - actions.reportExperimentResultsLoadingTimeout(values.experimentId) - } - return null - } - }, - }, - ], metricResults: [ null as (CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse | null)[] | null, { @@ -1184,83 +1109,40 @@ export const experimentLogic = kea([ }, ], areResultsSignificant: [ - (s) => [s.experimentResults], - (experimentResults): boolean => { - return experimentResults?.significant || false - }, - ], - // TODO: remove with the old UI - significanceBannerDetails: [ - (s) => [s.experimentResults], - (experimentResults): string | ReactElement => { - if (experimentResults?.significance_code === SignificanceCode.HighLoss) { - return ( - <> - This is because the expected loss in conversion is greater than 1% - Current value is {((experimentResults?.expected_loss || 0) * 100)?.toFixed(2)}% - } - > - - - . - - ) - } - - if (experimentResults?.significance_code === SignificanceCode.HighPValue) { - return ( - <> - This is because the p value is greater than 0.05 - Current value is {experimentResults?.p_value?.toFixed(3) || 1}.} - > - - - . - - ) - } - - if (experimentResults?.significance_code === SignificanceCode.LowWinProbability) { - return 'This is because the win probability of all test variants combined is less than 90%.' - } - - if (experimentResults?.significance_code === SignificanceCode.NotEnoughExposure) { - return 'This is because we need at least 100 people per variant to declare significance.' - } - - return '' - }, + (s) => [s.metricResults], + (metricResults: (CachedExperimentFunnelsQueryResponse | CachedExperimentTrendsQueryResponse | null)[]) => + (metricIndex: number = 0): boolean => { + return metricResults?.[metricIndex]?.significant || false + }, ], significanceDetails: [ - (s) => [s.experimentResults], - (experimentResults): string => { - if (experimentResults?.significance_code === SignificanceCode.HighLoss) { - return `This is because the expected loss in conversion is greater than 1% (current value is ${( - (experimentResults?.expected_loss || 0) * 100 - )?.toFixed(2)}%).` - } + (s) => [s.metricResults], + (metricResults: (CachedExperimentFunnelsQueryResponse | CachedExperimentTrendsQueryResponse | null)[]) => + (metricIndex: number = 0): string => { + const results = metricResults?.[metricIndex] + + if (results?.significance_code === ExperimentSignificanceCode.HighLoss) { + return `This is because the expected loss in conversion is greater than 1% (current value is ${( + (results as CachedExperimentFunnelsQueryResponse)?.expected_loss || 0 + )?.toFixed(2)}%).` + } - if (experimentResults?.significance_code === SignificanceCode.HighPValue) { - return `This is because the p value is greater than 0.05 (current value is ${ - experimentResults?.p_value?.toFixed(3) || 1 - }).` - } + if (results?.significance_code === ExperimentSignificanceCode.HighPValue) { + return `This is because the p value is greater than 0.05 (current value is ${ + (results as CachedExperimentTrendsQueryResponse)?.p_value?.toFixed(3) || 1 + }).` + } - if (experimentResults?.significance_code === SignificanceCode.LowWinProbability) { - return 'This is because the win probability of all test variants combined is less than 90%.' - } + if (results?.significance_code === ExperimentSignificanceCode.LowWinProbability) { + return 'This is because the win probability of all test variants combined is less than 90%.' + } - if (experimentResults?.significance_code === SignificanceCode.NotEnoughExposure) { - return 'This is because we need at least 100 people per variant to declare significance.' - } + if (results?.significance_code === ExperimentSignificanceCode.NotEnoughExposure) { + return 'This is because we need at least 100 people per variant to declare significance.' + } - return '' - }, + return '' + }, ], recommendedSampleSize: [ (s) => [s.conversionMetrics, s.minimumSampleSizePerVariant, s.variants], @@ -1350,17 +1232,17 @@ export const experimentLogic = kea([ () => [], () => ( - experimentResults: + metricResult: | Partial | CachedExperimentFunnelsQueryResponse | CachedExperimentTrendsQueryResponse | null, variantKey: string ): number | null => { - if (!experimentResults || !experimentResults.insight) { + if (!metricResult || !metricResult.insight) { return null } - const variantResults = (experimentResults.insight as FunnelStep[][]).find( + const variantResults = (metricResult.insight as FunnelStep[][]).find( (variantFunnel: FunnelStep[]) => { const breakdownValue = variantFunnel[0]?.breakdown_value return Array.isArray(breakdownValue) && breakdownValue[0] === variantKey @@ -1377,7 +1259,7 @@ export const experimentLogic = kea([ () => [], () => ( - experimentResults: + metricResult: | Partial | CachedSecondaryMetricExperimentFunnelsQueryResponse | CachedSecondaryMetricExperimentTrendsQueryResponse @@ -1385,13 +1267,13 @@ export const experimentLogic = kea([ variantKey: string, metricType: InsightType ): [number, number] | null => { - const credibleInterval = experimentResults?.credible_intervals?.[variantKey] + const credibleInterval = metricResult?.credible_intervals?.[variantKey] if (!credibleInterval) { return null } if (metricType === InsightType.FUNNELS) { - const controlVariant = (experimentResults.variants as FunnelExperimentVariant[]).find( + const controlVariant = (metricResult.variants as FunnelExperimentVariant[]).find( ({ key }) => key === 'control' ) as FunnelExperimentVariant const controlConversionRate = @@ -1408,7 +1290,7 @@ export const experimentLogic = kea([ return [lowerBound, upperBound] } - const controlVariant = (experimentResults.variants as TrendExperimentVariant[]).find( + const controlVariant = (metricResult.variants as TrendExperimentVariant[]).find( ({ key }) => key === 'control' ) as TrendExperimentVariant @@ -1425,7 +1307,7 @@ export const experimentLogic = kea([ (s) => [s.getMetricType], (getMetricType) => ( - experimentResults: + metricResult: | Partial | CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse @@ -1434,14 +1316,14 @@ export const experimentLogic = kea([ ): number | null => { // Ensures we get the right index from results, so the UI can // display the right colour for the variant - if (!experimentResults || !experimentResults.insight) { + if (!metricResult || !metricResult.insight) { return null } let index = -1 if (getMetricType(0) === InsightType.FUNNELS) { // Funnel Insight is displayed in order of decreasing count - index = (Array.isArray(experimentResults.insight) ? [...experimentResults.insight] : []) + index = (Array.isArray(metricResult.insight) ? [...metricResult.insight] : []) .sort((a, b) => { const aCount = (a && Array.isArray(a) && a[0]?.count) || 0 const bCount = (b && Array.isArray(b) && b[0]?.count) || 0 @@ -1455,7 +1337,7 @@ export const experimentLogic = kea([ return Array.isArray(breakdownValue) && breakdownValue[0] === variant }) } else { - index = (experimentResults.insight as TrendResult[]).findIndex( + index = (metricResult.insight as TrendResult[]).findIndex( (variantTrend: TrendResult) => variantTrend.breakdown_value === variant ) } @@ -1471,7 +1353,7 @@ export const experimentLogic = kea([ (s) => [s.experimentMathAggregationForTrends], (experimentMathAggregationForTrends) => ( - experimentResults: + metricResult: | Partial | CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse @@ -1480,10 +1362,10 @@ export const experimentLogic = kea([ type: 'primary' | 'secondary' = 'primary' ): number | null => { const usingMathAggregationType = type === 'primary' ? experimentMathAggregationForTrends() : false - if (!experimentResults || !experimentResults.insight) { + if (!metricResult || !metricResult.insight) { return null } - const variantResults = (experimentResults.insight as TrendResult[]).find( + const variantResults = (metricResult.insight as TrendResult[]).find( (variantTrend: TrendResult) => variantTrend.breakdown_value === variant ) if (!variantResults) { @@ -1521,17 +1403,17 @@ export const experimentLogic = kea([ () => [], () => ( - experimentResults: + metricResult: | Partial | CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse | null, variant: string ): number | null => { - if (!experimentResults || !experimentResults.variants) { + if (!metricResult || !metricResult.variants) { return null } - const variantResults = (experimentResults.variants as TrendExperimentVariant[]).find( + const variantResults = (metricResult.variants as TrendExperimentVariant[]).find( (variantTrend: TrendExperimentVariant) => variantTrend.key === variant ) if (!variantResults || !variantResults.absolute_exposure) { @@ -1561,58 +1443,50 @@ export const experimentLogic = kea([ } }, ], - sortedExperimentResultVariants: [ - (s) => [s.experimentResults, s.experiment], - (experimentResults, experiment): string[] => { - if (experimentResults) { - const sortedResults = Object.keys(experimentResults.probability).sort( - (a, b) => experimentResults.probability[b] - experimentResults.probability[a] - ) - - experiment?.parameters?.feature_flag_variants?.forEach((variant) => { - if (!sortedResults.includes(variant.key)) { - sortedResults.push(variant.key) - } - }) - return sortedResults - } - return [] - }, - ], tabularExperimentResults: [ - (s) => [s.experiment, s.experimentResults, s.getMetricType], - (experiment, experimentResults, getMetricType): any => { - const tabularResults = [] - const metricType = getMetricType(0) - - if (experimentResults) { - for (const variantObj of experimentResults.variants) { - if (metricType === InsightType.FUNNELS) { - const { key, success_count, failure_count } = variantObj as FunnelExperimentVariant - tabularResults.push({ key, success_count, failure_count }) - } else if (metricType === InsightType.TRENDS) { - const { key, count, exposure, absolute_exposure } = variantObj as TrendExperimentVariant - tabularResults.push({ key, count, exposure, absolute_exposure }) + (s) => [s.experiment, s.metricResults, s.getMetricType], + ( + experiment, + metricResults: ( + | CachedExperimentFunnelsQueryResponse + | CachedExperimentTrendsQueryResponse + | null + )[], + getMetricType + ) => + (metricIndex: number = 0): any[] => { + const tabularResults = [] + const metricType = getMetricType(metricIndex) + const result = metricResults?.[metricIndex] + + if (result) { + for (const variantObj of result.variants) { + if (metricType === InsightType.FUNNELS) { + const { key, success_count, failure_count } = variantObj as FunnelExperimentVariant + tabularResults.push({ key, success_count, failure_count }) + } else if (metricType === InsightType.TRENDS) { + const { key, count, exposure, absolute_exposure } = variantObj as TrendExperimentVariant + tabularResults.push({ key, count, exposure, absolute_exposure }) + } } } - } - if (experiment.feature_flag?.filters.multivariate?.variants) { - for (const { key } of experiment.feature_flag.filters.multivariate.variants) { - if (tabularResults.find((variantObj) => variantObj.key === key)) { - continue - } + if (experiment.feature_flag?.filters.multivariate?.variants) { + for (const { key } of experiment.feature_flag.filters.multivariate.variants) { + if (tabularResults.find((variantObj) => variantObj.key === key)) { + continue + } - if (metricType === InsightType.FUNNELS) { - tabularResults.push({ key, success_count: null, failure_count: null }) - } else if (metricType === InsightType.TRENDS) { - tabularResults.push({ key, count: null, exposure: null, absolute_exposure: null }) + if (metricType === InsightType.FUNNELS) { + tabularResults.push({ key, success_count: null, failure_count: null }) + } else if (metricType === InsightType.TRENDS) { + tabularResults.push({ key, count: null, exposure: null, absolute_exposure: null }) + } } } - } - return tabularResults - }, + return tabularResults + }, ], tabularSecondaryMetricResults: [ (s) => [s.experiment, s.secondaryMetricResults, s.conversionRateForVariant, s.countDataForVariant], @@ -1652,39 +1526,56 @@ export const experimentLogic = kea([ }, ], sortedWinProbabilities: [ - (s) => [s.experimentResults, s.conversionRateForVariant], + (s) => [s.metricResults, s.conversionRateForVariant], ( - experimentResults, - conversionRateForVariant - ): { key: string; winProbability: number; conversionRate: number | null }[] => { - if (!experimentResults) { - return [] - } + metricResults: ( + | CachedExperimentFunnelsQueryResponse + | CachedExperimentTrendsQueryResponse + | null + )[], + conversionRateForVariant + ) => + (metricIndex: number = 0) => { + const result = metricResults?.[metricIndex] + + if (!result || !result.probability) { + return [] + } - return Object.keys(experimentResults.probability) - .map((key) => ({ - key, - winProbability: experimentResults.probability[key], - conversionRate: conversionRateForVariant(experimentResults, key), - })) - .sort((a, b) => b.winProbability - a.winProbability) - }, + return Object.keys(result.probability) + .map((key) => ({ + key, + winProbability: result.probability[key], + conversionRate: conversionRateForVariant(result, key), + })) + .sort((a, b) => b.winProbability - a.winProbability) + }, ], funnelResultsPersonsTotal: [ - (s) => [s.experimentResults, s.getMetricType], - (experimentResults, getMetricType): number => { - if (getMetricType(0) !== InsightType.FUNNELS || !experimentResults?.insight) { - return 0 - } + (s) => [s.metricResults, s.getMetricType], + ( + metricResults: ( + | CachedExperimentFunnelsQueryResponse + | CachedExperimentTrendsQueryResponse + | null + )[], + getMetricType + ) => + (metricIndex: number = 0): number => { + const result = metricResults?.[metricIndex] - let sum = 0 - experimentResults.insight.forEach((variantResult) => { - if (variantResult[0]?.count) { - sum += variantResult[0].count + if (getMetricType(metricIndex) !== InsightType.FUNNELS || !result?.insight) { + return 0 } - }) - return sum - }, + + let sum = 0 + result.insight.forEach((variantResult) => { + if (variantResult[0]?.count) { + sum += variantResult[0].count + } + }) + return sum + }, ], actualRunningTime: [ (s) => [s.experiment], diff --git a/frontend/src/scenes/notebooks/Nodes/NotebookNodeExperiment.tsx b/frontend/src/scenes/notebooks/Nodes/NotebookNodeExperiment.tsx index e7bc3a324202c..afde3f1836415 100644 --- a/frontend/src/scenes/notebooks/Nodes/NotebookNodeExperiment.tsx +++ b/frontend/src/scenes/notebooks/Nodes/NotebookNodeExperiment.tsx @@ -18,7 +18,7 @@ import { INTEGER_REGEX_MATCH_GROUPS } from './utils' const Component = ({ attributes }: NotebookNodeProps): JSX.Element => { const { id } = attributes - const { experiment, experimentLoading, experimentMissing, isExperimentRunning, experimentResults } = useValues( + const { experiment, experimentLoading, experimentMissing, isExperimentRunning, metricResults } = useValues( experimentLogic({ experimentId: id }) ) const { loadExperiment } = useActions(experimentLogic({ experimentId: id })) @@ -41,6 +41,10 @@ const Component = ({ attributes }: NotebookNodeProps } + if (!metricResults) { + return <> + } + return (
@@ -78,7 +82,7 @@ const Component = ({ attributes }: NotebookNodeProps
- +
)}