[HDX-5090] Support creating alerts without saved searches or dashboard tiles (backend) - #3010
[HDX-5090] Support creating alerts without saved searches or dashboard tiles (backend)#3010wrn14897 wants to merge 2 commits into
Conversation
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.
wrn14897
added
the
ai-generated
🦋 Changeset detectedLatest commit: 9bdd3e8 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.
|
Greptile SummaryThe PR adds backend support for detached chart alerts, including persistence, internal API validation, evaluation, and notification links. The source/connection fix needs canonical ObjectId comparison so valid equivalent identifiers are not rejected.
Confidence Score: 4/5The PR should not merge until valid equivalent ObjectId representations pass the new source/connection association check. The new case-sensitive string comparison can reject a raw-SQL chart alert even after MongoDB resolves its source and connection to the same ObjectId. Files Needing Attention: packages/api/src/controllers/alerts.ts
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/types.ts | Adds the chart alert source and persisted chart-config schemas while excluding PromQL. |
| packages/api/src/utils/zod.ts | Adds the internal chart-alert union and write-time formula validation. |
| packages/api/src/controllers/alerts.ts | Persists and validates detached chart configurations, but compares equivalent ObjectIds using their unnormalized string representations. |
| packages/api/src/tasks/checkAlerts/index.ts | Shares chart-query assembly between tile and detached chart alerts and adds chart task handling. |
| packages/api/src/tasks/checkAlerts/providers/default.ts | Loads detached chart alert sources and connections for worker evaluation. |
| packages/api/src/tasks/checkAlerts/template.ts | Generates chart-explorer links and chart-config-based notification titles. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[Internal alerts API] --> Schema[Parse internal alert schema]
Schema --> Validate[Validate chart config]
Validate --> Persist[(Alert document)]
Persist --> Worker[Check-alerts worker]
Worker --> Query[Build and execute chart query]
Query --> Notify[Send notification with explorer link]
Reviews (2): Last reviewed commit: "fix(api): validate chart alert formulas ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export const AlertChartConfigSchema = z.union([ | ||
| BuilderSavedChartConfigWithoutAlertSchema, | ||
| RawSqlSavedChartConfigWithoutAlertSchema, | ||
| ]); |
There was a problem hiding this comment.
When a detached builder alert contains a malformed formula or references a nonexistent series, AlertChartConfigSchema accepts it without applying validateChartConfigFormulas. The evaluator later throws while rendering the formula, repeatedly recording query errors while the alert never fires or resolves.
Knowledge Base Used:
E2E Test Results✅ All tests passed • 318 passed • 1 skipped • 1254s
Tests ran across 4 shards in parallel. |
github-actions
Bot
added
the
review/tier-4
🔴 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
|
Deep ReviewBackend-only foundation for detached ✅ No critical issues found. 🟡 P2 — recommended
🔵 P3 nitpicks (3)
Reviewers (10): correctness, security, adversarial, api-contract, reliability, kieran-typescript, testing, maintainability, project-standards, previous-comments. Testing gaps:
|
… 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.
| // source on a different (even team-owned) connection would query the | ||
| // wrong database — silently wrong values when the table also exists | ||
| // there, repeated query failures when it does not. | ||
| if (source.connection.toString() !== chartConfig.connection) { |
There was a problem hiding this comment.
When a raw-SQL chart alert supplies the source's connection ID in a valid non-canonical representation such as uppercase hexadecimal, the MongoDB lookups resolve both references but this case-sensitive comparison rejects them as different, preventing creation or update of an otherwise valid alert.
1 participant
Summary
Backend foundation for detached alerts (HDX-5090): a new
chartalert 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 chart alerts evaluate through the same battle-tested code path as tile alerts:AlertSource.CHART, exportedAlertChartConfigSchema(builder + raw SQL, no PromQL),zChartAlert, newAlertSchemaunion member, and optionalchartConfigonAlertsPageItemSchema.chartConfig(Mixed) on the Alert document;makeAlertpersists it for chart alerts and clears it when the source changes (mirrors savedSearch/dashboard reference clearing). No migration needed.internalAlertSchemaaccepts the chart source;validateAlertInputenforces supported display types (Line/Stacked Bar/Number), validates raw SQL templates, and checks team-scoped source/connection ownership. Alert responses includechartConfig.AlertTaskType.CHART; the tile-alert config assembly is factored into a sharedbuildAlertChartConfigFromSavedConfigused by both tile and chart alerts, so group-by, multi-window, formulas, ratio mode, and raw SQL behavior are identical. 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: 'chart'(guarded by a test) until its OpenAPI/Terraform contract is extended. v2 GETs echo chart alerts read-only.isImportableAlert.How to test on Vercel preview
N/A — non-UI change (backend only; no UI creates chart alerts yet).
Testing done:
make ci-lint,make ci-unit— passrouters/api/alerts.int(60 tests, incl. new chart-alert CRUD/validation), fullcheckAlerts.int(176 tests, incl. new end-to-end chart alert evaluation + grouped notification + template link/title),external-api/alerts.int(54 tests, incl. the v2 rejection guard),checkAlerts/providers/default.int(33 tests)References