Group reamining breakdown values into "Other" for funnels #5538
Conversation
| self._filter, first_entity, "count(*)", self._team.pk, limit | ||
| ) | ||
| limit = self._filter.breakdown_limit_or_default | ||
| first_entity = next(x for x in self._filter.entities if x.order == 0) |
There was a problem hiding this comment.
Code issue:
This doesn't work as you'd like:
In [2]: next(iter([]))
---------------------------------------------------------------------------
StopIteration Traceback (most recent call last)
<ipython-input-2-bfed92c5b1cf> in <module>
----> 1 next(iter([]))
StopIteration:
Given that entities are sorted already entities[0] does the trick
There was a problem hiding this comment.
Makes sense! I guess we never hit this earlier because we have a check that entities exist. I'm just going to get rid of the validation error completely!
| if self._filter.breakdown_type == "person": | ||
| values = get_breakdown_person_prop_values(self._filter, first_entity, "count(*)", self._team.pk, limit) | ||
| elif self._filter.breakdown_type == "event": | ||
| values = get_breakdown_event_prop_values(self._filter, first_entity, "count(*)", self._team.pk, limit) |
There was a problem hiding this comment.
So what happens if the second entity has a different set of person property values? Is it impossible due to how funnels work?
If so, worth adding a comment.
There was a problem hiding this comment.
What do you mean by different set of person property values for second entity? Maybe an example?
If you mean something like: breakdown by X, and second entity has a filter like: X=a, where a is very rare, then earlier, yes, this would mean that drop off is 100%. After this PR, they'd all get grouped into "Other".
There was a problem hiding this comment.
So entities in this case are things like:
- { type: event, value: 'fooevent' }
- { type: event, value: 'barevent' }
Let's walk through a scenario
If you break down first with $some_property you get values A, B. If you broke the second with $some_property you would get B and C
Would the table contain A and B only or would C get included as well or is this scenario impossible?
There was a problem hiding this comment.
Earlier:
If possible, table would contain A, B only.
Now:
if possible, table will contain A, B, Other
Judging possibility:
Hmm, one case where I can imagine this being possible is if second never happened with A value of $some_property and first never happened with C value of $some_property. Unlikely, but possible. It smells of event-dependent values of $some_property though, which doesn't seem right. Like, it implies the user in a single session did first and then second event, but the properties changed in the middle.
The idea with choosing the first event: anything that occurs for the Nth event but not the first event wouldn't make it inside the funnel, since the first event needs to happen ... first.
There was a problem hiding this comment.
I hope you see now why this needs at minimum a clarifying comment - there's a ton to unpack here.
Why Other not C? 3 values will fall well below the line of 5 or 10. :)
Given you can break down by event properties, it's more than possible that the second step in the funnel has a different set of properties than the first. It's kind of how event properties work - they don't stick around for all events for a session.
There was a problem hiding this comment.
Ah, I see what you mean now, "Why other not C" is a very valid question!
There was a problem hiding this comment.
oh wait, actually, in the funnel, all event properties are constrained to the first in line, so even with other grouping, the breakdown values would only be A, B, no other. (will write a test to confirm). My bad here, I got confused: the query never does anything with props for events following the first one.
I'll chalk this one up to a funnel limitation. With event props, makes sense to extend this like you say.
@EDsCODE can confirm?
| @@ -397,30 +397,28 @@ def _get_cohort_breakdown_join(self) -> str: | |||
|
|
|||
| def _get_breakdown_conditions(self) -> str: | |||
There was a problem hiding this comment.
this pattern is turning out to be a bit icky, where this function modifies state (self.params) and returns empty string.
Will leave refactoring to a follow up PR - it might make sense to do it for the rest of the functions like this one.
Changes
Resolves #5427
Checklist