From 6876dd8e789500664632a87b8981769da4eb4bf8 Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Tue, 30 Apr 2024 16:29:29 +0100 Subject: [PATCH] fix(debug): View errors in /debug for trend queries (#21977) * View errors in /debug for trend queries * Fixed ts error * Fixed tests * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (2) * Update UI snapshots for `chromium` (1) * Update UI snapshots for `chromium` (2) --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- frontend/src/queries/schema.json | 72 ++++++++++++++++++- frontend/src/queries/schema.ts | 4 +- frontend/src/scenes/debug/DebugScene.tsx | 4 +- frontend/src/scenes/debug/DebugSceneQuery.tsx | 7 +- frontend/src/scenes/debug/HogQLDebug.tsx | 2 +- frontend/src/scenes/debug/QueryTabs.tsx | 18 ++++- .../insights/trends/trends_query_runner.py | 7 +- posthog/hogql_queries/query_runner.py | 8 ++- posthog/schema.py | 66 ++++++++++++++++- 9 files changed, 174 insertions(+), 14 deletions(-) diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index 2595e33fe8f8b..1dfc2cc6530dc 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -2336,6 +2336,10 @@ "FunnelsQueryResponse": { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -2720,7 +2724,7 @@ "type": "array" }, "error": { - "description": "Query error. Returned only if 'explain' is true. Throws an error otherwise.", + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", "type": "string" }, "explain": { @@ -3275,6 +3279,10 @@ "LifecycleQueryResponse": { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -3574,6 +3582,10 @@ "PathsQueryResponse": { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -3808,6 +3820,10 @@ "QueryResponse": { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -4119,7 +4135,7 @@ "type": "array" }, "error": { - "description": "Query error. Returned only if 'explain' is true. Throws an error otherwise.", + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", "type": "string" }, "explain": { @@ -4238,6 +4254,10 @@ { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -4279,6 +4299,10 @@ "items": {}, "type": "array" }, + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hasMore": { "type": "boolean" }, @@ -4331,6 +4355,10 @@ "items": {}, "type": "array" }, + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -4370,6 +4398,10 @@ { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -4404,6 +4436,10 @@ { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -4438,6 +4474,10 @@ { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -4472,6 +4512,10 @@ { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -4871,6 +4915,10 @@ "RetentionQueryResponse": { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -5268,6 +5316,10 @@ "StickinessQueryResponse": { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -5605,6 +5657,10 @@ "TrendsQueryResponse": { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -5790,6 +5846,10 @@ "WebOverviewQueryResponse": { "additionalProperties": false, "properties": { + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, @@ -5905,6 +5965,10 @@ "items": {}, "type": "array" }, + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hasMore": { "type": "boolean" }, @@ -5995,6 +6059,10 @@ "items": {}, "type": "array" }, + "error": { + "description": "Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + "type": "string" + }, "hogql": { "type": "string" }, diff --git a/frontend/src/queries/schema.ts b/frontend/src/queries/schema.ts index c716913253e99..c1d0c35c7067b 100644 --- a/frontend/src/queries/schema.ts +++ b/frontend/src/queries/schema.ts @@ -209,7 +209,7 @@ export interface HogQLQueryResponse { clickhouse?: string /** Query results */ results?: any[] - /** Query error. Returned only if 'explain' is true. Throws an error otherwise. */ + /** Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise. */ error?: string /** Returned columns */ columns?: any[] @@ -896,6 +896,8 @@ export interface QueryResponse { results: unknown timings?: QueryTiming[] hogql?: string + /** Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise. */ + error?: string is_cached?: boolean last_refresh?: string next_allowed_client_refresh?: string diff --git a/frontend/src/scenes/debug/DebugScene.tsx b/frontend/src/scenes/debug/DebugScene.tsx index 9742920c4af44..829dc2031b3ea 100644 --- a/frontend/src/scenes/debug/DebugScene.tsx +++ b/frontend/src/scenes/debug/DebugScene.tsx @@ -60,11 +60,11 @@ export function DebugScene(): JSX.Element { />
- +
{query2 ? (
- +
) : null}
diff --git a/frontend/src/scenes/debug/DebugSceneQuery.tsx b/frontend/src/scenes/debug/DebugSceneQuery.tsx index ae0fccd2d0d0d..1f4b3db89ea1c 100644 --- a/frontend/src/scenes/debug/DebugSceneQuery.tsx +++ b/frontend/src/scenes/debug/DebugSceneQuery.tsx @@ -4,15 +4,17 @@ import { Modifiers } from 'scenes/debug/Modifiers' import { QueryTabs } from 'scenes/debug/QueryTabs' import { dataNodeLogic, DataNodeLogicProps } from '~/queries/nodes/DataNode/dataNodeLogic' +import { insightVizDataNodeKey } from '~/queries/nodes/InsightViz/InsightViz' import { QueryEditor } from '~/queries/QueryEditor/QueryEditor' import { DataNode, HogQLQuery, Node } from '~/queries/schema' import { isDataTableNode, isInsightVizNode } from '~/queries/utils' interface DebugSceneQueryProps { - queryKey: string + queryKey: `new-${string}` query: string setQuery: (query: string) => void } + export function DebugSceneQuery({ query, setQuery, queryKey }: DebugSceneQueryProps): JSX.Element { let parsed: Record | null = null try { @@ -23,9 +25,10 @@ export function DebugSceneQuery({ query, setQuery, queryKey }: DebugSceneQueryPr const dataNode = parsed && (isInsightVizNode(parsed as Node) || isDataTableNode(parsed as Node)) ? parsed.source : parsed + const dataNodeKey = insightVizDataNodeKey({ dashboardItemId: queryKey }) const dataNodeLogicProps: DataNodeLogicProps = { query: dataNode as DataNode, - key: queryKey, + key: dataNodeKey, dataNodeCollectionId: queryKey, modifiers: { debug: true }, } diff --git a/frontend/src/scenes/debug/HogQLDebug.tsx b/frontend/src/scenes/debug/HogQLDebug.tsx index dab62f85b68cb..b2dcb340c1d45 100644 --- a/frontend/src/scenes/debug/HogQLDebug.tsx +++ b/frontend/src/scenes/debug/HogQLDebug.tsx @@ -13,7 +13,7 @@ import { DataNode, HogQLQuery, HogQLQueryResponse } from '~/queries/schema' import { QueryTabs } from './QueryTabs' interface HogQLDebugProps { - queryKey: string + queryKey: `new-${string}` query: HogQLQuery setQuery: (query: DataNode) => void } diff --git a/frontend/src/scenes/debug/QueryTabs.tsx b/frontend/src/scenes/debug/QueryTabs.tsx index a4185467c1f79..26beac0ef3eff 100644 --- a/frontend/src/scenes/debug/QueryTabs.tsx +++ b/frontend/src/scenes/debug/QueryTabs.tsx @@ -35,7 +35,7 @@ function toColumn(hogql: string, position: number): number { } interface QueryTabsProps { query: Node - queryKey: string + queryKey: `new-${string}` response?: Record | null setQuery: (query: DataNode) => void } @@ -62,7 +62,21 @@ export function QueryTabs({ query, queryKey, setQuery, response }: QueryTabsProp isInsightVizNode(query) && { key: 'viz', label: 'Visualization', - content: setQuery(query)} />, + content: ( + setQuery(query)} + context={{ + insightProps: { + dashboardItemId: queryKey, + query, + setQuery: (query) => setQuery(query), + dataNodeCollectionId: queryKey, + }, + }} + /> + ), }, isInsightQueryNode(query) && { key: 'insight', diff --git a/posthog/hogql_queries/insights/trends/trends_query_runner.py b/posthog/hogql_queries/insights/trends/trends_query_runner.py index 4425c4c88f580..42f1654da71fd 100644 --- a/posthog/hogql_queries/insights/trends/trends_query_runner.py +++ b/posthog/hogql_queries/insights/trends/trends_query_runner.py @@ -292,6 +292,7 @@ def calculate(self): res_matrix: list[list[Any] | Any | None] = [None] * len(queries) timings_matrix: list[list[QueryTiming] | None] = [None] * len(queries) errors: list[Exception] = [] + debug_errors: list[str] = [] def run(index: int, query: ast.SelectQuery | ast.SelectUnionQuery, is_parallel: bool): try: @@ -308,6 +309,8 @@ def run(index: int, query: ast.SelectQuery | ast.SelectUnionQuery, is_parallel: timings_matrix[index] = response.timings res_matrix[index] = self.build_series_response(response, series_with_extra, len(queries)) + if response.error: + debug_errors.append(response.error) except Exception as e: errors.append(e) finally: @@ -362,7 +365,9 @@ def run(index: int, query: ast.SelectQuery | ast.SelectUnionQuery, is_parallel: with self.timings.measure("apply_formula"): res = self.apply_formula(self.query.trendsFilter.formula, res) - return TrendsQueryResponse(results=res, timings=timings, hogql=response_hogql, modifiers=self.modifiers) + return TrendsQueryResponse( + results=res, timings=timings, hogql=response_hogql, modifiers=self.modifiers, error=". ".join(debug_errors) + ) def build_series_response(self, response: HogQLQueryResponse, series: SeriesWithExtras, series_count: int): if response.results is None: diff --git a/posthog/hogql_queries/query_runner.py b/posthog/hogql_queries/query_runner.py index babac93129ef2..02e2f18593453 100644 --- a/posthog/hogql_queries/query_runner.py +++ b/posthog/hogql_queries/query_runner.py @@ -83,6 +83,7 @@ class QueryResponse(BaseModel, Generic[DataT]): timings: Optional[list[QueryTiming]] = None types: Optional[list[Union[tuple[str, str], str]]] = None columns: Optional[list[str]] = None + error: Optional[str] = None hogql: Optional[str] = None hasMore: Optional[bool] = None limit: Optional[int] = None @@ -397,7 +398,12 @@ def run( fresh_response_dict["cache_key"] = cache_key fresh_response_dict["timezone"] = self.team.timezone fresh_response = CachedQueryResponse(**fresh_response_dict) - cache.set(cache_key, fresh_response, settings.CACHED_RESULTS_TTL) + + # Dont cache debug queries with errors + has_error = fresh_response_dict.get("error", None) + if has_error is None or len(has_error) == 0: + cache.set(cache_key, fresh_response, settings.CACHED_RESULTS_TTL) + QUERY_CACHE_WRITE_COUNTER.labels(team_id=self.team.pk).inc() return fresh_response diff --git a/posthog/schema.py b/posthog/schema.py index 6cbdda24026cc..655b41f356bfd 100644 --- a/posthog/schema.py +++ b/posthog/schema.py @@ -858,6 +858,10 @@ class StickinessQueryResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -952,6 +956,10 @@ class TrendsQueryResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1016,6 +1024,10 @@ class WebOverviewQueryResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1050,6 +1062,10 @@ class WebStatsTableQueryResponse(BaseModel): extra="forbid", ) columns: Optional[list] = None + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hasMore: Optional[bool] = None hogql: Optional[str] = None is_cached: Optional[bool] = None @@ -1069,6 +1085,10 @@ class WebTopClicksQueryResponse(BaseModel): extra="forbid", ) columns: Optional[list] = None + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1256,6 +1276,10 @@ class FunnelsQueryResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1318,7 +1342,8 @@ class HogQLQueryResponse(BaseModel): clickhouse: Optional[str] = Field(default=None, description="Executed ClickHouse query") columns: Optional[list] = Field(default=None, description="Returned columns") error: Optional[str] = Field( - default=None, description="Query error. Returned only if 'explain' is true. Throws an error otherwise." + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", ) explain: Optional[list[str]] = Field(default=None, description="Query explanation output") hasMore: Optional[bool] = None @@ -1368,6 +1393,10 @@ class LifecycleQueryResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1388,6 +1417,10 @@ class PathsQueryResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1412,6 +1445,10 @@ class QueryResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1469,7 +1506,8 @@ class QueryResponseAlternative7(BaseModel): clickhouse: Optional[str] = Field(default=None, description="Executed ClickHouse query") columns: Optional[list] = Field(default=None, description="Returned columns") error: Optional[str] = Field( - default=None, description="Query error. Returned only if 'explain' is true. Throws an error otherwise." + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", ) explain: Optional[list[str]] = Field(default=None, description="Query explanation output") hasMore: Optional[bool] = None @@ -1503,6 +1541,10 @@ class QueryResponseAlternative10(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1518,6 +1560,10 @@ class QueryResponseAlternative11(BaseModel): extra="forbid", ) columns: Optional[list] = None + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hasMore: Optional[bool] = None hogql: Optional[str] = None is_cached: Optional[bool] = None @@ -1537,6 +1583,10 @@ class QueryResponseAlternative12(BaseModel): extra="forbid", ) columns: Optional[list] = None + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -1552,6 +1602,10 @@ class QueryResponseAlternative13(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -2309,6 +2363,10 @@ class QueryResponseAlternative14(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None @@ -2366,6 +2424,10 @@ class RetentionQueryResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + error: Optional[str] = Field( + default=None, + description="Query error. Returned only if 'explain' or `modifiers.debug` is true. Throws an error otherwise.", + ) hogql: Optional[str] = None is_cached: Optional[bool] = None last_refresh: Optional[str] = None