Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(experiments): define custom Trends exposure in the modal #27097

Merged
merged 4 commits into from
Dec 23, 2024
Merged

feat(experiments): define custom Trends exposure in the modal #27097

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62c98ea
init
jurajmajerik Dec 20, 2024
ae939c0
Merge branch 'master' into experiment-metrics-trends-exposure
danielbachhuber Dec 20, 2024
6e0c536
Merge branch 'master' of https://github.com/PostHog/posthog into expe…
jurajmajerik Dec 23, 2024
3f7a8ab
update
jurajmajerik Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
311 changes: 245 additions & 66 deletions frontend/src/scenes/experiments/Metrics/PrimaryGoalTrends.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
import { LemonInput, LemonLabel } from '@posthog/lemon-ui'
import { IconCheckCircle } from '@posthog/icons'
import { LemonInput, LemonLabel, LemonTabs, LemonTag } from '@posthog/lemon-ui'
import { useActions, useValues } from 'kea'
import { TestAccountFilterSwitch } from 'lib/components/TestAccountFiltersSwitch'
import { EXPERIMENT_DEFAULT_DURATION } from 'lib/constants'
import { dayjs } from 'lib/dayjs'
import { LemonBanner } from 'lib/lemon-ui/LemonBanner'
import { useState } from 'react'
import { ActionFilter } from 'scenes/insights/filters/ActionFilter/ActionFilter'
import { MathAvailability } from 'scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow'
import { teamLogic } from 'scenes/teamLogic'

import { actionsAndEventsToSeries } from '~/queries/nodes/InsightQuery/utils/filtersToQueryNode'
import { queryNodeToFilter } from '~/queries/nodes/InsightQuery/utils/queryNodeToFilter'
import { Query } from '~/queries/Query/Query'
import { ExperimentTrendsQuery, NodeKind } from '~/queries/schema'
import { FilterType } from '~/types'
import { ExperimentTrendsQuery, InsightQueryNode, NodeKind } from '~/queries/schema'
import { ChartDisplayType, FilterType } from '~/types'

import { experimentLogic } from '../experimentLogic'
import { commonActionFilterProps } from './Selectors'

export function PrimaryGoalTrends(): JSX.Element {
const { experiment, isExperimentRunning, editingPrimaryMetricIndex } = useValues(experimentLogic)
const { setTrendsMetric } = useActions(experimentLogic)
const { setTrendsMetric, setTrendsExposureMetric, setExperiment } = useActions(experimentLogic)
const { currentTeam } = useValues(teamLogic)
const hasFilters = (currentTeam?.test_account_filters || []).length > 0
const [activeTab, setActiveTab] = useState('main')

if (!editingPrimaryMetricIndex && editingPrimaryMetricIndex !== 0) {
return <></>
Expand All @@ -31,70 +35,245 @@ export function PrimaryGoalTrends(): JSX.Element {

return (
<>
<div className="mb-4">
<LemonLabel>Name (optional)</LemonLabel>
<LemonInput
value={currentMetric.name}
onChange={(newName) => {
setTrendsMetric({
metricIdx,
name: newName,
})
}}
/>
</div>
<ActionFilter
bordered
filters={queryNodeToFilter(currentMetric.count_query)}
setFilters={({ actions, events, data_warehouse }: Partial<FilterType>): void => {
const series = actionsAndEventsToSeries(
{ actions, events, data_warehouse } as any,
true,
MathAvailability.All
)
<LemonTabs
activeKey={activeTab}
onChange={(newKey) => setActiveTab(newKey)}
tabs={[
{
key: 'main',
label: 'Main metric',
content: (
<>
<div className="mb-4">
<LemonLabel>Name (optional)</LemonLabel>
<LemonInput
value={currentMetric.name}
onChange={(newName) => {
setTrendsMetric({
metricIdx,
name: newName,
})
}}
/>
</div>
<ActionFilter
bordered
filters={queryNodeToFilter(currentMetric.count_query)}
setFilters={({ actions, events, data_warehouse }: Partial<FilterType>): void => {
const series = actionsAndEventsToSeries(
{ actions, events, data_warehouse } as any,
true,
MathAvailability.All
)

setTrendsMetric({
metricIdx,
series,
})
}}
typeKey="experiment-metric"
buttonCopy="Add graph series"
showSeriesIndicator={true}
entitiesLimit={1}
showNumericalPropsOnly={true}
{...commonActionFilterProps}
setTrendsMetric({
metricIdx,
series,
})
}}
typeKey="experiment-metric"
buttonCopy="Add graph series"
showSeriesIndicator={true}
entitiesLimit={1}
showNumericalPropsOnly={true}
{...commonActionFilterProps}
/>
<div className="mt-4 space-y-4">
<TestAccountFilterSwitch
checked={hasFilters ? !!currentMetric.count_query?.filterTestAccounts : false}
onChange={(checked: boolean) => {
setTrendsMetric({
metricIdx,
filterTestAccounts: checked,
})
}}
fullWidth
/>
</div>
{isExperimentRunning && (
<LemonBanner type="info" className="mt-3 mb-3">
Preview insights are generated based on {EXPERIMENT_DEFAULT_DURATION} days of
data. This can cause a mismatch between the preview and the actual results.
</LemonBanner>
)}
<div className="mt-4">
<Query
query={{
kind: NodeKind.InsightVizNode,
source: currentMetric.count_query,
showTable: false,
showLastComputation: true,
showLastComputationRefresh: false,
}}
readOnly
/>
</div>
</>
),
},
{
key: 'exposure',
label: 'Exposure',
content: (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I didn't realize we accidentally dropped the exposure metric UI for Trends. I'm surprised we haven't gotten any tickets about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm we didn't? https://us.posthog.com/project/2/experiments/73149

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jurajmajerik You're right. I was looking at a data warehouse trends experiment where we did remove the UI element.

<>
<div className="flex gap-4 mb-4">
<div
className={`flex-1 cursor-pointer p-4 rounded border ${
!currentMetric.exposure_query
? 'border-primary bg-primary-highlight'
: 'border-border'
}`}
onClick={() => {
setExperiment({
...experiment,
metrics: experiment.metrics.map((metric, idx) =>
idx === metricIdx
? { ...metric, exposure_query: undefined }
: metric
),
})
}}
>
<div className="font-semibold flex justify-between items-center">
<span>Default</span>
{!currentMetric.exposure_query && (
<IconCheckCircle fontSize={18} color="var(--primary)" />
)}
</div>
<div className="text-muted text-sm leading-relaxed">
Uses the number of unique users who trigger the{' '}
<LemonTag>$feature_flag_called</LemonTag> event as your exposure count. This
is the recommended setting for most experiments, as it accurately tracks
variant exposure.
</div>
</div>
<div
className={`flex-1 cursor-pointer p-4 rounded border ${
currentMetric.exposure_query
? 'border-primary bg-primary-highlight'
: 'border-border'
}`}
onClick={() => {
setExperiment({
...experiment,
metrics: experiment.metrics.map((metric, idx) =>
idx === metricIdx
? {
...metric,
exposure_query: {
kind: NodeKind.TrendsQuery,
series: [
{
kind: NodeKind.EventsNode,
name: '$pageview',
event: '$pageview',
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for using $pageview here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to set some default event here, and $pageview is our default everywhere. But maybe $feature_flag_called makes more sense.

],
interval: 'day',
dateRange: {
date_from: dayjs()
.subtract(EXPERIMENT_DEFAULT_DURATION, 'day')
.format('YYYY-MM-DDTHH:mm'),
date_to: dayjs()
.endOf('d')
.format('YYYY-MM-DDTHH:mm'),
explicitDate: true,
},
trendsFilter: {
display: ChartDisplayType.ActionsLineGraph,
},
filterTestAccounts: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does dateRange and filterTestAccounts need to be hard-coded? It seems like they should be derived from other configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dateRange should be okay as is - it's suitable for the preview (last 14 days), and it gets updated in the query runner when calculating results.

filterTestAccounts - probably a good idea..

}
: metric
),
})
}}
>
<div className="font-semibold flex justify-between items-center">
<span>Custom</span>
{currentMetric.exposure_query && (
<IconCheckCircle fontSize={18} color="var(--primary)" />
)}
</div>
<div className="text-muted text-sm leading-relaxed">
Define your own exposure metric for specific use cases, such as counting by
sessions instead of users. This gives you full control but requires careful
configuration.
</div>
</div>
</div>
{currentMetric.exposure_query && (
<>
<ActionFilter
bordered
filters={queryNodeToFilter(
currentMetric.exposure_query as InsightQueryNode
)}
setFilters={({
actions,
events,
data_warehouse,
}: Partial<FilterType>): void => {
const series = actionsAndEventsToSeries(
{ actions, events, data_warehouse } as any,
true,
MathAvailability.All
)

setTrendsExposureMetric({
metricIdx,
series,
})
}}
typeKey="experiment-metric"
buttonCopy="Add graph series"
showSeriesIndicator={true}
entitiesLimit={1}
showNumericalPropsOnly={true}
{...commonActionFilterProps}
/>
<div className="mt-4 space-y-4">
<TestAccountFilterSwitch
checked={(() => {
const val = currentMetric.exposure_query?.filterTestAccounts
return hasFilters ? !!val : false
})()}
onChange={(checked: boolean) => {
setTrendsExposureMetric({
metricIdx,
filterTestAccounts: checked,
})
}}
fullWidth
/>
</div>
{isExperimentRunning && (
<LemonBanner type="info" className="mt-3 mb-3">
Preview insights are generated based on {EXPERIMENT_DEFAULT_DURATION}{' '}
days of data. This can cause a mismatch between the preview and the
actual results.
</LemonBanner>
)}
<div className="mt-4">
<Query
query={{
kind: NodeKind.InsightVizNode,
source: currentMetric.exposure_query,
showTable: false,
showLastComputation: true,
showLastComputationRefresh: false,
}}
readOnly
/>
</div>
</>
)}
</>
),
},
]}
/>
<div className="mt-4 space-y-4">
<TestAccountFilterSwitch
checked={hasFilters ? !!currentMetric.count_query?.filterTestAccounts : false}
onChange={(checked: boolean) => {
setTrendsMetric({
metricIdx,
filterTestAccounts: checked,
})
}}
fullWidth
/>
</div>
{isExperimentRunning && (
<LemonBanner type="info" className="mt-3 mb-3">
Preview insights are generated based on {EXPERIMENT_DEFAULT_DURATION} days of data. This can cause a
mismatch between the preview and the actual results.
</LemonBanner>
)}
<div className="mt-4">
<Query
query={{
kind: NodeKind.InsightVizNode,
source: currentMetric.count_query,
showTable: false,
showLastComputation: true,
showLastComputationRefresh: false,
}}
readOnly
/>
</div>
</>
)
}
Loading