-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(aci): Retry process_delayed_workflows timeouts #95379
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 |
---|---|---|
|
@@ -65,6 +65,7 @@ | |
) | ||
from sentry.workflow_engine.processors.workflow_fire_history import create_workflow_fire_histories | ||
from sentry.workflow_engine.tasks.actions import build_trigger_action_task_params, trigger_action | ||
from sentry.workflow_engine.tasks.utils import retry_timeouts | ||
from sentry.workflow_engine.types import DataConditionHandler, WorkflowEventData | ||
from sentry.workflow_engine.utils import log_context | ||
|
||
|
@@ -760,6 +761,7 @@ def repr_keys[T, V](d: dict[T, V]) -> dict[str, V]: | |
), | ||
) | ||
@retry | ||
@retry_timeouts | ||
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. any thoughts on some convenience decorator to group the 2 retries together. something like probably will increase adoption. there are a lot of ecosystem tasks i would use this for. 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. follow-up coming soon. |
||
@log_context.root() | ||
def process_delayed_workflows( | ||
project_id: int, batch_key: str | None = None, *args: Any, **kwargs: Any | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
from functools import wraps | ||
|
||
import sentry_sdk | ||
from google.api_core.exceptions import DeadlineExceeded, RetryError, ServiceUnavailable | ||
|
||
from sentry import nodestore | ||
|
@@ -6,6 +9,8 @@ | |
from sentry.issues.issue_occurrence import IssueOccurrence | ||
from sentry.models.environment import Environment | ||
from sentry.models.group import Group | ||
from sentry.taskworker.retry import retry_task | ||
from sentry.taskworker.workerchild import ProcessingDeadlineExceeded | ||
from sentry.types.activity import ActivityType | ||
from sentry.utils.retries import ConditionalRetryPolicy, exponential_delay | ||
from sentry.workflow_engine.types import WorkflowEventData | ||
|
@@ -82,3 +87,20 @@ def build_workflow_event_data_from_event( | |
has_escalated=has_escalated, | ||
workflow_env=workflow_env, | ||
) | ||
|
||
|
||
def retry_timeouts(func): | ||
""" | ||
Schedule a task retry if the function raises ProcessingDeadlineExceeded. | ||
This exists because the standard retry decorator doesn't allow BaseExceptions. | ||
""" | ||
Comment on lines
+92
to
+96
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. If you find yourself using this frequently, we could introduce deadline approaching exceptions that happen some time before the processing deadline is crossed. |
||
|
||
@wraps(func) | ||
def wrapper(*args, **kwargs): | ||
try: | ||
return func(*args, **kwargs) | ||
except ProcessingDeadlineExceeded: | ||
sentry_sdk.capture_exception(level="info") | ||
retry_task() | ||
|
||
return wrapper |
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.
qq: why did we have to remove
return
from here?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.
Because I redefined
retry_task
as officially never returning as far as the type system is concerned.So, this
return
was kinda incorrect, and due to my change mypy was able to flag it.