feat(crons): Improve occurance evidence Failure reason #70785

evanpurkhiser · 2024-05-13T17:43:08Z

The failure reason of "incident" isn't super helpful

We can instead say something like

3 missed check-ins detected
2 missed check-ins, 1 timeout check-in and 1 error check-in were detected
A failed check-in was detected

This implements that

gaprl · 2024-05-13T17:55:13Z

src/sentry/monitors/logic/mark_failed.py

+    )
+
+    if sum(status_counts.values()) == 1:
+        return _("A %(status)s check-in was detected") % {"status": list(status_counts.keys())[0]}


Can we say "An error check-in was detected"?

JoshFerge · 2024-05-13T17:56:54Z

src/sentry/monitors/logic/mark_failed.py

@@ -166,8 +179,13 @@ def mark_failed_threshold(
    # - The monitor and env are not muted
    if not monitor_env.monitor.is_muted and not monitor_env.is_muted and incident:
        checkins = MonitorCheckIn.objects.filter(id__in=[c["id"] for c in previous_checkins])
-        for previous_checkin in checkins:
-            create_issue_platform_occurrence(previous_checkin, incident, received=received)
+        for failed_checkin in checkins:


there is some variable shadowing going on here has failed_checkin is a parameter to this function. would rename to limit the confusion

JoshFerge · 2024-05-13T18:00:15Z

src/sentry/monitors/logic/mark_failed.py

+    )
+
+    status_counts = Counter(
+        human_names[cast(int, checkin["status"])]


why do we need to cast this?

"CheckInStatus" is incompatible with "int"

It's because

human_names: Mapping[int, str] = dict( [ (CheckInStatus.ERROR, "error"), (CheckInStatus.MISSED, "missed"), (CheckInStatus.TIMEOUT, "timeout"), ] )

I would type this as

human_names: Mapping[CheckInStatus, str] = dict( [ (CheckInStatus.ERROR, "error"), (CheckInStatus.MISSED, "missed"), (CheckInStatus.TIMEOUT, "timeout"), ] )

But that produces

Type parameter "_KT@Mapping" is invariant, but "Literal[2, 4, 5]" is not the same as "CheckInStatus"

There's a dynamic guard for this because of the if in the generator

status_counts = Counter( HUMAN_FAILURE_STATUS_MAP[cast(int, checkin["status"])] for checkin in failed_checkins if checkin["status"] in HUMAN_FAILURE_STATUS_MAP.keys() )

src/sentry/monitors/logic/mark_failed.py

JoshFerge · 2024-05-13T18:28:22Z

src/sentry/monitors/logic/mark_failed.py

@@ -304,6 +326,41 @@ def create_issue_platform_occurrence(
    )


+HUMAN_FAILURE_STATUS_MAP: Mapping[int, str] = {


i don't think this type is correct? CheckInStatus is an enum not an IntEnum?

It's not an enum

sentry/src/sentry/monitors/models.py

Line 125 in ac80048

class CheckInStatus:

JoshFerge · 2024-05-13T18:30:23Z

src/sentry/monitors/logic/mark_failed.py

+    )
+
+    if sum(status_counts.values()) == 1:
+        return _("An %(status)s check-in was detected") % {"status": list(status_counts.keys())[0]}


not familiar but will the string logic here correctly do A vs 'An`?

good point. Let me see what django has for a e i o and u and sometimes y

worth adding a test for that

JoshFerge

would try and remove the casts, but other than that lgtm besides the A vs An bit

evanpurkhiser · 2024-05-13T18:40:13Z

Managed to get rid of most of the casts, the problem was the SimpleCheckIn was not typing it's status correctly. Indeed status is just a int

codecov · 2024-05-13T18:44:58Z

Codecov Report

Attention: Patch coverage is 27.27273% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 58.40%. Comparing base (add00de) to head (d36285b).
Report is 5 commits behind head on master.

❗ Current head d36285b differs from pull request most recent head 748e8fd. Consider uploading reports for the commit 748e8fd to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #70785       +/-   ##
===========================================
- Coverage   78.18%   58.40%   -19.78%     
===========================================
  Files        6506     6469       -37     
  Lines      290888   290327      -561     
  Branches    50135    50092       -43     
===========================================
- Hits       227434   169570    -57864     
- Misses      63019   120316    +57297     
- Partials      435      441        +6

Files	Coverage Δ
src/sentry/monitors/logic/mark_failed.py	`13.44% <27.27%> (-81.61%)`	⬇️

... and 1864 files with indirect coverage changes

The failure reason of "incident" isn't super helpful We can instead say something like > 3 missed check-ins were detected > 2 missed check-ins, 1 timeut check-in and 1 error check-in detected > A failed check-in detected This implements that

sentry-io · 2024-05-14T23:15:24Z

Suspect Issues

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

‼️ Environment.DoesNotExist: Environment matching query does not exist. sentry.db.models.manager.base in get_from_cache View Issue

_{Did you find this useful? React with a 👍 or 👎}

evanpurkhiser requested a review from a team as a code owner May 13, 2024 17:43

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 13, 2024

evanpurkhiser self-assigned this May 13, 2024

evanpurkhiser force-pushed the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch 2 times, most recently from 75dbbfd to 1615515 Compare May 13, 2024 17:49

vercel bot deployed to Preview May 13, 2024 17:52 View deployment

gaprl requested changes May 13, 2024

View reviewed changes

evanpurkhiser force-pushed the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch from 343b3fd to 31239ed Compare May 13, 2024 17:56

vercel bot deployed to Preview May 13, 2024 18:03 View deployment

JoshFerge reviewed May 13, 2024

View reviewed changes

evanpurkhiser force-pushed the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch from e9c4b4f to 5cbb9da Compare May 13, 2024 18:25

JoshFerge reviewed May 13, 2024

View reviewed changes

vercel bot deployed to Preview May 13, 2024 18:30 View deployment

JoshFerge reviewed May 13, 2024

View reviewed changes

JoshFerge approved these changes May 13, 2024

View reviewed changes

evanpurkhiser force-pushed the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch from 39be0c8 to bc408bd Compare May 13, 2024 18:39

evanpurkhiser enabled auto-merge (squash) May 13, 2024 18:40

evanpurkhiser force-pushed the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch from 9628078 to 3fff04d Compare May 13, 2024 18:41

vercel bot deployed to Preview May 13, 2024 18:44 View deployment

gaprl approved these changes May 13, 2024

View reviewed changes

evanpurkhiser force-pushed the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch from 3edcce9 to 620051c Compare May 14, 2024 19:09

vercel bot deployed to Preview May 14, 2024 19:16 View deployment

evanpurkhiser force-pushed the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch from 33fb44c to 2a448f7 Compare May 14, 2024 19:37

vercel bot deployed to Preview May 14, 2024 19:42 View deployment

evanpurkhiser force-pushed the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch from d36285b to 59b49e5 Compare May 14, 2024 19:56

🛠️ apply pre-commit fixes

748e8fd

vercel bot deployed to Preview May 14, 2024 20:01 View deployment

evanpurkhiser merged commit d6555a8 into master May 14, 2024
48 checks passed

evanpurkhiser deleted the evanpurkhiser/feat-crons-improve-occurance-evidence-failure-reason branch May 14, 2024 20:26

github-actions bot locked and limited conversation to collaborators May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(crons): Improve occurance evidence Failure reason #70785

feat(crons): Improve occurance evidence Failure reason #70785

evanpurkhiser commented May 13, 2024 •

edited

gaprl May 13, 2024

evanpurkhiser May 13, 2024

evanpurkhiser May 13, 2024

JoshFerge May 13, 2024

JoshFerge May 13, 2024

evanpurkhiser May 13, 2024 •

edited

JoshFerge May 13, 2024

evanpurkhiser May 13, 2024

evanpurkhiser May 13, 2024

JoshFerge May 13, 2024

JoshFerge May 13, 2024

evanpurkhiser May 13, 2024

JoshFerge May 13, 2024

JoshFerge left a comment

evanpurkhiser commented May 13, 2024

codecov bot commented May 13, 2024 •

edited

sentry-io bot commented May 14, 2024

		@@ -304,6 +326,41 @@ def create_issue_platform_occurrence(
		)


		HUMAN_FAILURE_STATUS_MAP: Mapping[int, str] = {

feat(crons): Improve occurance evidence Failure reason #70785

feat(crons): Improve occurance evidence Failure reason #70785

Conversation

evanpurkhiser commented May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanpurkhiser May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshFerge left a comment

Choose a reason for hiding this comment

evanpurkhiser commented May 13, 2024

codecov bot commented May 13, 2024 • edited

Codecov Report

sentry-io bot commented May 14, 2024

Suspect Issues

evanpurkhiser commented May 13, 2024 •

edited

evanpurkhiser May 13, 2024 •

edited

codecov bot commented May 13, 2024 •

edited