Fix issues with step conversion times#5174
Conversation
As-is this default hurts local dev - one more setting you need to export. Let's keep the setup simple instead. Also the new default would have broken helm chart and production when updating with new migrations - the cluster name there is `posthog` for both it seems. Let's force other deploys to be explicit instead.
| {steps_per_person_query} | ||
| ) GROUP BY person_id {breakdown_clause} | ||
| return f""" | ||
| SELECT person_id, steps {self._get_step_time_avgs(max_steps)} {breakdown_clause} FROM ( |
There was a problem hiding this comment.
When some sequence of steps is a substring of itself, this previously led to incorrect results (since you want to average out times to convert of only the most-completed attempts)
There was a problem hiding this comment.
This wasn't fully clear^
wrote a new test to explain this: test_funnel_with_multiple_incomplete_tries - when you do multiple incomplete tries, and multiple complete tries, only take into account the complete tries, since those are the ones that get counted (max(steps))
| }, | ||
| ) | ||
|
|
||
| def test_auto_bin_count_single_step_duplicate_events(self): |
There was a problem hiding this comment.
This demonstrates the CH bug, will probably move this into a separate PR, since this edge case will remain flakey till its fixed upstream.
There was a problem hiding this comment.
Thinking of removing these from consideration in our time to convert funnels as a patch.
EDsCODE
left a comment
EDsCODE
left a comment
There was a problem hiding this comment.
good catch, lgtm
*Counting everything will be important for the funnel pattern where we don't distinguish by person and aggregate all the funnel attempts
|
Yup, indeed! When we get to implement that, it can be a conditional in the |
4 participants
Changes
Resolves some step conversion time issues
Checklist