Skip to content

fix: remove funnel trends incorrect heuristic #31041

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

Merged
merged 10 commits into from
Apr 10, 2025

Conversation

aspicer
Copy link
Contributor

@aspicer aspicer commented Apr 10, 2025

Problem

Event filtering heuristic breaks for trends that have multiple first steps in a row that fall in different intervals. This is especially bad in unordered trends, because any event can be the first event.
Reported here: https://posthog.slack.com/archives/C045L1VEG87/p1744047732992239

Changes

Remove the event filtering for trends

How did you test this code?

Wrote a test for it.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR fixes a bug in funnel trends analysis by removing the event filtering heuristic that was causing issues with unordered trends having multiple first steps across different time intervals.

  • Removed the udf_event_array_filter method from funnel_trends_udf.py and now passes the raw events_array directly to the funnel aggregation function
  • Added a comprehensive test case in test_funnel_unordered.py that verifies correct conversion rates for users with varying event patterns across multiple days
  • The fix is particularly important for unordered funnels where any event can be the first step, making the previous filtering approach problematic
  • Test creates 5 users with different patterns of event occurrences to thoroughly validate the solution works across various scenarios
  • Addresses a customer-reported issue where trends analysis was breaking when events spanned different time intervals

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@aspicer aspicer requested a review from a team April 10, 2025 07:42
@aspicer aspicer enabled auto-merge (squash) April 10, 2025 08:28
@aspicer aspicer changed the title fix: funnel trends bug fix: remove funnel trends heuristic causing bad results Apr 10, 2025
@aspicer aspicer changed the title fix: remove funnel trends heuristic causing bad results fix: remove funnel trends incorrect heuristic Apr 10, 2025
@aspicer aspicer disabled auto-merge April 10, 2025 08:29
@aspicer aspicer enabled auto-merge (squash) April 10, 2025 08:29
@aspicer aspicer merged commit 973f888 into master Apr 10, 2025
98 checks passed
@aspicer aspicer deleted the aspicer/unordered_trends_bug branch April 10, 2025 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants