Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions frontend/src/queries/nodes/InsightViz/InsightVizDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,24 @@ function DashboardInsightRefreshHintOrLoading({
/>
)
}
return <InsightRefreshDataHint onRetry={onRetry} />
return <InsightRefreshDataHint onRetry={onRetry} {...refreshHintContext(insightProps)} />
}

function refreshHintContext(insightProps: InsightLogicProps): {
dashboardId: number | null
insightShortId: string | null
hasOverrides: boolean
} {
const { dashboardItemId } = insightProps
return {
dashboardId: insightProps.dashboardId ?? null,
insightShortId: typeof dashboardItemId === 'string' ? dashboardItemId : null,
hasOverrides: !!(
insightProps.filtersOverride ||
insightProps.variablesOverride ||
insightProps.tileFiltersOverride
),
}
}

/** Dashboard tile: show refresh when merged `result` is still nullish (empty success is `[]`, not `null`). */
Expand Down Expand Up @@ -304,7 +321,7 @@ export function InsightVizDisplay({
/>
)
}
return <InsightRefreshDataHint onRetry={onRetry} />
return <InsightRefreshDataHint onRetry={onRetry} {...refreshHintContext(insightProps)} />
}

if (activeView === InsightType.FUNNELS && !isFlowViz) {
Expand Down
21 changes: 20 additions & 1 deletion frontend/src/scenes/insights/EmptyStates/EmptyStates.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,26 @@ export function InsightEmptyState({
}

/** Shown when the chart area would otherwise be blank (e.g. cache miss + aborted refresh). */
export function InsightRefreshDataHint({ onRetry }: { onRetry: () => void }): JSX.Element {
export function InsightRefreshDataHint({
onRetry,
dashboardId,
insightShortId,
hasOverrides,
}: {
onRetry: () => void
dashboardId?: number | null
insightShortId?: string | null
hasOverrides?: boolean
}): JSX.Element {
// The hint used to be silent, so a blank tile was only ever visible in session replay.
useOnMountEffect(() => {
posthog.capture('insight refresh hint shown', {
dashboard_id: dashboardId ?? null,
insight_short_id: insightShortId ?? null,
has_overrides: !!hasOverrides,
})
})

return (
<div
data-attr="insight-refresh-data-hint"
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/scenes/insights/InsightSceneHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ export function InsightSceneHeader({ insightLogicProps }: InsightSceneHeaderProp
<LemonBanner type="warning" className="mb-4">
<div className="flex flex-row items-center justify-between gap-2">
<span>
You are viewing this insight with filter/variable overrides. Discard them to edit the
insight.
You're viewing this insight with a dashboard's filters applied, so you can't edit it.
Discard them to go back to the saved insight.
</span>

<LemonButton type="secondary" to={urls.insightView(insightId as InsightShortId)}>
Expand Down
23 changes: 23 additions & 0 deletions posthog/hogql_queries/refresh_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,21 @@ class ComputeSurface(StrEnum):
}


# Client-supplied overrides (dashboard filters, variables, tile filters) are merged into the query
# before it is fingerprinted, so the overridden variant has a cache key of its own that no scheduled
# refresh ever warms. Reading it cache-only returns `result: null`, which the UI renders as a
# dead-end "Chart data didn't load" tile.
OVERRIDE_QUERY_PARAMS = ("filters_override", "variables_override", "tile_filters_override")

# What an override-carrying request falls back to instead of cache-only: serve the cache when it is
# warm, refresh in the background when stale, and compute synchronously only on a genuine miss.
OVERRIDE_MISS_EXECUTION_MODE = ExecutionMode.RECENT_CACHE_CALCULATE_ASYNC_IF_STALE_AND_BLOCKING_ON_MISS


def _client_overrides_present(request: "Request") -> bool:
return any(request.query_params.get(param) for param in OVERRIDE_QUERY_PARAMS)


def _refresh_param_present(request: "Request") -> bool:
"""Whether the client sent a `refresh` param at all (query string or body), regardless of its
value — so an explicit `refresh=false` is distinguished from an absent param."""
Expand Down Expand Up @@ -87,6 +102,14 @@ def resolve_execution_mode(
execution_mode = execution_mode_from_refresh(refresh_requested_by_client(request))
else:
execution_mode = SURFACE_DEFAULT_EXECUTION_MODE[surface]
# Overrides are ignored on shared/embedded resources, so their cache key is unaffected there
# and anonymous demand must never force a recompute.
if (
not is_shared
and execution_mode == ExecutionMode.CACHE_ONLY_NEVER_CALCULATE
and _client_overrides_present(request)
):
execution_mode = OVERRIDE_MISS_EXECUTION_MODE

if is_shared:
return shared_insights_execution_mode(execution_mode)
Expand Down
30 changes: 28 additions & 2 deletions posthog/hogql_queries/test/test_refresh_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@
from posthog.hogql_queries.refresh_policy import ComputeSurface, resolve_execution_mode


def _request(refresh: str | None = None) -> Request:
drf_request = cast(Request, Request(HttpRequest()))
def _request(refresh: str | None = None, **query_params: str) -> Request:
http_request = HttpRequest()
for key, value in query_params.items():
http_request.GET[key] = value
drf_request = cast(Request, Request(http_request))
drf_request._full_data = {"refresh": refresh} if refresh is not None else {} # type: ignore[attr-defined]
return drf_request

Expand Down Expand Up @@ -59,6 +62,29 @@ def test_explicit_refresh_false_stays_cache_only_when_surface_default_is_flipped
false_mode, _ = resolve_execution_mode(_request("false"), surface=ComputeSurface.DASHBOARD_TILE)
assert false_mode == ExecutionMode.CACHE_ONLY_NEVER_CALCULATE

@parameterized.expand([("filters_override",), ("variables_override",), ("tile_filters_override",)])
def test_client_overrides_escalate_past_cache_only(self, param: str) -> None:
# Overrides change the cache key, so nothing warms it — cache-only would hand the UI
# result: null and a "Chart data didn't load" tile.
for surface in ComputeSurface:
mode, _ = resolve_execution_mode(_request(**{param: '{"date_from":"-14d"}'}), surface=surface)
assert mode == ExecutionMode.RECENT_CACHE_CALCULATE_ASYNC_IF_STALE_AND_BLOCKING_ON_MISS, surface

def test_overrides_do_not_escalate_an_explicit_opt_out(self) -> None:
mode, _ = resolve_execution_mode(
_request("force_cache", filters_override='{"date_from":"-14d"}'), surface=ComputeSurface.DASHBOARD_TILE
)
assert mode == ExecutionMode.CACHE_ONLY_NEVER_CALCULATE

def test_overrides_do_not_change_the_shared_path(self) -> None:
# Shared/embedded rendering ignores client overrides entirely, so its cache key — and its
# execution mode — must be identical with and without them.
without = resolve_execution_mode(_request(), surface=ComputeSurface.SHARED, is_shared=True)
with_override = resolve_execution_mode(
_request(filters_override='{"date_from":"-14d"}'), surface=ComputeSurface.SHARED, is_shared=True
)
assert with_override == without

def test_shared_flag_applies_the_clamp(self) -> None:
# resolve_execution_mode's own responsibility is to route through the shared clamp iff
# is_shared (the exhaustive clamp mapping is covered in test_query_runner). Contrast the
Expand Down
Loading