perf(query): Use long running async view for queries with poll #27758

timgl · 2025-01-22T04:01:02Z

Problem

Polling is slow, doubly so when the browser is super busy fetching a bunch of other things

Changes

Use an async view that checks celery to see if the response is done. If it's not done after 50 seconds, kick it do polling instead. This avoids us having to have long timeouts in the load balancer etc. Async view means that this should not increase load on django too much (in fact, will probably reduce it as we'll be doing way fewer requests).

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

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

How did you test this code?

greptile-apps

PR Summary

This PR replaces the polling mechanism with EventSource for handling queries, aiming to improve performance and reduce latency, particularly for long-running queries.

Added new eventsource endpoint in /posthog/api/query.py using Django's StreamingHttpResponse for real-time query updates
Implemented feature flag 'query-eventsource' in /frontend/src/queries/query.ts to control EventSource functionality
Added queryEventSource and queryEventSourceUrl methods in /frontend/src/lib/api.ts for server-sent events handling
Modified execution modes to ensure proper blocking behavior for eventsource requests
Implemented connection abort handling and error management for EventSource connections

_{3 file(s) reviewed, 4 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

posthog/api/query.py

frontend/src/queries/query.ts

greptile-apps · 2025-01-22T04:01:45Z

frontend/src/lib/api.ts

+    queryEventSourceUrl(): string {
+        return new ApiRequest().queryEventSource().assembleFullUrl()
+    },


style: consider adding error handling and connection management for EventSource

posthog/api/query.py

github-actions · 2025-01-22T04:13:05Z

Size Change: +251 B (+0.02%)

Total Size: 1.16 MB

ℹ️ View Unchanged

Filename	Size	Change
`frontend/dist/toolbar.js`	1.16 MB	+251 B (+0.02%)

_{compressed-size-action}

posthog-bot · 2025-01-22T04:58:51Z

📸 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 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog/api/query.py

posthog-bot · 2025-01-22T12:29:49Z

📸 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 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog/api/query.py

Twixes

That's a great idea – the wins here definitely are for queries under 60s, and the difference above that wouldn't matter as much. However, I think we can much more simply build this into the existing QueryViewSet.create method, instead of adding a separate hand-rolled request handler outside of DRF. Let's just add this polling-in-the-backend approach to that viewset method

frontend/src/queries/query.ts

frontend/src/lib/api.ts

Twixes · 2025-01-23T14:10:13Z

posthog/api/query.py

+ASYNC_FALLBACK_TO_POLLING_TIMEOUT = 50
+
+
+async def query_async(request: Request, *args, **kwargs) -> HttpResponse:


Let's not overload the term async so much, because async queries already are a thing in the existing query endpoint. This is rather an UX optimization to async querying.

Yeah fair.. I'm just going to roll it out, then I'll make this the default way for queries to happen once we've rolled it out

timgl · 2025-01-23T15:10:51Z

@Twixes not sure what you mean by adding it to the existing viewset. DRF doesn’t support async and the performance impact of having a ton of open connections would be high i suspect

Twixes

Ohh, so annoying DRF doesn't support async. Looks like they don't even really have plans. So in terms of duplicated logic, proposing a non-DRF but still simpler approach below:

posthog/api/query.py

posthog-bot · 2025-01-28T01:42:57Z

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.454. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!

posthog-bot · 2025-01-28T01:44:25Z

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.454. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!

posthog-bot · 2025-01-28T03:54:22Z

📸 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 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-01-28T04:13:55Z

📸 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 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Twixes · 2025-01-28T18:04:16Z

frontend/src/lib/api.ts

+    public queryAsync(teamId?: TeamType['id']): ApiRequest {
+        return this.environmentsDetail(teamId).addPathComponent('query_async')
+    }


Still I'm gonna say it's a bad idea to use "query_async" for something on top of existing async querying. More appropriate would be queryStream/query_stream

Or not "stream" anymore, since we're not streaming 🤔 More like query_awaiting… query_v2?

Pushed query_awaiting, steppin on toes

Twixes

Looks good, let's try it out. One thing to note: this only helps with ad-hoc queries (primarily the insight editor), but not insight/dashboard loading (since these endpoints have querying built-in, for minimized rounds trips indeed)

posthog-bot · 2025-01-28T19:24:33Z

📸 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 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-01-28T19:44:41Z

📸 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 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Michael Matloka <michael@matloka.com>

perf(query): Use eventsource for query

1fd48f5

timgl requested a review from Twixes January 22, 2025 04:01

greptile-apps bot reviewed Jan 22, 2025

View reviewed changes

github-advanced-security bot found potential problems Jan 22, 2025

View reviewed changes

posthog/api/query.py Fixed Show fixed Hide fixed

timgl added 2 commits January 21, 2025 20:20

fix issues

80df6bd

fair dues codeql

8608fdd

Twixes requested changes Jan 22, 2025

View reviewed changes

posthog/api/query.py Outdated Show resolved Hide resolved

move to using another thing

72478d3

timgl force-pushed the use-eventsource-for-queries branch from 714d562 to 72478d3 Compare January 23, 2025 02:40

timgl changed the title ~~perf(query): Use eventsource for query~~ perf(query): Use long running async view for queries with poll Jan 23, 2025

github-advanced-security bot found potential problems Jan 23, 2025

View reviewed changes

posthog/api/query.py Fixed Show fixed Hide fixed

posthog/api/query.py Fixed Show fixed Hide fixed

Twixes requested changes Jan 23, 2025

View reviewed changes

Twixes reviewed Jan 24, 2025

View reviewed changes

posthog/api/query.py Outdated Show resolved Hide resolved

timgl added 4 commits January 27, 2025 17:25

simplify async query endpoint

5a12f25

Merge branch 'master' into use-eventsource-for-queries

8d50c47

fix more stuff

e92de9e

fix

f930a4b

timgl force-pushed the use-eventsource-for-queries branch from 6435168 to 3f264a3 Compare January 28, 2025 01:43

timgl force-pushed the use-eventsource-for-queries branch from 3f264a3 to f930a4b Compare January 28, 2025 01:47

github-actions bot and others added 4 commits January 28, 2025 02:18

Update query snapshots

7af6eff

fix responses

6b59f66

fix

419c690

Update UI snapshots for chromium (1)

5b9083d

Update UI snapshots for chromium (1)

08aa3b4

reduce timeout so we get loading bar info faster

acca3be

Twixes reviewed Jan 28, 2025

View reviewed changes

Add one more test case I was curious about

76bcabc

Twixes force-pushed the use-eventsource-for-queries branch from 8ca8e43 to 8ff5ca3 Compare January 28, 2025 18:20

Twixes added 2 commits January 28, 2025 19:54

Use query_awaited naming to avoid overloading "async"

9610bba

Merge branch 'master' into use-eventsource-for-queries

fa0ac07

Twixes approved these changes Jan 28, 2025

View reviewed changes

Twixes force-pushed the use-eventsource-for-queries branch from 9f75598 to fa0ac07 Compare January 28, 2025 19:06

github-actions bot added 2 commits January 28, 2025 19:17

Update query snapshots

2cfc120

Update UI snapshots for chromium (1)

9775a76

Update UI snapshots for chromium (1)

02adf21

timgl merged commit 330e37b into master Jan 28, 2025
99 checks passed

timgl deleted the use-eventsource-for-queries branch January 28, 2025 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(query): Use long running async view for queries with poll #27758

perf(query): Use long running async view for queries with poll #27758

timgl commented Jan 22, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Jan 22, 2025

github-actions bot commented Jan 22, 2025 •

edited

Loading

posthog-bot commented Jan 22, 2025

posthog-bot commented Jan 22, 2025

Twixes left a comment

Twixes Jan 23, 2025

timgl Jan 28, 2025

timgl commented Jan 23, 2025

Twixes left a comment

posthog-bot commented Jan 28, 2025

posthog-bot commented Jan 28, 2025

posthog-bot commented Jan 28, 2025

posthog-bot commented Jan 28, 2025

Twixes Jan 28, 2025

Twixes Jan 28, 2025

Twixes Jan 28, 2025

Twixes left a comment

posthog-bot commented Jan 28, 2025

posthog-bot commented Jan 28, 2025

		ASYNC_FALLBACK_TO_POLLING_TIMEOUT = 50


		async def query_async(request: Request, args, *kwargs) -> HttpResponse:

perf(query): Use long running async view for queries with poll #27758

perf(query): Use long running async view for queries with poll #27758

Conversation

timgl commented Jan 22, 2025 • edited Loading

Problem

Changes

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

How did you test this code?

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 22, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 22, 2025 • edited Loading

posthog-bot commented Jan 22, 2025

📸 UI snapshots have been updated

posthog-bot commented Jan 22, 2025

📸 UI snapshots have been updated

Twixes left a comment

Choose a reason for hiding this comment

Twixes Jan 23, 2025

Choose a reason for hiding this comment

timgl Jan 28, 2025

Choose a reason for hiding this comment

timgl commented Jan 23, 2025

Twixes left a comment

Choose a reason for hiding this comment

posthog-bot commented Jan 28, 2025

posthog-bot commented Jan 28, 2025

posthog-bot commented Jan 28, 2025

📸 UI snapshots have been updated

posthog-bot commented Jan 28, 2025

📸 UI snapshots have been updated

Twixes Jan 28, 2025

Choose a reason for hiding this comment

Twixes Jan 28, 2025

Choose a reason for hiding this comment

Twixes Jan 28, 2025

Choose a reason for hiding this comment

Twixes left a comment

Choose a reason for hiding this comment

posthog-bot commented Jan 28, 2025

📸 UI snapshots have been updated

posthog-bot commented Jan 28, 2025

📸 UI snapshots have been updated

timgl commented Jan 22, 2025 •

edited

Loading

github-actions bot commented Jan 22, 2025 •

edited

Loading