Skip to content

fix(funnels): Improve "Time to convert" behavior #12474

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 4 commits into from
Nov 2, 2022

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Oct 27, 2022

Problem

When investigating #12460, I noticed "Time to convert" doesn't handle the case with zero conversion well either:

Screenshot 2022-10-27 at 15 28 44

Changes

There's no NaNNaN anymore:

Screenshot 2022-10-27 at 15 33 05

Also removed special handling from #5283, as it seems the ClickHouse bug got fixed. Let me know if this is correct @neilkakkar.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks good to me, yet a test is failing and I don't have enough context to verify the query 🤔

@Twixes Twixes requested a review from macobo October 31, 2022 14:43
Copy link
Contributor

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

Looks good, nice work!

@Twixes Twixes added the release-1.41.0 To be cherry-picked into `release-1.41.0` label Nov 1, 2022
@Twixes Twixes merged commit 982d97c into master Nov 2, 2022
@Twixes Twixes deleted the time-to-convert-facelift branch November 2, 2022 08:14
mariusandra pushed a commit that referenced this pull request Nov 2, 2022
* fix(funnels): Improve "Time to convert" behavior

* Update test_insight_funnels.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-1.41.0 To be cherry-picked into `release-1.41.0`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants