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
4 changes: 2 additions & 2 deletions src/sentry/notifications/notification_action/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
IntegrationConfigurationError,
IntegrationFormError,
)
from sentry.taskworker.retry import RetryError
from sentry.taskworker.retry import RetryTaskError
from sentry.taskworker.workerchild import ProcessingDeadlineExceeded
from sentry.types.activity import ActivityType
from sentry.types.rules import RuleFuture
Expand Down Expand Up @@ -97,7 +97,7 @@ def invoke_future_with_error_handling(
# monitor and potentially retry this action.
raise
except RETRYABLE_EXCEPTIONS as e:
raise RetryError from e
raise RetryTaskError from e
except Exception as e:
# This is just a redefinition of the safe_execute util function, as we
# still want to report any unhandled exceptions.
Expand Down
34 changes: 23 additions & 11 deletions src/sentry/tasks/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

from sentry.taskworker.constants import CompressionType
from sentry.taskworker.registry import TaskNamespace
from sentry.taskworker.retry import Retry, RetryError, retry_task
from sentry.taskworker.retry import Retry, RetryTaskError, retry_task
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the variable name makes it easier to search for this.

from sentry.taskworker.state import current_task
from sentry.taskworker.task import P, R, Task
from sentry.taskworker.workerchild import ProcessingDeadlineExceeded
Expand Down Expand Up @@ -123,10 +123,24 @@ def retry(
>>> def my_task():
>>> ...

If timeouts is True, task timeout exceptions will trigger a retry.
If it is False, timeout exceptions will behave as specified by the other parameters.
"""
The first set of parameters define how different exceptions are handled.
Raising an error will still report a Sentry event.

| Parameter | Retry | Report | Raise | Description |
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As reference, this renders like this within my IDE:

Image

|--------------------|-------|--------|-------|-------------|
| on | Yes | Yes | No | Exceptions that will trigger a retry & report to Sentry. |
| on_silent | Yes | No | No | Exceptions that will trigger a retry but not be captured to Sentry. |
| exclude | No | No | Yes | Exceptions that will not trigger a retry and will be raised. |
| ignore | No | No | No | Exceptions that will be ignored and not trigger a retry & not report to Sentry. |
| ignore_and_capture | No | Yes | No | Exceptions that will not trigger a retry and will be captured to Sentry. |

The following modifiers modify the behavior of the retry decorator.

| Modifier | Description |
|------------------------|-------------|
| timeouts | ProcessingDeadlineExceeded trigger a retry. |
| raise_on_no_retries | Makes a RetryTaskError not be raised if no retries are left. |
"""
if func:
return retry()(func)

Expand All @@ -138,18 +152,16 @@ def retry(
def inner(func):
@functools.wraps(func)
def wrapped(*args, **kwargs):
task_state = current_task()
no_retries_remaining = task_state and not task_state.retries_remaining
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting these variables here is to simplify a follow-up PR I have.

try:
return func(*args, **kwargs)
except ignore:
return
except RetryError:
if (
not raise_on_no_retries
and (task_state := current_task())
and not task_state.retries_remaining
):
except RetryTaskError:
if not raise_on_no_retries and no_retries_remaining:
return
# If we haven't been asked to ignore no-retries, pass along the RetryError.
# If we haven't been asked to ignore no-retries, pass along the RetryTaskError.
raise
except timeout_exceptions:
if timeouts:
Expand Down
10 changes: 5 additions & 5 deletions src/sentry/taskworker/retry.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@
from sentry.utils import metrics


class RetryError(Exception):
class RetryTaskError(Exception):
"""
Exception that tasks can raise to indicate that the current task activation
should be retried.
"""


class NoRetriesRemainingError(RetryError):
class NoRetriesRemainingError(RetryTaskError):
"""
Exception that is raised by retry helper methods to signal to tasks that
the current attempt is terminal and there won't be any further retries.
Expand Down Expand Up @@ -53,7 +53,7 @@ def retry_task(exc: Exception | None = None, raise_on_no_retries: bool = True) -
raise NoRetriesRemainingError()
else:
return
raise RetryError()
raise RetryTaskError()


class Retry:
Expand Down Expand Up @@ -84,8 +84,8 @@ def should_retry(self, state: RetryState, exc: Exception) -> bool:
if self.max_attempts_reached(state):
return False

# Explicit RetryError with attempts left.
if isinstance(exc, RetryError):
# Explicit RetryTaskError with attempts left.
if isinstance(exc, RetryTaskError):
return True

# No retries for types on the ignore list
Expand Down
8 changes: 4 additions & 4 deletions src/sentry/taskworker/tasks/examples.py
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From here on, there's only renaming of the variable across the codebase.

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from sentry.taskworker.constants import CompressionType
from sentry.taskworker.namespaces import exampletasks
from sentry.taskworker.retry import LastAction, NoRetriesRemainingError, Retry, RetryError
from sentry.taskworker.retry import LastAction, NoRetriesRemainingError, Retry, RetryTaskError
from sentry.taskworker.retry import retry_task as retry_task_helper
from sentry.utils.redis import redis_clusters

Expand All @@ -22,7 +22,7 @@ def say_hello(name: str, *args: list[Any], **kwargs: dict[str, Any]) -> None:
name="examples.retry_deadletter", retry=Retry(times=2, times_exceeded=LastAction.Deadletter)
)
def retry_deadletter() -> None:
raise RetryError
raise RetryTaskError


@exampletasks.register(
Expand All @@ -43,7 +43,7 @@ def retry_state() -> None:
def will_retry(failure: str) -> None:
if failure == "retry":
logger.debug("going to retry with explicit retry error")
raise RetryError
raise RetryTaskError
if failure == "raise":
logger.debug("raising runtimeerror")
raise RuntimeError("oh no")
Expand Down Expand Up @@ -71,7 +71,7 @@ def simple_task_wait_delivery() -> None:

@exampletasks.register(name="examples.retry_task", retry=Retry(times=2))
def retry_task() -> None:
raise RetryError
raise RetryTaskError


@exampletasks.register(name="examples.fail_task")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from sentry.integrations.types import EventLifecycleOutcome
from sentry.models.repository import Repository
from sentry.silo.base import SiloMode
from sentry.taskworker.retry import RetryError
from sentry.taskworker.retry import RetryTaskError
from sentry.testutils.asserts import assert_failure_metric, assert_halt_metric, assert_slo_metric
from sentry.testutils.cases import IntegrationTestCase
from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
Expand Down Expand Up @@ -194,7 +194,7 @@ def test_link_all_repos_api_error(self, mock_record: MagicMock, _: MagicMock) ->
status=400,
)

with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
link_all_repos(
integration_key=self.key,
integration_id=self.integration.id,
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/tasks/test_create_comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from sentry.integrations.tasks import create_comment
from sentry.integrations.types import EventLifecycleOutcome
from sentry.models.activity import Activity
from sentry.taskworker.retry import RetryError
from sentry.taskworker.retry import RetryTaskError
from sentry.testutils.asserts import assert_failure_metric, assert_slo_metric
from sentry.testutils.cases import TestCase
from sentry.testutils.silo import assume_test_silo_mode_of
Expand Down Expand Up @@ -157,7 +157,7 @@ def test_create_comment_failure(
integration=self.example_integration,
)

with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
create_comment(external_issue.id, self.user.id, self.activity.id)

assert_failure_metric(mock_record_event, Exception("Something went wrong creating comment"))
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from sentry.integrations.tasks import sync_assignee_outbound
from sentry.integrations.types import EventLifecycleOutcome
from sentry.shared_integrations.exceptions import IntegrationConfigurationError
from sentry.taskworker.retry import RetryError
from sentry.taskworker.retry import RetryTaskError
from sentry.testutils.asserts import assert_halt_metric, assert_success_metric
from sentry.testutils.cases import TestCase
from sentry.testutils.silo import assume_test_silo_mode_of
Expand Down Expand Up @@ -72,7 +72,7 @@ def test_sync_failure(
) -> None:
mock_sync_assignee.side_effect = raise_sync_assignee_exception

with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
sync_assignee_outbound(self.external_issue.id, self.user.id, True, None)

mock_record_failure.assert_called_once()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from sentry.integrations.tasks import sync_status_outbound
from sentry.integrations.types import EventLifecycleOutcome
from sentry.shared_integrations.exceptions import ApiUnauthorized, IntegrationFormError
from sentry.taskworker.retry import RetryError
from sentry.taskworker.retry import RetryTaskError
from sentry.testutils.asserts import assert_count_of_metric, assert_halt_metric
from sentry.testutils.cases import TestCase
from sentry.testutils.silo import assume_test_silo_mode_of, region_silo_test
Expand Down Expand Up @@ -111,7 +111,7 @@ def test_failed_sync(
group=self.group, key="foo_integration", integration=self.example_integration
)

with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
sync_status_outbound(self.group.id, external_issue_id=external_issue.id)

assert mock_record_failure.call_count == 1
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/integrations/tasks/test_update_comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from sentry.integrations.tasks import update_comment
from sentry.integrations.types import EventLifecycleOutcome
from sentry.models.activity import Activity
from sentry.taskworker.retry import RetryError
from sentry.taskworker.retry import RetryTaskError
from sentry.testutils.asserts import assert_failure_metric, assert_slo_metric
from sentry.testutils.cases import TestCase
from sentry.testutils.silo import assume_test_silo_mode_of
Expand Down Expand Up @@ -150,7 +150,7 @@ def test_update_comment_failure(
integration=self.example_integration,
)

with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
update_comment(external_issue.id, self.user.id, self.activity.id)

assert_failure_metric(mock_record_event, Exception("Something went wrong updating comment"))
Original file line number Diff line number Diff line change
Expand Up @@ -895,11 +895,11 @@ def test_reraises_processing_deadline_exceeded(self):
def test_raises_retry_error_for_api_error(self):
from sentry.notifications.notification_action.types import invoke_future_with_error_handling
from sentry.shared_integrations.exceptions import ApiError
from sentry.taskworker.retry import RetryError
from sentry.taskworker.retry import RetryTaskError

self.mock_callback.side_effect = ApiError("API error", 500)

with pytest.raises(RetryError) as excinfo:
with pytest.raises(RetryTaskError) as excinfo:
invoke_future_with_error_handling(
self.event_data, self.mock_callback, self.mock_futures
)
Expand Down
12 changes: 6 additions & 6 deletions tests/sentry/tasks/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from sentry.taskworker.constants import CompressionType
from sentry.taskworker.namespaces import exampletasks, test_tasks
from sentry.taskworker.registry import TaskRegistry
from sentry.taskworker.retry import Retry, RetryError
from sentry.taskworker.retry import Retry, RetryTaskError
from sentry.taskworker.state import CurrentTaskState
from sentry.taskworker.workerchild import ProcessingDeadlineExceeded

Expand Down Expand Up @@ -159,7 +159,7 @@ def test_exclude_exception_retry(capture_exception: MagicMock) -> None:
@override_settings(SILO_MODE=SiloMode.CONTROL)
@patch("sentry_sdk.capture_exception")
def test_retry_on(capture_exception: MagicMock) -> None:
with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
retry_on_task("bruh")

assert capture_exception.call_count == 1
Expand All @@ -186,7 +186,7 @@ def test_retry_timeout_enabled_taskbroker(capture_exception) -> None:
def timeout_retry_task():
raise ProcessingDeadlineExceeded()

with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
timeout_retry_task()

assert capture_exception.call_count == 1
Expand All @@ -213,7 +213,7 @@ def test_retry_timeout_enabled(capture_exception) -> None:
def soft_timeout_retry_task():
raise ProcessingDeadlineExceeded()

with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
soft_timeout_retry_task()

assert capture_exception.call_count == 1
Expand Down Expand Up @@ -265,13 +265,13 @@ def test_retry_raise_if_no_retries_false(mock_current_task):

@retry(on=(Exception,), raise_on_no_retries=False)
def task_that_raises_retry_error():
raise RetryError("try again")
raise RetryTaskError("try again")

# No exception.
task_that_raises_retry_error()

mock_task_state.retries_remaining = True
with pytest.raises(RetryError):
with pytest.raises(RetryTaskError):
task_that_raises_retry_error()


Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/taskworker/test_retry.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
ON_ATTEMPTS_EXCEEDED_DISCARD,
)

from sentry.taskworker.retry import LastAction, Retry, RetryError
from sentry.taskworker.retry import LastAction, Retry, RetryTaskError


class RuntimeChildError(RuntimeError):
Expand Down Expand Up @@ -64,7 +64,7 @@ def test_should_retry_retryerror() -> None:
retry = Retry(times=5)
state = retry.initial_state()

err = RetryError("something bad")
err = RetryTaskError("something bad")
assert retry.should_retry(state, err)

state.attempts = 4
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/taskworker/test_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
)

from sentry.taskworker.registry import TaskNamespace
from sentry.taskworker.retry import LastAction, Retry, RetryError
from sentry.taskworker.retry import LastAction, Retry, RetryTaskError
from sentry.taskworker.router import DefaultRouter
from sentry.taskworker.task import Task
from sentry.testutils.helpers.task_runner import TaskRunner
Expand Down Expand Up @@ -147,7 +147,7 @@ def test_should_retry(task_namespace: TaskNamespace) -> None:
namespace=task_namespace,
retry=retry,
)
err = RetryError("try again plz")
err = RetryTaskError("try again plz")
assert task.should_retry(state, err)

state.attempts = 3
Expand Down
Loading