[HDX-5090] Support creating alerts without saved searches or dashboard tiles (backend) - #3010
Merged
Merged
Conversation
wrn14897
added
the
ai-generated
🦋 Changeset detectedLatest commit: 1e7155c The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThe PR adds detached inline alerts whose chart configuration is persisted directly on the alert and evaluated through the existing chart-alert pipeline.
Confidence Score: 4/5The PR should not merge until equivalent uppercase ObjectIds are accepted consistently by the raw-SQL source/connection validation. The prior reply claims the ObjectId.equals change accepts uppercase hexadecimal IDs, but BSON 4.7.2 compares a 24-character string against the lowercase hexadecimal representation, so the unchanged raw string remains a concrete counterexample and valid inline-alert writes are rejected. Files Needing Attention: packages/api/src/controllers/alerts.ts and packages/api/src/tasks/checkAlerts/providers/default.ts
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/types.ts | Adds the inline alert source and shared chart-config schemas used across the API and worker. |
| packages/api/src/utils/zod.ts | Adds the internal inline-alert request union and formula validation while preserving the narrower external schema. |
| packages/api/src/controllers/alerts.ts | Persists and validates inline alert configurations, but its attempted ObjectId normalization still rejects equivalent uppercase connection IDs. |
| packages/api/src/tasks/checkAlerts/providers/default.ts | Loads inline alert query dependencies and guards stale raw-SQL source metadata, while repeating the noncanonical ObjectId comparison issue. |
| packages/api/src/tasks/checkAlerts/index.ts | Reuses saved chart assembly and evaluation behavior for inline alert tasks. |
| packages/api/src/routers/api/alerts.ts | Enables internal inline-alert CRUD and returns chartConfig from the single-alert response. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[Internal alerts API] --> Validate[Validate inline chart config]
Validate --> Mongo[(Persist alert and chartConfig)]
Mongo --> Worker[check-alerts worker]
Worker --> Resolve[Resolve source and connection]
Resolve --> Query[Render and execute chart query]
Query --> Evaluate[Evaluate threshold and history]
Evaluate --> Notify[Send notification with chart explorer link]
Reviews (9): Last reviewed commit: "Merge branch 'main' into warren/HDX-5090..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
E2E Test Results✅ All tests passed • 323 passed • 1 skipped • 1366s
Tests ran across 4 shards in parallel. |
wrn14897
marked this pull request as ready for review
August 27, 2026 05:41
github-actions
Bot
added
the
review/tier-4
Contributor
🔴 Tier 4 — CriticalTouches 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:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Contributor
Deep ReviewBackend foundation for detached "inline" alerts ( 🔴 P0/P1 — must fix
🟡 P2 — recommended
🔵 P3 nitpicks (5)
Reviewers (10): correctness, security, testing, maintainability, project-standards, api-contract, reliability, adversarial, kieran-typescript, previous-comments. Testing gaps:
|
Add a 'chart' alert source that persists its own chart config directly on the alert document, so alerts no longer require a saved search (logs) or a dashboard tile (metrics). Builder configs on log/trace/metric sources plus raw SQL (Line/StackedBar/Number) are supported; PromQL is rejected. - common-utils: AlertSource.CHART, AlertChartConfigSchema, zChartAlert, AlertSchema union member, chartConfig on AlertsPageItemSchema - model: chartConfig (Mixed) on Alert; makeAlert persists/clears it like the other source references - internal API: internalAlertSchema accepts the new source (external v2 keeps the narrower alertSchema until its contract is extended); validateAlertInput checks display type, raw SQL template, and team-scoped source/connection ownership; responses include chartConfig - check-alerts: new CHART task type evaluated through the same code path as tile alerts (shared buildAlertChartConfigFromSavedConfig), including group-by and multi-window behavior; notifications link to the chart explorer seeded with the alert's config and default their title to the config's name Backend only; the creation/edit UI and external API v2 support land separately.
… consistency (HDX-5090) Two write-path gaps in the new chart alert source, both of which would otherwise persist configs that fail on every evaluation tick: - Builder configs skipped validateChartConfigFormulas (dashboards get it from the editor and the external tile refinement, but chart alerts are authored through this API directly). internalAlertSchema now rejects malformed formulas, references to nonexistent series, and formulas combined with seriesReturnType: 'ratio' (mapped onto the helper's external-shape asRatio). - Raw-SQL configs accepted a team-owned source on a different team-owned connection. The worker executes through chartConfig.connection while expanding $__sourceTable/metricTables from the source, so a divergent pair yields wrong values or repeated query failures. validateAlertInput now requires the source to belong to the configured connection.
objectIdSchema admits every representation ObjectId.isValid accepts (uppercase hex, 12-byte strings) and the Mongo lookups cast them, but the raw-SQL source/connection consistency check compared strings — so an equivalent non-canonical connection ID resolved both documents and was then rejected as a mismatch. Compare via ObjectId.equals instead.
…ves connections (HDX-5090) Write-path validation ties a raw-SQL chart alert's source to its connection at save time, but a source can be moved to a different connection afterwards. The worker executes through the config's pinned connection while expanding $__sourceTable/metricTables from the source, so a moved source silently targeted the wrong database (when the same table exists there) or failed confusingly. Extract the optional source lookup shared by the tile and chart raw-SQL branches into getRawSqlSourceMetadata, which now drops (with a warning) a source whose connection no longer matches the config's. Templates that don't use source macros keep evaluating correctly; templates that do fail with a visible query error recorded on the alert instead of returning wrong values. This intentionally also covers raw-SQL tile alerts, which had the identical pre-existing exposure.
'chart' collided with existing vocabulary: ChartAlertBaseSchema and the
alerts-page copy ("created from dashboard charts") both already use
"chart" to mean tile alerts, and the enum's other values name the
referenced entity while the new kind's defining property is that the
definition lives inline on the alert with no referenced entity.
Wire value 'chart' -> 'inline' (unreleased, so no migration), plus the
matching identifiers: AlertSource.INLINE, AlertTaskType.INLINE,
zInlineAlert, getInlineAlertDetails, validateInlineAlertFormulas, and the
test fixtures. Shape-named identifiers keep their names (chartConfig,
AlertChartConfigSchema, buildChartExplorerLink) since they describe the
config's content or link target, not the source kind.
wrn14897
force-pushed
the
warren/HDX-5090-detached-alerts-backend
branch
from
August 27, 2026 17:00
b4c939f to
14175ec
Compare
jordan-simonovski
requested changes
Aug 28, 2026
GET /alerts is unpaginated, so attaching every inline alert's full chart config (raw SQL templates included) would bloat every alerts-page load at the 1000+-rule scale this feature targets, and bake in a collection contract that couldn't be paginated away later. formatAlertResponse gains an includeChartConfig flag; only GET /alerts/:id sets it, keeping the full config on the detail response for edit surfaces. A slim list-side summary (name/display type) is deferred to the UI PR that will consume it.
jordan-simonovski
approved these changes
Aug 28, 2026
Comment on lines
+165
to
+169
| if ( | ||
| !new Types.ObjectId(String(source.connection)).equals( | ||
| chartConfig.connection, | ||
| ) | ||
| ) { |
Contributor
There was a problem hiding this comment.
When a raw-SQL inline alert supplies its connection as an equivalent uppercase 24-character ObjectId, ObjectId.equals receives the original uppercase string and compares it against the lowercase hexadecimal representation, causing the API to reject a valid source/connection pair. The same comparison in the worker can discard valid source metadata from an already persisted noncanonical config.
Knowledge Base Used: API services and background tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backend foundation for detached alerts (HDX-5090): a new
inlinealert source that persists its own chart config directly on the alert document, so alerts no longer require a saved search (logs) or a dashboard tile (metrics). This unblocks customers (e.g. Epidemic Sound, migrating 1000+ Grafana alert rules) for whom creating a saved search or dashboard tile per alert does not scale.The persisted config is the exact shape a dashboard tile stores (
SavedChartConfigminus the embeddedalertfield), so inline alerts evaluate through the same battle-tested code path as tile alerts:AlertSource.INLINE, exportedAlertChartConfigSchema(builder + raw SQL, no PromQL),zInlineAlert, newAlertSchemaunion member, and optionalchartConfigonAlertsPageItemSchema.chartConfig(Mixed) on the Alert document;makeAlertpersists it for inline alerts and clears it when the source changes (mirrors savedSearch/dashboard reference clearing). No migration needed.internalAlertSchemaaccepts the inline source and validates metric formulas on builder configs (expression parse, series references, ratio exclusivity);validateAlertInputenforces supported display types (Line/Stacked Bar/Number), validates raw SQL templates, and checks team-scoped source/connection ownership — including that a raw-SQL config's source belongs to its connection (compared as ObjectIds). The single-alert response includeschartConfigfor edit surfaces; the unpaginated list omits it.AlertTaskType.INLINE; the tile-alert config assembly is factored into a sharedbuildAlertChartConfigFromSavedConfigused by both tile and inline alerts, so group-by, multi-window, formulas, ratio mode, and raw SQL behavior are identical. A raw-SQL source that was moved to a different connection after alert creation has its metadata dropped with a warning (also closes the identical pre-existing tile-alert exposure). Notifications link to the chart explorer seeded with the alert's config over the alerting window, and default their title to the config's name.Deliberately out of scope (follow-ups):
alertSchemaand rejectssource: 'inline'(guarded by a test) until its OpenAPI/Terraform contract is extended. v2 GETs echo inline alerts read-only.isImportableAlert.How to test on Vercel preview
N/A — non-UI change (backend only; no UI creates inline alerts yet).
Testing done:
make ci-lint,make ci-unit— passrouters/api/alerts.int(62 tests, incl. inline-alert CRUD + validation), fullcheckAlerts.int(176 tests, incl. end-to-end inline alert evaluation + grouped notification + template link/title),external-api/alerts.int(54 tests, incl. the v2 rejection guard),checkAlerts/providers/default.int(36 tests, incl. stale source-connection scenarios)References