fix(crons): Properly update monitorenvs in mark_ok with thresholds #57955
Conversation
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.
github-actions
bot
added
the
Scope: Backend
src/sentry/monitors/logic/mark_ok.py
Outdated
| and monitor_env.status != MonitorStatus.OK | ||
| ): | ||
| params["status"] = MonitorStatus.OK | ||
| recovery_threshold = monitor_env.monitor.config.get("recovery_threshold", 0) |
There was a problem hiding this comment.
Feels like we should just drop this confuging 0 default, it can be
- set and is enforced (by the validator) to be above zero
- set to
Nonewhen the user does not specify the field - not set, for old monitors
We should just leave the default blank and let it always be None
src/sentry/monitors/logic/mark_ok.py
Outdated
| previous_checkins = MonitorCheckIn.objects.filter( | ||
| monitor_environment=monitor_env | ||
| ).order_by("-date_added")[:recovery_threshold] |
There was a problem hiding this comment.
This does make me think @rjo100 we NEED a maximum value here. Do we have one?
There was a problem hiding this comment.
i can put a max in a diff PR on the validator and then we can reference that value and do a min(recovery_threshold, max_value)
...maybe 10? and then if people ask we can increase
There was a problem hiding this comment.
Yeah, we definitely need to have it. Otherwise someone is going to slap a huge number in here and break our whole system.
Codecov Report
@@ Coverage Diff @@
## master #57955 +/- ##
==========================================
+ Coverage 78.35% 79.07% +0.71%
==========================================
Files 5138 5139 +1
Lines 223631 223825 +194
Branches 37650 37684 +34
==========================================
+ Hits 175226 176979 +1753
+ Misses 42703 41180 -1523
+ Partials 5702 5666 -36
|
|
confirmed working in |
Properly updates next/last checkin timestamps in
mark_okif threshold is enabled (but isn't met)Fixes #57730