feat(workflow_engine): Only link workflows to the IssueStream#112276
feat(workflow_engine): Only link workflows to the IssueStream#112276saponifi3d merged 10 commits intomasterfrom
Conversation
9e8cb89 to
d1ae9bb
Compare
682bf2a to
2f3a48c
Compare
d1ae9bb to
f146ca8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
aa02f39 to
7659b1a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1783511 to
d5af62d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
014e847 to
12683dc
Compare
created a `defaults/workflows.py` so we can separate the signal handlers and the default creation code.
| if existing: | ||
| return existing | ||
|
|
||
| with transaction.atomic(router.db_for_write(Workflow)): |
There was a problem hiding this comment.
#todo - might be nice to provide a helper to create workflows in the future.
|
(still reviewing) |
kcons
left a comment
There was a problem hiding this comment.
LG, but a few notes on things that confused me.
| project=self.project, | ||
| defaults={"config": {}, "name": ERROR_DETECTOR_NAME}, | ||
| ) | ||
| AlertRuleDetector.objects.get_or_create(detector=error_detector, rule_id=self.rule.id) |
There was a problem hiding this comment.
i believe so - this code is really to setup the references on the legacy tables, and i think we'd need all these references until we are no longer dual writing in the API.
| workflow=workflow, | ||
| ) | ||
| for detector in default_detectors | ||
| if detector.type != ErrorGroupType.slug |
There was a problem hiding this comment.
maybe _create_detector_lookups shouldn't return the error detector if we're not trying to associate with it?
|
|
||
| DetectorWorkflow.objects.bulk_create( | ||
| references_to_create, | ||
| ignore_conflicts=True, |
There was a problem hiding this comment.
Crons. Crons creates all their workflows / alerts through this layer, so there may be cases where they are already created / have the connection. if i don't ignore_conflicts there's an edge case that those cause 💥
|
|
||
| error_detector_workflow = DetectorWorkflow.objects.get(detector=error_detector) | ||
| assert error_detector_workflow.workflow == workflow | ||
| assert not DetectorWorkflow.objects.filter(detector=error_detector).exists() |
There was a problem hiding this comment.
Seems like maybe this assert method can go away, or maybe needs to be renamed?
assert_issue_stream_detector_migrated covers the issue stream presence/existence, and I'm not sure we need to verify that an error detector exists and has specific values in order to check it isn't associated.
12683dc to
c986a48
Compare
| ) -> Sequence[DetectorWorkflow]: | ||
| # Because we don't know if this signal is handled already or not... | ||
| issue_stream_detector = _ensure_detector(project, IssueStreamGroupType.slug) | ||
|
|
There was a problem hiding this comment.
Bug: An UnableToAcquireLockApiError in ensure_default_workflows is silently swallowed by send_robust(), causing workflow creation to fail without any logging or alerts.
Severity: MEDIUM
Suggested Fix
Wrap the call to _ensure_detector within ensure_default_workflows in a try/except block. Explicitly catch the UnableToAcquireLockApiError, log a warning, and capture the exception to Sentry for visibility, similar to the pattern used in project_detectors.py.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/workflow_engine/defaults/workflows.py#L28-L31
Potential issue: The `ensure_default_workflows` function, called via the
`project_created` signal, can raise an `UnableToAcquireLockApiError` if it fails to
acquire a lock when creating a detector. Because the signal is dispatched using
`send_robust()`, this exception is silently swallowed without any logging or error
handling. This results in a silent failure where the default workflow is not created for
the new project, and there is no visibility into the failure, making it difficult to
debug in production.
# Description Since these workflows are connected to both the Issue Stream and the Error detector, we can remove the connection to the error detector because it's a subset of the Issue Stream. This PR will find where this redundant connection is occurring, and remove the relationship for the Error Detector, while preserving it. We don't need to make any processing changes etc, because of how these are selected in `processors/worfklow.py` This PR should only be merged after: #112276 so we don't add any new connections after migrating.
Description
Update the on project creation to only link to the issue stream detector, rather than the issue stream and error detectors.
Moves the code into workflow_engine to keep things organized.