ref(crons): Correct logic for mark_ok #57730
Conversation
github-actions
bot
added
the
Scope: Backend
| @with_feature("organizations:issue-platform") | ||
| @patch("sentry.issues.producer.produce_occurrence_to_kafka") |
There was a problem hiding this comment.
I've made this more unit-test like by removing the dependency on mark_failed from this test.
src/sentry/monitors/logic/mark_ok.py
Outdated
| "-date_added" | ||
| )[:recovery_threshold] | ||
|
|
||
| # Incident recovers when ew have successive threshold check-ins |
src/sentry/monitors/logic/mark_ok.py
Outdated
| # is not recovering | ||
| allow_status_update = True | ||
|
|
||
| # Resolve the incident if we have met the recovery recovery_threshold |
There was a problem hiding this comment.
should this just be if we have met the recovery_threshold?
src/sentry/monitors/logic/mark_ok.py
Outdated
| ) | ||
|
|
||
| # Resolve the incident | ||
| if incident_recovering and monitor_env.status != MonitorStatus.OK: |
There was a problem hiding this comment.
could we simplify the logic/execution overall here? Right now it seems like the order of conditions are:
- Check if we have a recovery threshold (if not just update the status)
- If we have, fetch N recent check-ins and check that they are all ok
- If they are all ok check to see if our monitor is in an incident (!= MonitorStatus.OK)
- If true, resolve the incident, and then allow a status update on the monitor
But would it be better to instead
- Check
monitor_env.status != MonitorStatus.OKfirst, if it ISOKthen we can do nothing - If not
OKthen chcek for a recovery threshold, proceed with the previous steps 2 and then 4
Only mentioning this because hopefully the majority of the time, user's monitor statuses should be OK which means we shouldn't check the N most recent check-ins every time they send an OK check-in (assuming they have a recovery threshold)
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-crons-correct-logic-for-mark-ok
branch
from
481f494
to
63fc2e1
Compare
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-crons-correct-logic-for-mark-ok
branch
from
63fc2e1
to
57ebe7e
Compare
Previously when incidents were enabled (a recovery threshold was set) a monitor would not have it's `next_checkin` or `next_checkin_latest` updated until it recovered.
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-crons-correct-logic-for-mark-ok
branch
from
57ebe7e
to
5893654
Compare
| ) | ||
| return | ||
|
|
||
| recovery_threshold = monitor_env.monitor.config.get("recovery_threshold", 0) |
There was a problem hiding this comment.
this should be 1 as the default? or idk maybe a comment. the min value is 1
There was a problem hiding this comment.
I think you have the default here as 0 since if it's not set we don't want to enable incidents yet
| return | ||
|
|
||
| recovery_threshold = monitor_env.monitor.config.get("recovery_threshold", 0) | ||
| using_incidents = bool(recovery_threshold) |
There was a problem hiding this comment.
maybe just change this to recovery_threshold > 1 which is functionally the same thing for now
There was a problem hiding this comment.
Yeah I basically just duplicated what the logic was before if recovery_threshold:
|
@rjo100 mind just approving as is and we can clean up after? |
evanpurkhiser
deleted the
evanpurkhiser/ref-crons-correct-logic-for-mark-ok
branch
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
evanpurkhiser
added
the
Trigger: Revert
|
PR reverted: 8aa99c7 |
This reverts commit dcd6da4. Co-authored-by: evanpurkhiser <1421724+evanpurkhiser@users.noreply.github.com>
Previously when incidents were enabled (a recovery threshold was set) a
monitor would not have it's
next_checkinornext_checkin_latestupdated until it recovered.