Skip to content

feat: Add additional alert threshold types#2122

Open
pulpdrew wants to merge 4 commits intomainfrom
drew/alert-thresholds
Open

feat: Add additional alert threshold types#2122
pulpdrew wants to merge 4 commits intomainfrom
drew/alert-thresholds

Conversation

@pulpdrew
Copy link
Copy Markdown
Contributor

@pulpdrew pulpdrew commented Apr 15, 2026

Summary

This PR adds new types of alert thresholds: >, <=, =, and !=.

Screenshots or video

Screen.Recording.2026-04-15.at.10.20.08.AM.mov

How to test locally or on Vercel

This must be tested locally, since alerts are not supported in the preview environment.

To see the notification content, run an echo server locally and create a webhook that targets it (http://localhost:3000):

npx http-echo-server

References

  • Linear Issue: Closes HDX-3988
  • Related PRs:

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 15, 2026

🦋 Changeset detected

Latest commit: badd661

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

This PR includes changesets to release 5 packages
Name Type
@hyperdx/common-utils Patch
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/cli 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

@github-actions github-actions bot added the review/tier-4 Critical — deep review + domain expert sign-off label Apr 15, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Critical-path files (4):
    • packages/api/src/routers/external-api/v2/alerts.ts
    • packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.test.ts
    • packages/api/src/tasks/checkAlerts/index.ts
    • 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
  • Files changed: 18
  • Lines changed: 236 (+ 1099 in test files, excluded from tier calculation)
  • Branch: drew/alert-thresholds
  • Author: pulpdrew

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

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 15, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Apr 15, 2026 5:15pm

Request Review

@pulpdrew pulpdrew added review/tier-3 Standard — full human review required and removed review/tier-4 Critical — deep review + domain expert sign-off labels Apr 15, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 15, 2026

PR Review

✅ Overall this is a clean, well-tested feature. No critical blockers found.

  • ⚠️ Float equality via === in doesExceedThreshold (checkAlerts/index.ts:1403-1404) → For EQUAL/NOT_EQUAL, floating-point arithmetic can produce unexpected mismatches (e.g., 0.1 + 0.2 !== 0.3). The UI note about "floating-point results are not rounded" is a good disclosure, but consider whether an epsilon-based comparison (e.g., Math.abs(value - threshold) < Number.EPSILON) would be safer for user-facing alerting — or at least document this as a known limitation in the API docs.

  • ⚠️ parseInt silently truncates floats returned as strings (checkAlerts/index.ts:1418). The comment explains this is safe under default ClickHouse settings, but EQUAL/NOT_EQUAL on a threshold like 2.5 would silently fail if a quoted float ever appeared. The previous parseFloat revert commit (fb672b4d) makes this a known, accepted trade-off — fine as-is, but worth a follow-up test case documenting that edge.

  • ℹ️ doesExceedThreshold has no exhaustive default (checkAlerts/index.ts:1393-1407). TypeScript will correctly mark this as a compile error if a new enum value is ever added without updating the switch — which is the desired behavior. No action needed, just confirming this is intentional.

  • ✅ Mongoose schema (enum: AlertThresholdType) and Zod schema (z.nativeEnum(AlertThresholdType)) both automatically pick up the new values — no migration needed.

  • ✅ "grouped by" warning logic is correct: ABOVE/ABOVE_EXCLUSIVE don't alert on missing data (0 > positive_threshold = false), while BELOW/BELOW_OR_EQUAL/EQUAL/NOT_EQUAL can, so warnings are appropriately scoped.

  • ✅ Comprehensive unit + integration tests covering boundary conditions for all 4 new types across both SAVED_SEARCH and TILE sources.

@pulpdrew pulpdrew force-pushed the drew/alert-thresholds branch from 9b44f74 to c0dd8c9 Compare April 15, 2026 14:28
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 15, 2026

E2E Test Results

All tests passed • 133 passed • 3 skipped • 1069s

Status Count
✅ Passed 133
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew pulpdrew requested review from a team and teeohhem and removed request for a team April 15, 2026 14:38
return 'exceeds';
case AlertThresholdType.BELOW:
case AlertThresholdType.BELOW_OR_EQUAL:
return 'falls below';
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.

This may be misleading if the value is equal to the threshold, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good callout. I was following the existing pattern, where >= used exceeds/falls below, but I have updated these to be more precise.

for (const [k, v] of Object.entries(data)) {
if (meta.valueColumnNames.has(k)) {
value = isString(v) ? parseInt(v) : v;
value = isString(v) ? parseFloat(v) : v;
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.

I think the parseFloat is the right behavior here, nice change.

However, it will potentially make existing alerts behave/trigger differently so it may be worth a bigger callout somewhere.

It also makes the equality case particularly interesting (e.g. 3.0 == 3.0000002). Do we give the user control over rounding etc....?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the parseFloat is the right behavior here, nice change.
However, it will potentially make existing alerts behave/trigger differently so it may be worth a bigger callout somewhere.

Thanks for commenting on this. I did some more research, and it turns out parseInt was actually correct, since only integer columns are potentially returned as strings, so can be parsed as integers reliably for the formats and settings we use. I've reverted the change with a comment for the next person who finds this "bug."

It also makes the equality case particularly interesting (e.g. 3.0 == 3.0000002). Do we give the user control over rounding etc....?

Yeah this is a good question. I think maybe one way to do it is to support BETWEEN and NOT BETWEEN threshold types, which would give the user a straightforward way to specify their preferred range, while also being re-usable for other cases (like ranges >1). I can do that in a followup PR if that sounds like a reasonable solution.

I've also added a UI element to tell the user that floating point values are not rounded when compared for equality

Screenshot 2026-04-15 at 1 10 51 PM

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

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants