fix(api): read previous alert history with one index seek per group by bcanvural · Pull Request #3509 · ClickHouse/ClickStack

bcanvural · 2026-10-31T12:42:15Z

Summary

getPreviousAlertHistories looks up each group's newest AlertHistory row before every alert check. The {alert, group, createdAt} index is meant to make that one seek per group, but MongoDB fetched every row of the 7-day lookback for the alert. A 1-minute alert with ~1,300 groups writes ~1.9 M rows a day, so its check got slower every day. On our deployment that reached 4 minutes per run and an out-of-memory crash loop on catch-up.

Two details of the pipeline prevented the per-group plan (DISTINCT_SCAN): the compound $group key {alert, group}, and the state: {$ne: ERROR} predicate, which the old index could not answer.

This PR:

adds state as the last field of the index: {alert: 1, group: 1, createdAt: -1, state: 1}. The ERROR exclusion becomes an index bound, so the planner keeps the DISTINCT_SCAN and steps over ERROR keys without fetching those documents;
keeps main's single aggregation, with state: {$ne: 'ERROR'} in $match. The only change is the $group key: _id: '$group' instead of {alert, group}, because a compound _id rules out the DISTINCT_SCAN;
returns the same map as before (callers read group || '') and uses only operators DocumentDB supports; callers are unchanged;
drops the replaced {alert, group, createdAt} index at API startup (dropReplacedAlertHistoryIndex in runStartupMigrations), after Mongoose has built the new one. Mongoose creates indexes but never drops them.

explain() on 20 groups × 200 rows plus 200 group-less ERROR rows, the same on MongoDB 5.0.32 (CI) and 8.0.34:

	plan	keys examined	docs examined
`main`	IXSCAN + FETCH	4,201	4,200
this PR	DISTINCT_SCAN	221	20

At a larger scale (MongoDB 5.0.32): 1,500 groups × 300 one-minute evaluations, plus a group-less ERROR row every minute (450,300 rows): main examines 450,300 docs in ~700 ms; this PR examines 1,500 docs and 1,800 keys in 10–18 ms. Over the full 7-day window main grows with groups × evaluations (about 15 million docs for this alert); this PR stays at one doc per group, plus one key per ERROR row in the window.

Index change (for operators)

The API builds {alert: 1, group: 1, createdAt: -1, state: 1} at startup (Mongoose autoIndex), like the index added in perf: optimize AlertHistory queries (HDX-3706) #1928. On a large AlertHistory collection this takes a while. The build holds an exclusive lock only briefly at the start and the end, and the 30-day TTL bounds the collection's size. The API serves requests during the build.
After the build, the API drops alert_1_group_1_createdAt_-1. Until then, checks keep using it. If the new index is missing (for example, its build failed), the old index is kept.
The check-alerts task connects with autoIndex: false and doesn't touch indexes; the API owns them.
Downgrade: an older API recreates the old index at startup.

How to reproduce

Starting point: make dev-int-style services (MongoDB and ClickHouse from docker-compose.ci.yml); no UI involved.

On main:

Insert AlertHistory rows for one alert: 1,500 groups × 300 evaluations one minute apart, state: 'OK', plus a group-less ERROR row every minute.
explain('executionStats') on the getPreviousAlertHistories pipeline: FETCH > IXSCAN with totalDocsExamined = 450,300.

On this branch:

Same data: DISTINCT_SCAN, totalDocsExamined = 1,500.

How to test on Vercel preview

N/A — non-UI change.

Tests

getPreviousAlertHistories returns the newest non-ERROR row per group, with ERROR rows with and without a group, a group that stopped reporting while in ALERT, rows without a group (OK, then a newer ERROR), and a group with only ERROR rows. It passes on main too: same results.
The perf guard inserts group-less ERROR rows too, and asserts a single aggregate call that examines at most one document per group. It fails on main's index (4,200 docs examined vs ≤ 21).
New: dropReplacedAlertHistoryIndex drops the old index once the new one exists, is idempotent, and keeps the old index while the new one is missing.
The checkAlerts, alerts API and startup-migration integration tests: 522 passed.

References

Written with help from an AI assistant (Claude Code). The numbers come from the integration test setup on main and on this branch.

…lickHouse#3508) getPreviousAlertHistories grouped on a compound _id and filtered state in $match, so MongoDB fetched every history row of the 7-day lookback for each alert instead of the newest row per group. For a 1-minute alert with ~1,300 groups that is ~1.9M rows a day, and the check-alerts run slowed down (and ran out of memory on catch-up) as history grew. Group on _id: '$group' with only index fields in $match, which MongoDB answers with a DISTINCT_SCAN on {alert, group, createdAt}, and look up the newest non-ERROR row separately for a group whose newest row is ERROR. The returned map is unchanged.

changeset-bot · 2026-10-31T12:42:20Z

🦋 Changeset detected

Latest commit: 6ccc760

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	Patch
@hyperdx/app	Patch
@hyperdx/otel-collector	Patch

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 · 2026-10-31T12:42:22Z

@bcanvural is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

github-actions · 2026-10-31T12:42:33Z

Hi @bcanvural, thanks for the pull request!

Before we review code from a first-time contributor, a maintainer vouches for them, and you're not on our list yet. There's nothing else to file and this PR stays open — a maintainer will look at it, usually within a day or two, and vouch for you right here if it's a genuine attempt at the problem.

To make that quick, check the PR description has:

The issue it addresses (ideally one labelled Accepting PR) and why the change is needed
Steps to reproduce the problem and verify the fix, starting from https://play.hyperdx.io or yarn app:dev:local connected to the demo server
Before/after screenshots if anything in the UI changed

More detail in our contributing guide.

github-actions · 2026-10-31T12:43:52Z

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.

🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.

Anything about surrounding code, or asking you to widen the change (hoist a helper,
dedupe with another file, fix other call sites): never fix here, whatever the
severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

1 posted as inline comment(s) on the changed lines.

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

github-actions · 2026-10-31T12:47:16Z

Deep Review

✅ No critical issues found. The correctness and performance reviewers independently traced the rewritten aggregation and the startup index-drop and confirmed the output map shape is preserved, the DISTINCT_SCAN is real on the CI engine (MongoDB 5.0.32), and the migration is replica-safe and idempotent. The remaining items are test-coverage and maintainability recommendations.

🟡 P2 — recommended

packages/api/src/migrations.ts:136 — The replaced/current index names are free-standing string literals that must track the schema's .index({alert, group, createdAt, state}) definition; if that spec is ever reordered or renamed, the guard silently returns early and the old index is never dropped, with no compile-time link to catch it.
- Fix: Derive the index name from the schema's index spec (or co-locate the constant with the .index() call) so the guard cannot drift from the real index name.
- _{maintainability, data-migrations}
packages/api/src/migrations.ts:154 — The IndexNotFound swallow and the non-IndexNotFound rethrow branches of dropReplacedAlertHistoryIndex are never executed: the idempotency test's second call returns early through the names.includes(...) guard, so the concurrent-replica race handler the catch exists for is untested.
- Fix: Add tests that stub dropIndex to throw a MongoServerError with codeName: 'IndexNotFound' (asserting no throw) and a different error (asserting it propagates).
- _{testing, data-migrations, correctness}
packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts:12111 — The totalDocsExamined <= groups + 1 guard proves the one-seek-per-group property only on the CI MongoDB planner; DocumentDB (named as a production target in the aggregation comments) has no DISTINCT_SCAN, so the speed-up is unverified there and could silently regress to a full-window scan.
- Fix: Document the MongoDB-version assumption in the test and track separate DocumentDB perf verification outside CI.
- _{testing, performance, kieran-typescript, project-standards, data-migrations}

🔵 P3 nitpicks (6)

packages/api/src/migrations.ts:140 — If an operator runs the API with autoIndex disabled, the new index is never built, the guard permanently keeps the old index, and getPreviousAlertHistories loses its DISTINCT_SCAN with nothing surfacing the regression.
- Fix: Document that the API must run with autoIndex enabled, or build the new index explicitly with createIndex in the migration instead of relying solely on autoIndex.
packages/api/src/migrations.ts:140 — AlertHistory.init() triggers an unbounded, no-timeout build of the new 4-field index at startup; on a multi-million-row collection this call can run for a long time with no maxTimeMS guard.
- Fix: Document the expected build duration on large collections, or wrap init()/dropIndex() with a timeout so a stuck build cannot hang the migration indefinitely.
packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts:12104 — const explain: any (plus the as any[] pipeline and derived cursor) disables type checking on the exact assertion that guards the regression, so a typo in executionStats/totalDocsExamined would read undefined and throw a confusing error rather than fail cleanly.
- Fix: Type the explain read with a minimal local interface or unknown + narrowing so the guarded field access is checked.
- _{kieran-typescript, testing}
packages/api/src/migrations.ts:136 — Both constants name group indexes, so ALERT_HISTORY_GROUP_INDEX does not convey that it is the new/current one that must exist before the drop.
- Fix: Name the pair on one axis, e.g. REPLACED_...GROUP_INDEX and CURRENT_...GROUP_INDEX, so the guard reads unambiguously.
packages/api/src/__tests__/migrations.int.test.ts:245 — afterAll calls AlertHistory.createIndexes() immediately before closeDB(), which drops the database, making the rebuild dead code.
- Fix: Remove the redundant createIndexes(), or move index restoration to a beforeEach/afterEach if inter-test isolation was intended.
packages/api/src/migrations.ts:133 — The index drop runs through runStartupMigrations rather than the versioned migrate-mongo framework that agent_docs/architecture.md documents for schema changes (the author notes migrate-mongo only runs in dev; there is in-repo precedent in backfillAlertDisplayFields).
- Fix: Confirm with a maintainer that startup migrations are the sanctioned pattern for index changes.

Reviewers (11): correctness, performance, data-migrations, reliability, kieran-typescript, testing, maintainability, project-standards, previous-comments, learnings-researcher, adversarial (timed out — no result).

Testing gaps:

The IndexNotFound swallow / rethrow race path of the index-drop migration is not exercised.
The DISTINCT_SCAN one-seek-per-group guarantee is verified only on CI MongoDB 5.0.32, not on DocumentDB.
The realistic worst case — a long streak of ERROR rows newer than a group's newest non-ERROR row within the same group — is unmeasured; the perf test places all ERROR rows in a group-less bucket, so same-group index-key-skip cost is untested.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

bcanvural · 2026-10-31T12:47:42Z

I'd really appreciate being vouched for btw! I'm trying to make metrics work with our 'unified observability' project, where we collect logs/metrics/traces in a single datastore. Opening issues as I see issues around dashboards containing metrics. Mostly using mcp to configure hyperdx, so also testing that path along the way. Cheers!

Address review: the follow-up lookups ran via Promise.all, outside the concurrency limit. Run them one at a time in the slot the alert already holds (queuing them on the same queue could deadlock when it's full). The plan test now checks only the docs-examined bound, not the plan name.

bcanvural · 2026-10-31T13:05:28Z

Thanks for the review. On the three P3s:

Follow-up lookups outside the concurrency limit: fixed in 1abe091. They now run one at a time inside the queue slot the alert already holds, so they never add concurrency, however ERROR rows are keyed. I didn't route them through the same queue: a task waiting on subtasks queued behind it can deadlock when the queue is full.
'' and missing groups sharing a map key: this predates the PR (computeHistoryMapKey and group || '' are unchanged), so I've left it here. New rows don't store '' either: histories are saved with group: groupKey || undefined.
Plan-coupled test: the test now asserts only the docs-examined bound (at most one row per group), which is the property that matters. main fails it with 4,000 rows examined vs ≤ 21. A comment notes that MongoDB 5.0–8.0 use a DISTINCT_SCAN here.

All 383 checkAlerts and alerts API integration tests pass.

bcanvural · 2026-10-31T13:33:14Z

Thanks. That's two review rounds, so per AGENTS.md I'll stop changing code here and leave the rest for a maintainer.

Plan-coupled perf test (P2): intended. CI runs MongoDB 5.0.32 (docker-compose.ci.yml), and the guard protects that path: main reads 4,000 rows where the fix reads ≤ 21. DocumentDB isn't in CI. There, the equivalence test still shows the results are identical; only the speed-up can't be checked.
ERROR-row invariant (P3): correctness doesn't depend on it. The follow-up matches the row's own group (group: latest.group ?? null), and the test covers a grouped ERROR row (group b). Only the cost bound does: grouped ERROR rows would mean one lookup per such group, still sequential within the alert's queue slot.
Unrestored spy / any in the test (minor): the spy passes calls through and follows the existing test above it; the anys are only for reading explain() output. Happy to tighten both if a maintainer prefers.

knudtty · 2026-11-02T17:19:11Z

I don't really like the idea of introducing multiple queries here, I'd rather one query fulfill the request. Is it possible to do all in one query and address the identified issue? Potentially by changing the index?

…xed index Add state as the last field of the {alert, group, createdAt} index, so the ERROR exclusion becomes an index bound and the single aggregation keeps a DISTINCT_SCAN. This replaces the per-group ERROR follow-up lookups.

bcanvural · 2026-11-03T13:14:44Z

Thanks, good point. It can be one query if state goes into the index.

The change: {alert: 1, group: 1, createdAt: -1} becomes {alert: 1, group: 1, createdAt: -1, state: 1}, and the query is main's single aggregation again, with state: {$ne: 'ERROR'} in $match. The only other difference from main is the $group key: _id: '$group' instead of {alert, group}, because a compound _id rules out the DISTINCT_SCAN. The follow-up lookups are gone.

With state as the last index field, the ERROR exclusion becomes an index bound, so the planner keeps the DISTINCT_SCAN. It seeks to each group's newest key and steps over ERROR keys inside the index without fetching those documents.

explain() on 20 groups × 200 rows plus 200 group-less ERROR rows. The results are the same on MongoDB 5.0.32 (CI) and 8.0.34:

	plan	keys examined	docs examined
main	IXSCAN + FETCH	4,201	4,200
this PR, first version	DISTINCT_SCAN + one follow-up	22 + follow-up	21 + follow-up
one query, new index	DISTINCT_SCAN	221	20

An ungrouped alert whose newest 50 rows are ERROR: 51 keys, 1 doc.

At a larger scale (MongoDB 5.0.32): 1,500 groups × 300 one-minute evaluations, plus a group-less ERROR row every minute (450,300 rows):

main: 450,300 docs in ~700 ms;
one query: 1,500 docs and 1,800 keys in 10–18 ms.

Over the full 7-day window, main grows with groups × evaluations: about 15 million docs for this alert. The new query stays at one doc per group, plus one key per ERROR row in the window.

The remaining keys are the ERROR rows newer than a group's newest non-ERROR row. They are read as keys only, and there is at most one per failed window, because ERROR rows are upserted per window.

Other notes:

Other index orders I tried:
- {alert, group, state, createdAt} also gets a DISTINCT_SCAN, but $first then returns the newest row of the alphabetically first state, not the newest row. So it's wrong.
- A compound _id: {group, state} falls back to a full IXSCAN (4,201 keys).
Old and new index together: the planner picks the new one. Checked on 5.0 and 8.0.
Index migration:
- The API creates the new index at startup through Mongoose, like the index added in perf: optimize AlertHistory queries (HDX-3706) #1928.
- On a large AlertHistory collection the build takes a while, but it holds an exclusive lock only briefly at the start and the end. The 30-day TTL bounds the collection's size.
- Existing deployments keep the old index, because Mongoose doesn't drop indexes. The old index is a prefix of the new one, so it can be dropped by hand, or left in place at some extra write cost.
- Happy to add a note or a migration if you prefer a specific way.
DocumentDB: I couldn't test it. The results are the same either way (the equivalence test); only the speed-up depends on the planner.
Tests:
- The perf guard now also inserts a group-less ERROR row each minute, and asserts that there is a single aggregate call.
- With main's index it fails: 4,200 docs examined vs ≤ 21.
- All 518 checkAlerts and alerts API integration tests pass.

If this looks good, I'll push it to this PR.

github-actions · 2026-11-03T13:16:17Z

+// `state` comes last so the ERROR exclusion is an index bound and the query
+// keeps a DISTINCT_SCAN. Replaces {alert: 1, group: 1, createdAt: -1}, which is
+// a prefix of this one.
+AlertHistorySchema.index({ alert: 1, group: 1, createdAt: -1, state: 1 });


🟠 major — Old {alert, group, createdAt} index is never dropped, so the busiest collection keeps both indexes on every deployment

Mongoose autoIndex (run by the API; the worker passes autoIndex: false in packages/api/src/tasks/checkAlerts/providers/default.ts:417) only creates indexes and never drops them. Every existing deployment will build {alert:1, group:1, createdAt:-1, state:1} and also keep alert_1_group_1_createdAt_-1. The comment says the new index replaces the old one, but nothing removes it. Nothing reads the old index any more, yet every AlertHistory insert still maintains it, on a collection that this PR's own description puts at about 1.9M rows a day for one alert. Add a migrate-mongo migration under packages/api/migrations/mongo/ that drops alert_1_group_1_createdAt_-1 once the new index exists. Also correct the PR description and changeset: they still say "No index or schema change" and describe ERROR follow-up lookups this diff no longer has. Operators need to know that an index build on a large collection happens at API startup.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

Fixed in 6ccc760: the API now drops alert_1_group_1_createdAt_-1 at startup, in runStartupMigrations, once Mongoose has built the new index (kept if the new one is missing). I used the startup migration rather than migrations/mongo/, because migrate-mongo runs only in dev (make dev-migrate-db against localhost) and no deployment would run it. The changeset and PR description are updated too.

Mongoose creates indexes but never drops them, so every deployment kept {alert, group, createdAt} next to {alert, group, createdAt, state} and paid for both on every insert. runStartupMigrations now drops the old index once the new one exists. Also update the changeset.

github-actions · 2026-11-03T13:31:29Z

+
+export async function dropReplacedAlertHistoryIndex() {
+  // Resolves when Mongoose's index builds finish (autoIndex).
+  await AlertHistory.init();


🔵 minor — AlertHistory.init() makes API startup wait for the whole new index build

Server.start() awaits runStartupMigrations() at packages/api/src/server.ts:103, and await AlertHistory.init() doesn't resolve until Mongoose finishes building {alert, group, createdAt, state}. The changeset says that build "takes a while" on a large collection. Until it finishes, everything after line 103 waits: setupTeamDefaults in local app mode, plus any startup migration added later. If the build fails, init() rejects and only an error is logged. Fix: don't await this step in the startup path (void dropReplacedAlertHistoryIndex().catch(log)), or run it after the other startup work.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

bcanvural · 2026-11-03T16:59:06Z

Update: I pushed this right away rather than waiting, sorry for the confusion in my last comment. The PR now has:

bf3daf2: one query, with the state-suffixed index;
6ccc760: the API drops the replaced index at startup, once the new one is built (from the bot review).

The description is updated. The remaining bot items are minor or P2; happy to change any of them you want. @knudtty, ready for another look when you have time.

bcanvural · 2026-11-05T16:12:56Z

@knudtty, this is now a single query as you suggested: state is in the index ({alert, group, createdAt: -1, state}), and the API drops the replaced index at startup once the new one is built. Could you take another look when you have time?

bcanvural · 2026-11-10T10:19:21Z

@knudtty, a gentle ping on this one when you have a moment. It's the single-query version you suggested, and it still merges cleanly. Happy to update the branch or change anything else.

bcanvural requested a review from a team as a code owner October 31, 2026 12:42

bcanvural requested a review from knudtty October 31, 2026 12:42

github-actions Bot added external needs-vouch Author needs a maintainer to vouch for them labels Oct 31, 2026

github-actions Bot reviewed Nov 3, 2026

View reviewed changes

github-actions Bot mentioned this pull request Nov 4, 2026

docs(mcp): recommend quoted Lucene values for exact matches and bare terms for body text #3548

Merged

This was referenced Nov 4, 2026

fix(mcp): shorten instead of replacing when trimming tool responses, ordered reductions, trim notes #3554

Merged

clickstack_table: no limit, having or default order, so large grouped results come back trimmed in arbitrary order #3555

Closed

Conversation

bcanvural commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Index change (for operators)

How to reproduce

How to test on Vercel preview

Tests

References

Uh oh!

changeset-bot Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

vercel Bot commented Oct 31, 2026

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

github-actions Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review

Uh oh!

github-actions Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deep Review

🟡 P2 — recommended

Uh oh!

bcanvural commented Oct 31, 2026

Uh oh!

bcanvural commented Oct 31, 2026

Uh oh!

bcanvural commented Oct 31, 2026

Uh oh!

knudtty commented Nov 2, 2026

Uh oh!

bcanvural commented Nov 3, 2026

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

bcanvural Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

bcanvural commented Nov 3, 2026

Uh oh!

bcanvural commented Nov 5, 2026

Uh oh!

bcanvural commented Nov 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bcanvural commented Oct 31, 2026 •

edited

Loading

changeset-bot Bot commented Oct 31, 2026 •

edited

Loading

github-actions Bot commented Oct 31, 2026 •

edited

Loading

github-actions Bot commented Oct 31, 2026 •

edited

Loading