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): show missing required events #19969

Closed
wants to merge 2 commits into from
+85 −8
Closed

feat(experiments): show missing required events #19969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f40914
show required events
jurajmajerik Jan 25, 2024
68fb126
Merge branch 'master' of https://github.com/PostHog/posthog into feat…
jurajmajerik Jan 25, 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
28 changes: 21 additions & 7 deletions frontend/src/scenes/experiments/ExperimentResult.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ export function ExperimentResult(): JSX.Element {
conversionRateForVariant,
getIndexForVariant,
areTrendResultsConfusing,
experimentResultCalculationError,
sortedExperimentResultVariants,
experimentMathAggregationForTrends,
requiredEventsBreakdown,
} = useValues(experimentLogic)

return (
Expand Down Expand Up @@ -183,13 +183,27 @@ export function ExperimentResult(): JSX.Element {
<>
<div className="no-experiment-results p-4">
{!experimentResultsLoading && (
<div className="text-center">
<div className="text-center space-y-4">
<b>There are no results for this experiment yet.</b>
<div className="text-sm ">
{!!experimentResultCalculationError && `${experimentResultCalculationError}. `}{' '}
Wait a bit longer for your users to be exposed to the experiment. Double check
your feature flag implementation if you're still not seeing results.
</div>
{requiredEventsBreakdown && requiredEventsBreakdown.missingEvents.length && (
<>
<div className="text-sm">
To be able to see the results, the following events must yet be
ingested. Allow some time for the users to generate them, or double
check your feature flag implementation.
</div>
<div className="w-fit m-auto">
{requiredEventsBreakdown.missingEvents.map(({ event, variant }) => (
<div key={`${event}:${variant}`} className="w-fit mb-2 leading-4">
<span className="text-sm font-bold">{event}</span>
<span className="bg-muted rounded text-white text-xs px-2 py-1 ml-2">
{variant}
</span>
</div>
))}
</div>
</>
)}
</div>
)}
</div>
Expand Down
65 changes: 64 additions & 1 deletion frontend/src/scenes/experiments/experimentLogic.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { urls } from 'scenes/urls'
import { groupsModel } from '~/models/groupsModel'
import { filtersToQueryNode } from '~/queries/nodes/InsightQuery/utils/filtersToQueryNode'
import { queryNodeToFilter } from '~/queries/nodes/InsightQuery/utils/queryNodeToFilter'
import { InsightVizNode } from '~/queries/schema'
import { HogQLQuery, InsightVizNode, NodeKind } from '~/queries/schema'
import {
ActionFilter as ActionFilterType,
Breadcrumb,
Expand Down Expand Up @@ -79,6 +79,11 @@ export interface TabularSecondaryMetricResults {
results?: SecondaryMetricResult[]
}

export interface RequiredEventsBreakdown {
presentEvents: { event: string; variant: string }[]
missingEvents: { event: string; variant: string }[]
}

export const experimentLogic = kea<experimentLogicType>([
props({} as ExperimentLogicProps),
key((props) => props.experimentId || 'new'),
Expand Down Expand Up @@ -406,6 +411,7 @@ export const experimentLogic = kea<experimentLogicType>([
} else {
actions.loadExperimentResults()
actions.loadSecondaryMetricResults()
actions.loadRequiredEventsBreakdown()
}
},
launchExperiment: async () => {
Expand Down Expand Up @@ -616,6 +622,63 @@ export const experimentLogic = kea<experimentLogicType>([
},
},
],
requiredEventsBreakdown: [
Copy link
Collaborator

@neilkakkar neilkakkar Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd much prefer rolling this into the backend API errors: https://github.com/PostHog/posthog/blob/master/ee/clickhouse/queries/experiments/funnel_experiment_result.py#L129

for a few reasons:

  1. This makes an extra unnecessary query and can be slow
  2. Doesn't handle all edge cases, like when actions are present
  3. We technically don't need flag information on all events: On atleast one step is sufficient.

All these are already handled by the backend, so we can adjust shape of data to give us what we want here.

Something like:

(a) Detect if the events are coming in
(b) And if no flag value on above^, change validation error to implementation_error, and pass on appropriate message in the frontend

This way, we can easily distinguish the two cases: (x): No data, and (y): No flag information; and have an explicit message explaining what to do in the latter case -> https://posthog.com/docs/experiments/running-experiments-without-feature-flags#step-3-add-the-feature-flag-property-to-your-events

Copy link
Collaborator

Choose a reason for hiding this comment

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

You have the funnel results here: https://github.com/PostHog/posthog/blob/master/ee/clickhouse/queries/experiments/funnel_experiment_result.py#L87

which give us all the information we need about whether events are coming in and whether they have flag info associated with them.

As long as step 1 has flag information, we're good, so we don't need to check every step, and can always link to step 1 issues.

null as RequiredEventsBreakdown | null,
{
loadRequiredEventsBreakdown: async (): Promise<RequiredEventsBreakdown> => {
const output = {
presentEvents: [] as { event: string; variant: string }[],
missingEvents: [] as { event: string; variant: string }[],
}

const { experiment } = values
const { feature_flag, feature_flag_key } = experiment
const events = experiment.filters.events?.map((e) => e.id)
const variants = feature_flag?.filters.multivariate?.variants.map((v) => v.key)

if (!events?.length || !variants?.length) {
return output
}

const query: HogQLQuery = {
kind: NodeKind.HogQLQuery,
query: `
SELECT
event,
JSONExtractString(properties, '$feature/${feature_flag_key}') AS variant,
COUNT(*) AS count
FROM events
jurajmajerik marked this conversation as resolved.
Show resolved Hide resolved
WHERE event IN (${events.map((e) => `'${e}'`).join(', ')})
AND variant IN (${variants.map((v) => `'${v}'`).join(', ')})
GROUP BY event, variant
`,
}

const responseJSON = await api.query(query)
const { results } = responseJSON
const presentEvents = new Set()
if (results && results.length) {
for (const [event, variant, count] of results) {
if (count && count > 0) {
presentEvents.add(`${event}::${variant}`)
}
}
}

for (const event of events) {
for (const variant of variants) {
if (presentEvents.has(`${event}::${variant}`)) {
output.presentEvents.push({ event, variant })
} else {
output.missingEvents.push({ event, variant })
}
}
}

return output
},
},
],
})),
selectors({
props: [() => [(_, props) => props], (props) => props],
Expand Down
Loading