Skip to content

feat: Add between and not-between alert thresholds#2130

Merged
kodiakhq[bot] merged 4 commits intomainfrom
drew/between-thresholds
Apr 17, 2026
Merged

feat: Add between and not-between alert thresholds#2130
kodiakhq[bot] merged 4 commits intomainfrom
drew/between-thresholds

Conversation

@pulpdrew
Copy link
Copy Markdown
Contributor

Summary

This PR adds BETWEEN and NOT BETWEEN alert threshold types.

Screenshots or video

Screenshot 2026-04-16 at 2 44 10 PM Screenshot 2026-04-16 at 2 44 17 PM

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 16, 2026

🦋 Changeset detected

Latest commit: 2aafb7b

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

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

vercel bot commented Apr 16, 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 17, 2026 11:03am

Request Review

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

github-actions bot commented Apr 16, 2026

🔴 Tier 4 — Critical

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

Why this tier:

  • Critical-path files (3):
    • packages/api/src/routers/external-api/v2/alerts.ts
    • packages/api/src/tasks/checkAlerts/index.ts
    • packages/api/src/tasks/checkAlerts/template.ts
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

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: 20
  • Production lines changed: 350 (+ 1659 in test files, excluded from tier calculation)
  • Branch: drew/between-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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 16, 2026

PR Review

This PR is well-implemented with validation at multiple layers (Zod schemas, form-level, API), comprehensive test coverage (unit, integration, E2E), and consistent handling across all surfaces. No critical issues found.


  • ⚠️ doesExceedThreshold throws for missing thresholdMax but there is no visible try-catch wrapping the call sites in processAlert. The test 'should not fire or record history when thresholdMax is missing' passes because existing broader error handling in processAlert swallows the exception — this is implicit, brittle behavior. Consider returning false (no alert) instead of throwing when thresholdMax is absent, which makes the safe-default explicit and removes the dependency on ambient error suppression.

  • ⚠️ thresholdMax is not cleared when the user switches from a range type (e.g. between) back to a single-threshold type (e.g. above) in both TileAlertEditor and DBSearchPageAlertModal. The stale value persists in the form and gets submitted to the API. The API ignores it (validation only checks for range types), but the value is stored in MongoDB unnecessarily. A small setValue('thresholdMax', undefined) in the onChange handler for non-range types would clean this up.

  • ✅ Removal of isBuilderSavedChartConfig guards in AppNav.tsx and DashboardsListPage.tsx is safe — the downstream .filter(a => a != null) handles undefined alert fields, and these were pre-existing patterns tightened in this PR.

  • describeThreshold fallback '?' for missing thresholdMax is a reasonable defensive guard for the template rendering path.

@pulpdrew pulpdrew marked this pull request as draft April 16, 2026 18:52
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 16, 2026

E2E Test Results

All tests passed • 146 passed • 3 skipped • 1058s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew pulpdrew force-pushed the drew/between-thresholds branch from 0538da4 to 221ecfe Compare April 16, 2026 19:59
@pulpdrew pulpdrew marked this pull request as ready for review April 16, 2026 20:03
Comment on lines -235 to -237
.map(t =>
isBuilderSavedChartConfig(t.config) ? t.config.alert : undefined,
)
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.

Fixes bug: dashboards with alerts on raw sql tiles were not showing up with an alert indicator on the appnav

@pulpdrew pulpdrew requested review from a team and fleon and removed request for a team April 16, 2026 20:28
@kodiakhq kodiakhq bot merged commit 7953c02 into main Apr 17, 2026
18 checks passed
@kodiakhq kodiakhq bot deleted the drew/between-thresholds branch April 17, 2026 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants