-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Conversation
…/experiment-show-required-events
@@ -616,6 +622,63 @@ export const experimentLogic = kea<experimentLogicType>([ | |||
}, | |||
}, | |||
], | |||
requiredEventsBreakdown: [ |
There was a problem hiding this comment.
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:
- This makes an extra unnecessary query and can be slow
- Doesn't handle all edge cases, like when actions are present
- 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
There was a problem hiding this comment.
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.
Size Change: 0 B Total Size: 2.21 MB ℹ️ View Unchanged
|
I'm gonna split this into two separate PRs - funnels and trends. Closing this one. |
Changes
No longer show
experimentResultCalculationError
sent from the backend. This is because this message can be one of:I'm only showing the missing events, but the present events are also available. I can imagine changing this into something like: This is what you've ingested so far: [present events], this is still what you need to ingest: [missing events]
How did you test this code?
👀