-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(issues): Replace 'Internal Error' message on Postgres query timeout errors #87691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,11 @@ | |
import unittest | ||
from unittest.mock import MagicMock, patch | ||
|
||
import psycopg2 | ||
import pytest | ||
from django.db import OperationalError | ||
from django.utils import timezone | ||
from rest_framework.exceptions import APIException | ||
from rest_framework.exceptions import APIException, Throttled | ||
from sentry_sdk import Scope | ||
|
||
from sentry.api.utils import ( | ||
|
@@ -17,6 +19,7 @@ | |
from sentry.exceptions import IncompatibleMetricsQuery, InvalidParams, InvalidSearchQuery | ||
from sentry.testutils.cases import APITestCase | ||
from sentry.testutils.helpers.datetime import freeze_time | ||
from sentry.testutils.helpers.options import override_options | ||
from sentry.utils.snuba import ( | ||
DatasetSelectionError, | ||
QueryConnectionFailed, | ||
|
@@ -168,13 +171,12 @@ class FooBarError(Exception): | |
pass | ||
|
||
|
||
class HandleQueryErrorsTest: | ||
class HandleQueryErrorsTest(APITestCase): | ||
@patch("sentry.api.utils.ParseError") | ||
def test_handle_query_errors(self, mock_parse_error): | ||
exceptions = [ | ||
DatasetSelectionError, | ||
IncompatibleMetricsQuery, | ||
InvalidParams, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. InvalidParams is not being converted in the actual code, and fails since now I actually run this test. From what I could tell InvalidParams is actually correctly not being converted to a user error since it is sometimes indeed an 'Internal Error' so I removed it from this test (which didnt run until now). |
||
InvalidSearchQuery, | ||
QueryConnectionFailed, | ||
QueryExecutionError, | ||
|
@@ -198,6 +200,42 @@ def test_handle_query_errors(self, mock_parse_error): | |
except Exception as e: | ||
assert isinstance(e, (FooBarError, APIException)) | ||
|
||
def test_handle_postgres_timeout(self): | ||
class TimeoutError(OperationalError): | ||
pgcode = psycopg2.errorcodes.QUERY_CANCELED | ||
|
||
# Test when option is disabled (default) | ||
try: | ||
with handle_query_errors(): | ||
raise TimeoutError() | ||
except Exception as e: | ||
assert isinstance(e, TimeoutError) # Should propagate original error | ||
|
||
# Test when option is enabled | ||
with override_options({"api.postgres-query-timeout-error-handling.enabled": True}): | ||
try: | ||
with handle_query_errors(): | ||
raise TimeoutError() | ||
except Exception as e: | ||
assert isinstance(e, Throttled) | ||
assert ( | ||
str(e) | ||
== "Query timeout. Please try with a smaller date range or fewer conditions." | ||
) | ||
|
||
@patch("sentry.api.utils.ParseError") | ||
def test_handle_other_operational_error(self, mock_parse_error): | ||
class OtherError(OperationalError): | ||
# No pgcode attribute | ||
pass | ||
|
||
try: | ||
with handle_query_errors(): | ||
raise OtherError() | ||
except Exception as e: | ||
assert isinstance(e, OtherError) # Should propagate original error | ||
mock_parse_error.assert_not_called() | ||
|
||
|
||
class ClampDateRangeTest(unittest.TestCase): | ||
def test_no_clamp_if_range_under_max(self): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this test wasn't running at all from what I can tell since it didnt inherit APITestCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!