Skip to content

[HDX-4997] Alert evaluations read model + GET /alerts/:id/evaluations - #2833

Merged
kodiakhq[bot] merged 1 commit into
mainfrom
warren/HDX-4997-alert-evaluations-read-model
Aug 10, 2026
Merged

[HDX-4997] Alert evaluations read model + GET /alerts/:id/evaluations#2833
kodiakhq[bot] merged 1 commit into
mainfrom
warren/HDX-4997-alert-evaluations-read-model

Conversation

@wrn14897

@wrn14897 wrn14897 commented Aug 7, 2026

Copy link
Copy Markdown
Member

Linear Issue: HDX-4997

Stack (1/3)

This is the base of a 3-PR stack that splits #2786 for reviewability:

  1. → this PR — evaluations read model + endpoint (api, common-utils)
  2. [HDX-4997] Persist alert evaluation errors and analytics in AlertHistory #2834 — persist evaluation errors/analytics in the alert task (api)
  3. [HDX-4997] Alert detail page with evaluation history #2835 — alert detail page UI (app)

PRs 2 and 3 both base on this branch but are independent of each other; once this merges they can land in either order (GitHub retargets them to main automatically when this branch is deleted on merge).

Why

To surface alert evaluation history (including failures) on a per-alert detail page, we need a read model over AlertHistory that can answer "what happened in each evaluation window?" — including windows that errored, per-group results for group-by alerts, and evaluation analytics. Today AlertHistory only stores OK/ALERT rows and there is no per-alert evaluations API.

What

  • Types (common-utils) for evaluation errors (AlertError/AlertErrorType incl. QUERY_TIMEOUT), per-window evaluations with per-group breakdown (capped at ALERT_EVALUATION_GROUPS_LIMIT, firing-first), and evaluation analytics (queryDurationMs, webhookDurationMs, backfilledBuckets).
  • AlertHistory schema gains optional errors + analytics fields, and AlertState gains ERROR (only ever used on history rows).
  • GET /alerts/:id/evaluations: per-window evaluation history scoped to a startTime/endTime range (clamped to the 31d retention window), grouped across group-by groups newest-first, with a hard-bounded scan of at most ~(limit+1) intervals per request and a server-provided nextBefore cursor that always advances past the scanned slice so paging progresses across gaps instead of stalling.
  • Windows with ERROR rows surface their errors (deduped, newest-first) and rank as ERROR; firing-transition annotations exclude ERROR rows.

Nothing writes ERROR rows or analytics yet — the alert task's write side lands in PR 2 of the stack.

Testing

  • packages/api + packages/common-utils: ci:lint (eslint + tsc), ci:unit green
  • Integration: alertHistory.int.test.ts (new, 80 cases), routers/api/alerts.int.test.ts, and the full *alerts.int* set pass locally (278 tests)

@changeset-bot

changeset-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c32b78e

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

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Minor
@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 7, 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 10, 2026 7:59pm
hyperdx-storybook Ready Ready Preview Aug 10, 2026 7:59pm

Request Review

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 621 production lines changed (Tier 2 max: < 250)
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)
  • Touches API routes or data models — hidden complexity risk

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 6
  • Production lines changed: 621 (+ 766 in test files, excluded from tier calculation)
  • Branch: warren/HDX-4997-alert-evaluations-read-model
  • Author: wrn14897

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

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds the read model and team-scoped API endpoint for retrieving per-window alert evaluations, while extending shared and persistence types for future evaluation errors and analytics.

  • Groups AlertHistory records into paginated evaluation windows with bounded scans and advancing cursors.
  • Adds per-group results, deduplicated errors, analytics, and ERROR-aware transition handling.
  • Exposes GET /alerts/:id/evaluations with range validation, retention clamping, and tenant scoping.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains in the current changeset.

Important Files Changed

Filename Overview
packages/api/src/controllers/alertHistory.ts Adds bounded, cursor-paginated evaluation aggregation with grouped results, error merging, analytics resolution, and ERROR-aware transition filtering.
packages/api/src/routers/api/alerts.ts Adds the authenticated, team-scoped evaluations endpoint with validated limits and retention-clamped time ranges.
packages/api/src/models/alertHistory.ts Extends AlertHistory persistence with optional structured errors and evaluation analytics.
packages/api/src/models/alert.ts Adds the history-only ERROR state; no current write path creates that state in this changeset.
packages/common-utils/src/types.ts Defines the shared evaluation, error, analytics, pagination, and alert group-by response contracts.
packages/app/src/components/alerts/AlertHistoryCards.tsx Extends alert-state presentation to render the new ERROR state consistently.

Sequence Diagram

sequenceDiagram
  participant Client
  participant API as GET /alerts/:id/evaluations
  participant Alert as Alert model
  participant History as AlertHistory
  Client->>API: id, time range, limit, before
  API->>Alert: Load alert scoped to caller's team
  Alert-->>API: Alert interval and metadata
  API->>History: Aggregate bounded createdAt slice
  History-->>API: Grouped evaluation windows
  API-->>Client: data, hasMore, nextBefore
Loading

Reviews (4): Last reviewed commit: "feat(alerts): evaluations read model — E..." | Re-trigger Greptile

Comment thread packages/api/src/models/alert.ts
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 276 passed • 1 skipped • 1105s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

<!-- deep-review -->

Deep Review

🔴 P0/P1 — must fix

  • packages/app/src/components/alerts/AlertHistoryCards.tsx:97ALERT_ERROR_TYPE_LABELS is typed Record<AlertErrorType, string> but supplies only four of the enum's five members, so the QUERY_TIMEOUT member added to AlertErrorType in this diff makes the object literal a TS2739 error and packages/app no longer typechecks.
    • Fix: Add a [AlertErrorType.QUERY_TIMEOUT] entry to ALERT_ERROR_TYPE_LABELS and run tsc on packages/app.
  • packages/api/src/controllers/alertHistory.ts:314fetchStructuredWindows places $limit after $group, so the $push accumulator materializes every matched row before any bound applies, and with no allowDiskUse a group-by alert with a few thousand groups over the 201-window scan slice exceeds Mongo's 100MB $group ceiling and returns a deterministic 500 on every request for that alert.
    • Fix: Resolve the newest N distinct createdAt values in a projection-only stage first, then $match rows to that set before grouping, and pass allowDiskUse as defence in depth.
    • adversarial, security, performance, correctness

🟡 P2 — recommended

  • packages/api/src/controllers/alertHistory.ts:255ALERT_EVALUATION_GROUPS_LIMIT caps groups at 50 per window, but the window-level lastValues returned alongside it is the uncapped flattened union of every group row's buckets, so a high-cardinality grouped alert returns roughly 250x more data than the documented cap implies.
    • Fix: Build lastValues from the retained (post-cap) group rows only, or omit the flattened window-level array for grouped alerts and rely on each group's lastValue.
  • packages/api/src/controllers/alertHistory.ts:412truncatedByScanBound is true whenever the scan slice is narrower than the requested range, which for any short-interval alert is always true on the default 31-day range, so hasMore is true and nextBefore advances by only one slice even when no older data exists — a client following the cursor needs ~223 requests (22,320 at limit=1) to discover an alert has no history.
    • Fix: Probe for any remaining row below the scan floor with a projection-only findOne before setting hasMore, so the flag reflects actual remaining data rather than scan truncation.
  • packages/api/src/models/alert.ts:15 — The new AlertState.ERROR doc comment asserts that ERROR history rows are excluded from scheduling and backfill computations, but getPreviousAlertHistories and getConsecutiveWindowHistories match on {alert, createdAt} with no state filter, so the invariant the comment promises is not implemented anywhere.
    • Fix: Either add the state: { $ne: AlertState.ERROR } filter to the scheduling and consecutive-window lookups, or reword the comment to state that the exclusion is not yet in place.
    • correctness, reliability
  • packages/api/src/routers/external-api/v2/alerts.ts:51 — The hand-written OpenAPI JSDoc still lists AlertState as five values without ERROR and AlertErrorType without QUERY_TIMEOUT, so the published external API spec no longer matches the enums this diff widened.
    • Fix: Add ERROR to the AlertState enum list and QUERY_TIMEOUT to the AlertErrorType enum list in the JSDoc schema block.
  • packages/api/src/controllers/alertHistory.ts:387 — A before cursor greater than endTime, or large enough that new Date(before) is invalid, makes the <= comparison false and silently drops usableBefore, so the request degrades into an unfiltered newest page instead of an empty page or a 400.
    • Fix: Bound before with .max(8640000000000000) in the query schema and return an empty page with hasMore: false when a valid cursor exceeds endTime.
    • correctness, adversarial
  • packages/app/src/components/alerts/AlertHistoryCards.tsx:29groupStateToOverallState can now return ERROR on the pre-existing /alerts and /alerts/:id history arrays, and stateToBgColorClass has no ERROR branch, so an evaluation failure will render in the same red alarm styling as a real firing window with no distinguishing tooltip text.
    • Fix: Add an explicit AlertState.ERROR case with its own style token and tooltip label before the write side starts emitting ERROR rows.
    • api-contract, correctness
  • packages/api/src/controllers/alertHistory.ts:282 — The per-group sort chain falls through state priority to lastValue.count descending and then to group.localeCompare, but every group-ordering test uses groups with distinct states, so neither secondary key is exercised.
    • Fix: Add a case with several same-state groups at differing counts, and two at identical counts, asserting the full resulting order.
🔵 P3 nitpicks (9)
  • packages/api/src/controllers/alertHistory.ts:275group: r.group as string is needed only because the preceding .filter() uses a plain boolean callback that does not narrow, while mapGroupedHistories a few lines above already uses a proper type predicate for an analogous filter.
    • Fix: Change the groupRows filter callback to a type predicate returning r is EvaluationWindowRow & { group: string } and drop the cast.
  • packages/api/src/models/alert.ts:12AlertState is declared independently here and in packages/common-utils/src/types.ts, with no import or compile-time link, even though this same file already imports AlertErrorType and re-exports AlertThresholdType from common-utils.
    • Fix: Import and re-export the common-utils AlertState instead of maintaining a second declaration.
  • packages/api/src/controllers/alertHistory.ts:33GroupedAlertHistory.errors is declared as a required IAlertError[][] while mapGroupedHistories guards it with ?? [] and a comment explaining the field can be missing, so the type contradicts the code that defends against it.
    • Fix: Declare the field optional so the type documents the same uncertainty the guard handles.
  • packages/api/src/controllers/alertHistory.ts:396 — The createdAt filter is typed Record<string, Date>, so a misspelled or invalid Mongo range operator assigned at any of the call sites typechecks cleanly.
    • Fix: Type it as { $gte: Date; $lt?: Date; $lte?: Date }.
  • packages/api/src/routers/api/alerts.ts:190 — The .refine short-circuits when either bound is absent, so a startTime in the future with no endTime yields an inverted range that returns an empty 200 rather than the 400 the sibling /:id/history route would produce.
    • Fix: Validate the resolved post-default bounds, or clamp startTime to endTime.
  • packages/api/src/controllers/alertHistory.ts:299groupsTotal is set to the number of group rows rather than distinct groups, so a window that ends up holding two rows for the same group reports double the true group count and lets a duplicate displace a real group under the cap.
    • Fix: Deduplicate groupRows by group, keeping the highest-priority state, before sorting and slicing.
  • packages/api/src/controllers/alertHistory.ts:262 — Ungrouped rows are excluded from groupRows but still counted in the window's state and counts, so a grouped alert firing on the zero-value path reports state: ALERT with groups omitted entirely.
    • Fix: Emit a synthetic entry for the ungrouped row, or document that a grouped window may legitimately carry no breakdown.
  • packages/api/src/controllers/alertHistory.ts:314 — Neither new aggregation sets maxTimeMS, and the Mongo connection options set no socket timeout, so a slow primary can hold an Express handler and a pooled connection open indefinitely.
    • Fix: Chain .option({ maxTimeMS }) onto the aggregation calls.
  • packages/api/src/routers/api/alerts.ts:187startTime and endTime use .positive() here but plain .int() on the sibling /:id/history route, so startTime=0 is a 400 on one endpoint and a 200 on the other.
    • Fix: Align the two routes on the same numeric constraint.

Reviewers (9): correctness, adversarial, testing, api-contract, performance, security, reliability, maintainability, kieran-typescript.

Testing gaps:

  • No test writes a group-by alert with high per-window row cardinality against /:id/evaluations, which is the exact condition behind the aggregation fan-out finding.
  • No test asserts hasMore === false for a default request against an alert with only a few windows of history.
  • No test covers before greater than endTime, below startTime, or beyond the valid Date range — all three currently take silent fallback paths.
  • No test covers count truncation and scan-bound truncation in the same page, or pins the $gte/$lt boundary at exactly the scan floor across two pages.
  • The 403 branch on the new route and the limit min/max boundaries (0, 1, 200, 201) are unexercised.
  • No end-to-end assertion that an ERROR history row serializes correctly through formatAlertResponse on /alerts and /alerts/:id.

Environment note: Bash, Grep, and Glob were non-functional in this session (sandbox bootstrap failure), so the diff could not be computed with git; scope was reconstructed by reading the files named in the change and verifying each finding against current file contents. Reviewers requiring repository-wide search — learnings-researcher, agent-native, and previous-comments — were not run.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: GET /alerts/:id/evaluations + AlertHistory evaluations read model (9 files, ~1370 additions). Base: 463fd6a1. Mode: report-only (read-only; no edits, no artifacts).

No critical issues found. Pagination cursor math was traced end-to-end by two independent reviewers with no stall/skip/duplication; auth and tenant isolation mirror the sibling /:id/history endpoint exactly; query params are int-coerced and bounded; no happy-path crash, data loss, or authz bypass is introduced. The items below are recommended hardening.

🟡 P2 -- recommended

  • packages/api/src/controllers/alertHistory.ts:313 -- fetchStructuredWindows $group $pushes every matched row into a per-window rows array before the post-$group $limit, with no allowDiskUse and no DB-side row cap; a group-by alert with high group cardinality can exceed Mongo's 16MB per-document / 100MB stage limit and hard-fail the aggregation, 500ing the exact per-group feature this endpoint ships (the ALERT_EVALUATION_GROUPS_LIMIT=50 cap is applied in JS after the fetch).
    • Fix: cap rows-per-window inside the pipeline (e.g. $slice on the $push, or reduce group rows server-side before $group) rather than relying on the post-fetch JS slice.
    • performance, adversarial
  • packages/app/src/components/alerts/AlertHistoryCards.tsx:35 -- stateToBgColorClass falls through to default: return styles.alarm, so once AlertState.ERROR becomes representable (added here and surfaced by the shared groupStateToOverallState, which also feeds the existing /alerts and /alerts/:id history), failed-evaluation windows render as a red firing indicator indistinguishable from a real ALERT.
    • Fix: add an explicit case AlertState.ERROR mapping to a distinct error style so failures are not mislabeled as firing.
    • api-contract
  • packages/api/src/controllers/alertHistory.ts:94 -- fetchGroupedWindows/mapGroupedHistories and fetchStructuredWindows/mapStructuredWindow are structurally near-identical (same match/sort/group/limit skeleton, duplicated error-dedupe and lastValues flatten/sort), so a fix to error handling or window grouping must be made in two places or the two response shapes silently drift.
    • Fix: factor the shared aggregate skeleton and the error/lastValues reducers into one helper, or derive the grouped result as a projection of the structured one.
    • maintainability
  • packages/api/src/controllers/alertHistory.ts:162 -- the controller hand-declares AlertEvaluationEntry/AlertEvaluationGroupEntry (and the AlertState/IAlertError enums in models/alert.ts) as verbatim copies of the common-utils zod-inferred types; the boundary type-checks only because PreSerialized widens string enums to string, so a member added to one enum but not the other is not caught at compile time.
    • Fix: import the inferred AlertEvaluation/AlertEvaluationGroup types and re-export AlertState/IAlertError from common-utils instead of redeclaring them, keeping a single source of truth.
    • maintainability, kieran-typescript
🔵 P3 nitpicks (6)
  • packages/api/src/routers/api/alerts.ts:188 -- startTime/endTime/before are validated as positive ints with no upper bound, so a value beyond the max JS date range yields new Date(NaN) bounds that reach the aggregation as an Invalid Date and produce a 500 (BSON cast error) instead of a 400.
    • Fix: add an upper bound to the numeric query params (or clamp/reject dates outside the valid range) before constructing Dates.
  • packages/api/src/controllers/alertHistory.ts:411 -- for a sparse 1m-interval alert over the clamped 31-day span, each page advances the cursor by only ~(limit+1) intervals across empty gaps, so a "load all" client can trigger ~200+ sequential empty aggregations.
    • Fix: on an empty scan slice, probe for the next-older window with data and skip the cursor to it instead of returning empty pages.
  • packages/api/src/routers/api/alerts.ts:190 -- the refine only enforces startTime < endTime when both are supplied, so a future startTime with omitted endTime passes validation and silently returns an empty page rather than a 400.
    • Fix: validate startTime against the effective (defaulted) endTime after applying the default.
  • .changeset/alert-evaluations-read-model.md:4 -- @hyperdx/app is declared patch but is in a fixed release group with @hyperdx/api (minor), so the declared bump is inconsistent and app will actually be bumped to minor.
    • Fix: set @hyperdx/app to minor or omit it so it inherits the group bump.
  • packages/api/src/controllers/alertHistory.ts:274 -- groupRows is filtered with a non-narrowing predicate, forcing the r.group as string cast.
    • Fix: use a type-predicate filter ((r): r is EvaluationWindowRow & { group: string } => ...) so the cast can be dropped.
  • packages/api/src/controllers/alertHistory.ts:18 -- ALERT_EVALUATION_GROUPS_LIMIT is re-exported from the controller solely for a test, which could import it from @hyperdx/common-utils directly like the app does.
    • Fix: drop the re-export and import the constant from common-utils in the test.

Pre-existing (not introduced by this diff; excluded from verdict)

  • packages/api/src/controllers/alertHistory.ts:443 -- getRecentAlertHistoriesBatch awaits Promise.all over per-alert queries, so one failing alert query rejects the whole /alerts list (500). Verified unchanged from base 463fd6a1. Worth an allSettled-style isolation follow-up so one bad alert degrades to an empty-history row.

Agent-Native Gaps

  • No MCP tool exposes the new evaluations read model; clickstack_get_alert still returns only getRecentAlertHistories({ limit: 20 }). Agents debugging an alert cannot time-scope, paginate, or see per-group/error breakdowns that the UI will get. The reusable getAlertEvaluations primitive makes a thin clickstack_get_alert_evaluations wrapper cheap — reasonable as a stack follow-up alongside the detail-page PR.

Reviewers (12): correctness, adversarial, security, api-contract, performance, reliability, testing, maintainability, kieran-typescript, project-standards, agent-native, learnings-researcher.

Testing gaps:

  • No regression test asserts the cross-tenant (404) or unauthenticated (403) path for /alerts/:id/evaluations specifically.
  • The before > endTime cursor-ignore branch (usableBefore fallback) is never exercised — every pagination test uses an in-range cursor.
  • dedupeErrors collapse-by-type||message and newest-first ordering are unverified (no window has duplicate errors).
  • resolveWindowAnalytics undefined-return branch (no analytics + ≤1 distinct bucket) is hit but never asserted.
  • No unit-level assertion that a scan-bound-truncated empty page returns hasMore=true with nextBefore advanced to scanFloor (the core cross-gap guarantee).
  • No test parses a response through AlertEvaluationsApiResponseSchema, so controller-vs-schema drift would go uncaught.

Residual risks: new aggregations set no maxTimeMS (consistent with existing pattern); surfaced errors[].message is returned verbatim, so confirm it is sanitized at write time (write side lands in a later PR); MAX_HISTORY_SPAN_MS (31d) exceeds the AlertHistory TTL (30d), so the far end of the clamped span is always empty (harmless).

// firing transitions for chart annotations, which is a different shape.
const EVALUATIONS_LIMIT = 200;
type AlertEvaluationsExpRes = express.Response<AlertEvaluationsApiResponse>;
router.get(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not used yet

pulpdrew
pulpdrew previously approved these changes Aug 10, 2026
…up windows (HDX-4997)

AlertHistory read-side support for the alert detail page:

- Types (common-utils) for evaluation errors (AlertError/AlertErrorType incl.
  QUERY_TIMEOUT), per-window evaluations with per-group breakdown (capped at
  ALERT_EVALUATION_GROUPS_LIMIT, firing-first), and evaluation analytics
  (queryDurationMs, webhookDurationMs, backfilledBuckets).
- AlertHistory schema gains optional errors + analytics fields, and AlertState
  gains ERROR (only ever used on history rows).
- GET /alerts/:id/evaluations: per-window evaluation history scoped to a
  startTime/endTime range (clamped to the 31d retention window), grouped
  across group-by groups newest-first with a hard-bounded scan of at most
  ~(limit+1) intervals per request and a server-provided nextBefore cursor
  that always advances past the scanned slice, so paging progresses across
  gaps instead of stalling.
- Windows with ERROR rows surface their errors (deduped, newest-first) and
  rank as ERROR; firing-transition annotations exclude ERROR rows.

Nothing writes ERROR rows or analytics yet — the alert task's write side
lands separately.
@kodiakhq
kodiakhq Bot merged commit 05a3fd8 into main Aug 10, 2026
27 checks passed
@wrn14897
wrn14897 deleted the warren/HDX-4997-alert-evaluations-read-model branch August 10, 2026 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants