Skip to content

feat(api): backfill alert name and tags - #3029

Open
SpencerTorres wants to merge 1 commit into
mainfrom
backfill_alerts
Open

feat(api): backfill alert name and tags#3029
SpencerTorres wants to merge 1 commit into
mainfrom
backfill_alerts

Conversation

@SpencerTorres

Copy link
Copy Markdown
Contributor

Summary

Backfills alert name and tags from the referenced saved search, dashboard tile, or inline chart config.

  • Adds runStartupMigrations() called from Server.start(). Idempotent and re-runs on every start, alerts with an existing name or tags are skipped.
  • Adds optional tags field to the Alert model.
  • Since alert.name is 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.ts and FILE=renderAlertTemplate.int.test.ts, plus unit tests for the name/tags derivation.

References

  • Linear Issue: HDX-5149

@SpencerTorres
SpencerTorres requested a review from pulpdrew August 29, 2026 02:26
@changeset-bot

changeset-bot Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ea67086

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Minor
@hyperdx/app Minor
@hyperdx/otel-collector Minor

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

@vercel

vercel Bot commented Aug 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 29, 2026 2:26am
hyperdx-storybook Ready Ready Preview Aug 29, 2026 2:26am

Request Review

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Aug 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches 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:

  • Background tasks or delivery pipeline substantially modified — 49 lines (bar: 30):
    • packages/api/src/tasks/checkAlerts/template.ts

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 4
  • Production lines changed: 245 (+ 322 in test files, excluded from tier calculation)
  • Critical-path lines changed: 49
  • Branch: backfill_alerts
  • Author: SpencerTorres

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@greptile-apps

greptile-apps Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds an idempotent startup migration that derives missing alert names and tags from saved searches, dashboard tiles, or inline chart configuration, and makes malformed alert title templates fall back to their raw text.

  • Adds the alert tags schema field and name/tag derivation with unit and integration coverage.
  • Runs the backfill during API startup using conditional bulk updates.
  • Makes alert-title rendering resilient to malformed Handlebars templates.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking scalability concern in how the startup migration discovers records to batch.

The migration's writes are guarded and its behavioral changes are covered, but every API restart performs an unindexed scan and retains all matching alert IDs before bounded processing begins.

Files Needing Attention: packages/api/src/migrations.ts

Important Files Changed

Filename Overview
packages/api/src/migrations.ts Adds derivation and conditional bulk backfill logic, but its initial query loads every matching alert ID into memory before batching.
packages/api/src/server.ts Invokes the idempotent alert backfill after connecting to MongoDB on every API startup.
packages/api/src/models/alert.ts Adds an optional string-array tags field without changing existing alert requirements.
packages/api/src/tasks/checkAlerts/template.ts Compiles alert title templates once and intentionally falls back to raw text when rendering fails.
packages/api/src/tests/migrations.int.test.ts Covers persisted backfill results, dangling references, legacy alerts, preservation of populated values, and reruns.
packages/api/src/tests/migrations.test.ts Covers source-specific derivation, normalization, fallback names, and maximum name length.
packages/api/src/tasks/checkAlerts/tests/renderAlertTemplate.int.test.ts Covers valid title interpolation and the intentional malformed-template fallback.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Start[API startup] --> Connect[Connect to MongoDB]
  Connect --> Query[Find alerts missing name or tags]
  Query --> Batch[Process alert IDs in batches]
  Batch --> Sources[Load referenced saved searches and dashboards]
  Sources --> Derive[Derive normalized name and tags]
  Derive --> Write[Conditionally bulk-update alerts]
  Write --> Serve[Complete startup]
Loading

Fix all with Greploop Fix All in Claude Code Fix All in Conductor Fix All in Cursor Fix All in Codex

Reviews (1): Last reviewed commit: "feat(api): backfill alert name and tags ..." | Re-trigger Greptile

Comment on lines +68 to +74
export async function backfillAlertNameAndTags() {
const ids = (
await Alert.find(
{ $or: [NAME_MISSING_FILTER, ...TAGS_MISSING_FILTER.$or] },
{ _id: 1 },
).lean()
).map(doc => doc._id);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Backfill materializes every matching ID

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.

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

No critical issues found. The largest reviewer claim — that the startup backfill blocks Kubernetes readiness and can crash-loop a rollout — was refuted by inspection: /ready (packages/api/src/routers/api/root.ts:46) returns 200 as soon as isMongoConnected() is true, which connectDBWithRetry() establishes before runStartupMigrations() runs, and the HTTP servers are already listening. The backfill delays only start()'s continuation (local-app-mode defaults), not readiness. The remaining findings are efficiency, a behavior change, logging, and test-coverage concerns.

🟡 P2 -- recommended

  • packages/api/src/migrations.ts:70 -- Because the new tags field defaults to undefined, every untaggable alert (all INLINE, untagged saved searches, dangling refs) permanently matches the missing-name/tags filter, so the unindexed Alert.find COLLSCAN and per-batch SavedSearch/Dashboard hydration re-run on every boot of every replica with no completion marker or config kill-switch and never converge.
    • Fix: Gate the backfill behind a persisted completion marker or a config env flag so it becomes a no-op after the first successful run.
    • maintainability, data-migrations, performance, reliability, correctness
  • packages/api/src/tasks/checkAlerts/template.ts:245 -- Backfilling user-authored saved-search/dashboard/tile names into alert.name makes them executable Handlebars title templates, so a name containing valid syntax like {{alert.channel.webhookId}} or {{source}} is now compiled against the full notification view and rendered into the delivered title, a behavior these alerts did not have before.
    • Fix: Store the derived display name in a field distinct from the title-template, or neutralize {{ sequences when backfilling.
    • adversarial, data-migrations
  • packages/api/src/tasks/checkAlerts/template.ts:255 -- A backfilled name that is malformed Handlebars (e.g. truncated mid-expression) now logs at error level on every alert evaluation instead of once, and the backfill widens the population of names that were never authored as templates, so a single unfixable name can emit an error-log stream that trips error-rate monitors.
    • Fix: Log the compile failure at warn/debug and/or validate the template once at write/backfill time instead of on every render.
    • adversarial
  • packages/api/src/__tests__/migrations.int.test.ts:166 -- The inputsUnchangedFilter optimistic-concurrency guard (the mechanism that prevents writing a name derived from stale inputs) and the >BACKFILL_BATCH_SIZE (>500) multi-batch loop are both unexercised, so a regression weakening either would pass CI.
    • Fix: Add tests that mutate an alert's source/reference between read and write asserting no stale write, and that seed >500 backfillable alerts asserting every batch is processed.
    • testing, correctness, kieran-typescript, data-migrations
🔵 P3 nitpicks (7)
  • packages/api/src/migrations.ts:60 -- name.slice(0, ALERT_NAME_MAX_LENGTH) counts UTF-16 code units, so a 512 boundary can split a surrogate pair (lone surrogate in the stored name) or bisect a {{...}} expression into a newly-malformed template.
    • Fix: Truncate on a code-point boundary and avoid cutting inside a Handlebars expression.
  • packages/api/src/migrations.ts:170 -- An alert missing both name and tags emits two updateOne ops, so updatedCount += result.modifiedCount counts it twice, inflating the logged count relative to distinct alerts changed.
    • Fix: Track distinct affected _ids, or rename the field to reflect operations rather than alerts.
  • packages/api/src/migrations.ts:26 -- deriveAlertNameAndTags is typed against ad-hoc { name?: unknown; tags?: unknown } shapes instead of the real ISavedSearch/IDashboard lean types, so a model field rename degrades silently to null rather than failing the type check.
    • Fix: Type the params as Pick<> subsets of the actual model interfaces while keeping the runtime normalizers.
  • packages/api/src/models/alert.ts:103 -- tags is typed string[] | null but the schema uses default: undefined and the write path only ever $sets a string[], so the null arm is an unreachable state consumers must needlessly narrow against.
    • Fix: Type it tags?: string[] to match the persisted shape.
  • packages/api/src/migrations.ts:71 -- The initial scan spreads ...TAGS_MISSING_FILTER.$or, depending on that constant's internal { $or: [...] } shape; rewriting the constant to another form silently drops tag-missing alerts from the scan with no error.
    • Fix: Extract a single reusable missing-fields filter instead of reaching into .$or.
  • packages/api/src/migrations.ts:180 -- runStartupMigrations / migrations.ts naming implies an ordered, tracked migration framework that does not exist, inviting future one-offs to be appended with no versioning or idempotency guards.
    • Fix: Name it for what it does (e.g. backfillOnStartup) or introduce real tracking primitives.
  • packages/api/src/migrations.ts:166 -- Alert.bulkWrite with ordered: false still throws a BulkWriteError after applying successful ops, unwinding the loop before updatedCount accrues and before later batches run, so one un-writable doc skips backfill for all subsequent batches in that run (converges across restarts).
    • Fix: Wrap bulkWrite in try/catch, read modifiedCount from the partial result, and continue to the next batch.

Reviewers (8): correctness, testing, maintainability, data-migrations, reliability, adversarial, performance, kieran-typescript.

Testing gaps:

  • No end-to-end test that a backfilled name containing valid {{...}} Handlebars renders as a live title template (the injection/behavior-change path).
  • No test that a backfilled malformed-Handlebars name falls back to the raw string and does not throw across TILE/INLINE sources (only SAVED_SEARCH asserted).
  • No multibyte/surrogate-pair case for the 512-char truncation (only ASCII 'x'.repeat(600)).
  • No assertion that the steady state (zero backfillable alerts) or repeated runs against permanently-unfillable alerts perform no writes.

Note on prior comments: the 2 existing PR comments are automated (changeset-bot, vercel-bot) with no substantive human feedback to verify, so the previous-comments reviewer was not spawned.

@github-actions

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 324 passed • 1 skipped • 1124s

Status Count
✅ Passed 324
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant