Skip to content

Conversation

ceorourke
Copy link
Member

@ceorourke ceorourke commented Aug 14, 2025

#97697 but it's behind a feature flag this time so we can toggle it on and off, plus converts incident projects to a list rather than passing a queryset. See this commit for the only change in this from the original implementation.

@ceorourke ceorourke requested review from a team as code owners August 14, 2025 23:11
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 14, 2025
Comment on lines +59 to +60
alert_id = AlertRuleDetector.objects.get(detector_id=detector_id).alert_rule_id
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential bug: The call to AlertRuleDetector.objects.get() lacks error handling, which will cause a server crash if the object does not exist, breaking issue ingestion.
  • Description: The method create_from_occurrence calls AlertRuleDetector.objects.get(detector_id=detector_id) without handling the AlertRuleDetector.DoesNotExist exception. The detector_id value, derived from occurrence.evidence_data, can be missing or invalid. Other parts of the codebase, such as in src/sentry/incidents/grouptype.py, consistently wrap similar queries in try...except blocks, establishing a pattern that this is a known and expected failure case. An unhandled DoesNotExist exception will propagate up the call stack, causing a server crash during the critical issue ingestion process.

  • Suggested fix: Wrap the AlertRuleDetector.objects.get() call in a try...except AlertRuleDetector.DoesNotExist block. In the except block, log a warning and handle the case gracefully, for example by setting alert_id to None and allowing execution to continue. This matches established patterns elsewhere in the codebase.
    severity: 0.85, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link

codecov bot commented Aug 14, 2025

Codecov Report

❌ Patch coverage is 80.88235% with 13 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...workflow_engine/models/incident_groupopenperiod.py 80.35% 11 Missing ⚠️
src/sentry/incidents/logic.py 66.66% 1 Missing ⚠️
src/sentry/issues/ingest.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #97906   +/-   ##
=======================================
  Coverage   80.65%   80.65%           
=======================================
  Files        8591     8591           
  Lines      378854   378910   +56     
  Branches    24660    24660           
=======================================
+ Hits       305561   305611   +50     
- Misses      72924    72930    +6     
  Partials      369      369           

Copy link
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

flag looks good, hopefully we can learn some more about the problems by easing in traffic 🙏

@ceorourke ceorourke merged commit 9a854de into master Aug 15, 2025
67 checks passed
@ceorourke ceorourke deleted the ceorourke/write-incgroupopenperiod branch August 15, 2025 20:05
evanh pushed a commit that referenced this pull request Aug 18, 2025
#97697 but it's behind a feature
flag this time so we can toggle it on and off, plus converts incident
projects to a list rather than passing a queryset. See [this
commit](cd0b7f9)
for the only change in this from the original implementation.

---------

Co-authored-by: Snigdha Sharma <[email protected]>
priscilawebdev pushed a commit that referenced this pull request Aug 25, 2025
#97697 but it's behind a feature
flag this time so we can toggle it on and off, plus converts incident
projects to a list rather than passing a queryset. See [this
commit](cd0b7f9)
for the only change in this from the original implementation.

---------

Co-authored-by: Snigdha Sharma <[email protected]>
Copy link

sentry-io bot commented Aug 25, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a 👍 or 👎

andrewshie-sentry pushed a commit that referenced this pull request Aug 26, 2025
#97697 but it's behind a feature
flag this time so we can toggle it on and off, plus converts incident
projects to a list rather than passing a queryset. See [this
commit](cd0b7f9)
for the only change in this from the original implementation.

---------

Co-authored-by: Snigdha Sharma <[email protected]>
constantinius pushed a commit that referenced this pull request Sep 1, 2025
#97697 but it's behind a feature
flag this time so we can toggle it on and off, plus converts incident
projects to a list rather than passing a queryset. See [this
commit](cd0b7f9)
for the only change in this from the original implementation.

---------

Co-authored-by: Snigdha Sharma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants