-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(trends): use hogql for legacy insight trends api #27213
Conversation
# Conflicts: # posthog/api/insight.py
@@ -1939,118 +1928,6 @@ def test_insight_trends_breakdown_pagination(self) -> None: | |||
self.assertEqual(response.status_code, status.HTTP_200_OK, response.json()) | |||
self.assertIn("offset=25", response.json()["next"]) | |||
|
|||
def test_insight_trends_breakdown_persons_with_histogram(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just couldn't get this test to work. During debugging I found that raw_sessions
isn't populated in tests, but using a different method to create the events or passing in a valid uuid7 for the session id all didn't work. I couldn't find additional steps in other tests.
Running a session histogram breakdown locally works, e.g.
{
"events": [
{
"id": "$pageview",
"math": "total",
"type": "events",
"order": 0
}
],
"date_from": "-7d",
"breakdown": "$session_duration",
"breakdown_type": "session",
"breakdown_histogram_bin_count": 2
}
query_runner = get_query_runner(query, team, limit_context=None) | ||
|
||
# we use the legacy caching mechanism (@cached_by_filters decorator), no need to cache in the query runner | ||
result = query_runner.run(execution_mode=ExecutionMode.CALCULATE_BLOCKING_ALWAYS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do customers call this endpoint from and does this have the same rate limit as /query
?
As CALCULATE_BLOCKING_ALWAYS
gives them a way around async processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They call https://us.posthog.com/api/projects/:team_id/insights/trend/
, and yes this allows bypassing async processing and always has (legacy endpoint didn't have async processing).
I think we'll eventually bug them to use the new /query
api, but this allows us to get rid of legacy code while giving them a longer grace period.
Current usage can be seen on this dashboard https://us.posthog.com/project/2/dashboard/289911 e.g. the funnels endpoint is only used by one customer.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Problem
We want to use HogQL queries for computing the legacy insight API endpoints. We use
/query
everywhere in our codebase, but the endpoints can still be called by customers.Changes
Changes computation of the trends(-like) endpoint to HogQL.
How did you test this code?
CI run
and calling
http://localhost:8000/api/projects/1/insights/trend
http://localhost:8000/api/projects/1/insights/trend?query_method=legacy
with payload