Skip to content
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

fix: exclusion logic in udfs #25890

Merged
merged 59 commits into from
Oct 31, 2024
Merged

fix: exclusion logic in udfs #25890

merged 59 commits into from
Oct 31, 2024

Conversation

aspicer
Copy link
Contributor

@aspicer aspicer commented Oct 30, 2024

Deploys with #25918 and https://github.com/PostHog/posthog-cloud-infra/pull/3197

Problem

Certain aspects of exclusion behavior differed across UDF funnels and classic funnels, but was untested.

Specifically what happens to a user if they reach an exclusion step but don't go past it.

This was causing large discrepancies especially in the trends view

See here for more details

Changes

Add tests for a variety of exclusion cases that were differing between UDFs and classic trends. Modify the UDF trends to match the classic trends on most of these cases.

One breaking change which is actually a correction is that old trends would exclude a user if the exclusion and following event happened after the funnel interval. This shouldn't be the case, so I changed the behavior here. This causes a few more users to show up in the denominator of the trends query, but not a lot (~0.2% change).

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Wrote a bunch of tests.

Tested it by running the new code against the production data that was incorrect (for two large users) and verifying that we get the correct result.

@posthog-bot
Copy link
Contributor

Hey @aspicer! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

@aspicer aspicer changed the title Aspicer/trends exclusions fix: exclusion logic in udfs Oct 30, 2024
@aspicer aspicer requested review from skoob13 and a team October 30, 2024 23:20
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@skoob13 skoob13 left a comment

Choose a reason for hiding this comment

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

LGTM

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@aspicer aspicer enabled auto-merge (squash) October 31, 2024 21:23
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Oct 31, 2024

Size Change: 0 B

Total Size: 1.15 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.15 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@aspicer aspicer merged commit 968aede into master Oct 31, 2024
96 checks passed
@aspicer aspicer deleted the aspicer/trends_exclusions branch October 31, 2024 22:29
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.

3 participants