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.

Sign up for GitHub

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

Jump to bottom

Funnels endpoint failing on posthog + postgresql #6530

Closed
1 of 3 tasks
hazzadous opened this issue Oct 19, 2021 · 1 comment · Fixed by #6538
Closed
1 of 3 tasks

Funnels endpoint failing on posthog + postgresql #6530

hazzadous opened this issue Oct 19, 2021 · 1 comment · Fixed by #6538
Assignees
@hazzadous
Labels
bug Something isn't working right

Comments

@hazzadous
Copy link
Contributor

hazzadous commented Oct 19, 2021 •
edited
Loading

Bug description

A posthog user has been experiencing a crash that requires them to run docker restart to get things working again. The slack user discussion of this is here

The error message they are seeing is:

[28/Sep/2021:11:50:38 +0000] "POST /api/insight/funnel/? HTTP/1.0" 200 27 "https://posthog.structured.today/dashboard/1" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.1.2 Safari/605.1.15"
web_1    | 172.25.0.1 - - [28/Sep/2021:11:50:38 +0000] "POST /batch HTTP/1.0" 200 13 "-" "posthog-ios/1.0"
web_1    | [2021-09-28 11:50:38,990: ERROR/ForkPoolWorker-3] Task posthog.celery.update_cache_item_task[29023742-c58a-4c86-b7ea-56fb341c0921] raised unexpected: TypeError('Composed elements must be Composable, got \'\\n            SELECT \\n                DISTINCT ON (person.id)\\n                person.uuid, \\n                person.created_at,\\n                person.team_id,\\n    

...

From the traceback it looks like this is specifically caused when the url query param is set to ActionsLineGraph

Expected behavior

A couple of oddities here:

  1. Funnel queries should be successful
  2. Query failures should knock out the whole process

More investigation required

How to reproduce

TBC

Environment

  • PostHog Cloud
  • self-hosted PostHog (ClickHouse-based), version/commit: please provide
  • self-hosted PostHog (Postgres-based, legacy), version/commit: please provide

Additional context

  1. The internal conversation that alerted me to this
  2. The PR that possibly introduced this issue. Merged 20th Sep

Thank you for your bug report – we love squashing them!
@hazzadous hazzadous added the bug Something isn't working right label Oct 19, 2021
@hazzadous hazzadous self-assigned this Oct 19, 2021
@hazzadous
Copy link
Contributor Author

It looks as though the _build_trends_query was not covered by a test, according to this file coverage

I'll add a test and hopefully that will highlight the issue.

hazzadous pushed a commit that referenced this issue Oct 19, 2021
test(funnel): add snapshot test for funnel trend query for postgres
ac06072 
This test just checks the Funnel with `display="ActionsLineGraph"`
against what it previously did before
#5997 was merged. This code path
was previously untested, so the issue wasn't picked up and resulted in
#6530
hazzadous pushed a commit that referenced this issue Oct 19, 2021
Revert "Revert "perf(funnels): add filter on pdi.team_id to speed up
1131c5d 
query"

Now that we have a test in place for
#6530 I'm reverting this revert
so we can implement a fix.

This reverts commit 49761a3.
hazzadous pushed a commit that referenced this issue Oct 19, 2021
fix(funnel): TypeError for funnel with display=ActionsLineGraph
7b0eadd 
This resolves an issue what was introduced by
#5997 where we would fail to
calculate a funnel result if display="ActionsLineGraph" was specified.
The fix was simply to wrap a string in `sql.SQL`.

Resolves #6530
@hazzadous hazzadous mentioned this issue Oct 19, 2021
Merged
hazzadous pushed a commit that referenced this issue Oct 19, 2021
fix(funnel): TypeError for funnel with display=ActionsLineGraph (#6538
d10605d 
)

* Revert "perf(funnels): add filter on pdi.team_id to speed up query
(#5997)"

This merged caused an issue with the funnels endpoint when display was
set to `ActionsLineGraph`

I'm reverting so we can add in a snapshot test that will fail when this
revert is reverted.

This reverts commit 2fb7cf8.

* test(funnel): add snapshot test for funnel trend query for postgres

This test just checks the Funnel with `display="ActionsLineGraph"`
against what it previously did before
#5997 was merged. This code path
was previously untested, so the issue wasn't picked up and resulted in
#6530

* Revert "Revert "perf(funnels): add filter on pdi.team_id to speed up
query"

Now that we have a test in place for
#6530 I'm reverting this revert
so we can implement a fix.

This reverts commit 49761a3.

* only test on postgres

* fix(funnel): `TypeError` for funnel with display=ActionsLineGraph

This resolves an issue what was introduced by
#5997 where we would fail to
calculate a funnel result if display="ActionsLineGraph" was specified.
The fix was simply to wrap a string in `sql.SQL`.

Resolves #6530

* fix import order

* move test outside of test factory

* remove imports

* move test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hazzadous hazzadous

Labels
bug Something isn't working right
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(funnel): TypeError for funnel with display=ActionsLineGraph PostHog/posthog
1 participant
@hazzadous