Phil/alert evals #2583
Draft
Phil/alert evals #2583
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds a rust crate for rendering notification messages using handlebars templates. This will be used by the new alert system for rendering alert emails. The previous alerts edge function used the `mjml-browser` library to style the base alert templates so that they match the visual style of the application UI. This removes that in favor plain HTML, which was generated by an LLM from the original mjml template. The resulting emails are pretty similar to what we had before, but not quite identical. This was considered a pretty fair tradeoff, since otherwise we'd need a JS runtime in order to render the mjml.
Adds a database migration for moving to the new alerts system. This migration disables the previous alerting system by disabling the evaluation cron job, and disabling the triggers on `alert_history`. The new alerts system has an automations task for each alert, which gets created when the alert is fired, and which cleans itself up automatically after resolution emails are sent. The migration adds a new `id` column to `alert_history`, in order to associate each alert with the notification automation tasks. The `task_id` of the notifications task is the same as the `id` of the alert. Technically, this column could have been named `notification_task_id`. But it also happens to function as a surrogate id, since there is at most 1 notification task per alert, and the notification tasks need to be able to query alerts by this id. So I'm kinda just leaning into that, and calling it `id`. This doesn't replace the unique index on `(catalog_name, alert_type, fired_at)` (though we might one day revisit that index). control-plane: complete alerts migration Disables the old alerting system and enables the new one, completing the cutover.
Adds end-to-end alerting to the agent. Previously, alerting was handled by a postgres cron job, which would evaluate alert views, add/update `alert_history`, and send email notifications via the alerts edge function. This adds all of that functionality to the agent, along with a number of improvements. The new system is able to tolerate faults in both sending notifications and in alert evaluation, so we should no longer have problems with missing alert emails. And any alerts tht get fired by controllers will no longer suffer from a delay due to the cron-based evaluation. The `agent::alerts` module documentation has an overview of how the main pieces fit together, so I won't repeat that here. During development and testing, I stumbled across some problems that were ultimately caused by cloning the `models::IdGenerator`, causing duplicate ids to be generated. So I removed the `Clone` impl from `IdGenerator`, so it's no longer a footgun.
Adds some basic integration tests for alerts, and improves the current `assert_alert_firing|resolved` test assertions to include exercising the alert notification code.
This is no longer used now that we have agent-based alerting.
psFried
force-pushed
the
phil/alert-evals
branch
from
08a8629 to
18346e2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Refactors alerts, and makes them more reliable.
Our previous alerting system suffered from a few different reliability issues:
The new design fixes those issues, and adds a few other improvements. The basic idea is to use the
automationscrate to manage a few new background tasks for evaluating alerts and sending any notifications.AlertNotificationstask gets created for every alert, as part of adding it toalert_history. This task is responsible for sending all of the notifications pertaining to a single alert. After the alert has resolved and all resolution notifications have been sent, the task will delete itself.AlertEvaluatortasks get created as part of the migration. One evaluatesdata_movement_stalledalerts, and the other evaluates all of the tenant-related alerts (free_trial,missing_payment_method, etc). They each just add/updatealert_history.AlertActions, which will fire and resolve alerts while committing the controller outcome.The
alert_historytable structure is unchanged except for the addition of anidcolumn. Theidis essentially a surrogate key, though the original(catalog_name, alert_type, fired_at)primary key was left in place. The shape of theargumentsandresolved_argumentsJSON remains as it was, with the resolvedrecipientsbeing embedded in it. I have regrets about therecipientsbeing embedded like that, but I'm not sure that it's worth changing at this point, and I thought it would help to keep the transition as simple as possible.Deployment plan:
Alerts that were fired by the legacy system can be resolved by the new system. The migration creates a notification task for each open alert, and sets the state to indicate that the "fired" notifications have already been sent. When the alert resolves, the new notification system will send the emails.
If we need to rollback, we can do so without much risk by deploying the previous agent version and re-enabling the evaluation cron job and
alert_historytriggers.