Skip to content
Merged
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
33 changes: 32 additions & 1 deletion products/experiments/backend/hogql_queries/error_handling.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@
from posthog.hogql.errors import ExposedHogQLError, InternalHogQLError

from posthog.clickhouse.client.limit import ConcurrencyLimitExceeded
from posthog.errors import ExposedCHQueryError, QueryErrorCategory, look_up_clickhouse_error_code_meta
from posthog.errors import (
CHQueryErrorNotAnAggregate,
ExposedCHQueryError,
QueryErrorCategory,
look_up_clickhouse_error_code_meta,
)
from posthog.event_usage import groups
from posthog.exceptions import (
ClickHouseAtCapacity,
Expand Down Expand Up @@ -114,6 +119,10 @@ def classify_experiment_query_error(error: Exception) -> str:
return "rate_limited"
if isinstance(error, ServerException):
meta = look_up_clickhouse_error_code_meta(error)
if meta.name == "NOT_AN_AGGREGATE":
# Only known producer in experiment queries is user-authored HogQL referencing a
# row-level column outside an aggregate — a metric-config error, not a platform one.
return "validation_error"
if meta.name in ("TIMEOUT_EXCEEDED", "SOCKET_TIMEOUT"):
return "timeout"
if meta.name == "MEMORY_LIMIT_EXCEEDED":
Expand Down Expand Up @@ -225,6 +234,28 @@ def wrapper(*args, **kwargs):
# Still terminal user pain (the metric fails to load every time), so still counted.
_emit_runner_terminal_error_event(args[0] if args else None, e)
raise
except CHQueryErrorNotAnAggregate as e:
# Only known producer in experiment queries is user-authored HogQL (math_hogql /
# warehouse math_property) referencing a row-level column outside an aggregate —
# builder-generated SQL is snapshot-tested, and every tracked occurrence carried
# contains_user_hogql. A metric-config error: give an actionable message, keep it
# out of error tracking, and let classify_experiment_query_error's validation_error
# mapping make the recalculation worker fail it permanently instead of retrying.
self = args[0] if args else None
logger.warning(
"Experiment metric HogQL references column outside aggregate",
experiment_id=getattr(self, "experiment_id", None),
error_message=str(e),
)
user_error = ValidationError(
"This metric's HogQL expression references an event column outside an aggregate "
"function. Wrap the value in an aggregation, e.g. sum(properties.value)."
)
_emit_runner_terminal_error_event(self, user_error)
if self is not None and not getattr(self, "user_facing", True):
# Internal callers (recalculation, timeseries) expect raw types and classify themselves.
raise
raise user_error from e
except Exception as e:
# Get context for logging
self = args[0] if args else None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from posthog.hogql.errors import ExposedHogQLError

from posthog.clickhouse.client.limit import ConcurrencyLimitExceeded
from posthog.errors import CHQueryErrorNotAnAggregate
from posthog.exceptions import ClickHouseAtCapacity, ClickHouseQueryMemoryLimitExceeded, ClickHouseQueryTimeOut

from products.experiments.backend.hogql_queries.error_handling import (
Expand Down Expand Up @@ -146,6 +147,52 @@ def failing_method(self):
# Should still capture for internal tracking
mock_capture.assert_called_once()

@patch("products.experiments.backend.hogql_queries.error_handling.capture_exception")
def test_decorator_converts_not_an_aggregate_without_capture(self, mock_capture):
# User-authored HogQL referencing a column outside an aggregate is a metric-config
# error: actionable ValidationError, no error-tracking capture.

@experiment_error_handler
def failing_method(self):
raise CHQueryErrorNotAnAggregate(
"Column 'events__override.distinct_id' is not under aggregate function",
code=215,
code_name="not_an_aggregate",
)

mock_self = Mock()
mock_self.experiment = Mock(id=123)
mock_self.metric = None
mock_self.user_facing = True

with self.assertRaises(ValidationError) as context:
failing_method(mock_self)

detail_list = cast(list[ErrorDetail], context.exception.detail)
self.assertIn("outside an aggregate", str(detail_list[0]))
mock_capture.assert_not_called()

@patch("products.experiments.backend.hogql_queries.error_handling.capture_exception")
def test_decorator_reraises_not_an_aggregate_raw_for_non_user_facing(self, mock_capture):
# Internal callers (recalculation, timeseries) classify raw types themselves —
# classify_experiment_query_error maps this to validation_error (permanent, no retry).

@experiment_error_handler
def failing_method(self):
raise CHQueryErrorNotAnAggregate(
"Column 'timestamp' is not under aggregate function", code=215, code_name="not_an_aggregate"
)

mock_self = Mock()
mock_self.experiment = Mock(id=123)
mock_self.metric = None
mock_self.user_facing = False

with self.assertRaises(CHQueryErrorNotAnAggregate):
failing_method(mock_self)

mock_capture.assert_not_called()

def test_error_type_to_code_mapping(self):
"""Test that ClickHouseQueryMemoryLimitExceeded has a code mapping."""
self.assertIn(ClickHouseQueryMemoryLimitExceeded, ERROR_TYPE_TO_CODE)
Expand All @@ -165,6 +212,12 @@ def test_error_type_to_code_mapping(self):
("zero_division", ZeroDivisionError(), "insufficient_data"),
("validation_error", ValidationError("bad metric config"), "validation_error"),
("exposed_hogql_error", ExposedHogQLError("unknown property"), "validation_error"),
("ch_not_an_aggregate_code", ServerException("not under aggregate", code=215), "validation_error"),
(
"wrapped_not_an_aggregate",
CHQueryErrorNotAnAggregate("not under aggregate", code=215, code_name="not_an_aggregate"),
"validation_error",
),
("anything_else", RuntimeError("kaboom"), "server_error"),
]
)
Expand Down
19 changes: 11 additions & 8 deletions products/experiments/backend/temporal/recalculation_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -822,14 +822,17 @@ def _calculate_experiment_metric_for_recalculation_sync(
message = str(e)[:_MAX_ERROR_MESSAGE_LENGTH]
error_type = classify_experiment_query_error(e)
is_permanent = error_type in NON_RETRYABLE_ERROR_TYPES or isinstance(e, ValueError)
capture_exception(
e,
additional_properties={
"experiment_id": experiment_id,
"metric_uuid": metric_uuid,
"recalculation_id": recalculation_id,
},
)
# validation_error = the user's metric config is broken (e.g. HogQL referencing a
# column outside an aggregate) — a stored failure, not a platform error to track.
if error_type != "validation_error":
capture_exception(
e,
additional_properties={
"experiment_id": experiment_id,
"metric_uuid": metric_uuid,
"recalculation_id": recalculation_id,
},
)
if is_final_attempt or is_permanent:
_store_result(
experiment_id=experiment_id,
Expand Down
Loading