fix(detectors): Clarify Detector status vs enabled; update code accordingly#113761
fix(detectors): Clarify Detector status vs enabled; update code accordingly#113761
Conversation
saponifi3d
left a comment
There was a problem hiding this comment.
high level this makes sense, is the plan to go back and update the detector selection in processing separately?
| # The detector's status - used for tracking deletion state | ||
| # The detector's status - used for tracking disabled and deletion state. | ||
| # A Detector that isn't allowed by plan or is effectively temporarily | ||
| # deleted for some other reason should have status=ObjectStatus.DISABLED. |
There was a problem hiding this comment.
should we update the get_queryset to automatically filter out the disabled detectors? these seem like they're not recoverable directly by a user, but would require some other action before being re-enabled.
that or we should filter detectors that are disabled in process_data_sources -- i'd lean more towards the manager so it's consistent, even outside of our primary use cases.
There was a problem hiding this comment.
I'm unsure. At present, this distinction is only really relevant to Metric Detectors, and we already unsubscribe those on plan change and filtering the disallowed ones in the APIs, so the difference behaviorally isn't substantial.
But, filtering out disabled detectors by default makes all of that much cheaper, because we only really have to evaluate it once and cache it, so I do want to go that way.
I'm thinking of going further as a follow-up; for now, I'm trying to just correct current behavioral patterns enough to fix the getsentry-side processing.
But it does make sense to make more of the general code aware of this by default; we want new uses of this status work automatically.
| # A Detector that isn't allowed by plan or is effectively temporarily | ||
| # deleted for some other reason should have status=ObjectStatus.DISABLED. | ||
| # The 'enabled' field is for user-applied deactivation like snoozes. | ||
| status = models.SmallIntegerField(db_default=ObjectStatus.ACTIVE) |
There was a problem hiding this comment.
🎉 - glad we can reuse this field instead of adding a new one!
Backend Test FailuresFailures on
|
|
getsentry side, required for cross-repo tests to pass: https://github.com/getsentry/getsentry/pull/20011 |
We've been blurring the distinction between plan disabling of Detectors and snooze disabling.
This change aims to clarify the distinction, and update existing code that cares about one or the other to only update the field they should be modifying.
Note that code and tests in getsentry also use this code, so a legacy alias in retained here, and there is another PR (https://github.com/getsentry/getsentry/pull/20011) to update the tests in that repo to still pass with this PR applied.