fix(aci): Ensure GroupOpenPeriod close times aren't before start time #102345
Conversation
github-actions
bot
added
the
Scope: Backend
| close_time = resolution_time | ||
| if resolution_time < self.date_started: | ||
| # There may be more externally accurate close times, but since we're closing it | ||
| # now, timezone.now() is arguably always a reasonable choice. | ||
| close_time = timezone.now() |
There was a problem hiding this comment.
🤔 not 100% sure we should close in these scenarios. curious why we are getting into these states in general? a little nervous that we could be getting something out of order, and we're closing a period that wasn't meant to be closed yet.
There was a problem hiding this comment.
These seem to occur because for uptime monitors we create the open period through occurrence processing queues using a datetime.now() (so, with some delay, using Sentry-app-based clock), while they are closed using a timestamp provided as part of the check-in request (from Relay?) and I think that timestamp is when we actually did the checkin on another service, so it can be before we actually opened it in some cases.
So, they are two different timestamp sources identifying different things. I'm not sure which one we want to use, but we probably would prefer to be consistent. This change uses a valid and reasonable timestamp when the one being offered isn't valid/reasonable. I still think we want to address the inconsistency; this just does a reasonable thing when the input is unreasonable.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #102345 +/- ##
=========================================
Coverage 80.95% 80.96%
=========================================
Files 8769 8771 +2
Lines 389622 389724 +102
Branches 24777 24777
=========================================
+ Hits 315431 315523 +92
- Misses 73813 73823 +10
Partials 378 378 |
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Fixes SENTRY-56V1.
Independently, we need to talk to uptime monitors about timestamp consistency, as the issue arises from them sending resolved checkin time (supplied from Relay?) for close time while we use a Sentry-originating timestamp for star time.