Skip to content

Commit

Permalink
fix(insights): Do NOT calculate non-cacheables when cache-only (#21823)
Browse files Browse the repository at this point in the history
* fix(insights): Do NOT calculate non-cacheables when cache-only

* Fix circular import

* Cover all queries outside of `QUERY_WITH_RUNNER_USING_CACHE`

* Use the right `CacheMissResponse`

* Satisfy mypy

* Don't simplify caching key

* Fix string Python considers unterminated
  • Loading branch information
Twixes authored Apr 24, 2024
1 parent f6681fc commit af5b138
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 13 deletions.
13 changes: 10 additions & 3 deletions posthog/api/services/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from posthog.hogql.autocomplete import get_hogql_autocomplete
from posthog.hogql.metadata import get_hogql_metadata
from posthog.hogql.modifiers import create_default_modifiers_for_team
from posthog.hogql_queries.query_runner import ExecutionMode, get_query_runner
from posthog.hogql_queries.query_runner import CacheMissResponse, ExecutionMode, get_query_runner
from posthog.models import Team
from posthog.queries.time_to_see_data.serializers import SessionEventsQuerySerializer, SessionsQuerySerializer
from posthog.queries.time_to_see_data.sessions import get_session_events, get_sessions
Expand Down Expand Up @@ -41,7 +41,7 @@

logger = structlog.get_logger(__name__)

QUERY_WITH_RUNNER = (
QUERY_WITH_RUNNER_USING_CACHE = (
TrendsQuery
| FunnelsQuery
| RetentionQuery
Expand Down Expand Up @@ -82,10 +82,17 @@ def process_query_model(
) -> dict:
result: dict | BaseModel

if isinstance(query, QUERY_WITH_RUNNER): # type: ignore
if execution_mode == ExecutionMode.CACHE_ONLY_NEVER_CALCULATE and not isinstance(
query,
QUERY_WITH_RUNNER_USING_CACHE, # type: ignore
):
result = CacheMissResponse(cache_key=None)

if isinstance(query, QUERY_WITH_RUNNER_USING_CACHE): # type: ignore
query_runner = get_query_runner(query, team, limit_context=limit_context)
result = query_runner.run(execution_mode=execution_mode)
elif isinstance(query, QUERY_WITH_RUNNER_NO_CACHE): # type: ignore
# TODO: These queries should be using the QueryRunner caching layer too
query_runner = get_query_runner(query, team, limit_context=limit_context)
result = query_runner.calculate()
elif isinstance(query, HogQLAutocomplete):
Expand Down
3 changes: 1 addition & 2 deletions posthog/caching/calculate_results.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, Union

from posthog.api.services.query import ExecutionMode
import structlog
from sentry_sdk import capture_exception

Expand Down Expand Up @@ -110,7 +109,7 @@ def get_cache_type(cacheable: Optional[FilterType] | Optional[Dict]) -> CacheTyp
def calculate_for_query_based_insight(
insight: Insight, *, dashboard: Optional[Dashboard] = None, refresh_requested: bool
) -> "InsightResult":
from posthog.api.services.query import process_query
from posthog.api.services.query import process_query, ExecutionMode
from posthog.caching.fetch_from_cache import InsightResult, NothingInCacheResult

tag_queries(team_id=insight.team_id, insight_id=insight.pk)
Expand Down
7 changes: 4 additions & 3 deletions posthog/hogql_queries/query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class CacheMissResponse(BaseModel):
model_config = ConfigDict(
extra="forbid",
)
cache_key: str
cache_key: Optional[str]


RunnableQueryNode = Union[
Expand Down Expand Up @@ -329,7 +329,8 @@ def calculate(self) -> BaseModel:
def run(
self, execution_mode: ExecutionMode = ExecutionMode.RECENT_CACHE_CALCULATE_IF_STALE
) -> CachedQueryResponse | CacheMissResponse:
cache_key = f"{self._cache_key()}_{self.limit_context or LimitContext.QUERY}"
# TODO: `self.limit_context` should probably just be in get_cache_key()
cache_key = cache_key = f"{self.get_cache_key()}_{self.limit_context or LimitContext.QUERY}"
tag_queries(cache_key=cache_key)

if execution_mode != ExecutionMode.CALCULATION_ALWAYS:
Expand Down Expand Up @@ -406,7 +407,7 @@ def to_hogql(self) -> str:
def toJSON(self) -> str:
return self.query.model_dump_json(exclude_defaults=True, exclude_none=True)

def _cache_key(self) -> str:
def get_cache_key(self) -> str:
modifiers = self.modifiers.model_dump_json(exclude_defaults=True, exclude_none=True)
return generate_cache_key(
f"query_{self.toJSON()}_{self.__class__.__name__}_{self.team.pk}_{self.team.timezone}_{modifiers}"
Expand Down
6 changes: 3 additions & 3 deletions posthog/hogql_queries/test/test_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def test_cache_key(self):

runner = TestQueryRunner(query={"some_attr": "bla"}, team=team)

cache_key = runner._cache_key()
cache_key = runner.get_cache_key()
self.assertEqual(cache_key, "cache_b6f14c97c218e0b9c9a8258f7460fd5b")

def test_cache_key_runner_subclass(self):
Expand All @@ -108,7 +108,7 @@ class TestSubclassQueryRunner(TestQueryRunner):

runner = TestSubclassQueryRunner(query={"some_attr": "bla"}, team=team)

cache_key = runner._cache_key()
cache_key = runner.get_cache_key()
self.assertEqual(cache_key, "cache_ec1c2f9715cf9c424b1284b94b1205e6")

def test_cache_key_different_timezone(self):
Expand All @@ -119,7 +119,7 @@ def test_cache_key_different_timezone(self):

runner = TestQueryRunner(query={"some_attr": "bla"}, team=team)

cache_key = runner._cache_key()
cache_key = runner.get_cache_key()
self.assertEqual(cache_key, "cache_a6614c0fb564f9c98b1d7b830928c7a1")

def test_cache_response(self):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,8 @@ def _unsample(self, n: Optional[int | float]):
else n / self._sample_rate.numerator
)

def _cache_key(self) -> str:
original = super()._cache_key()
def get_cache_key(self) -> str:
original = super().get_cache_key()
return f"{original}_{self.team.path_cleaning_filters}"


Expand Down

0 comments on commit af5b138

Please sign in to comment.