From 66762d8ee1fa2f2aba1b347d1a13367a571bcdd8 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 26 Apr 2024 11:11:46 +0100 Subject: [PATCH 1/3] fix: hot path is cooler now --- .../queries/session_replay_events.py | 41 ++++++++++++++----- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/posthog/session_recordings/queries/session_replay_events.py b/posthog/session_recordings/queries/session_replay_events.py index 226d27154fd85..cb58bb4b18c5b 100644 --- a/posthog/session_recordings/queries/session_replay_events.py +++ b/posthog/session_recordings/queries/session_replay_events.py @@ -1,7 +1,9 @@ from datetime import datetime, timedelta from typing import Optional +import pytz from django.conf import settings +from django.core.cache import cache from posthog.clickhouse.client import sync_execute from posthog.cloud_utils import is_cloud @@ -15,26 +17,43 @@ ) +def seconds_until_midnight(): + now = datetime.now(pytz.timezone("UTC")) + midnight = (now + timedelta(days=1)).replace(hour=0, minute=0, second=0, microsecond=0) + difference = midnight - now + return difference.seconds + + class SessionReplayEvents: def exists(self, session_id: str, team: Team) -> bool: - # TODO we could cache this result when its result is True. + cache_key = f"summarize_recording_existence_team_{team.pk}_id_{session_id}" + cached_response = cache.get(cache_key) + if isinstance(cached_response, bool): + return cached_response + # Once we know that session exists we don't need to check again (until the end of the day since TTL might apply) + existence = self._check_exists_within_days(ttl_days(team), session_id, team) or self._check_exists_within_days( + 370, session_id, team + ) + + cache.set(cache_key, existence, timeout=seconds_until_midnight()) + return existence + + @staticmethod + def _check_exists_within_days(days: int, session_id: str, team: Team) -> bool: result = sync_execute( """ - SELECT count(1) + SELECT count() FROM session_replay_events - WHERE team_id = %(team_id)s + PREWHERE team_id = %(team_id)s AND session_id = %(session_id)s - -- we should check for the `ttl_days(team)` TTL here, - -- but for a shared/pinned recording - -- the TTL effectively becomes 1 year - -- and we don't know which we're dealing with - AND min_first_timestamp >= now() - INTERVAL 370 DAY + AND min_first_timestamp >= now() - INTERVAL %(days) DAY + AND min_first_timestamp <= now() """, { "team_id": team.pk, "session_id": session_id, - "recording_ttl_days": ttl_days(team), + "days": days, }, ) return result[0][0] > 0 @@ -144,7 +163,6 @@ def get_events( def ttl_days(team: Team) -> int: - ttl_days = (get_instance_setting("RECORDINGS_TTL_WEEKS") or 3) * 7 if is_cloud(): # NOTE: We use Playlists as a proxy to see if they are subbed to Recordings is_paid = team.organization.is_feature_available(AvailableFeature.RECORDINGS_PLAYLISTS) @@ -155,5 +173,6 @@ def ttl_days(team: Team) -> int: if days_since_blob_ingestion < ttl_days: ttl_days = days_since_blob_ingestion - + else: + ttl_days = (get_instance_setting("RECORDINGS_TTL_WEEKS") or 3) * 7 return ttl_days From 1ffe48bb6ddddc4225d9d92367e1e4de411d7df6 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 26 Apr 2024 11:15:08 +0100 Subject: [PATCH 2/3] fix --- posthog/session_recordings/queries/session_replay_events.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/posthog/session_recordings/queries/session_replay_events.py b/posthog/session_recordings/queries/session_replay_events.py index cb58bb4b18c5b..5284325c0aa63 100644 --- a/posthog/session_recordings/queries/session_replay_events.py +++ b/posthog/session_recordings/queries/session_replay_events.py @@ -36,7 +36,11 @@ def exists(self, session_id: str, team: Team) -> bool: 370, session_id, team ) - cache.set(cache_key, existence, timeout=seconds_until_midnight()) + if existence: + # let's be cautious and not cache non-existence + # in case we manage to check existence just before the first event hits ClickHouse + # that should be impossible but cache invalidation is hard etc etc + cache.set(cache_key, existence, timeout=seconds_until_midnight()) return existence @staticmethod From 5b23193688d88255838685b8f603a02b6a69bd4b Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Fri, 26 Apr 2024 11:40:53 +0100 Subject: [PATCH 3/3] fix --- .../queries/session_replay_events.py | 2 +- .../test/test_session_recordings.py | 70 ++++++++++--------- 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/posthog/session_recordings/queries/session_replay_events.py b/posthog/session_recordings/queries/session_replay_events.py index 5284325c0aa63..1607aad167176 100644 --- a/posthog/session_recordings/queries/session_replay_events.py +++ b/posthog/session_recordings/queries/session_replay_events.py @@ -51,7 +51,7 @@ def _check_exists_within_days(days: int, session_id: str, team: Team) -> bool: FROM session_replay_events PREWHERE team_id = %(team_id)s AND session_id = %(session_id)s - AND min_first_timestamp >= now() - INTERVAL %(days) DAY + AND min_first_timestamp >= now() - INTERVAL %(days)s DAY AND min_first_timestamp <= now() """, { diff --git a/posthog/session_recordings/test/test_session_recordings.py b/posthog/session_recordings/test/test_session_recordings.py index b8341dc3fb314..2038b39cacb2b 100644 --- a/posthog/session_recordings/test/test_session_recordings.py +++ b/posthog/session_recordings/test/test_session_recordings.py @@ -41,7 +41,7 @@ def setUp(self): # TODO this is pretty slow, we should change assertions so that we don't need it self.team = Team.objects.create(organization=self.organization, name="New Team") - def create_snapshot( + def produce_replay_summary( self, distinct_id, session_id, @@ -79,11 +79,11 @@ def test_get_session_recordings(self): base_time = (now() - relativedelta(days=1)).replace(microsecond=0) session_id_one = f"test_get_session_recordings-1" - self.create_snapshot("user_one_0", session_id_one, base_time) - self.create_snapshot("user_one_0", session_id_one, base_time + relativedelta(seconds=10)) - self.create_snapshot("user_one_0", session_id_one, base_time + relativedelta(seconds=30)) + self.produce_replay_summary("user_one_0", session_id_one, base_time) + self.produce_replay_summary("user_one_0", session_id_one, base_time + relativedelta(seconds=10)) + self.produce_replay_summary("user_one_0", session_id_one, base_time + relativedelta(seconds=30)) session_id_two = f"test_get_session_recordings-2" - self.create_snapshot("user2", session_id_two, base_time + relativedelta(seconds=20)) + self.produce_replay_summary("user2", session_id_two, base_time + relativedelta(seconds=20)) response = self.client.get(f"/api/projects/{self.team.id}/session_recordings") self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -146,11 +146,11 @@ def test_can_list_recordings_even_when_the_person_has_multiple_distinct_ids(self base_time = (now() - relativedelta(days=1)).replace(microsecond=0) session_id_one = f"test_get_session_recordings-1" - self.create_snapshot("user_one_0", session_id_one, base_time) - self.create_snapshot("user_one_1", session_id_one, base_time + relativedelta(seconds=10)) - self.create_snapshot("user_one_2", session_id_one, base_time + relativedelta(seconds=30)) + self.produce_replay_summary("user_one_0", session_id_one, base_time) + self.produce_replay_summary("user_one_1", session_id_one, base_time + relativedelta(seconds=10)) + self.produce_replay_summary("user_one_2", session_id_one, base_time + relativedelta(seconds=30)) session_id_two = f"test_get_session_recordings-2" - self.create_snapshot("user2", session_id_two, base_time + relativedelta(seconds=20)) + self.produce_replay_summary("user2", session_id_two, base_time + relativedelta(seconds=20)) response = self.client.get(f"/api/projects/{self.team.id}/session_recordings") self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -200,8 +200,8 @@ def _person_with_snapshots(self, base_time: datetime, distinct_id: str = "user", distinct_ids=[distinct_id], properties={"$some_prop": "something", "email": "bob@bob.com"}, ) - self.create_snapshot(distinct_id, session_id, base_time) - self.create_snapshot(distinct_id, session_id, base_time + relativedelta(seconds=10)) + self.produce_replay_summary(distinct_id, session_id, base_time) + self.produce_replay_summary(distinct_id, session_id, base_time + relativedelta(seconds=10)) flush_persons_and_events() def test_session_recordings_dont_leak_teams(self) -> None: @@ -218,8 +218,8 @@ def test_session_recordings_dont_leak_teams(self) -> None: ) base_time = (now() - relativedelta(days=1)).replace(microsecond=0) - self.create_snapshot("user", "1", base_time, team_id=another_team.pk) - self.create_snapshot("user", "2", base_time) + self.produce_replay_summary("user", "1", base_time, team_id=another_team.pk) + self.produce_replay_summary("user", "2", base_time) response = self.client.get(f"/api/projects/{self.team.id}/session_recordings") self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -234,8 +234,8 @@ def test_session_recording_for_user_with_multiple_distinct_ids(self) -> None: distinct_ids=["d1", "d2"], properties={"$some_prop": "something", "email": "bob@bob.com"}, ) - self.create_snapshot("d1", "1", base_time) - self.create_snapshot("d2", "2", base_time + relativedelta(seconds=30)) + self.produce_replay_summary("d1", "1", base_time) + self.produce_replay_summary("d2", "2", base_time + relativedelta(seconds=30)) response = self.client.get(f"/api/projects/{self.team.id}/session_recordings") response_data = response.json() @@ -250,8 +250,8 @@ def test_viewed_state_of_session_recording_version_1(self): ) base_time = (now() - timedelta(days=1)).replace(microsecond=0) SessionRecordingViewed.objects.create(team=self.team, user=self.user, session_id="1") - self.create_snapshot("u1", "1", base_time) - self.create_snapshot("u1", "2", base_time + relativedelta(seconds=30)) + self.produce_replay_summary("u1", "1", base_time) + self.produce_replay_summary("u1", "2", base_time + relativedelta(seconds=30)) response = self.client.get(f"/api/projects/{self.team.id}/session_recordings") response_data = response.json() self.assertEqual(len(response_data["results"]), 2) @@ -271,8 +271,8 @@ def test_viewed_state_of_session_recording_version_3(self): session_id_two = "2" SessionRecordingViewed.objects.create(team=self.team, user=self.user, session_id=session_id_one) - self.create_snapshot("u1", session_id_one, base_time) - self.create_snapshot("u1", session_id_two, base_time + relativedelta(seconds=30)) + self.produce_replay_summary("u1", session_id_one, base_time) + self.produce_replay_summary("u1", session_id_two, base_time + relativedelta(seconds=30)) response = self.client.get(f"/api/projects/{self.team.id}/session_recordings") response_data = response.json() @@ -387,7 +387,7 @@ def test_get_single_session_recording_metadata(self): def test_single_session_recording_doesnt_leak_teams(self): another_team = Team.objects.create(organization=self.organization) - self.create_snapshot( + self.produce_replay_summary( "user", "id_no_team_leaking", now() - relativedelta(days=1), @@ -433,7 +433,7 @@ def test_session_recording_doesnt_exist(self): def test_request_to_another_teams_endpoint_returns_401(self): org = Organization.objects.create(name="Separate Org") another_team = Team.objects.create(organization=org) - self.create_snapshot( + self.produce_replay_summary( "user", "id_no_team_leaking", now() - relativedelta(days=1), @@ -455,17 +455,17 @@ def test_session_ids_filter(self, use_recording_events: bool, api_version: int): distinct_ids=["user"], properties={"$some_prop": "something", "email": "bob@bob.com"}, ) - self.create_snapshot( + self.produce_replay_summary( "user", "1", now() - relativedelta(days=1), ) - self.create_snapshot( + self.produce_replay_summary( "user", "2", now() - relativedelta(days=2), ) - self.create_snapshot( + self.produce_replay_summary( "user", "3", now() - relativedelta(days=3), @@ -489,9 +489,9 @@ def test_empty_list_session_ids_filter_returns_no_recordings(self): distinct_ids=["user"], properties={"$some_prop": "something", "email": "bob@bob.com"}, ) - self.create_snapshot("user", "1", now() - relativedelta(days=1)) - self.create_snapshot("user", "2", now() - relativedelta(days=2)) - self.create_snapshot("user", "3", now() - relativedelta(days=3)) + self.produce_replay_summary("user", "1", now() - relativedelta(days=1)) + self.produce_replay_summary("user", "2", now() - relativedelta(days=2)) + self.produce_replay_summary("user", "3", now() - relativedelta(days=3)) # Fetch playlist params_string = urlencode({"session_ids": "[]"}) @@ -502,7 +502,7 @@ def test_empty_list_session_ids_filter_returns_no_recordings(self): self.assertEqual(len(response_data["results"]), 0) def test_delete_session_recording(self): - self.create_snapshot("user", "1", now() - relativedelta(days=1), team_id=self.team.pk) + self.produce_replay_summary("user", "1", now() - relativedelta(days=1), team_id=self.team.pk) response = self.client.delete(f"/api/projects/{self.team.id}/session_recordings/1") self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) # Trying to delete same recording again returns 404 @@ -514,7 +514,7 @@ def test_delete_session_recording(self): return_value=2, ) def test_persist_session_recording(self, _mock_copy_objects: MagicMock) -> None: - self.create_snapshot("user", "1", now() - relativedelta(days=1), team_id=self.team.pk) + self.produce_replay_summary("user", "1", now() - relativedelta(days=1), team_id=self.team.pk) response = self.client.get(f"/api/projects/{self.team.id}/session_recordings/1") assert response.status_code == status.HTTP_200_OK @@ -838,7 +838,7 @@ def test_get_via_sharing_token(self, mock_copy_objects: MagicMock) -> None: session_id = str(uuid.uuid4()) with freeze_time("2023-01-01T12:00:00Z"): - self.create_snapshot( + self.produce_replay_summary( "user", session_id, now() - relativedelta(days=1), @@ -877,10 +877,12 @@ def test_get_via_sharing_token(self, mock_copy_objects: MagicMock) -> None: } # now create a snapshot record that doesn't have a fixed date, as it needs to be within TTL for the request below to complete - self.create_snapshot( + self.produce_replay_summary( "user", session_id, - now(), + # a little before now, since the DB checks if the snapshot is within TTL and before now + # if the test runs too quickly it looks like the snapshot is not there + now() - relativedelta(seconds=1), team_id=self.team.pk, ) @@ -947,7 +949,7 @@ def test_get_matching_events(self) -> None: # the matching session session_id = f"test_get_matching_events-1-{uuid.uuid4()}" - self.create_snapshot("user", session_id, base_time) + self.produce_replay_summary("user", session_id, base_time) event_id = _create_event( event="$pageview", properties={"$session_id": session_id}, @@ -957,7 +959,7 @@ def test_get_matching_events(self) -> None: # a non-matching session non_matching_session_id = f"test_get_matching_events-2-{uuid.uuid4()}" - self.create_snapshot("user", non_matching_session_id, base_time) + self.produce_replay_summary("user", non_matching_session_id, base_time) _create_event( event="$pageview", properties={"$session_id": non_matching_session_id},