feat(hogql): tag user-supplied hogql in queries + prometheus label#60214
Conversation
Adds a `contains_user_hogql` field to `QueryTags` that's serialized into ClickHouse `system.query_log.log_comment`. It lights up whenever a query includes a HogQL string that originated from the end user — full SQL editor queries, HogQL property filters, custom `math_hogql`/`breakdown` expressions, `EventsQuery` select/where/orderBy strings, `funnelAggregateByHogQL`, `pathsHogQLExpression`, and the field-expression strings on `DataWarehouseNode` / `ExperimentDataWarehouseNode`. Triage motivation: when ClickHouse returns an error, this flag separates queries we built end-to-end (likely platform bugs) from queries that embed user-authored HogQL (likely user input issues). The tag is set at the canonical parse sites where user strings hit `parse_expr`/`parse_select`. Generated-By: PostHog Code Task-Id: 0d95bc6d-f72d-4511-ad52-408b5fcea5a5
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/hogql_queries/events_query_runner.py:208-211
**Unconditional tag causes false positives in internal callers**
`tag_contains_user_hogql()` fires every time `to_query()` is called, but `EventsQueryRunner` is also invoked by platform code that constructs `EventsQuery` programmatically. For example, `hogql_cohort_query.py` (line 200–233) builds `EventsQuery(select=["person_id", "count()"], where=[f"count() >= {count}"])` entirely in platform code and calls `events_query_runner.to_query()` directly. The resulting ClickHouse query will carry `contains_user_hogql: true` even though no user-supplied HogQL is involved, so any platform error from that path will be mis-attributed as a user error — exactly the opposite of what the feature is meant to prevent.
Moving the call to `_calculate()` instead of `to_query()` would fix the false positive for callers that use `to_query()` as a sub-query helper, since those callers never invoke `_calculate()`.
Reviews (1): Last reviewed commit: "feat(hogql): tag clickhouse queries that..." | Re-trigger Greptile |
Two follow-ups on top of the initial commit:
1. Ruff import-sort failures (CI: Python code quality). Sorted the new
`tag_contains_user_hogql` imports into the project's own
`posthog.clickhouse.*` block in `posthog/hogql/property.py`,
`posthog/hogql_queries/experiments/base_query_utils.py`,
`posthog/hogql_queries/experiments/metric_source.py`,
`posthog/hogql_queries/insights/trends/breakdown.py`, and the inline
import inside the new test in `posthog/clickhouse/test/test_query_tagging.py`.
2. Greptile flagged that tagging in `EventsQueryRunner.to_query()` causes
false positives for platform callers that use it as a sub-query helper
(e.g. `hogql_cohort_query.py` builds `EventsQuery(select=["person_id",
"count()"], where=[f"count() >= {count}"])` from platform constants and
calls `to_query()` directly, never `_calculate()`). Moved the
`tag_contains_user_hogql()` call to `_calculate()` so it only fires when
the runner is the top-level execution path — which is exactly where
`EventsQuery.select / where / orderBy` came from the end user.
Generated-By: PostHog Code
Task-Id: 0d95bc6d-f72d-4511-ad52-408b5fcea5a5
sampennington
left a comment
There was a problem hiding this comment.
🤖 Automated review
Verdict: Needs discussion
No correctness or security issues — this is observability-only and the helper is well-shaped. Main concerns:
- Coverage symmetry. A grep over
parse_expr(user_field)surfaces ~3-4 sites in other files that look identical to the ones tagged here (funnels-over-DW, customer-analytics usage metrics,ActorsQuery.select/orderBy, possiblySessionsQuery). Either tag them too or document why they're out of scope. - Performance.
tag_contains_user_hogql()callstag_queries(...)which does amodel_copy(deep=True)of a ~100-fieldQueryTags. Several call sites (@propertymethods inlifecycle_query_runner.py, the breakdown loops, recursiveproperty_to_expr) re-fire on every access. One-line idempotency short-circuit in the helper fixes everything at once. - Comment/docstring duplication. The field-level comment and the helper docstring enumerate the same caller list in two places; both will rot.
Good call already on moving the EventsQuery tag into _calculate() instead of to_query() — the comment explaining the hogql_cohort_query.py sub-query pitfall is exactly the kind of "why" worth keeping.
Non-anchored findings
Missed parse sites (Should fix or document):
posthog/hogql_queries/insights/funnels/funnel_event_query.py:~252—parse_expr(table_entity.aggregation_target_field)is the same DW-field pattern that's tagged inlifecycle_query_runner.py(timestamp_field/target_field/created_at_field). Asymmetric with the rest of the PR.products/customer_analytics/backend/hogql_queries/usage_metrics_query_runner.py:~218—parse_expr(timestamp_field)fromGroupUsageMetric.Source.DATA_WAREHOUSE. Same shape.posthog/hogql_queries/actors_query_runner.py:~349 / :~404—parse_expr(self.query.select)andparse_order_expr(self.query.orderBy).ActorsQueryis the persons-explorer sibling ofEventsQuery(which is tagged); leaving it out is inconsistent.posthog/hogql_queries/sessions_query_runner.py:~137/~301/~527— sameselect/where/orderBypattern forSessionsQuery. If that query node is user-facing, should be tagged; if not, worth a one-line comment ruling it out.
Architectural alternative (Suggestion): posthog/hogql/parser.py already has a CacheOrigin.USER classification that property.py even passes explicitly. Setting contains_user_hogql=True inside _parse_cached whenever cache_origin resolves to USER would replace all 12 scattered call sites with one source of truth, and would automatically catch the missed sites above (and any future ones). Sibling tags like has_joins / has_json_operations already follow this "derive centrally" pattern in HogQLQuery._execute_clickhouse_query. Heavier lift than this PR — fine as a follow-up.
Reviewers: code-reviewer, personal, reuse, quality, efficiency. Inline comments above are tagged with the reviewer that raised them and the level of concern.
…rcuit Addresses the PR-author review (coverage symmetry, performance, comment duplication) — no logic changes to the helper or how the tag is consumed, just wider, cheaper, and de-duplicated. Coverage symmetry — tag the remaining user-HogQL parse sites that mirror ones already tagged: - `posthog/hogql_queries/insights/funnels/funnel_event_query.py` — `parse_expr(table_entity.aggregation_target_field)` for funnels over a DataWarehouseNode (same shape as the lifecycle DW field tags). - `products/customer_analytics/backend/hogql_queries/usage_metrics_query_runner.py` — `timestamp_field`, `math_property`, and `key_field` parses from `GroupUsageMetric` DW sources. - `posthog/hogql_queries/actors_query_runner.py` — `ActorsQuery.select` / `orderBy`. Tag in `_calculate()` (same reason as `EventsQueryRunner`: `hogql_cohort_query._actors_query_from_source` calls `to_query()` with a platform-constant `select=["id"]` and must not false-positive). - `posthog/hogql_queries/sessions_query_runner.py` — `SessionsQuery.select` / `where` / `orderBy`. Performance — `tag_contains_user_hogql()` now short-circuits when the flag is already set, avoiding the `model_copy(deep=True)` inside `tag_queries` on every subsequent call. Matters for hot paths: recursive `property_to_expr`, breakdown loops, `@property` accessors in `lifecycle_query_runner.py`. New test verifies the short-circuit returns the same `QueryTags` object. Comment dedupe — the caller list lived in two places (the field-level comment on `contains_user_hogql` and the helper docstring) and would rot on every new site. Kept the canonical list in `tag_contains_user_hogql` and shortened the field comment to point at the helper. Generated-By: PostHog Code Task-Id: 0d95bc6d-f72d-4511-ad52-408b5fcea5a5
- Shorten contains_user_hogql field comment and helper docstring to one line — the enumeration drifted vs the actual call sites. - breakdown.py: flatten the duplicated `if breakdown_type == "hogql"` branch so the tag call isn't visually buried. - metric_source.py: add a one-liner naming the user-HogQL field on ExperimentDataWarehouseNode, since the tag isn't beside its parse_expr call. - test_query_tagging.py: drop narrating comments above the two contains_user_hogql tests — the test names say it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Quick follow-up on the coverage symmetry concern from the "Needs discussion" verdict. The four sites flagged as look-alike gaps are all already tagged on HEAD — looks like the verdict was based on the initial commit (
I also swept the other plausible call sites — So the coverage-symmetry concern is resolved on HEAD. The other two concerns from the verdict (perf early-return + comment/docstring duplication) are also addressed: early-return at |
A failed query carries a ClickHouse error code that cannot say whether the user (a bad HogQL expression they wrote) or our query builder (structured input, but invalid SQL generated) caused it. The existing `category` label lumps both together. This labels the `posthog_query_execution_total` counter with `has_user_authored_hogql` so observability can target builder bugs precisely: `category="error", has_user_authored_hogql="false"` is a high-confidence builder-bug signal. The label reads `QueryTags.contains_user_hogql` — the canonical flag set by `tag_contains_user_hogql()` at every HogQL parse site (see `posthog.clickhouse.query_tagging`). Reusing that flag keeps the Prometheus and `system.query_log` surfaces in lockstep and inherits the broader site coverage (events query select/where/orderBy, paths expression, lifecycle warehouse fields, experiment warehouse fields) without a separate detector to maintain. Stacked on the user-HogQL tagging work in PR #60214. Generated-By: PostHog Code Task-Id: a72d30b8-cbf7-4b97-834f-212596a51d6e
Rename the Prometheus label on `posthog_query_execution_total` from `has_user_authored_hogql` to `contains_user_hogql` so it matches the `QueryTags.contains_user_hogql` field that already lands in `system.query_log.log_comment`. One name everywhere makes log-vs-metric triage queries copy-paste cleanly. Generated-By: PostHog Code Task-Id: 3664bd5c-c410-40e4-966f-a051ad518227
c3ebc65 to
3f5b962
Compare
…nflict Drops the prom-label additions from this file so the GitHub merges API can fast-merge master in cleanly. Re-applied after the merge. Generated-By: PostHog Code Task-Id: 3664bd5c-c410-40e4-966f-a051ad518227
Restores the `contains_user_hogql` Prometheus label and `_contains_user_hogql_label()` helper that were temporarily removed to let the GitHub merges API auto-merge master in. Generated-By: PostHog Code Task-Id: 3664bd5c-c410-40e4-966f-a051ad518227
Problem
When ClickHouse returns an error, we have no easy way to separate failures we caused (a platform bug in how we built the SQL) from failures the user caused (a syntax error or bad column reference in HogQL they wrote). The
query_typetag tells us a query came fromHogQLQueryRunner, but it doesn't surface user HogQL embedded inside otherwise platform-generated queries —math_hogqlon a trend, anHogQLPropertyFilteron a funnel, apathsHogQLExpression, etc.Same distinction is missing from real-time observability: the
posthog_query_execution_totalPrometheus counter labels failures ascategory="user_error"but lumps user-HogQL failures together with structured-input builder bugs.Changes
Adds a
contains_user_hogql: boolfield toQueryTags(serialized intosystem.query_log.log_comment) and a small helpertag_contains_user_hogql()that flips it. The helper is called at every canonical site where a user-controlled HogQL string is handed toparse_expr/parse_order_expr/parse_select:HogQLQuery.query(SQL editor)posthog/hogql_queries/hogql_query_runner.pyHogQLPropertyFilter.keyposthog/hogql/property.pyEventsNode.math_hogqlposthog/hogql_queries/insights/trends/aggregation_operations.pyBreakdownFilter.breakdown(whenbreakdown_type='hogql')posthog/hogql_queries/insights/trends/breakdown.pyEventsQuery.select / where / orderByposthog/hogql_queries/events_query_runner.pyFunnelsFilter.funnelAggregateByHogQLposthog/hogql_queries/insights/funnels/funnel_event_query.py, paths runnerPathsFilter.pathsHogQLExpressionproducts/product_analytics/backend/hogql_queries/paths/paths_query_runner.pyLifecycleDataWarehouseNode.{timestamp,aggregation_target,created_at}_fieldposthog/hogql_queries/insights/lifecycle/lifecycle_query_runner.pyExperimentDataWarehouseNode.{data_warehouse_join_key, math_property}and experimentmath_hogqlposthog/hogql_queries/experiments/metric_source.py,base_query_utils.pyThe flag is optional and excluded from JSON when unset, so platform-only queries are unaffected. Once landed, log triage queries like
become possible.
Also adds a
has_user_authored_hogqllabel to theposthog_query_execution_totalPrometheus counter, sourced from the sameQueryTags.contains_user_hogqlflag. This lets observability target builder bugs precisely in real time —category="error", has_user_authored_hogql="false"is a high-confidence builder-bug signal — without a separate detector to maintain. (Folds in the work from the now-closed #58715.)How did you test this code?
I'm an agent. I did not run tests in this environment (no Python venv with project deps was available). I did:
python3 -c "import ast; ast.parse(open(...).read())"TestQueryTaggingSourceInQueryLogconfirming the flag lands insystem.query_logfor a HogQL query and is absent for a platform-builtsync_executetest_query_runner.pyconfirming the Prometheus label reflects the QueryTags flag for both success and error pathsPlease run
hogli test posthog/clickhouse/test/test_query_tagging.py posthog/hogql_queries/test/test_query_runner.pybefore merge.Publish to changelog?
no
🤖 Agent context
parse_expr. Considered usingcache_origin=CacheOrigin.USERas the trigger (since the parse cache already classifies user vs builtin), but only two sites currently passCacheOrigin.USER; making it implicit would either miss sites or require touching all of them anyway. Explicittag_contains_user_hogql()calls are grep-able and conservative.user_hogql_kinds: list[str]to attribute by which fragment failed (filter vs breakdown vs math), the helper is the one place to extend.breakdown_type == "event_metadata": that branch parses the property name string, but the name comes from a constrained taxonomy picker, not free-form HogQL.query.model_dump()schema-side. Folded in here because the Prometheus andsystem.query_logsurfaces need to stay in lockstep and the QueryTags flag is a strictly better detection source (covers more sites: events query select/where/orderBy, paths expression, lifecycle/experiment warehouse fields — all missed by the schema walker).