Skip to content

Commit

Permalink
fix(slo): Track exception class name on errors (#4981)
Browse files Browse the repository at this point in the history
Add the exception class name to the metrics we emit on failure, so we can get a
better idea of what the actual bugs are. get_request_status didn't seem to be
correctly classifying invalid requests, so also change that.
  • Loading branch information
evanh authored Nov 6, 2023
1 parent 662fade commit 6165f0a
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 7 deletions.
20 changes: 16 additions & 4 deletions snuba/querylog/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from __future__ import annotations

from random import random
from typing import Any, Mapping, Optional, Union

Expand Down Expand Up @@ -172,34 +174,43 @@ def _add_tags(


def record_invalid_request(
timer: Timer, request_status: Status, referrer: Optional[str]
timer: Timer,
request_status: Status,
referrer: Optional[str],
exception_name: str | None = None,
) -> None:
"""
Records a failed request before the request object is created, so
it records failures during parsing/validation.
This is for client errors.
"""
_record_failure_building_request(
QueryStatus.INVALID_REQUEST, request_status, timer, referrer
QueryStatus.INVALID_REQUEST, request_status, timer, referrer, exception_name
)


def record_error_building_request(
timer: Timer, request_status: Status, referrer: Optional[str]
timer: Timer,
request_status: Status,
referrer: Optional[str],
exception_name: str | None = None,
) -> None:
"""
Records a failed request before the request object is created, so
it records failures during parsing/validation.
This is for system errors during parsing/validation.
"""
_record_failure_building_request(QueryStatus.ERROR, request_status, timer, referrer)
_record_failure_building_request(
QueryStatus.ERROR, request_status, timer, referrer, exception_name
)


def _record_failure_building_request(
status: QueryStatus,
request_status: Status,
timer: Timer,
referrer: Optional[str],
exception_name: str | None = None,
) -> None:
# TODO: Revisit if recording some data for these queries in the querylog
# table would be useful.
Expand All @@ -211,6 +222,7 @@ def _record_failure_building_request(
"referrer": referrer or "none",
"request_status": request_status.status.value,
"slo": request_status.slo.value,
"exception": exception_name or "none",
},
)
_add_tags(timer)
6 changes: 5 additions & 1 deletion snuba/querylog/query_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@

from snuba.clickhouse.errors import ClickhouseError
from snuba.datasets.storage import StorageNotAvailable
from snuba.query.exceptions import InvalidQueryException
from snuba.request import Request
from snuba.request.exceptions import InvalidJsonRequestException
from snuba.state.cache.abstract import ExecutionTimeoutError
from snuba.state.rate_limit import TABLE_RATE_LIMIT_NAME, RateLimitExceeded
from snuba.utils.metrics.timer import Timer
Expand Down Expand Up @@ -123,7 +125,9 @@ def get_request_status(cause: Exception | None = None) -> Status:
slo_status = RequestStatus.CACHE_SET_TIMEOUT
elif isinstance(cause, ExecutionTimeoutError):
slo_status = RequestStatus.CACHE_WAIT_TIMEOUT
elif isinstance(cause, StorageNotAvailable):
elif isinstance(
cause, (StorageNotAvailable, InvalidJsonRequestException, InvalidQueryException)
):
slo_status = RequestStatus.INVALID_REQUEST
else:
slo_status = RequestStatus.ERROR
Expand Down
8 changes: 6 additions & 2 deletions snuba/request/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,15 @@ def build_request(
)
except (InvalidJsonRequestException, InvalidQueryException) as exception:
request_status = get_request_status(exception)
record_invalid_request(timer, request_status, referrer)
record_invalid_request(
timer, request_status, referrer, str(type(exception).__name__)
)
raise exception
except Exception as exception:
request_status = get_request_status(exception)
record_error_building_request(timer, request_status, referrer)
record_error_building_request(
timer, request_status, referrer, str(type(exception).__name__)
)
raise exception

span.set_data(
Expand Down

0 comments on commit 6165f0a

Please sign in to comment.