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))
| @@ -78,6 +78,58 @@ def test_auto_bin_count_single_step(self): | |||
| }, | |||
| ) | |||
|
|
|||
| 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.
|
Yup, indeed! When we get to implement that, it can be a conditional in the |
Changes
Resolves some step conversion time issues
Checklist