-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(crons): Send detector_id
along with the occurrence, when available
#97963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…able This starts sending the `detector_id` to the issue platform, so that we can trigger workflows.
@@ -144,6 +145,11 @@ def send_incident_occurrence( | |||
if last_successful_checkin: | |||
last_successful_checkin_timestamp = last_successful_checkin.date_added.isoformat() | |||
|
|||
detector = get_detector_for_monitor(monitor_env.monitor) | |||
evidence_data = {} | |||
if detector: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I should also put this behind a feature flag, but I think it's not likely to cause many issues. What do you think @saponifi3d?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now this should be fine because the post_process
is only setup for error events and metric issues -- so this should be good 👍
we will need issue alerts to start responding to more than just error events soon -- but we'll add an allow/deny list when we go to do that so it's easy to enable/disable different issue types.
detector = get_detector_for_monitor(monitor_env.monitor) | ||
evidence_data = {} | ||
if detector: | ||
evidence_data["detector_id"] = detector.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The call to get_detector_for_monitor
only handles DoesNotExist
errors. Other database exceptions will crash send_incident_occurrence
, preventing incident creation.
-
Description: The
get_detector_for_monitor
function is called to retrieve an optionalDetector
. This function's database query only handles theDetector.DoesNotExist
exception. Other potential database exceptions, such asOperationalError
from a connection timeout or transaction issue, are not caught. Whensend_incident_occurrence
calls this function, an unhandled database exception will propagate and cause the caller to crash. As a consequence, the system will fail to create the incident occurrence, silently dropping a critical monitoring alert. -
Suggested fix: Update
get_detector_for_monitor
to catch broader database exceptions, such asOperationalError
andDatabaseError
, alongsideDoesNotExist
. In these cases, the function should returnNone
. This prevents transient database issues from crashing thesend_incident_occurrence
process and ensures incident occurrences can still be created, even if the optional detector information cannot be retrieved.
severity: 0.65, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
This starts sending the
detector_id
to the issue platform, so that we can trigger workflows.