Skip to content

Conversation

kcons
Copy link
Member

@kcons kcons commented Oct 7, 2025

No intended behavioral change here.
The aim is to prepare for the move to not requiring AlertRule to exist when single processing by removing mandatory uses of self.alert_rule in single processing-only contexts.

  • Rather than using a conditionally-defined self.alert_rule, we use a self._alert_rule: AlertRule | None, and use a property that asserts in legacy code paths. This makes tool-based validation a bit easier, and avoids some scary hasattr.
  • Pull the AlertRuleDetectionType from Detector.config rather than from alert_rule; should be identical when both exist.
  • update_alert_rule_stats/reset_trigger_counts changed to take an AlertRule argument to clarify their requirements; as we allow absent AlertRule, we'll need to make a variant of this logic for Detector-only, but that's for a follow-up.

Testing:
Existing testing should be sufficient, as no behavior is expected to change here.

@kcons kcons requested a review from mifu67 October 7, 2025 20:53
@kcons kcons requested a review from a team as a code owner October 7, 2025 20:53
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 7, 2025
cursor[bot]

This comment was marked as outdated.

Copy link
Member

@ceorourke ceorourke left a comment

Choose a reason for hiding this comment

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

left a couple


comparison_delta: int | None
detection_type: Literal["static", "percent", "dynamic"]

Copy link
Member

Choose a reason for hiding this comment

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

Detector config can also have:

sensitivity: str | None
seasonality: str | None
threshold_period: int

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, interesting. I based this on

config_schema={
"$schema": "https://json-schema.org/draft/2020-12/schema",
"description": "A representation of a metric alert firing",
"type": "object",
"required": ["detection_type"],
"properties": {
"comparison_delta": {
"type": ["integer", "null"],
"enum": COMPARISON_DELTA_CHOICES,
},
"detection_type": {
"type": "string",
"enum": [detection_type.value for detection_type in AlertRuleDetectionType],
},
},
},
and what fields were actually used here in the file. The lesson for me is that the default value of 'additionalProperties' is 'true', which I should keep in mind.

Interesting, the current db data doesn't seem to have any with seasonality, has only 4 (~0.04%) with sensitivity, and it looks like threshold_period isn't required: https://redash.getsentry.net/queries/9648
I'm not sure what that says about the group type schema or this here.

Copy link
Member

Choose a reason for hiding this comment

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

Let me look into this - seasonality and sensitivity are only used for anomaly detection alerts.

seasonality I was wrong about. We have this in the alert rule model but as we never released anything besides "auto" we decided to not migrate it and not let users change this option.

sensitivity we should have on more than 4 detectors though. I'll follow up separately, we might need to run a migration to heal these missing values.

threshold_period isn't required because it's for an unreleased feature that lets you say the alert has to exceed the threshold x number of times before triggering. We have a handful of rules in the Sentry organization that use it, but you can't create a rule like this today. We should probably get rid of these if we don't plan to ever release it, but for now that's what that is.

Copy link
Member

Choose a reason for hiding this comment

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

Update: Michelle let me know we moved sensitivity off of the Detector model, I was unaware. These are on the DataCondition now :phew:

Copy link

codecov bot commented Oct 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #101116      +/-   ##
===========================================
+ Coverage   81.09%    81.11%   +0.01%     
===========================================
  Files        8661      8668       +7     
  Lines      384342    384579     +237     
  Branches    24263     24263              
===========================================
+ Hits       311694    311947     +253     
+ Misses      72303     72287      -16     
  Partials      345       345              

@kcons kcons merged commit dce91fa into master Oct 8, 2025
65 checks passed
@kcons kcons deleted the kcons/mundane branch October 8, 2025 21:30
Copy link

sentry-io bot commented Oct 8, 2025

Issues attributed to commits in this pull request

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

@kcons kcons added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Oct 8, 2025
@getsentry-bot
Copy link
Contributor

PR reverted: 6075243

getsentry-bot added a commit that referenced this pull request Oct 8, 2025
…ssing contexts (#101116)"

This reverts commit dce91fa.

Co-authored-by: kcons <6990351+kcons@users.noreply.github.com>
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 Trigger: Revert Add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants