From 3407f75b03d13aacad9e50d78b40bc7b5bf65c47 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 17:46:56 +0200 Subject: [PATCH 01/19] ci: Run core backend tests with both HogQL and legacy insights --- .github/actions/run-backend-tests/action.yml | 7 ++++--- .github/workflows/ci-backend.yml | 6 ++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/.github/actions/run-backend-tests/action.yml b/.github/actions/run-backend-tests/action.yml index db44d4fdf4bde..5ffc9244383e8 100644 --- a/.github/actions/run-backend-tests/action.yml +++ b/.github/actions/run-backend-tests/action.yml @@ -12,7 +12,7 @@ inputs: description: ClickHouse server image tag, e.g. clickhouse/clickhouse-server:latest segment: required: true - description: Either 'Core' or 'Temporal' segment + description: Either 'Core', 'Legacy insights backend', or 'Temporal' concurrency: required: true description: Count of concurrency groups @@ -143,12 +143,13 @@ runs: run: echo "PYTEST_ARGS=--snapshot-update" >> $GITHUB_ENV # We can only update snapshots within the PostHog org # Tests - - name: Run Core tests + - name: Run core tests id: run-core-tests - if: ${{ inputs.segment == 'Core' }} + if: ${{ inputs.segment != 'Temporal' }} env: PERSON_ON_EVENTS_V2_ENABLED: ${{ inputs.person-on-events }} GROUPS_ON_EVENTS_ENABLED: ${{ inputs.person-on-events }} + HOGQL_INSIGHTS_OVERRIDE: ${{ inputs.segment == 'Legacy insights backend' && 0 || 1 }} shell: bash run: | # async_migrations covered in ci-async-migrations.yml pytest ${{ diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml index 577a27e240eba..b301cefb0cefb 100644 --- a/.github/workflows/ci-backend.yml +++ b/.github/workflows/ci-backend.yml @@ -260,6 +260,12 @@ jobs: concurrency: [10] group: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] include: + - segment: 'Legacy insights backend' + person-on-events: false + clickhouse-server-image: 'clickhouse/clickhouse-server:23.12.5.81-alpine' + python-version: '3.10.10' + concurrency: [10] + group: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] - segment: 'Temporal' person-on-events: false clickhouse-server-image: 'clickhouse/clickhouse-server:23.12.5.81-alpine' From adb4fe8c366ddb103afa7425860aa437dbf73a9e Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 17:53:34 +0200 Subject: [PATCH 02/19] Double the number of tests --- .github/workflows/ci-backend.yml | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml index b301cefb0cefb..ed8a5efc054e7 100644 --- a/.github/workflows/ci-backend.yml +++ b/.github/workflows/ci-backend.yml @@ -254,18 +254,12 @@ jobs: matrix: python-version: ['3.10.10'] clickhouse-server-image: ['clickhouse/clickhouse-server:23.12.5.81-alpine'] - segment: ['Core'] + segment: ['Core', 'Legacy insights backend'] person-on-events: [false, true] # :NOTE: Keep concurrency and groups in sync concurrency: [10] group: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] include: - - segment: 'Legacy insights backend' - person-on-events: false - clickhouse-server-image: 'clickhouse/clickhouse-server:23.12.5.81-alpine' - python-version: '3.10.10' - concurrency: [10] - group: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] - segment: 'Temporal' person-on-events: false clickhouse-server-image: 'clickhouse/clickhouse-server:23.12.5.81-alpine' From e8e59c63b412e0177eb28b946309684b7b2246c5 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 20:03:36 +0200 Subject: [PATCH 03/19] Fix env var setting --- .github/actions/run-backend-tests/action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/run-backend-tests/action.yml b/.github/actions/run-backend-tests/action.yml index 5ffc9244383e8..c475e15714138 100644 --- a/.github/actions/run-backend-tests/action.yml +++ b/.github/actions/run-backend-tests/action.yml @@ -149,7 +149,7 @@ runs: env: PERSON_ON_EVENTS_V2_ENABLED: ${{ inputs.person-on-events }} GROUPS_ON_EVENTS_ENABLED: ${{ inputs.person-on-events }} - HOGQL_INSIGHTS_OVERRIDE: ${{ inputs.segment == 'Legacy insights backend' && 0 || 1 }} + HOGQL_INSIGHTS_OVERRIDE: ${{ inputs.segment == 'Legacy insights backend' && '0' || '1' }} shell: bash run: | # async_migrations covered in ci-async-migrations.yml pytest ${{ From 1f2469ca1fdeebb4f4ff5d43db768e03fd916c8b Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 23:21:44 +0200 Subject: [PATCH 04/19] Add some HOGQL_INSIGHTS_OVERRIDE overrides in tests --- posthog/api/team.py | 5 +++-- posthog/api/test/dashboards/test_dashboard.py | 2 ++ posthog/api/test/test_insight.py | 1 + posthog/api/test/test_team.py | 2 ++ 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/posthog/api/team.py b/posthog/api/team.py index ca55dbbf1d032..c29732d4e2f1c 100644 --- a/posthog/api/team.py +++ b/posthog/api/team.py @@ -338,8 +338,9 @@ def create(self, validated_data: dict[str, Any], **kwargs) -> Team: return team - def _clear_team_insight_cache(self, team: Team) -> None: - # :KLUDGE: This is incorrect as it doesn't wipe caches not currently linked to insights. Fix this some day! + def _handle_timezone_update(self, team: Team) -> None: + # TODO: Remove when legacy insight calculation is no more, + # as the HogQL backend's caching always includes timezone in the cache key hashes = InsightCachingState.objects.filter(team=team).values_list("cache_key", flat=True) cache.delete_many(hashes) diff --git a/posthog/api/test/dashboards/test_dashboard.py b/posthog/api/test/dashboards/test_dashboard.py index 7eefc3cb8ebab..8f897426c3c84 100644 --- a/posthog/api/test/dashboards/test_dashboard.py +++ b/posthog/api/test/dashboards/test_dashboard.py @@ -193,6 +193,7 @@ def test_shared_dashboard(self): response = self.client.get("/shared_dashboard/testtoken") self.assertEqual(response.status_code, status.HTTP_200_OK) + @override_settings(HOGQL_INSIGHTS_OVERRIDE=False) # .../insights/trend/ can't run in HogQL yet def test_return_cached_results_bleh(self): dashboard = Dashboard.objects.create(team=self.team, name="dashboard") filter_dict = { @@ -930,6 +931,7 @@ def test_duplication_fail_for_different_team(self): expected_status=status.HTTP_400_BAD_REQUEST, ) + @override_settings(HOGQL_INSIGHTS_OVERRIDE=False) # .../insights/trend/ can't run in HogQL yet def test_return_cached_results_dashboard_has_filters(self): # Regression test, we were diff --git a/posthog/api/test/test_insight.py b/posthog/api/test/test_insight.py index ab75f8c8adec5..26b0f7a420993 100644 --- a/posthog/api/test/test_insight.py +++ b/posthog/api/test/test_insight.py @@ -1022,6 +1022,7 @@ def test_save_new_funnel(self) -> None: self.assertEqual(objects[0].filters["layout"], "horizontal") self.assertEqual(len(objects[0].short_id), 8) + @override_settings(HOGQL_INSIGHTS_OVERRIDE=False) # synchronously_update_cache is a legacy-only code path @patch("posthog.api.insight.synchronously_update_cache", wraps=synchronously_update_cache) def test_insight_refreshing_legacy(self, spy_update_insight_cache) -> None: dashboard_id, _ = self.dashboard_api.create_dashboard({"filters": {"date_from": "-14d"}}) diff --git a/posthog/api/test/test_team.py b/posthog/api/test/test_team.py index 0bf9723bdba21..896a4ba2c433f 100644 --- a/posthog/api/test/test_team.py +++ b/posthog/api/test/test_team.py @@ -7,6 +7,7 @@ from asgiref.sync import sync_to_async from django.core.cache import cache from django.http import HttpResponse +from django.test import override_settings from freezegun import freeze_time from parameterized import parameterized from rest_framework import status @@ -503,6 +504,7 @@ def test_cant_set_primary_dashboard_to_another_teams_dashboard(self): response_data = response.json() self.assertEqual(response_data["primary_dashboard"], None) + @override_settings(HOGQL_INSIGHTS_OVERRIDE=False) # .../insights/trend/ can't run in HogQL yet def test_update_timezone_remove_cache(self): # Seed cache with some insights self.client.post( From f15ecb900df0d269ce19d47d7d365dc4b1827e17 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 25 Apr 2024 23:22:03 +0200 Subject: [PATCH 05/19] Mark `Insight.last_refresh` as deprecated --- posthog/api/insight.py | 1 - posthog/models/insight.py | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/posthog/api/insight.py b/posthog/api/insight.py index e4595fe776c9e..895aa0fc99852 100644 --- a/posthog/api/insight.py +++ b/posthog/api/insight.py @@ -326,7 +326,6 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Insight: raise serializers.ValidationError("Dashboard not found") DashboardTile.objects.create(insight=insight, dashboard=dashboard, last_refresh=now()) - insight.last_refresh = now() # set last refresh if the insight is on at least one dashboard # Manual tag creation since this create method doesn't call super() self._attempt_set_tags(tags, insight) diff --git a/posthog/models/insight.py b/posthog/models/insight.py index b4949771d6d80..2b93b3c214d4f 100644 --- a/posthog/models/insight.py +++ b/posthog/models/insight.py @@ -43,7 +43,6 @@ class Insight(models.Model): deleted: models.BooleanField = models.BooleanField(default=False) saved: models.BooleanField = models.BooleanField(default=False) created_at: models.DateTimeField = models.DateTimeField(null=True, blank=True, auto_now_add=True) - last_refresh: models.DateTimeField = models.DateTimeField(blank=True, null=True) refreshing: models.BooleanField = models.BooleanField(default=False) created_by: models.ForeignKey = models.ForeignKey("User", on_delete=models.SET_NULL, null=True, blank=True) # Indicates if it's a sample graph generated by dashboard templates @@ -69,6 +68,8 @@ class Insight(models.Model): null=True, blank=True, ) + # DEPRECATED: within cached results package now + last_refresh: models.DateTimeField = models.DateTimeField(blank=True, null=True) # DEPRECATED: on dashboard_insight now layouts: models.JSONField = models.JSONField(default=dict) # DEPRECATED: on dashboard_insight now From dd9aa72e01ccd9729a004c0a0a7c6fc56bec4032 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Fri, 10 May 2024 11:14:18 +0200 Subject: [PATCH 06/19] Fix bad merge --- posthog/api/team.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/posthog/api/team.py b/posthog/api/team.py index c29732d4e2f1c..65920c5de3121 100644 --- a/posthog/api/team.py +++ b/posthog/api/team.py @@ -338,9 +338,9 @@ def create(self, validated_data: dict[str, Any], **kwargs) -> Team: return team - def _handle_timezone_update(self, team: Team) -> None: - # TODO: Remove when legacy insight calculation is no more, - # as the HogQL backend's caching always includes timezone in the cache key + def _clear_team_insight_caching_states(self, team: Team) -> None: + # TODO: Remove this method - we should 100% rely on cache keys being different for materially different queries, + # not on remembering to call this method when project settings change. We probably already are in the clear hashes = InsightCachingState.objects.filter(team=team).values_list("cache_key", flat=True) cache.delete_many(hashes) @@ -350,7 +350,7 @@ def update(self, instance: Team, validated_data: dict[str, Any]) -> Team: if ("timezone" in validated_data and validated_data["timezone"] != instance.timezone) or ( "modifiers" in validated_data and validated_data["modifiers"] != instance.modifiers ): - self._clear_team_insight_cache(instance) + self._clear_team_insight_caching_states(instance) if ( "session_replay_config" in validated_data From 88fa51d0729833d8a056f22903c1508929d7d5eb Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 22 May 2024 10:19:12 +0000 Subject: [PATCH 07/19] Update query snapshots --- .../__snapshots__/test_dashboard.ambr | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/posthog/api/test/dashboards/__snapshots__/test_dashboard.ambr b/posthog/api/test/dashboards/__snapshots__/test_dashboard.ambr index c07a67a43368f..18333aa62d64e 100644 --- a/posthog/api/test/dashboards/__snapshots__/test_dashboard.ambr +++ b/posthog/api/test/dashboards/__snapshots__/test_dashboard.ambr @@ -422,7 +422,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -432,6 +431,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -816,7 +816,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -826,6 +825,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -1665,7 +1665,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -1675,6 +1674,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -2611,7 +2611,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -2621,6 +2620,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -3272,7 +3272,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -3282,6 +3281,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -3720,7 +3720,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -3730,6 +3729,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -4273,7 +4273,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -4283,6 +4282,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -4635,7 +4635,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -4645,6 +4644,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -7208,7 +7208,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -7218,6 +7217,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -9497,7 +9497,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -9507,6 +9506,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -9988,7 +9988,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -9998,6 +9997,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -10269,7 +10269,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -10279,6 +10278,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -10716,7 +10716,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -10726,6 +10725,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -11051,7 +11051,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -11061,6 +11060,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -11493,7 +11493,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -11503,6 +11502,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -11876,7 +11876,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -11886,6 +11885,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -12325,7 +12325,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -12335,6 +12334,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -12829,7 +12829,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -12839,6 +12838,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -13007,7 +13007,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -13017,6 +13016,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -13307,7 +13307,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -13317,6 +13316,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -13785,7 +13785,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -13795,6 +13794,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", From e5af07c576fedd415968006cd0db24c00411b6b8 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 22 May 2024 10:31:35 +0000 Subject: [PATCH 08/19] Update query snapshots --- posthog/api/test/__snapshots__/test_annotation.ambr | 4 ++-- posthog/api/test/__snapshots__/test_insight.ambr | 6 +++--- .../__snapshots__/test_organization_feature_flag.ambr | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/posthog/api/test/__snapshots__/test_annotation.ambr b/posthog/api/test/__snapshots__/test_annotation.ambr index fa55ea1daffec..63a18652d9a79 100644 --- a/posthog/api/test/__snapshots__/test_annotation.ambr +++ b/posthog/api/test/__snapshots__/test_annotation.ambr @@ -210,7 +210,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -220,6 +219,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -619,7 +619,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -629,6 +628,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", diff --git a/posthog/api/test/__snapshots__/test_insight.ambr b/posthog/api/test/__snapshots__/test_insight.ambr index 12838e63f48ef..427b3a8ffb4bf 100644 --- a/posthog/api/test/__snapshots__/test_insight.ambr +++ b/posthog/api/test/__snapshots__/test_insight.ambr @@ -812,7 +812,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -822,6 +821,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -1235,7 +1235,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -1245,6 +1244,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -1703,7 +1703,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -1713,6 +1712,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", diff --git a/posthog/api/test/__snapshots__/test_organization_feature_flag.ambr b/posthog/api/test/__snapshots__/test_organization_feature_flag.ambr index 19adb84ced677..36babee9b09b7 100644 --- a/posthog/api/test/__snapshots__/test_organization_feature_flag.ambr +++ b/posthog/api/test/__snapshots__/test_organization_feature_flag.ambr @@ -259,7 +259,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -269,6 +268,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -438,7 +438,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -448,6 +447,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -613,7 +613,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -623,6 +622,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", @@ -783,7 +783,6 @@ "posthog_dashboarditem"."deleted", "posthog_dashboarditem"."saved", "posthog_dashboarditem"."created_at", - "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."refreshing", "posthog_dashboarditem"."created_by_id", "posthog_dashboarditem"."is_sample", @@ -793,6 +792,7 @@ "posthog_dashboarditem"."last_modified_at", "posthog_dashboarditem"."last_modified_by_id", "posthog_dashboarditem"."dashboard_id", + "posthog_dashboarditem"."last_refresh", "posthog_dashboarditem"."layouts", "posthog_dashboarditem"."color", "posthog_dashboarditem"."dive_dashboard_id", From b83c6b9c2600682f4aedcbe94ac35efcb8c9ad1d Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Wed, 22 May 2024 19:49:59 +0200 Subject: [PATCH 09/19] Update test_team.py --- posthog/api/test/test_team.py | 1 + 1 file changed, 1 insertion(+) diff --git a/posthog/api/test/test_team.py b/posthog/api/test/test_team.py index 896a4ba2c433f..2657d96d7564b 100644 --- a/posthog/api/test/test_team.py +++ b/posthog/api/test/test_team.py @@ -529,6 +529,7 @@ def test_update_timezone_remove_cache(self): # Verify cache was deleted self.assertEqual(cache.get(response["filters_hash"]), None) + @override_settings(HOGQL_INSIGHTS_OVERRIDE=False) # .../insights/trend/ can't run in HogQL yet def test_update_modifiers_remove_cache(self): self.client.patch( f"/api/projects/{self.team.id}/", From d91cd2062bc6dd2986d527fa3380ffc09f9846ab Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 18:38:49 +0200 Subject: [PATCH 10/19] Actually remove legacy backend from matrix --- .github/actions/run-backend-tests/action.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/actions/run-backend-tests/action.yml b/.github/actions/run-backend-tests/action.yml index c475e15714138..d2f83360955dd 100644 --- a/.github/actions/run-backend-tests/action.yml +++ b/.github/actions/run-backend-tests/action.yml @@ -12,7 +12,7 @@ inputs: description: ClickHouse server image tag, e.g. clickhouse/clickhouse-server:latest segment: required: true - description: Either 'Core', 'Legacy insights backend', or 'Temporal' + description: Either 'Core' or 'Temporal' concurrency: required: true description: Count of concurrency groups @@ -149,7 +149,7 @@ runs: env: PERSON_ON_EVENTS_V2_ENABLED: ${{ inputs.person-on-events }} GROUPS_ON_EVENTS_ENABLED: ${{ inputs.person-on-events }} - HOGQL_INSIGHTS_OVERRIDE: ${{ inputs.segment == 'Legacy insights backend' && '0' || '1' }} + HOGQL_INSIGHTS_OVERRIDE: '1' # HogQL insights are fully rolled out in production shell: bash run: | # async_migrations covered in ci-async-migrations.yml pytest ${{ From c32b377a75a7ad6fc7f4d45f17115ffbfabcacc3 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 19:02:41 +0200 Subject: [PATCH 11/19] Update test_fetch_from_cache.py --- posthog/caching/test/test_fetch_from_cache.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/posthog/caching/test/test_fetch_from_cache.py b/posthog/caching/test/test_fetch_from_cache.py index 6ac03f0a0e451..d1d38b2812f01 100644 --- a/posthog/caching/test/test_fetch_from_cache.py +++ b/posthog/caching/test/test_fetch_from_cache.py @@ -1,5 +1,6 @@ from datetime import timedelta +from django.test import override_settings from django.utils.timezone import now from freezegun import freeze_time @@ -90,7 +91,10 @@ def test_synchronously_update_cache_dashboard_tile(self): "next_allowed_client_refresh": None, } - def test_fetch_cached_insight_result_from_cache(self): + @override_settings( + HOGQL_INSIGHTS_OVERRIDE=False # synchronously_update_cache and fetch_cached_insight_result are legacy-only code paths + ) + def test_fetch_cached_insight_result_from_cache_legacy(self): cached_result = synchronously_update_cache(self.insight, self.dashboard, timedelta(minutes=3)) from_cache_result = fetch_cached_insight_result(self.dashboard_tile, timedelta(minutes=3)) From b28e699e31e85236a4ad1b7f1c41dbbe2b040173 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 19:03:01 +0200 Subject: [PATCH 12/19] Use `update_cached_state` in `calculate_for_query_based_insight()` --- posthog/caching/calculate_results.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/posthog/caching/calculate_results.py b/posthog/caching/calculate_results.py index dc0ecdf6ec4f9..c57c76fa26cf3 100644 --- a/posthog/caching/calculate_results.py +++ b/posthog/caching/calculate_results.py @@ -125,6 +125,7 @@ def calculate_for_query_based_insight( ) -> "InsightResult": from posthog.api.services.query import process_query_dict, ExecutionMode from posthog.caching.fetch_from_cache import InsightResult, NothingInCacheResult + from posthog.caching.insight_cache import update_cached_state tag_queries(team_id=insight.team_id, insight_id=insight.pk) if dashboard: @@ -145,6 +146,12 @@ def calculate_for_query_based_insight( if isinstance(response, BaseModel): response = response.model_dump() + update_cached_state( # Updating the relevant InsightCachingState + insight.team_id, + response.get("cache_key"), + response.get("last_refresh"), + result=None, # Not caching the result here, since in HogQL this is the query runner's responsibility + ) return InsightResult( # Translating `QueryResponse` to legacy insights shape # The response may not be conformant with that, hence these are all `.get()`s From e1160c5790bc23685e3d257b9d794639c25ddefa Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 19:07:03 +0200 Subject: [PATCH 13/19] Clean up CI changes and a comment --- .github/actions/run-backend-tests/action.yml | 6 +++--- .github/workflows/ci-backend.yml | 2 +- posthog/api/team.py | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/actions/run-backend-tests/action.yml b/.github/actions/run-backend-tests/action.yml index d2f83360955dd..e8375a2dad2d1 100644 --- a/.github/actions/run-backend-tests/action.yml +++ b/.github/actions/run-backend-tests/action.yml @@ -12,7 +12,7 @@ inputs: description: ClickHouse server image tag, e.g. clickhouse/clickhouse-server:latest segment: required: true - description: Either 'Core' or 'Temporal' + description: Either 'Core' or 'Temporal' segment concurrency: required: true description: Count of concurrency groups @@ -143,9 +143,9 @@ runs: run: echo "PYTEST_ARGS=--snapshot-update" >> $GITHUB_ENV # We can only update snapshots within the PostHog org # Tests - - name: Run core tests + - name: Run Core tests id: run-core-tests - if: ${{ inputs.segment != 'Temporal' }} + if: ${{ inputs.segment == 'Core' }} env: PERSON_ON_EVENTS_V2_ENABLED: ${{ inputs.person-on-events }} GROUPS_ON_EVENTS_ENABLED: ${{ inputs.person-on-events }} diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml index ed8a5efc054e7..577a27e240eba 100644 --- a/.github/workflows/ci-backend.yml +++ b/.github/workflows/ci-backend.yml @@ -254,7 +254,7 @@ jobs: matrix: python-version: ['3.10.10'] clickhouse-server-image: ['clickhouse/clickhouse-server:23.12.5.81-alpine'] - segment: ['Core', 'Legacy insights backend'] + segment: ['Core'] person-on-events: [false, true] # :NOTE: Keep concurrency and groups in sync concurrency: [10] diff --git a/posthog/api/team.py b/posthog/api/team.py index 65920c5de3121..48413d725e584 100644 --- a/posthog/api/team.py +++ b/posthog/api/team.py @@ -339,8 +339,10 @@ def create(self, validated_data: dict[str, Any], **kwargs) -> Team: return team def _clear_team_insight_caching_states(self, team: Team) -> None: - # TODO: Remove this method - we should 100% rely on cache keys being different for materially different queries, - # not on remembering to call this method when project settings change. We probably already are in the clear + # TODO: Remove this method: + # 1. It only clear the cache for saved insights, queries not linked to one are being ignored here + # 2. We should anyway 100% be relying on cache keys being different for materially different queries, instead of + # on remembering to call this method when project settings change. We probably already are in the clear here! hashes = InsightCachingState.objects.filter(team=team).values_list("cache_key", flat=True) cache.delete_many(hashes) From 483d78676bc4d0de2329a6e5acdb0666cba53582 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 19:15:36 +0200 Subject: [PATCH 14/19] Fix `update_cached_state` typing --- posthog/caching/calculate_results.py | 20 ++++++++++++-------- posthog/caching/insight_cache.py | 2 +- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/posthog/caching/calculate_results.py b/posthog/caching/calculate_results.py index c57c76fa26cf3..4437bc157b43b 100644 --- a/posthog/caching/calculate_results.py +++ b/posthog/caching/calculate_results.py @@ -146,19 +146,23 @@ def calculate_for_query_based_insight( if isinstance(response, BaseModel): response = response.model_dump() - update_cached_state( # Updating the relevant InsightCachingState - insight.team_id, - response.get("cache_key"), - response.get("last_refresh"), - result=None, # Not caching the result here, since in HogQL this is the query runner's responsibility - ) + cache_key = response.get("cache_key") + last_refresh = response.get("last_refresh") + if isinstance(cache_key, str) and isinstance(last_refresh, str): + update_cached_state( # Updating the relevant InsightCachingState + insight.team_id, + cache_key, + last_refresh, + result=None, # Not caching the result here, since in HogQL this is the query runner's responsibility + ) + return InsightResult( # Translating `QueryResponse` to legacy insights shape # The response may not be conformant with that, hence these are all `.get()`s result=response.get("results"), columns=response.get("columns"), - last_refresh=response.get("last_refresh"), - cache_key=response.get("cache_key"), + last_refresh=last_refresh, + cache_key=cache_key, is_cached=response.get("is_cached", False), timezone=response.get("timezone"), next_allowed_client_refresh=response.get("next_allowed_client_refresh"), diff --git a/posthog/caching/insight_cache.py b/posthog/caching/insight_cache.py index d8989a9edb515..12fc26d8fc5cd 100644 --- a/posthog/caching/insight_cache.py +++ b/posthog/caching/insight_cache.py @@ -171,7 +171,7 @@ def update_cache(caching_state_id: UUID): def update_cached_state( team_id: int, cache_key: str, - timestamp: datetime, + timestamp: datetime | str, result: Any, ttl: Optional[int] = None, ): From 1a8dffd327a93eb8d8c1b9a3be3dc58a50c39774 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 21:12:36 +0200 Subject: [PATCH 15/19] Update test_fetch_from_cache.py --- posthog/caching/test/test_fetch_from_cache.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/posthog/caching/test/test_fetch_from_cache.py b/posthog/caching/test/test_fetch_from_cache.py index d1d38b2812f01..568c67cd574fb 100644 --- a/posthog/caching/test/test_fetch_from_cache.py +++ b/posthog/caching/test/test_fetch_from_cache.py @@ -22,6 +22,9 @@ from posthog.utils import get_safe_cache +@override_settings( + HOGQL_INSIGHTS_OVERRIDE=False # fetch_cached_insight_result and synchronously_update_cache are legacy-only code paths +) @freeze_time("2012-01-14T03:21:34.000Z") class TestFetchFromCache(ClickhouseTestMixin, BaseTest): def setUp(self): @@ -91,9 +94,6 @@ def test_synchronously_update_cache_dashboard_tile(self): "next_allowed_client_refresh": None, } - @override_settings( - HOGQL_INSIGHTS_OVERRIDE=False # synchronously_update_cache and fetch_cached_insight_result are legacy-only code paths - ) def test_fetch_cached_insight_result_from_cache_legacy(self): cached_result = synchronously_update_cache(self.insight, self.dashboard, timedelta(minutes=3)) from_cache_result = fetch_cached_insight_result(self.dashboard_tile, timedelta(minutes=3)) From ee7e7c02916234f72e265c1b3b699016caa3baaf Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 22:03:09 +0200 Subject: [PATCH 16/19] Update test_insight_cache.py --- posthog/caching/test/test_insight_cache.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/posthog/caching/test/test_insight_cache.py b/posthog/caching/test/test_insight_cache.py index b86ac56a3de99..830fc232d5d37 100644 --- a/posthog/caching/test/test_insight_cache.py +++ b/posthog/caching/test/test_insight_cache.py @@ -3,6 +3,7 @@ from collections.abc import Callable from unittest.mock import call, patch +from django.test import override_settings import pytest from django.utils.timezone import now from freezegun import freeze_time @@ -40,7 +41,10 @@ def create_insight_caching_state( **kw, ): with mute_selected_signals(): - insight = create_insight(team, user, filters=filters) + with override_settings(HOGQL_INSIGHTS_OVERRIDE=False): + # HogQL-based insights handle caching fully inside query runners, + # so the tests in this file only pertain to the legacy engine + insight = create_insight(team, user, filters=filters) upsert(team, insight) From 8fa0b5f50ead53e285730189e2a89916c08321f8 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 22:36:16 +0200 Subject: [PATCH 17/19] Update test_insight_cache.py --- posthog/caching/test/test_insight_cache.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/posthog/caching/test/test_insight_cache.py b/posthog/caching/test/test_insight_cache.py index 830fc232d5d37..9fcdf96190aaf 100644 --- a/posthog/caching/test/test_insight_cache.py +++ b/posthog/caching/test/test_insight_cache.py @@ -41,10 +41,7 @@ def create_insight_caching_state( **kw, ): with mute_selected_signals(): - with override_settings(HOGQL_INSIGHTS_OVERRIDE=False): - # HogQL-based insights handle caching fully inside query runners, - # so the tests in this file only pertain to the legacy engine - insight = create_insight(team, user, filters=filters) + insight = create_insight(team, user, filters=filters) upsert(team, insight) @@ -133,6 +130,9 @@ def test_fetch_states_in_need_of_updating(team: Team, user: User, params, expect @pytest.mark.django_db @freeze_time("2020-01-04T13:01:01Z") +@override_settings( + HOGQL_INSIGHTS_OVERRIDE=False # get_safe_cache is too low-level for HogQL, which serializes differently +) def test_update_cache(team: Team, user: User, cache): caching_state = create_insight_caching_state(team, user, refresh_attempt=1) @@ -170,9 +170,11 @@ def test_update_cache_updates_identical_cache_keys(team: Team, user: User, cache @pytest.mark.django_db @freeze_time("2020-01-04T13:01:01Z") @patch("posthog.caching.insight_cache.update_cache_task") -@patch("posthog.caching.insight_cache.calculate_for_filter_based_insight", side_effect=Exception()) +@patch("posthog.caching.insight_cache.process_query_dict", side_effect=Exception()) # HogQL branch +@patch("posthog.caching.insight_cache.calculate_for_filter_based_insight", side_effect=Exception()) # Legacy branch def test_update_cache_when_calculation_fails( spy_calculate_for_filter_based_insight, + spy_process_query_dict, spy_update_cache_task, team: Team, user: User, From 0c0bb441601354e8aabb036c2ee03ee929165d91 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 22:36:41 +0200 Subject: [PATCH 18/19] Clarify `generate_insight_cache_key` as legacy function --- posthog/caching/calculate_results.py | 6 +++--- posthog/models/dashboard_tile.py | 4 ++-- posthog/models/insight.py | 2 +- posthog/models/test/test_insight_model.py | 14 +++++++------- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/posthog/caching/calculate_results.py b/posthog/caching/calculate_results.py index 4437bc157b43b..42420061e59a2 100644 --- a/posthog/caching/calculate_results.py +++ b/posthog/caching/calculate_results.py @@ -31,7 +31,7 @@ from posthog.models.filters import PathFilter from posthog.models.filters.stickiness_filter import StickinessFilter from posthog.models.filters.utils import get_filter -from posthog.models.insight import generate_insight_cache_key +from posthog.models.insight import generate_insight_filters_hash from posthog.queries.funnels import ClickhouseFunnelTimeToConvert, ClickhouseFunnelTrends from posthog.queries.funnels.utils import get_funnel_order_class from posthog.queries.paths import Paths @@ -69,7 +69,7 @@ def calculate_cache_key(target: Union[DashboardTile, Insight]) -> Optional[str]: return query_runner.get_cache_key() if insight.filters: - return generate_insight_cache_key(insight, dashboard) + return generate_insight_filters_hash(insight, dashboard) return None @@ -174,7 +174,7 @@ def calculate_for_filter_based_insight( insight: Insight, dashboard: Optional[Dashboard] ) -> tuple[str, str, list | dict]: filter = get_filter(data=insight.dashboard_filters(dashboard), team=insight.team) - cache_key = generate_insight_cache_key(insight, dashboard) + cache_key = generate_insight_filters_hash(insight, dashboard) cache_type = get_cache_type(filter) tag_queries( diff --git a/posthog/models/dashboard_tile.py b/posthog/models/dashboard_tile.py index 9d39028e49bf0..59738ee2033fa 100644 --- a/posthog/models/dashboard_tile.py +++ b/posthog/models/dashboard_tile.py @@ -4,7 +4,7 @@ from django.utils import timezone from posthog.models.dashboard import Dashboard -from posthog.models.insight import generate_insight_cache_key +from posthog.models.insight import generate_insight_filters_hash from posthog.models.tagged_item import build_check @@ -105,7 +105,7 @@ def save(self, *args, **kwargs) -> None: if self.insight is not None: has_no_filters_hash = self.filters_hash is None if has_no_filters_hash and self.insight.filters != {}: - self.filters_hash = generate_insight_cache_key(self.insight, self.dashboard) + self.filters_hash = generate_insight_filters_hash(self.insight, self.dashboard) if "update_fields" in kwargs: kwargs["update_fields"].append("filters_hash") diff --git a/posthog/models/insight.py b/posthog/models/insight.py index 2b93b3c214d4f..c4d0c29b4929b 100644 --- a/posthog/models/insight.py +++ b/posthog/models/insight.py @@ -206,7 +206,7 @@ class Meta: @timed("generate_insight_cache_key") -def generate_insight_cache_key(insight: Insight, dashboard: Optional[Dashboard]) -> str: +def generate_insight_filters_hash(insight: Insight, dashboard: Optional[Dashboard]) -> str: try: dashboard_insight_filter = get_filter(data=insight.dashboard_filters(dashboard=dashboard), team=insight.team) candidate_filters_hash = generate_cache_key("{}_{}".format(dashboard_insight_filter.toJSON(), insight.team_id)) diff --git a/posthog/models/test/test_insight_model.py b/posthog/models/test/test_insight_model.py index 9fc61485f7453..2570a5e70f051 100644 --- a/posthog/models/test/test_insight_model.py +++ b/posthog/models/test/test_insight_model.py @@ -1,7 +1,7 @@ from django.db.utils import IntegrityError from posthog.models import Dashboard, Insight, Team -from posthog.models.insight import generate_insight_cache_key +from posthog.models.insight import generate_insight_filters_hash from posthog.test.base import BaseTest @@ -76,8 +76,8 @@ def test_dashboard_does_not_affect_filters_hash_with_absent_date_from(self) -> N insight = Insight.objects.create(team=self.team, filters={"date_from": "-30d"}) dashboard = Dashboard.objects.create(team=self.team, filters={}) - filters_hash_no_dashboard = generate_insight_cache_key(insight, None) - filters_hash_with_absent_date_from = generate_insight_cache_key(insight, dashboard) + filters_hash_no_dashboard = generate_insight_filters_hash(insight, None) + filters_hash_with_absent_date_from = generate_insight_filters_hash(insight, dashboard) assert filters_hash_no_dashboard == filters_hash_with_absent_date_from @@ -85,8 +85,8 @@ def test_dashboard_does_not_affect_filters_hash_with_null_date_from(self) -> Non insight = Insight.objects.create(team=self.team, filters={"date_from": "-30d"}) dashboard = Dashboard.objects.create(team=self.team, filters={"date_from": None}) - filters_hash_no_dashboard = generate_insight_cache_key(insight, None) - filters_hash_with_null_date_from = generate_insight_cache_key(insight, dashboard) + filters_hash_no_dashboard = generate_insight_filters_hash(insight, None) + filters_hash_with_null_date_from = generate_insight_filters_hash(insight, dashboard) assert filters_hash_no_dashboard == filters_hash_with_null_date_from @@ -94,8 +94,8 @@ def test_dashboard_with_date_from_changes_filters_hash(self) -> None: insight = Insight.objects.create(team=self.team, filters={"date_from": "-30d"}) dashboard = Dashboard.objects.create(team=self.team, filters={"date_from": "-90d"}) - filters_hash_one = generate_insight_cache_key(insight, None) - filters_hash_two = generate_insight_cache_key(insight, dashboard) + filters_hash_one = generate_insight_filters_hash(insight, None) + filters_hash_two = generate_insight_filters_hash(insight, dashboard) assert filters_hash_one != filters_hash_two From 95cc8beef79b05010872e7150edc4588c666d264 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Thu, 23 May 2024 22:51:57 +0200 Subject: [PATCH 19/19] Update test_should_refresh_insight.py --- posthog/caching/test/test_should_refresh_insight.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/posthog/caching/test/test_should_refresh_insight.py b/posthog/caching/test/test_should_refresh_insight.py index b46be50b2c86f..9eb5c2ae94214 100644 --- a/posthog/caching/test/test_should_refresh_insight.py +++ b/posthog/caching/test/test_should_refresh_insight.py @@ -122,8 +122,8 @@ def test_insights_with_ranges_lower_than_7_days_can_be_refreshed_more_often(self def test_dashboard_filters_should_override_insight_filters_when_deciding_on_refresh_time(self): insight, _, dashboard_tile = _create_insight( self.team, - {"events": [{"id": "$pageview"}], "interval": "month"}, - {"interval": "hour"}, + {"events": [{"id": "$pageview"}], "date_from": "-30d"}, + {"date_from": "-3d"}, ) should_refresh_now, refresh_frequency = should_refresh_insight(