feat(api): backfill alert name and tags - #3029
Conversation
🦋 Changeset detectedLatest commit: 897648a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Greptile SummaryThe PR adds an idempotent startup migration that derives missing alert display names and tags from saved searches, dashboard tiles, or inline chart configuration. It also makes malformed alert-title templates fall back to their verbatim text.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/api/src/migrations.ts | Adds the idempotent alert display-field backfill with conditional bulk updates and timestamp preservation. |
| packages/api/src/server.ts | Invokes startup migrations after the initial MongoDB connection succeeds. |
| packages/api/src/tasks/checkAlerts/template.ts | Pre-renders alert title templates and falls back to verbatim text when Handlebars rendering fails. |
| packages/api/src/tests/migrations.int.test.ts | Covers derivation sources, populated-field preservation, dangling references, legacy alerts, timestamps, and rerun behavior. |
| packages/api/src/tasks/checkAlerts/tests/renderAlertTemplate.int.test.ts | Verifies malformed alert-title templates no longer prevent title generation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Start[API process starts] --> Mongo[Connect to MongoDB]
Mongo --> Migration[Run alert display-field backfill]
Migration --> Alerts[Load alerts missing name or tags]
Alerts --> Source{Alert source}
Source -->|Saved search| Search[Load saved search]
Source -->|Dashboard tile| Dashboard[Load dashboard and tile]
Source -->|Inline chart| Inline[Read chart configuration]
Search --> Derive[Derive display name and tags]
Dashboard --> Derive
Inline --> Derive
Derive --> Update[Write only missing fields]
Update --> Continue[Continue server initialization]
Reviews (4): Last reviewed commit: "Merge branch 'main' of https://github.co..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function backfillAlertNameAndTags() { | ||
| const ids = ( | ||
| await Alert.find( | ||
| { $or: [NAME_MISSING_FILTER, ...TAGS_MISSING_FILTER.$or] }, | ||
| { _id: 1 }, | ||
| ).lean() | ||
| ).map(doc => doc._id); |
There was a problem hiding this comment.
If an installation has a large alerts collection, this unindexed query scans the collection and retains every matching alert ID in memory before the 500-record batching begins. Because the migration runs on every API startup, this adds repeated database load and potentially high process-memory usage; discover records incrementally with a cursor or pagination so the batch bound applies to the initial read as well.
Deep ReviewScope: 6 files vs base Intent: Backfill alert Note: ✅ No critical issues found. The migration is idempotent, guards user-set values, uses 🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (8): correctness, data-migrations, performance, reliability, testing, maintainability, previous-comments, adversarial. Testing gaps:
|
E2E Test Results✅ All tests passed • 356 passed • 1 skipped • 1488s
Tests ran across 4 shards in parallel. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| tileId: alert.tileId ?? null, | ||
| 'chartConfig.name': alert.chartConfig?.name ?? null, | ||
| }; | ||
| if (!hasName && name != null) { |
There was a problem hiding this comment.
🟠 major — Backfilling name silently rewrites every existing alert's notification title
alert.name is not just a display name — it is passed as the Handlebars title template (packages/api/src/tasks/checkAlerts/index.ts:573: template: alert.name). Today an unnamed saved-search alert notifies 🚨 Alert for "Error spikes" - 10 lines found; after this migration runs on the next restart its title becomes 🚨 Error spikes, dropping the value and the threshold from every fire/resolve message. The same happens to tile alerts (🚨 Service health P95 latency instead of the value/threshold sentence), and for alerts whose tile was deleted the title becomes the nonsense 🚨 Service health Tile. This is not recoverable from the product UI: EditAlertModal deliberately does not edit name (see the comment at packages/app/src/components/alerts/EditAlertModal.tsx:56-58), it only round-trips it, so a user cannot clear the backfilled name. Note also that nothing in the app reads alert.name for display — AlertsPage/AlertDetailPage use getAlertDisplayName() — so the write buys no UI benefit while costing the title. Either drop name from the backfill (backfill only what a reader actually needs), or split the concept first: give the alert a separate titleTemplate field and migrate the existing name values onto it, so name can be a plain display name. The two halves of this PR are also tested only in isolation — migrations.int.test.ts never asserts a rendered title and the new renderAlertTemplate tests pass an explicit template, so this regression is invisible to the suite.
There was a problem hiding this comment.
Since the templating functionality is completely undocumented, and currently not reachable through the UI, I think I'm going to remove it in the next PR since alert.name is not a good field for a template anyway, since it will be rendered on the UI.
| update: { $set: { name } }, | ||
| }); | ||
| } | ||
| if (!hasTags && tags.length > 0) { |
There was a problem hiding this comment.
🟠 major — Backfilled alert.tags has no reader, and creates a second copy that goes stale
Nothing can read the new field: formatAlertResponse (packages/api/src/routers/api/alerts.ts:90) does not pick tags, AlertsPageItemSchema (packages/common-utils/src/types.ts:2483) has no top-level tags, translateAlertDocumentToExternalAlert/ExternalAlert (packages/api/src/utils/externalApi.ts:242) omits it, and alertBaseSchema (packages/api/src/utils/zod.ts:770) does not accept it, so no API can set or return it. The app already gets alert tags live from the joined document — getAlertTags() at packages/app/src/utils/alerts.ts:282 returns alert.dashboard?.tags ?? alert.savedSearch?.tags and AlertsPage filters/searches on that. Persisting a copy therefore adds a second source of truth that is never refreshed (the backfill skips any alert whose tags is non-empty), so renaming a dashboard tag later leaves the alert's snapshot permanently wrong. Drop the tags write and the model field until a reader exists, or make the read path source tags from the alert and keep it in sync on dashboard/saved-search update.
| ); | ||
| } | ||
|
|
||
| export function deriveAlertNameAndTags( |
There was a problem hiding this comment.
🔵 minor — deriveAlertNameAndTags re-implements existing display-name/tag derivation
getAlertDisplayName() (packages/app/src/utils/alerts.ts:259) already produces exactly ${dashboard.name} ${tile.config.name || 'Tile'} for tile alerts and savedSearch.name for saved-search alerts, and getAlertTags() (same file, line 282) already produces the dashboard/saved-search tag list; packages/cli/src/components/AlertsPage.tsx:41 is a third copy with a different separator (—). The two now disagree on inline alerts (the app returns '', this returns chartConfig.name), so the notification title and the UI label will differ for the same alert. ALERT_NAME_MAX_LENGTH = 512 also restates the cap already declared as z.string().min(1).max(512) at packages/api/src/utils/zod.ts:780. Lift one derivation into common-utils and have the app, CLI and this migration call it, and export the 512 cap as a shared constant.
| const TAGS_MISSING_FILTER = { $or: [{ tags: null }, { tags: { $size: 0 } }] }; | ||
|
|
||
| export async function backfillAlertNameAndTags() { | ||
| const ids = ( |
There was a problem hiding this comment.
🔵 minor — Migration has no completion marker, so it full-scans the alerts collection on every boot forever
{ $or: [{name: {$in: [null, '']}}, {tags: null}, {tags: {$size: 0}}] } matches on unindexed fields (the only index is {team: 1, _id: 1}, packages/api/src/models/alert.ts:296), so this is a COLLSCAN, and it matches essentially every alert since almost none will ever get tags (a saved search with no tags can never satisfy the filter). It runs unconditionally on every process start with no recorded version, and every replica in a deployment runs it simultaneously; ids also materialises every matching _id into one array before the batching loop even begins, which makes the batching pointless for memory. Record a migration-version document (or a per-alert backfilledAt marker) so a completed migration is a single indexed no-op query, and stream with a cursor instead of collecting all ids up front.
PR Review6 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 6 minor 6 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
| tileId: alert.tileId ?? null, | ||
| 'chartConfig.name': alert.chartConfig?.name ?? null, | ||
| }; | ||
| if (!hasName && name != null) { |
There was a problem hiding this comment.
🔴 critical — Backfilling name silently rewrites the notification title of every existing alert
alert.name is not a display label — it is the notification title template (packages/api/src/tasks/checkAlerts/index.ts:573 passes template: alert.name into buildAlertMessageTemplateTitle, and that function returns renderedTemplate ?? <default>). Today an alert with no name gets 🚨 Alert for "Error spikes" - 1234 lines found (or, for tile/inline, the formatted value plus threshold description). After this backfill runs, the same alert's title becomes 🚨 Error spikes — every saved-search, tile and inline alert in every existing deployment loses the value/threshold context in its Slack/webhook title on the first restart after upgrade, and the INLINE default branch documented at template.ts:288 becomes dead. It is also not recoverable from the product: EditAlertModal does not edit name (it only round-trips it, see the comment at packages/app/src/components/alerts/EditAlertModal.tsx:56), and clearing it through the API just makes the next boot re-fill it. Either give the alert a real name field distinct from the title template (add titleTemplate, keep name for display), or don't backfill name at all and derive the display name at read time as the app already does.
| const TAGS_MISSING_FILTER = { tags: null }; | ||
|
|
||
| export async function backfillAlertNameAndTags() { | ||
| const ids = ( |
There was a problem hiding this comment.
🟠 major — Migration re-scans the whole alerts collection on every boot; the repo already has migrate-mongo for run-once work
There is no completion record, and the scan set never empties: an alert whose saved search has no tags (and every INLINE alert, since deriveAlertNameAndTags always returns tags: [] for inline) keeps tags missing, so it matches TAGS_MISSING_FILTER forever; dangling-reference alerts keep matching NAME_MISSING_FILTER forever. Each process start therefore runs an unindexed $or scan of the full collection (the only index is {team:1,_id:1}, models/alert.ts:296), loads every matching _id into memory unbounded, then re-reads them in batches plus their saved searches/dashboards — on every replica, forever. The repo already has the right mechanism: packages/api/migrations/mongo/ + migrate-mongo-config.ts (changelog collection, yarn dev:migrate-db), which runs each migration exactly once. Move the backfill there, or at minimum write a sentinel/tags: [] so processed alerts stop matching.
| type: String, | ||
| required: false, | ||
| }, | ||
| tags: { |
There was a problem hiding this comment.
🟠 major — alert.tags is written but never read, and is frozen at backfill time
Nothing reads alert.tags: the API never projects or serializes it (translateAlertDocumentToExternalAlert and AlertBaseObjectSchema have no tags field), makeAlert (controllers/alerts.ts:240-283) never writes it so no API call can ever change it, and the app derives tags from the populated source instead — getAlertTags (packages/app/src/utils/alerts.ts:282) and AlertDetailProperties.tsx:38 both read alert.dashboard?.tags ?? alert.savedSearch?.tags. So the copied tags are dead data that immediately becomes a stale second source of truth: retag a saved search from prod to staging and the alert keeps ['prod'] forever, because the backfill skips any alert whose tags is non-null. Either drop the tags half of this change and keep deriving at read time, or land the reader (filter/serialize alert.tags) plus a write path that keeps it in sync.
| typeof alert.tileId === 'string' | ||
| ? tiles.find(t => t?.id === alert.tileId) | ||
| : undefined; | ||
| name = `${dashboardName} - ${normalizeName(tile?.config?.name) ?? 'Tile'}`; |
There was a problem hiding this comment.
🔵 minor — Server-side name derivation duplicates getAlertDisplayName with a different format
packages/app/src/utils/alerts.ts:259 already derives the same name and formats a tile alert as `${dashboard.name} ${tileName}` (space, fallback 'Tile'); this new copy emits `${dashboardName} - ${tileName}` (hyphen). Both render on the same screen after the backfill — AlertDetailPage.tsx:145 shows getAlertDisplayName in the header while AlertDetailProperties.tsx:47 now shows a "Name" row with the backfilled string — so a user sees "Service health P95 latency" and "Service health - P95 latency" for one alert. Pick one derivation (share it via common-utils, or make the app read alert.name) rather than keeping two.
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
This looks solid, thanks!
However, I need to implement the functionality that allows the user to update alert names before we merge this, otherwise an alert that is backfilled could have a name that gets out of sync with changes made to the saved search / dashboard tile, without the user having the ability to rename the alert. I am working on that now
## Summary This PR is part 1 of 3 in introducing alert-level name and tags. **Context**: We require alert-level name and tags so that the alerts page can sort, filter, and paginate alert documents by name and tags without joining the full alerts collection with the dashboard and saved search collections. In this PR: 1. Add `displayName` and `tags` to the alert model, types, and schemas. `displayName` is chosen because the existing `name` can be a handlebars template (confirmed that production data includes handlebars templates) and we don't want to show an unrendered handlebars template as an alert's name on the alerts page. 2. Functions for deriving an alert's name and tags from the dashboard tile or saved search it references, more or less matching how these values are derived today for the alerts page, and more or less matching this [backfill PR.](#3029) 3. When writing alerts through internal endpoints (create/update dashboard, create/update alert), if an explicit displayName or tags is provided in the request, use that value. If none is provided, write the name and tags derived from the referenced dashboard or saved search. 4. When reading /alerts or /alert/:id, return a stored displayName and tags if available, otherwise derive the displayName and tags from the referenced dashboard or saved search In future PRs: 1. Part 2: External API and MCP support. Notification titles use displayName if available (and not overridden by `name`) 2. Part 3: UI updated to allow the user to specify alert's name and tags directly. Alerts page + alerts details page updated to show displayName and tags 3. Part 4: Backfill displayName and tags for existing alerts 4. Part 5+: paginate the alerts page ### Screenshots or video <details> <summary>Create dashboard alert</summary> Request is sent without alert-level displayName or tags, the value saved in Mongo is derived from the dashboard+tile names, and the dashboard's tags <img width="800" height="363" alt="Screenshot 2026-09-03 at 8 30 53 AM" src="https://github.com/user-attachments/assets/350fc85c-7f71-4d5b-9963-c8360845349d" /> <img width="381" height="199" alt="Screenshot 2026-09-03 at 8 30 00 AM" src="https://github.com/user-attachments/assets/4e435321-7d4d-4654-9e37-48de77c3b87c" /> If displayName or tags are included in the request, they are persisted, instead of a derived displayName and tags: <img width="360" height="93" alt="Screenshot 2026-09-03 at 8 36 05 AM" src="https://github.com/user-attachments/assets/f0bea253-4453-4dfb-9787-7e1d00da66f4" /> </details> <details> <summary>Update dashboard alert</summary> If an existing tile alert has no persisted displayName or tags, and the request doesn't include one, then the derived values will be saved <img width="332" height="65" alt="Screenshot 2026-09-03 at 8 33 56 AM" src="https://github.com/user-attachments/assets/7ee69581-6eb4-47a0-88ee-2503ef429bc1" /> If an existing tile is updated and a displayName or tags are included in the request, they are persisted, instead of a derived displayName and tags: <img width="360" height="93" alt="Screenshot 2026-09-03 at 8 36 05 AM" src="https://github.com/user-attachments/assets/f0bea253-4453-4dfb-9787-7e1d00da66f4" /> </details> <details> <summary>Create saved search alert</summary> Derived values are saved in Mongo <img width="294" height="76" alt="Screenshot 2026-09-03 at 8 44 56 AM" src="https://github.com/user-attachments/assets/295aa546-05d2-4ef7-9969-b19dbc0d2fd7" /> <img width="553" height="269" alt="Screenshot 2026-09-03 at 8 45 20 AM" src="https://github.com/user-attachments/assets/4b904ca3-309e-4a6a-b32a-5fb52c7cf821" /> </details> <details> <summary>Update saved search alert</summary> Derived values are saved in Mongo, if no displayName/tags are provided in the request <img width="553" height="269" alt="Screenshot 2026-09-03 at 8 45 20 AM" src="https://github.com/user-attachments/assets/71c53bbd-673b-4ff7-b9b7-45488187f829" /> <img width="259" height="58" alt="Screenshot 2026-09-03 at 8 47 00 AM" src="https://github.com/user-attachments/assets/a63487da-bb2b-4cae-8090-2e8ccf41c0a4" /> </details> <details> <summary>GET alerts</summary> Derived values are returned for displayName / tags if not present in the DB <img width="915" height="911" alt="Screenshot 2026-09-03 at 8 48 20 AM" src="https://github.com/user-attachments/assets/076460e3-de1f-4a69-9f43-a6f8a92c9422" /> Persisted values are returned, if present <img width="884" height="362" alt="Screenshot 2026-09-03 at 8 49 45 AM" src="https://github.com/user-attachments/assets/894032b8-cee9-405f-93da-b757d6dac023" /> </details> <details> <summary>GET alert/:id</summary> Derived values are returned if no persistent values exist <img width="732" height="444" alt="Screenshot 2026-09-03 at 8 50 26 AM" src="https://github.com/user-attachments/assets/622b12e6-de79-4710-99be-65aad62d7fec" /> Persisted values are returned if present <img width="759" height="444" alt="Screenshot 2026-09-03 at 8 50 40 AM" src="https://github.com/user-attachments/assets/69ed40a7-d8fd-40d8-bc99-546646a40bd7" /> </details> ### How to test locally Create and update alerts on dashboard tiles and saved searches, observe the displayName and tags values persisted in mongodb. Observe the response from the `/alerts` and `/alerts/:id` internal endpoints, they should include a derived displayName and tags, or the stored value if it's available. ### References - Linear Issue: Related to HDX-5116 - Related PRs:
| update: { $set: { displayName: derived.displayName } }, | ||
| }); | ||
| } | ||
| if (!hasTags && derived.tags != null && derived.tags.length > 0) { |
There was a problem hiding this comment.
🔵 minor — Backfill never converges: an unindexed full scan of alerts re-runs on every API start
derived.tags.length > 0 means an alert whose referenced entity has no tags is never written, and TAGS_MISSING_FILTER = { tags: null } keeps selecting it forever. That is not an edge case: Dashboard.tags defaults to [] (models/dashboard.ts:29-32), SavedSearch.tags materialises [], and deriveAlertDisplayFields returns [] for every INLINE alert (utils/alerts.ts:126) — so most alerts stay in the selection set permanently, and every replica boot repeats the $or collscan (the only index is {team:1,_id:1}) plus the per-batch saved-search/dashboard lookups, producing zero ops. Drop the length > 0 guard and write tags: derived.tags even when empty — that is exactly what makeAlert already persists for a parent with no tags (controllers/alerts.ts:267) — so the filter stops matching once an alert is done.
There was a problem hiding this comment.
This is probably worth addressing
|
The agent comment about |
| // Matches missing/null only; a stored [] is already resolved and left alone. | ||
| const TAGS_MISSING_FILTER = { tags: null }; | ||
|
|
||
| export async function backfillAlertDisplayFields() { |
There was a problem hiding this comment.
🔵 minor — Backfill has no completion record, so every pod start repeats a full unindexed scan of alerts
backfillAlertDisplayFields is re-run unconditionally on every Server.start() (server.ts:96), with an $or on displayName/tags that no index covers (AlertSchema.index({team:1,_id:1}) is the only one), and every replica in a rolling deploy runs it concurrently. Worse, it never converges: an alert whose savedSearch/dashboard ref is dangling derives null, is never written, and therefore matches the filter forever — so the scan + per-alert derivation repeats at every boot for as long as those alerts exist. The repo already has a migration mechanism with a completion ledger (packages/api/migrate-mongo-config.ts, changelogCollectionName: 'changelog', packages/api/migrations/mongo/); either run this there, or persist a marker/flag so a completed backfill is a no-op on subsequent starts.
| try { | ||
| renderedTemplate = handlebars.compile(alert.name)(view); | ||
| } catch (e) { | ||
| logger.error( |
There was a problem hiding this comment.
🔵 minor — Swallowing the template error removes the only user-visible signal that a title template is broken
Before this change a malformed alert.name threw, was caught at tasks/checkAlerts/index.ts:1369-1376, and pushed onto executionErrors (makeWebhookAlertError) — the user saw an error on the alert. Now the failure is a server-side logger.error only, and every notification is delivered with raw Handlebars in its title (🚨 Errors {{spike) indefinitely, with nothing surfaced in the product. name: z.string().min(1).max(512) (common-utils AlertBaseObjectSchema) does no syntax check, so there is no save-time signal either. Either record an executionErrors entry alongside the log, or reject a non-compiling name at write time so the user learns at save.
| ops.map(op => ({ updateOne: { ...op, timestamps: false } })), | ||
| { ordered: false }, | ||
| ); | ||
| updatedCount += result.modifiedCount; |
There was a problem hiding this comment.
🔵 minor — updatedCount double-counts alerts and the backfill issues two writes per alert
An alert missing both fields — the case for every pre-existing alert — produces two updateOne ops (lines 92-112), so result.modifiedCount counts it twice: the log reads scannedCount: 100, updatedCount: 200. Emit one op per alert with a combined $set and a filter that ANDs the guards for the fields actually being set, which both halves the writes and makes the count mean "alerts updated".
| typeof alert.displayName === 'string' && alert.displayName !== ''; | ||
| const hasTags = alert.tags != null; | ||
|
|
||
| const inputsUnchangedFilter = { |
There was a problem hiding this comment.
🔵 minor — inputsUnchangedFilter — the trickiest part of the backfill — is never exercised in its rejecting state
Both tests only cover the case where the filter matches; nothing asserts that a concurrent change to source/savedSearch/dashboard/tileId/chartConfig.name between the read and the write suppresses the update. A version of this filter that is too lax (e.g. dropping source, or comparing the ref with the wrong representation) passes the suite unchanged. Add a case that mutates the alert's ref via Alert.updateOne after seeding and asserts no name derived from the old ref is written.
| savedSearchIds.length > 0 | ||
| ? SavedSearch.find( | ||
| { _id: { $in: savedSearchIds } }, | ||
| { name: 1, tags: 1 }, |
There was a problem hiding this comment.
🔵 minor — Ref projections restate DISPLAY_REF_POPULATE from the alerts controller
{ name: 1, tags: 1 } and { name: 1, tags: 1, 'tiles.id': 1, 'tiles.config.name': 1 } are the same field sets as DISPLAY_REF_POPULATE in packages/api/src/controllers/alerts.ts:473-476 (select: 'name tags' / 'name tags tiles.id tiles.config.name'), and both must stay in sync with what deriveAlertDisplayFields reads. Export the field lists once next to deriveAlertDisplayFields in packages/api/src/utils/alerts.ts and derive both the populate select and this projection from them.
| ? describeThresholdViolation(alert.thresholdType) | ||
| : describeThresholdResolution(alert.thresholdType) | ||
| } ${describeThreshold(alert)}`; | ||
| const baseTitle = |
There was a problem hiding this comment.
🔵 minor — TILE and INLINE branches now build a byte-identical default title
After the rewrite, lines 387-393 and 400-406 are character-for-character the same expression. Compute formattedValue and the default title once before the branch (or in a small local helper) and keep only the source-specific guards — dashboard == null / tile lookup for TILE — inside the branches.
2 participants
Summary
Backfills alert
nameandtagsfrom the referenced saved search, dashboard tile, or inline chart config.runStartupMigrations()called fromServer.start(). Idempotent and re-runs on every start, alerts with an existing name or tags are skipped.tagsfield to the Alert model.alert.nameis also used as the notification title template, a name that isn't valid Handlebars now renders as is to avoid breaking the notification.Tested with
make dev-int FILE=src/__tests__/migrations.int.test.tsandFILE=renderAlertTemplate.int.test.ts, plus unit tests for the name/tags derivation.References