From f80624b3512007e8e8384af6be6b453be912a61b Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:00:29 +0000 Subject: [PATCH 1/6] fix(cli): Send native detector datasets and conditions for metric alerts `sentry alert metrics create/edit` moved onto the detectors API but still sent the legacy alert-rule payload: CLI dataset names (`spans`, `errors`) that the detector `SnubaQueryValidator` rejects, and legacy trigger objects as condition-group conditions that lack the required `type`. There was also no way to target logs. - Map CLI datasets onto native data sources (errors/events -> events, spans/logs -> events_analytics_platform with trace_item_span/trace_item_log, sessions/metrics -> crash-rate metrics) and add the `logs` dataset. - Map them back on read so list/view show CLI names and edit keeps log alerts on trace_item_log. - Translate alertThreshold triggers into threshold + resolution conditions; pass native detector conditions through. - Reject trigger actions (the detector API drops them) and add `--workflow` to connect existing alerts. Fixes #1588 Co-Authored-By: Burak Yigit Kaya --- apps/cli-docs/src/fragments/commands/alert.md | 21 +- .../skills/sentry-cli/references/alert.md | 22 ++- .../cli/src/commands/alert/metrics/create.ts | 37 +++- .../cli/src/commands/alert/metrics/edit.ts | 18 +- .../cli/src/commands/alert/mutation-utils.ts | 107 ++++++++-- packages/cli/src/lib/api/alerts.ts | 173 +++++++++++++--- .../commands/alert/metrics/create.test.ts | 24 +-- .../test/commands/alert/metrics/edit.test.ts | 16 +- .../commands/alert/mutation-utils.test.ts | 71 +++++-- packages/cli/test/lib/api/alerts.test.ts | 185 +++++++++++++++++- 10 files changed, 577 insertions(+), 97 deletions(-) diff --git a/apps/cli-docs/src/fragments/commands/alert.md b/apps/cli-docs/src/fragments/commands/alert.md index ee32b001f..9b6d80c1d 100644 --- a/apps/cli-docs/src/fragments/commands/alert.md +++ b/apps/cli-docs/src/fragments/commands/alert.md @@ -49,16 +49,33 @@ sentry alert issues delete my-org/my-project/12345 --dry-run ### Create a metric alert rule ```bash -# Create an organization metric alert rule +# Alert when P95 span duration goes above 500ms sentry alert metrics create my-org \ --name "P95 Latency" \ + --project my-app \ --query "environment:prod" \ --aggregate "p95(span.duration)" \ --dataset spans \ --time-window 5 \ - --trigger '{"alertThreshold":500,"actions":[{"id":"sentry.mail.actions.NotifyEmailAction","targetType":"Team","targetIdentifier":1}]}' + --trigger '{"alertThreshold":500}' + +# Alert on any matching log line and notify through an existing alert +sentry alert metrics create my-org \ + --name "AI chat failures" \ + --project my-app \ + --query "action:ai.chat result:error" \ + --aggregate "count()" \ + --dataset logs \ + --time-window 5 \ + --trigger '{"alertThreshold":0,"resolveThreshold":0}' \ + --workflow 12345 ``` +Triggers take `alertThreshold`, an optional `label` (`critical` or `warning`), +`resolveThreshold`, and `thresholdType` (`0` above, `1` below). Notification +actions are not part of triggers; pass `--workflow` with the ID of an existing +alert to notify. + ### List metric alert rules ```bash diff --git a/packages/cli/plugins/sentry-cli/skills/sentry-cli/references/alert.md b/packages/cli/plugins/sentry-cli/skills/sentry-cli/references/alert.md index 75108dc2d..fa4b244cf 100644 --- a/packages/cli/plugins/sentry-cli/skills/sentry-cli/references/alert.md +++ b/packages/cli/plugins/sentry-cli/skills/sentry-cli/references/alert.md @@ -155,25 +155,38 @@ Create a metric alert rule - `--name - Rule name` - `--query - Metric query filter string` - `--aggregate - Aggregate expression (for example count(), p95(span.duration))` -- `--dataset - Dataset: errors (error-events), sessions, events, spans, metrics (transaction(s) routes to spans)` +- `--dataset - Dataset: errors (error-events), events, spans, logs, sessions, metrics (transaction(s) routes to spans)` - `--time-window - Evaluation window in minutes` - `-t, --trigger ... - Trigger object JSON (repeatable, or pass one JSON array)` - `-p, --project ... - Project slug filter (repeatable or comma-separated)` - `--environment - Environment filter` - `--owner - Owner value accepted by Sentry API` +- `--workflow ... - Alert (workflow) id to notify (repeatable or comma-separated)` - `-n, --dry-run - Show what would happen without making changes` **Examples:** ```bash -# Create an organization metric alert rule +# Alert when P95 span duration goes above 500ms sentry alert metrics create my-org \ --name "P95 Latency" \ + --project my-app \ --query "environment:prod" \ --aggregate "p95(span.duration)" \ --dataset spans \ --time-window 5 \ - --trigger '{"alertThreshold":500,"actions":[{"id":"sentry.mail.actions.NotifyEmailAction","targetType":"Team","targetIdentifier":1}]}' + --trigger '{"alertThreshold":500}' + +# Alert on any matching log line and notify through an existing alert +sentry alert metrics create my-org \ + --name "AI chat failures" \ + --project my-app \ + --query "action:ai.chat result:error" \ + --aggregate "count()" \ + --dataset logs \ + --time-window 5 \ + --trigger '{"alertThreshold":0,"resolveThreshold":0}' \ + --workflow 12345 ``` ### `sentry alert metrics delete ` @@ -201,12 +214,13 @@ Edit a metric alert rule - `--status - active or disabled` - `--query - Metric query filter` - `--aggregate - Aggregate expression` -- `--dataset - Dataset: errors (error-events), sessions, events, spans, metrics (transaction(s) routes to spans)` +- `--dataset - Dataset: errors (error-events), events, spans, logs, sessions, metrics (transaction(s) routes to spans)` - `--time-window - Evaluation window in minutes` - `-t, --trigger ... - Trigger object JSON (repeatable, or pass one JSON array)` - `-p, --project ... - Project slug filter (repeatable or comma-separated)` - `--environment - Environment value (pass empty string to clear)` - `--owner - Owner value (pass empty string to clear)` +- `--workflow ... - Replace connected alert (workflow) ids (repeatable or comma-separated; empty string disconnects all)` **Examples:** diff --git a/packages/cli/src/commands/alert/metrics/create.ts b/packages/cli/src/commands/alert/metrics/create.ts index 79390420a..cc83f4d4f 100644 --- a/packages/cli/src/commands/alert/metrics/create.ts +++ b/packages/cli/src/commands/alert/metrics/create.ts @@ -15,6 +15,7 @@ import { resolveOrgOnlyFromArg } from "../../../lib/resolve-target.js"; import { normalizeProjectList, parseJsonObjectList, + parseWorkflowIds, resolveMetricDataset, validateMetricDataset, validateMetricTimeWindow, @@ -32,6 +33,7 @@ type CreateFlags = { readonly project?: string[]; readonly environment?: string; readonly owner?: string; + readonly workflow?: string[]; readonly "dry-run": boolean; readonly json: boolean; readonly fields?: string[]; @@ -69,20 +71,26 @@ export const createCommand = buildCommand({ docs: { brief: "Create a metric alert rule", fullDescription: - "Create an organization-scoped metric alert rule.\n\n" + + "Create a metric alert rule (a metric monitor in the detectors API).\n\n" + "Required fields:\n" + " --name, --query, --aggregate, --dataset, --time-window, --trigger (>=1), --project\n\n" + "Optional fields:\n" + - " --environment, --owner\n\n" + + " --environment, --owner, --workflow\n\n" + + "Triggers:\n" + + ' {"alertThreshold":N, "label":"critical"|"warning", "resolveThreshold":N, "thresholdType":0|1}\n' + + " thresholdType 0 alerts above the threshold (default), 1 below. Without resolveThreshold\n" + + " the rule resolves at the critical threshold. Native detector conditions\n" + + ' ({"type":"gt","comparison":N,"conditionResult":75}) are passed through as-is.\n' + + " Notifications are not part of triggers: pass --workflow with an existing alert id.\n\n" + "Examples:\n" + " sentry alert metrics create my-org/ --name 'P95 latency' \\\n" + " --query 'environment:prod' --aggregate 'p95(span.duration)' \\\n" + - " --dataset spans --time-window 5 \\\n" + - ' --trigger \'{"alertThreshold":500,"actions":[{"id":"sentry.mail.actions.NotifyEmailAction","targetType":"Team","targetIdentifier":1}]}\'\n\n' + - " sentry alert metrics create my-org/ --name 'Error volume' \\\n" + - " --query 'event.type:error' --aggregate 'count()' --dataset errors \\\n" + - ' --time-window 15 --trigger \'[{"alertThreshold":100,"actions":[{"id":"sentry.mail.actions.NotifyEmailAction","targetType":"Team","targetIdentifier":1}]}]\' \\\n' + - " --project my-app --dry-run", + " --dataset spans --time-window 5 --project my-app \\\n" + + " --trigger '{\"alertThreshold\":500}'\n\n" + + " sentry alert metrics create my-org/ --name 'AI chat failures' \\\n" + + " --query 'action:ai.chat result:error' --aggregate 'count()' --dataset logs \\\n" + + ' --time-window 5 --trigger \'{"alertThreshold":0,"resolveThreshold":0}\' \\\n' + + " --project my-app --workflow 123 --dry-run", }, output: { human: formatCreated, @@ -119,7 +127,7 @@ export const createCommand = buildCommand({ kind: "parsed", parse: String, brief: - "Dataset: errors (error-events), sessions, events, spans, metrics (transaction(s) routes to spans)", + "Dataset: errors (error-events), events, spans, logs, sessions, metrics (transaction(s) routes to spans)", }, "time-window": { kind: "parsed", @@ -152,6 +160,13 @@ export const createCommand = buildCommand({ optional: true, brief: "Owner value accepted by Sentry API", }, + workflow: { + kind: "parsed", + parse: String, + variadic: true, + optional: true, + brief: "Alert (workflow) id to notify (repeatable or comma-separated)", + }, "dry-run": DRY_RUN_FLAG, }, aliases: { ...DRY_RUN_ALIASES, t: "trigger", p: "project" }, @@ -208,6 +223,10 @@ export const createCommand = buildCommand({ if (flags.owner !== undefined) { body.owner = flags.owner; } + const workflowIds = parseWorkflowIds(flags.workflow); + if (workflowIds) { + body.workflowIds = workflowIds; + } if (flags["dry-run"]) { yield new CommandOutput({ diff --git a/packages/cli/src/commands/alert/metrics/edit.ts b/packages/cli/src/commands/alert/metrics/edit.ts index 6bba1666a..e7aa622e2 100644 --- a/packages/cli/src/commands/alert/metrics/edit.ts +++ b/packages/cli/src/commands/alert/metrics/edit.ts @@ -19,6 +19,7 @@ import { normalizeProjectList, parseJsonObjectList, parseStatusFlag, + parseWorkflowIds, resolveMetricDataset, statusToMetricValue, validateMetricDataset, @@ -42,6 +43,7 @@ type EditFlags = { readonly project?: string[]; readonly environment?: string; readonly owner?: string; + readonly workflow?: string[]; readonly json: boolean; readonly fields?: string[]; }; @@ -62,7 +64,8 @@ function hasMetricMutations(flags: EditFlags): boolean { flags.trigger !== undefined || flags.project !== undefined || flags.environment !== undefined || - flags.owner !== undefined + flags.owner !== undefined || + flags.workflow !== undefined ); } @@ -130,6 +133,9 @@ function applyMetricOptionalFields( if (flags.owner !== undefined) { body.owner = flags.owner.trim() === "" ? null : flags.owner; } + if (flags.workflow !== undefined) { + body.workflowIds = parseWorkflowIds(flags.workflow) ?? []; + } return body; } @@ -222,7 +228,7 @@ export const editCommand = buildCommand({ parse: String, optional: true, brief: - "Dataset: errors (error-events), sessions, events, spans, metrics (transaction(s) routes to spans)", + "Dataset: errors (error-events), events, spans, logs, sessions, metrics (transaction(s) routes to spans)", }, "time-window": { kind: "parsed", @@ -256,6 +262,14 @@ export const editCommand = buildCommand({ optional: true, brief: "Owner value (pass empty string to clear)", }, + workflow: { + kind: "parsed", + parse: String, + variadic: true, + optional: true, + brief: + "Replace connected alert (workflow) ids (repeatable or comma-separated; empty string disconnects all)", + }, }, aliases: { t: "trigger", diff --git a/packages/cli/src/commands/alert/mutation-utils.ts b/packages/cli/src/commands/alert/mutation-utils.ts index 059bd59b8..07c98301a 100644 --- a/packages/cli/src/commands/alert/mutation-utils.ts +++ b/packages/cli/src/commands/alert/mutation-utils.ts @@ -10,6 +10,7 @@ const METRIC_DATASET_VALUES = new Set([ "sessions", "events", "spans", + "logs", "metrics", ]); @@ -40,6 +41,7 @@ const METRIC_DATASET_ALIASES: Record = { session: "sessions", metric: "metrics", span: "spans", + log: "logs", // Dashboard terminology for the error dataset "error-events": "errors", }; @@ -52,6 +54,8 @@ const IS_TRANSACTION_FILTER = "is_transaction:true"; /** Splits a query string on runs of whitespace. */ const QUERY_TOKEN_SEPARATOR = /\s+/; +/** Workflow (alert) ids are numeric. */ +const WORKFLOW_ID_PATTERN = /^\d+$/; const METRIC_TIME_WINDOWS = new Set([ 1, 5, 10, 15, 30, 60, 120, 240, 360, 720, 1440, ]); @@ -298,7 +302,68 @@ export function validateMetricTimeWindow(timeWindow: number): void { ); } -/** Validate that each trigger has an `alertThreshold` and a non-empty `actions` array. */ +const METRIC_TRIGGER_LABELS = new Set(["critical", "warning"]); + +/** Whether a trigger is a native detector condition rather than an `alertThreshold` trigger. */ +function isDetectorCondition(trigger: Record): boolean { + return typeof trigger.type === "string"; +} + +/** + * Validate one native detector condition (`{type, comparison, conditionResult}`). + * The backend checks the operator and priority values; only presence is checked here. + */ +function validateNativeMetricCondition(trigger: Record): void { + if ( + trigger.comparison === undefined || + trigger.conditionResult === undefined + ) { + throw new ValidationError( + "Detector conditions with 'type' must include comparison and conditionResult.", + "trigger", + ); + } +} + +/** Validate one `alertThreshold`-style trigger. */ +function validateThresholdTrigger(trigger: Record): void { + const threshold = trigger.alertThreshold; + if ( + (typeof threshold !== "number" && typeof threshold !== "string") || + !Number.isFinite(Number(threshold)) + ) { + throw new ValidationError( + "Each trigger must include a numeric alertThreshold.", + "trigger", + ); + } + if ( + trigger.label !== undefined && + !METRIC_TRIGGER_LABELS.has(String(trigger.label).toLowerCase()) + ) { + throw new ValidationError( + `Trigger label must be 'critical' or 'warning' (got ${JSON.stringify(trigger.label)}).`, + "trigger", + ); + } + if (Array.isArray(trigger.actions) && trigger.actions.length > 0) { + throw new ValidationError( + "Metric alerts no longer carry notification actions on triggers. " + + "Remove 'actions' and connect an existing alert with --workflow .", + "trigger", + ); + } +} + +/** + * Validate metric alert triggers. + * + * Each trigger is either a threshold (`alertThreshold`, optional `label` + * critical|warning, `resolveThreshold`, `thresholdType` 0 above / 1 below) or + * a native detector condition (`type`, `comparison`, `conditionResult`). + * Notification actions are rejected: the detectors API does not accept them, + * so they would otherwise be dropped silently. + */ export function validateMetricTriggers( triggers: readonly Record[] | undefined, ): void { @@ -309,22 +374,40 @@ export function validateMetricTriggers( ); } + const nativeCount = triggers.filter(isDetectorCondition).length; + if (nativeCount > 0 && nativeCount < triggers.length) { + throw new ValidationError( + "Do not mix detector conditions (with 'type') and alertThreshold triggers.", + "trigger", + ); + } + for (const trigger of triggers) { - const threshold = trigger.alertThreshold; - if (typeof threshold !== "number" && typeof threshold !== "string") { - throw new ValidationError( - "Each trigger must include alertThreshold.", - "trigger", - ); + if (isDetectorCondition(trigger)) { + validateNativeMetricCondition(trigger); + } else { + validateThresholdTrigger(trigger); } - const actions = trigger.actions; - if (!Array.isArray(actions) || actions.length === 0) { + } +} + +/** Parse `--workflow` values (repeatable or comma-separated) into numeric workflow ids. */ +export function parseWorkflowIds( + values: readonly string[] | undefined, +): number[] | undefined { + const ids = normalizeProjectList(values); + if (!ids) { + return; + } + return ids.map((id) => { + if (!WORKFLOW_ID_PATTERN.test(id)) { throw new ValidationError( - "Each trigger must include a non-empty actions array.", - "trigger", + `Workflow id must be numeric (got ${JSON.stringify(id)}).`, + "workflow", ); } - } + return Number(id); + }); } /** Map a human-readable status string to the numeric value the metric alert API expects. */ diff --git a/packages/cli/src/lib/api/alerts.ts b/packages/cli/src/lib/api/alerts.ts index 084c2edc9..d501796e2 100644 --- a/packages/cli/src/lib/api/alerts.ts +++ b/packages/cli/src/lib/api/alerts.ts @@ -205,7 +205,10 @@ function mapDetectorToMetricAlertRule( status: detector.enabled === false ? 1 : 0, query: pickDetectorString(nested.query), aggregate: pickDetectorString(nested.aggregate), - dataset: pickDetectorString(nested.dataset), + dataset: cliDatasetName( + pickDetectorString(nested.dataset), + nested.eventTypes, + ), timeWindow: timeWindowSeconds > 0 ? Math.round(timeWindowSeconds / 60) : 0, environment, owner: pickDetectorOwner(detector.owner), @@ -547,27 +550,144 @@ export async function createIssueAlertRule( // `/alert-rules/` endpoint (getsentry/cli#1274, #1277). /** - * Datasets whose events flow through the error/transaction pipeline - * (`queryType: 0`) rather than the trace-item pipeline (`queryType: 1`). + * How a CLI metric alert dataset maps onto a detector data source. + * + * The CLI keeps the friendly legacy dataset names (`errors`, `spans`, `logs`, + * …), but the detector `SnubaQueryValidator` only accepts native `Dataset` + * values and checks them against `queryType` and `eventTypes`: errors live in + * `events` (type 0), spans and logs in `events_analytics_platform` (type 1) + * distinguished by `eventTypes`, and crash-rate alerts in `metrics` (type 2). + */ +type DetectorDatasetSource = { + readonly dataset: string; + readonly queryType: 0 | 1 | 2; + readonly eventTypes: readonly string[]; +}; + +const ERROR_SOURCE: DetectorDatasetSource = { + dataset: "events", + queryType: 0, + eventTypes: ["error", "default"], +}; +const SPAN_SOURCE: DetectorDatasetSource = { + dataset: "events_analytics_platform", + queryType: 1, + eventTypes: ["trace_item_span"], +}; +const LOG_SOURCE: DetectorDatasetSource = { + dataset: "events_analytics_platform", + queryType: 1, + eventTypes: ["trace_item_log"], +}; +const CRASH_RATE_SOURCE: DetectorDatasetSource = { + dataset: "metrics", + queryType: 2, + eventTypes: [], +}; + +const DETECTOR_DATASET_SOURCES: Record = { + errors: ERROR_SOURCE, + events: ERROR_SOURCE, + spans: SPAN_SOURCE, + logs: LOG_SOURCE, + sessions: CRASH_RATE_SOURCE, + metrics: CRASH_RATE_SOURCE, +}; + +/** + * Resolve the native detector dataset fields for a CLI dataset name. + * + * Unknown names fall back to the span source so an edit of a detector whose + * dataset the CLI does not model keeps the previous behavior. + */ +function detectorDatasetSource(dataset: string): DetectorDatasetSource { + return ( + DETECTOR_DATASET_SOURCES[dataset.trim().toLowerCase()] ?? { + ...SPAN_SOURCE, + dataset, + } + ); +} + +/** + * Map a native detector dataset back onto the CLI dataset name. + * + * Inverse of {@link detectorDatasetSource}, so `view`/`list` show the same + * names `create` accepts and an `edit` round-trip keeps log alerts on + * `trace_item_log`. Unmodelled combinations are returned verbatim. */ -const ERROR_LIKE_DATASETS = new Set(["errors", "events"]); +function cliDatasetName(dataset: string, eventTypes: unknown): string { + const types = Array.isArray(eventTypes) ? eventTypes : []; + if (dataset === "events") { + return "errors"; + } + if (dataset === "events_analytics_platform") { + if (types.includes("trace_item_log")) { + return "logs"; + } + if (types.includes("trace_item_span")) { + return "spans"; + } + } + return dataset; +} + +/** Detector priority levels used as metric condition results. */ +const PRIORITY_BY_LABEL: Record = { critical: 75, warning: 50 }; +const PRIORITY_OK = 0; -/** Detector `queryType` for a metric alert dataset. */ -function datasetQueryType(dataset: string): 0 | 1 { - return ERROR_LIKE_DATASETS.has(dataset.trim().toLowerCase()) ? 0 : 1; +/** Trigger severity label, defaulting to `critical` like the legacy alert-rule API. */ +function triggerLabel(trigger: Record): string { + return String(trigger.label ?? "critical").toLowerCase(); +} + +/** Whether a trigger is already a native detector condition (`{type, comparison, conditionResult}`). */ +function isNativeCondition(trigger: Record): boolean { + return typeof trigger.type === "string"; } /** - * Default `eventTypes` for a metric alert dataset. + * Translate CLI triggers into detector condition-group conditions. * - * Error-like datasets track `error`/`default` events; everything else measures - * spans. The backend accepts an explicit `eventTypes` on the data source and - * derives the internal query type from `queryType`. + * Legacy-shaped triggers (`alertThreshold`, optional `label`, + * `resolveThreshold`, `thresholdType` 0 above / 1 below) become a threshold + * condition per trigger plus the single resolution condition the + * metric detector validator requires. Resolution uses the first + * `resolveThreshold` given, else the critical (or first) alert threshold. + * Native conditions (with a `type`) pass through untouched. */ -function datasetEventTypes(dataset: string): string[] { - return ERROR_LIKE_DATASETS.has(dataset.trim().toLowerCase()) - ? ["error", "default"] - : ["trace_item_span"]; +function buildDetectorConditions( + triggers: readonly Record[], +): Record[] { + // validateMetricTriggers rejects mixing the two shapes. + if (triggers.some(isNativeCondition)) { + return [...triggers]; + } + + const below = triggers.some((trigger) => Number(trigger.thresholdType) === 1); + const conditions: Record[] = triggers.map((trigger) => ({ + type: below ? "lt" : "gt", + comparison: Number(trigger.alertThreshold), + conditionResult: + PRIORITY_BY_LABEL[triggerLabel(trigger)] ?? PRIORITY_BY_LABEL.critical, + })); + + const explicitResolve = triggers.find( + (trigger) => + trigger.resolveThreshold !== undefined && + trigger.resolveThreshold !== null, + ); + const critical = + triggers.find((trigger) => triggerLabel(trigger) === "critical") ?? + triggers[0]; + conditions.push({ + type: below ? "gte" : "lte", + comparison: Number( + explicitResolve?.resolveThreshold ?? critical?.alertThreshold, + ), + conditionResult: PRIORITY_OK, + }); + return conditions; } /** @@ -580,15 +700,17 @@ function datasetEventTypes(dataset: string): string[] { function buildDetectorDataSource( body: Record, ): Record { - const dataset = typeof body.dataset === "string" ? body.dataset : ""; + const native = detectorDatasetSource( + typeof body.dataset === "string" ? body.dataset : "", + ); const timeWindowMinutes = typeof body.timeWindow === "number" ? body.timeWindow : 0; const source: Record = { aggregate: typeof body.aggregate === "string" ? body.aggregate : "", - dataset, + dataset: native.dataset, query: typeof body.query === "string" ? body.query : "", - queryType: datasetQueryType(dataset), - eventTypes: datasetEventTypes(dataset), + queryType: native.queryType, + eventTypes: [...native.eventTypes], timeWindow: timeWindowMinutes * 60, }; if (typeof body.environment === "string" && body.environment !== "") { @@ -601,10 +723,10 @@ function buildDetectorDataSource( * Translate a flat metric alert body into the nested detector write payload. * * The flat body is what the create/edit commands assemble (and the edit merge - * baseline, which reuses the `MetricAlertRule` shape). Trigger objects are - * passed through verbatim as the condition group's conditions — their - * `alertThreshold`/`actions` shape is already validated command-side. `status` - * (0 active / 1 disabled) maps onto the detector's `enabled` boolean. + * baseline, which reuses the `MetricAlertRule` shape). Triggers are translated + * into detector conditions by {@link buildDetectorConditions}; `workflowIds` + * connects existing alerts. `status` (0 active / 1 disabled) maps onto the + * detector's `enabled` boolean. */ function buildDetectorBody( body: Record, @@ -620,9 +742,12 @@ function buildDetectorBody( if (triggers) { detectorBody.conditionGroup = { logicType: "any", - conditions: triggers, + conditions: buildDetectorConditions(triggers), }; } + if (Array.isArray(body.workflowIds)) { + detectorBody.workflowIds = body.workflowIds; + } if (body.status !== undefined) { detectorBody.enabled = Number(body.status) !== 1; diff --git a/packages/cli/test/commands/alert/metrics/create.test.ts b/packages/cli/test/commands/alert/metrics/create.test.ts index a0692b72e..5d2f11484 100644 --- a/packages/cli/test/commands/alert/metrics/create.test.ts +++ b/packages/cli/test/commands/alert/metrics/create.test.ts @@ -74,7 +74,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "unknown", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], "dry-run": true, json: true, }, @@ -113,7 +113,7 @@ describe("alert metrics create", () => { aggregate: overrides.aggregate, dataset: "errors", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], "dry-run": true, json: true, }, @@ -139,7 +139,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "errors", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], "dry-run": true, json: true, }, @@ -164,7 +164,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "errors", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], project: ["backend"], "dry-run": true, json: true, @@ -206,7 +206,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "errors", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], project: ["backend"], "dry-run": true, json: true, @@ -236,7 +236,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "errors", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], project: ["backend"], "dry-run": true, json: true, @@ -264,7 +264,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "errors", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], project: ["frontend, backend", "api"], environment: "prod", owner: "team:ops", @@ -287,7 +287,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "errors", timeWindow: 5, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], projects: ["frontend", "backend", "api"], environment: "prod", owner: "team:ops", @@ -317,7 +317,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "errors", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], project: ["backend"], "dry-run": false, json: true, @@ -331,7 +331,7 @@ describe("alert metrics create", () => { aggregate: "count()", dataset: "errors", timeWindow: 5, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], projects: ["backend"], }); }); @@ -357,7 +357,7 @@ describe("alert metrics create", () => { aggregate: "p95(span.duration)", dataset: "transactions", "time-window": 5, - trigger: ['{"alertThreshold":100,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":100}'], project: ["backend"], "dry-run": false, json: true, @@ -371,7 +371,7 @@ describe("alert metrics create", () => { aggregate: "p95(span.duration)", dataset: "spans", timeWindow: 5, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], projects: ["backend"], }); expect(context.stderr.write).toHaveBeenCalledWith( diff --git a/packages/cli/test/commands/alert/metrics/edit.test.ts b/packages/cli/test/commands/alert/metrics/edit.test.ts index c3134f012..3aed4abac 100644 --- a/packages/cli/test/commands/alert/metrics/edit.test.ts +++ b/packages/cli/test/commands/alert/metrics/edit.test.ts @@ -90,7 +90,7 @@ describe("alert metrics edit", () => { aggregate: "count()", dataset: "errors", timeWindow: 5, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], }); putSpy.mockResolvedValue({ id: "9", @@ -100,7 +100,7 @@ describe("alert metrics edit", () => { aggregate: "count()", dataset: "spans", timeWindow: 15, - triggers: [{ alertThreshold: 200, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 200 }], }); const func = (await editCommand.loader()) as unknown as ( this: unknown, @@ -116,7 +116,7 @@ describe("alert metrics edit", () => { aggregate: "count()", dataset: "spans", "time-window": 15, - trigger: ['{"alertThreshold":200,"actions":[{"id":"notify"}]}'], + trigger: ['{"alertThreshold":200}'], json: true, }, "test-org/9", @@ -130,7 +130,7 @@ describe("alert metrics edit", () => { aggregate: "count()", dataset: "spans", timeWindow: 15, - triggers: [{ alertThreshold: 200, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 200 }], }); }); @@ -199,7 +199,7 @@ describe("alert metrics edit", () => { aggregate: "count()", dataset: "errors", timeWindow: 5, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], }); putSpy.mockResolvedValue({ id: "9", @@ -209,7 +209,7 @@ describe("alert metrics edit", () => { aggregate: "count()", dataset: "errors", timeWindow: 5, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], }); const func = (await editCommand.loader()) as unknown as ( this: unknown, @@ -245,7 +245,7 @@ describe("alert metrics edit", () => { aggregate: "count()", dataset: "errors", timeWindow: 5, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], }); putSpy.mockResolvedValue({ id: "9", @@ -255,7 +255,7 @@ describe("alert metrics edit", () => { aggregate: "count()", dataset: "spans", timeWindow: 5, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], }); const func = (await editCommand.loader()) as unknown as ( this: unknown, diff --git a/packages/cli/test/commands/alert/mutation-utils.test.ts b/packages/cli/test/commands/alert/mutation-utils.test.ts index 9c1ab5f1b..682b64185 100644 --- a/packages/cli/test/commands/alert/mutation-utils.test.ts +++ b/packages/cli/test/commands/alert/mutation-utils.test.ts @@ -12,6 +12,7 @@ import { validateIssueRuleArrays, validateMetricDataset, validateMetricTimeWindow, + parseWorkflowIds, validateMetricTriggers, } from "../../../src/commands/alert/mutation-utils.js"; import { ValidationError } from "../../../src/lib/errors.js"; @@ -291,7 +292,7 @@ describe("resolveMetricDataset", () => { }); describe("validateMetricDataset", () => { - const valid = ["errors", "sessions", "events", "spans", "metrics"]; + const valid = ["errors", "sessions", "events", "spans", "logs", "metrics"]; for (const dataset of valid) { test(`passes for "${dataset}"`, () => { @@ -299,7 +300,7 @@ describe("validateMetricDataset", () => { }); } - test.each(["error", "error-events", "METRIC", "span"])( + test.each(["error", "error-events", "METRIC", "span", "log"])( 'passes for alias "%s"', (dataset) => { expect(() => validateMetricDataset(dataset)).not.toThrow(); @@ -373,31 +374,65 @@ describe("validateMetricTriggers", () => { } }); - test("throws for missing actions array", () => { - try { - validateMetricTriggers([{ alertThreshold: 100 }]); - expect.unreachable("should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ValidationError); - } + test("passes for a threshold trigger without actions", () => { + expect(() => + validateMetricTriggers([ + { alertThreshold: 100, label: "warning", resolveThreshold: 50 }, + ]), + ).not.toThrow(); }); - test("throws for empty actions array", () => { - try { - validateMetricTriggers([{ alertThreshold: 100, actions: [] }]); - expect.unreachable("should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ValidationError); - } + test("rejects trigger actions, pointing at --workflow", () => { + expect(() => + validateMetricTriggers([ + { alertThreshold: 100, actions: [{ type: "email" }] }, + ]), + ).toThrow(/--workflow/); }); - test("passes for valid trigger", () => { + test("rejects an unknown label", () => { + expect(() => + validateMetricTriggers([{ alertThreshold: 100, label: "info" }]), + ).toThrow(ValidationError); + }); + + test("passes native detector conditions", () => { expect(() => validateMetricTriggers([ - { alertThreshold: 100, actions: [{ type: "email" }] }, + { type: "gt", comparison: 0, conditionResult: 75 }, + { type: "lte", comparison: 0, conditionResult: 0 }, ]), ).not.toThrow(); }); + + test("rejects native conditions missing comparison", () => { + expect(() => + validateMetricTriggers([{ type: "gt", conditionResult: 75 }]), + ).toThrow(ValidationError); + }); + + test("rejects mixing native conditions and threshold triggers", () => { + expect(() => + validateMetricTriggers([ + { alertThreshold: 100 }, + { type: "lte", comparison: 0, conditionResult: 0 }, + ]), + ).toThrow(ValidationError); + }); +}); + +describe("parseWorkflowIds", () => { + test("returns undefined when absent", () => { + expect(parseWorkflowIds(undefined)).toBeUndefined(); + }); + + test("splits repeatable and comma-separated ids", () => { + expect(parseWorkflowIds(["12,34", "56"])).toEqual([12, 34, 56]); + }); + + test("rejects non-numeric ids", () => { + expect(() => parseWorkflowIds(["abc"])).toThrow(ValidationError); + }); }); describe("statusToMetricValue", () => { diff --git a/packages/cli/test/lib/api/alerts.test.ts b/packages/cli/test/lib/api/alerts.test.ts index 11397a5a9..340d8f563 100644 --- a/packages/cli/test/lib/api/alerts.test.ts +++ b/packages/cli/test/lib/api/alerts.test.ts @@ -239,6 +239,45 @@ describe("getMetricAlertRule", () => { expect(rule.timeWindow).toBe(10); }); + test.each([ + ["events", ["error", "default"], "errors"], + ["events_analytics_platform", ["trace_item_span"], "spans"], + ["events_analytics_platform", ["trace_item_log"], "logs"], + [ + "events_analytics_platform", + ["trace_item_metric"], + "events_analytics_platform", + ], + ["metrics", [], "metrics"], + ])( + "maps native dataset %s with eventTypes %j to CLI dataset %s", + async (dataset, eventTypes, expected) => { + globalThis.fetch = mockFetch(async () => + Response.json( + metricDetector({ + dataSources: [ + { + type: "snuba_query_subscription", + queryObj: { + snubaQuery: { + aggregate: "count()", + dataset, + eventTypes, + query: "", + timeWindow: 300, + }, + }, + }, + ], + }), + ), + ); + + const rule = await getMetricAlertRule("test-org", "9"); + expect(rule.dataset).toBe(expected); + }, + ); + test("prefers projectSlug and falls back to a projects array for projects", async () => { globalThis.fetch = mockFetch(async () => Response.json( @@ -394,7 +433,7 @@ describe("createMetricAlertRule", () => { dataSources: [ { aggregate: "p95(span.duration)", - dataset: "spans", + dataset: "events_analytics_platform", query: "environment:prod", queryType: 1, eventTypes: ["trace_item_span"], @@ -406,7 +445,11 @@ describe("createMetricAlertRule", () => { config: { detectionType: "static" }, conditionGroup: { logicType: "any", - conditions: [{ alertThreshold: 500, actions: [{ id: "notify" }] }], + // The legacy trigger becomes a threshold plus a resolution condition. + conditions: [ + { type: "gt", comparison: 500, conditionResult: 75 }, + { type: "lte", comparison: 500, conditionResult: 0 }, + ], }, }); return Response.json( @@ -422,7 +465,7 @@ describe("createMetricAlertRule", () => { dataset: "spans", timeWindow: 5, environment: "prod", - triggers: [{ alertThreshold: 500, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 500 }], projects: ["backend"], }); expect(created.id).toBe("77"); @@ -444,16 +487,146 @@ describe("createMetricAlertRule", () => { aggregate: "count()", dataset: "errors", timeWindow: 15, - triggers: [{ alertThreshold: 100, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 100 }], projects: ["backend"], }); const source = (sentBody.dataSources as Record[])[0]; + expect(source?.dataset).toBe("events"); expect(source?.queryType).toBe(0); expect(source?.eventTypes).toEqual(["error", "default"]); expect(source?.timeWindow).toBe(900); }); + test("maps logs to the EAP dataset with trace_item_log and connects workflows", async () => { + let sentBody: Record = {}; + globalThis.fetch = mockFetch(async (input, init) => { + sentBody = (await new Request(input!, init).json()) as Record< + string, + unknown + >; + return Response.json(metricDetector({ id: "1" }), { status: 201 }); + }); + + await createMetricAlertRule("test-org", { + name: "AI chat failures", + query: "action:ai.chat result:error", + aggregate: "count()", + dataset: "logs", + timeWindow: 5, + triggers: [{ alertThreshold: 0, resolveThreshold: 0 }], + projects: ["backend"], + workflowIds: [42], + }); + + expect(sentBody.dataSources).toEqual([ + { + aggregate: "count()", + dataset: "events_analytics_platform", + query: "action:ai.chat result:error", + queryType: 1, + eventTypes: ["trace_item_log"], + timeWindow: 300, + }, + ]); + expect(sentBody.conditionGroup).toEqual({ + logicType: "any", + conditions: [ + { type: "gt", comparison: 0, conditionResult: 75 }, + { type: "lte", comparison: 0, conditionResult: 0 }, + ], + }); + expect(sentBody.workflowIds).toEqual([42]); + }); + + test("maps sessions to the crash-rate metrics dataset", async () => { + let sentBody: Record = {}; + globalThis.fetch = mockFetch(async (input, init) => { + sentBody = (await new Request(input!, init).json()) as Record< + string, + unknown + >; + return Response.json(metricDetector({ id: "1" }), { status: 201 }); + }); + + await createMetricAlertRule("test-org", { + name: "Crash rate", + query: "", + aggregate: + "percentage(sessions_crashed, sessions) AS _crash_rate_alert_aggregate", + dataset: "sessions", + timeWindow: 60, + triggers: [{ alertThreshold: 1 }], + projects: ["backend"], + }); + + const source = (sentBody.dataSources as Record[])[0]; + expect(source?.dataset).toBe("metrics"); + expect(source?.queryType).toBe(2); + expect(source?.eventTypes).toEqual([]); + }); + + test("translates warning and below-threshold triggers", async () => { + let sentBody: Record = {}; + globalThis.fetch = mockFetch(async (input, init) => { + sentBody = (await new Request(input!, init).json()) as Record< + string, + unknown + >; + return Response.json(metricDetector({ id: "1" }), { status: 201 }); + }); + + await createMetricAlertRule("test-org", { + name: "Low throughput", + query: "", + aggregate: "count()", + dataset: "spans", + timeWindow: 5, + triggers: [ + { alertThreshold: 10, label: "critical", thresholdType: 1 }, + { alertThreshold: 50, label: "warning", thresholdType: 1 }, + ], + projects: ["backend"], + }); + + expect( + (sentBody.conditionGroup as Record).conditions, + ).toEqual([ + { type: "lt", comparison: 10, conditionResult: 75 }, + { type: "lt", comparison: 50, conditionResult: 50 }, + { type: "gte", comparison: 10, conditionResult: 0 }, + ]); + }); + + test("passes native detector conditions through unchanged", async () => { + let sentBody: Record = {}; + globalThis.fetch = mockFetch(async (input, init) => { + sentBody = (await new Request(input!, init).json()) as Record< + string, + unknown + >; + return Response.json(metricDetector({ id: "1" }), { status: 201 }); + }); + + const conditions = [ + { type: "gte", comparison: 5, conditionResult: 75 }, + { type: "lt", comparison: 5, conditionResult: 0 }, + ]; + await createMetricAlertRule("test-org", { + name: "Native", + query: "", + aggregate: "count()", + dataset: "errors", + timeWindow: 5, + triggers: conditions, + projects: ["backend"], + }); + + expect( + (sentBody.conditionGroup as Record).conditions, + ).toEqual(conditions); + }); + test("throws ValidationError when no project is provided", async () => { await expect( createMetricAlertRule("test-org", { @@ -462,7 +635,7 @@ describe("createMetricAlertRule", () => { aggregate: "count()", dataset: "errors", timeWindow: 5, - triggers: [{ alertThreshold: 1, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 1 }], }), ).rejects.toMatchObject({ name: "ValidationError" }); }); @@ -510,7 +683,7 @@ describe("putMetricAlertRule", () => { dataset: "spans", timeWindow: 5, status: 1, - triggers: [{ alertThreshold: 500, actions: [{ id: "notify" }] }], + triggers: [{ alertThreshold: 500 }], }); expect(updated.id).toBe("9"); }); From 7e998409c2a06df3cf8cb00ba67ebf9252ca3eb5 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:17:41 +0000 Subject: [PATCH 2/6] fix(cli): Keep stored event types when editing unmodelled metric monitors The edit baseline flattened the detector to a CLI dataset name, so a rename or status change on an EAP monitor the CLI does not model (e.g. `trace_item_metric`) rebuilt its data source with span defaults. Carry the stored `eventTypes`/`queryType` through the baseline and reuse them for non-CLI datasets. --- packages/cli/src/lib/api/alerts.ts | 62 ++++++++++++++++++------ packages/cli/test/lib/api/alerts.test.ts | 36 ++++++++++++++ 2 files changed, 82 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/lib/api/alerts.ts b/packages/cli/src/lib/api/alerts.ts index d501796e2..af5d5764c 100644 --- a/packages/cli/src/lib/api/alerts.ts +++ b/packages/cli/src/lib/api/alerts.ts @@ -431,6 +431,14 @@ export async function getMetricAlertRule( orgSlug: string, ruleId: string, ): Promise { + return mapDetectorToMetricAlertRule(await getMetricDetector(orgSlug, ruleId)); +} + +/** Fetch a metric-issue detector, rejecting other detector kinds with a 404. */ +async function getMetricDetector( + orgSlug: string, + ruleId: string, +): Promise { const config = await getOrgSdkConfig(orgSlug); const result = await getOrganizationDetector({ ...config, @@ -456,7 +464,7 @@ export async function getMetricAlertRule( `/organizations/${orgSlug}/detectors/${encodeURIComponent(ruleId)}/`, ); } - return mapDetectorToMetricAlertRule(data); + return data; } // Issue alert write operations @@ -595,18 +603,30 @@ const DETECTOR_DATASET_SOURCES: Record = { }; /** - * Resolve the native detector dataset fields for a CLI dataset name. + * Resolve the native detector dataset fields for a flat metric alert body. * - * Unknown names fall back to the span source so an edit of a detector whose - * dataset the CLI does not model keeps the previous behavior. + * CLI dataset names use the mapping table. Other (native) datasets come from + * an edit baseline, so they keep the stored `eventTypes`/`queryType`, falling + * back to the span defaults only when the baseline carries none. */ -function detectorDatasetSource(dataset: string): DetectorDatasetSource { - return ( - DETECTOR_DATASET_SOURCES[dataset.trim().toLowerCase()] ?? { - ...SPAN_SOURCE, - dataset, - } - ); +function detectorDatasetSource( + body: Record, +): DetectorDatasetSource { + const dataset = typeof body.dataset === "string" ? body.dataset : ""; + const modelled = DETECTOR_DATASET_SOURCES[dataset.trim().toLowerCase()]; + if (modelled) { + return modelled; + } + return { + dataset, + queryType: + body.queryType === 0 || body.queryType === 1 || body.queryType === 2 + ? body.queryType + : SPAN_SOURCE.queryType, + eventTypes: Array.isArray(body.eventTypes) + ? body.eventTypes.filter((t): t is string => typeof t === "string") + : SPAN_SOURCE.eventTypes, + }; } /** @@ -700,9 +720,7 @@ function buildDetectorConditions( function buildDetectorDataSource( body: Record, ): Record { - const native = detectorDatasetSource( - typeof body.dataset === "string" ? body.dataset : "", - ); + const native = detectorDatasetSource(body); const timeWindowMinutes = typeof body.timeWindow === "number" ? body.timeWindow : 0; const source: Record = { @@ -816,13 +834,25 @@ export async function deleteMetricAlertRule( * `MetricAlertRule` shape, then hands the merged body to `putMetricAlertRule`, * which translates it into the nested detector document. Returning the flat * shape (rather than the raw detector) keeps the command-side merge coherent. + * The stored `eventTypes`/`queryType` ride along so an edit of a data source + * the CLI does not model (e.g. `trace_item_metric`) keeps its signal. */ export async function getMetricAlertRuleDocument( orgSlug: string, ruleId: string, ): Promise> { - const rule = await getMetricAlertRule(orgSlug, ruleId); - return { ...rule }; + const detector = await getMetricDetector(orgSlug, ruleId); + const source = resolveThresholdSource(detector.dataSources?.[0] ?? {}); + const document: Record = { + ...mapDetectorToMetricAlertRule(detector), + }; + if (Array.isArray(source.eventTypes)) { + document.eventTypes = source.eventTypes; + } + if (typeof source.queryType === "number") { + document.queryType = source.queryType; + } + return document; } /** diff --git a/packages/cli/test/lib/api/alerts.test.ts b/packages/cli/test/lib/api/alerts.test.ts index 340d8f563..573840007 100644 --- a/packages/cli/test/lib/api/alerts.test.ts +++ b/packages/cli/test/lib/api/alerts.test.ts @@ -660,6 +660,42 @@ describe("getMetricAlertRuleDocument", () => { }); }); +describe("metric alert edit round-trip", () => { + test("keeps the stored eventTypes of an unmodelled EAP monitor", async () => { + const storedSource = { + type: "snuba_query_subscription", + queryObj: { + snubaQuery: { + aggregate: "sum(value,my.metric,counter,-)", + dataset: "events_analytics_platform", + eventTypes: ["trace_item_metric"], + queryType: 1, + query: "", + timeWindow: 300, + }, + }, + }; + let putSource: Record | undefined; + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + if (req.method === "PUT") { + const body = (await req.json()) as Record; + putSource = (body.dataSources as Record[])[0]; + } + return Response.json(metricDetector({ dataSources: [storedSource] })); + }); + + const doc = await getMetricAlertRuleDocument("test-org", "9"); + await putMetricAlertRule("test-org", "9", { ...doc, name: "Renamed" }); + + expect(putSource).toMatchObject({ + dataset: "events_analytics_platform", + eventTypes: ["trace_item_metric"], + queryType: 1, + }); + }); +}); + describe("putMetricAlertRule", () => { test("PUTs a nested detector body to the /detectors/{id}/ endpoint", async () => { globalThis.fetch = mockFetch(async (input, init) => { From c7fc71f1a06fc7eab600ffe2eb61a390886ba5fe Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:39:41 +0000 Subject: [PATCH 3/6] ref(cli): Derive metric alert datasets and triggers from shared schemas Address review on #1591: - Add `@sentry/toolkit-core/metric-monitor` with the native detector datasets, threshold operators, priorities, and per-signal data sources (from the Sentry validators, since `@sentry/api` types data sources and conditions as `unknown[]`). MCP's metric monitor schemas now use it too. - Add `src/types/metric-alert.ts` as the single CLI dataset table: the `--dataset` help, validation, aliases, write mapping, and read-back name are all derived from it. - Validate `--trigger` with Valibot schemas and pass the typed result to the detector body builder, replacing the ad-hoc checks and casts. --- .../skills/sentry-cli/references/alert.md | 4 +- .../cli/src/commands/alert/metrics/create.ts | 13 +- .../cli/src/commands/alert/metrics/edit.ts | 17 +- .../cli/src/commands/alert/mutation-utils.ts | 142 +----------- packages/cli/src/lib/api/alerts.ts | 176 +++++---------- packages/cli/src/types/metric-alert.ts | 205 ++++++++++++++++++ .../commands/alert/mutation-utils.test.ts | 76 ------- packages/cli/test/types/metric-alert.test.ts | 128 +++++++++++ .../tools/support/metric-monitor-config.ts | 24 +- packages/toolkit-core/package.json | 4 + packages/toolkit-core/src/metric-monitor.ts | 65 ++++++ 11 files changed, 492 insertions(+), 362 deletions(-) create mode 100644 packages/cli/src/types/metric-alert.ts create mode 100644 packages/cli/test/types/metric-alert.test.ts create mode 100644 packages/toolkit-core/src/metric-monitor.ts diff --git a/packages/cli/plugins/sentry-cli/skills/sentry-cli/references/alert.md b/packages/cli/plugins/sentry-cli/skills/sentry-cli/references/alert.md index fa4b244cf..b7b150e57 100644 --- a/packages/cli/plugins/sentry-cli/skills/sentry-cli/references/alert.md +++ b/packages/cli/plugins/sentry-cli/skills/sentry-cli/references/alert.md @@ -155,7 +155,7 @@ Create a metric alert rule - `--name - Rule name` - `--query - Metric query filter string` - `--aggregate - Aggregate expression (for example count(), p95(span.duration))` -- `--dataset - Dataset: errors (error-events), events, spans, logs, sessions, metrics (transaction(s) routes to spans)` +- `--dataset - Dataset: errors, events, spans, logs, metrics, sessions (error-events is errors; transaction(s) routes to spans)` - `--time-window - Evaluation window in minutes` - `-t, --trigger ... - Trigger object JSON (repeatable, or pass one JSON array)` - `-p, --project ... - Project slug filter (repeatable or comma-separated)` @@ -214,7 +214,7 @@ Edit a metric alert rule - `--status - active or disabled` - `--query - Metric query filter` - `--aggregate - Aggregate expression` -- `--dataset - Dataset: errors (error-events), events, spans, logs, sessions, metrics (transaction(s) routes to spans)` +- `--dataset - Dataset: errors, events, spans, logs, metrics, sessions (error-events is errors; transaction(s) routes to spans)` - `--time-window - Evaluation window in minutes` - `-t, --trigger ... - Trigger object JSON (repeatable, or pass one JSON array)` - `-p, --project ... - Project slug filter (repeatable or comma-separated)` diff --git a/packages/cli/src/commands/alert/metrics/create.ts b/packages/cli/src/commands/alert/metrics/create.ts index cc83f4d4f..a29922906 100644 --- a/packages/cli/src/commands/alert/metrics/create.ts +++ b/packages/cli/src/commands/alert/metrics/create.ts @@ -12,6 +12,10 @@ import { warning } from "../../../lib/formatters/colors.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; import { DRY_RUN_ALIASES, DRY_RUN_FLAG } from "../../../lib/mutate-command.js"; import { resolveOrgOnlyFromArg } from "../../../lib/resolve-target.js"; +import { + METRIC_ALERT_DATASET_BRIEF, + parseMetricTriggers, +} from "../../../types/metric-alert.js"; import { normalizeProjectList, parseJsonObjectList, @@ -19,7 +23,6 @@ import { resolveMetricDataset, validateMetricDataset, validateMetricTimeWindow, - validateMetricTriggers, } from "../mutation-utils.js"; import { metricAlertStatusLabel } from "./status.js"; @@ -126,8 +129,7 @@ export const createCommand = buildCommand({ dataset: { kind: "parsed", parse: String, - brief: - "Dataset: errors (error-events), events, spans, logs, sessions, metrics (transaction(s) routes to spans)", + brief: METRIC_ALERT_DATASET_BRIEF, }, "time-window": { kind: "parsed", @@ -194,8 +196,9 @@ export const createCommand = buildCommand({ this.stderr.write(`${warning(`Tip: ${datasetNotice}`)}\n`); } - const triggers = parseJsonObjectList(flags.trigger, "trigger"); - validateMetricTriggers(triggers); + const triggers = parseMetricTriggers( + parseJsonObjectList(flags.trigger, "trigger"), + ); const projects = normalizeProjectList(flags.project); if (!projects || projects.length === 0) { throw new ValidationError( diff --git a/packages/cli/src/commands/alert/metrics/edit.ts b/packages/cli/src/commands/alert/metrics/edit.ts index e7aa622e2..e9c480af7 100644 --- a/packages/cli/src/commands/alert/metrics/edit.ts +++ b/packages/cli/src/commands/alert/metrics/edit.ts @@ -14,6 +14,10 @@ import { ValidationError } from "../../../lib/errors.js"; import { warning } from "../../../lib/formatters/colors.js"; import { CommandOutput } from "../../../lib/formatters/output.js"; import { resolveOrgOnlyFromArg } from "../../../lib/resolve-target.js"; +import { + METRIC_ALERT_DATASET_BRIEF, + parseMetricTriggers, +} from "../../../types/metric-alert.js"; import { normalizeMetricDataset, normalizeProjectList, @@ -24,7 +28,6 @@ import { statusToMetricValue, validateMetricDataset, validateMetricTimeWindow, - validateMetricTriggers, } from "../mutation-utils.js"; import { parseMetricRuleArg, resolveMetricAlertRule } from "./rule-resolve.js"; import { metricAlertStatusLabel } from "./status.js"; @@ -121,7 +124,9 @@ function applyMetricOptionalFields( flags: EditFlags, ): Record { if (flags.trigger !== undefined) { - body.triggers = parseJsonObjectList(flags.trigger, "trigger"); + body.triggers = parseMetricTriggers( + parseJsonObjectList(flags.trigger, "trigger"), + ); } if (flags.project !== undefined) { body.projects = normalizeProjectList(flags.project) ?? []; @@ -143,11 +148,6 @@ function validateMetricBody( body: Record, flags: EditFlags, ): void { - if (flags.trigger !== undefined) { - validateMetricTriggers( - body.triggers as Record[] | undefined, - ); - } if (flags.dataset !== undefined) { validateMetricDataset(normalizeMetricDataset(String(body.dataset))); } @@ -227,8 +227,7 @@ export const editCommand = buildCommand({ kind: "parsed", parse: String, optional: true, - brief: - "Dataset: errors (error-events), events, spans, logs, sessions, metrics (transaction(s) routes to spans)", + brief: METRIC_ALERT_DATASET_BRIEF, }, "time-window": { kind: "parsed", diff --git a/packages/cli/src/commands/alert/mutation-utils.ts b/packages/cli/src/commands/alert/mutation-utils.ts index 07c98301a..97275e4c9 100644 --- a/packages/cli/src/commands/alert/mutation-utils.ts +++ b/packages/cli/src/commands/alert/mutation-utils.ts @@ -3,49 +3,13 @@ */ import { ValidationError } from "../../lib/errors.js"; +import { + METRIC_ALERT_DATASET_ALIASES, + METRIC_ALERT_DATASET_NAMES, + metricAlertSource, +} from "../../types/metric-alert.js"; const ISSUE_MATCH_MODES = new Set(["all", "any"]); -const METRIC_DATASET_VALUES = new Set([ - "errors", - "sessions", - "events", - "spans", - "logs", - "metrics", -]); - -/** - * Maps user-provided dataset aliases to canonical metric alert dataset values. - * - * Only aliases that resolve to a value already accepted by the metric alert API - * are listed here: the singular forms (`error` → `errors`, `span` → `spans`) - * and the dashboard terminology for the error dataset (`error-events` → - * `errors`). This lets users copying from dashboard docs avoid a validation - * error without changing which dataset is sent. - * - * Names that denote a *distinct* dataset in the metric alert path — e.g. - * `tracemetrics`, `metricsenhanced`, `eap`, `events_analytics_platform` — are - * deliberately NOT aliased here. Rewriting them onto `metrics`/`spans` would - * silently send the wrong dataset in the create/edit payload (the CLI's - * canonical `metrics` is session/crash-rate, not trace metrics), with no - * validation error to catch it. - * - * The deprecated `transaction`/`transactions` datasets are handled separately - * by {@link resolveMetricDataset} — they route to `spans` with an added - * `is_transaction:true` filter rather than a plain rename, so they are not - * listed here. - */ -const METRIC_DATASET_ALIASES: Record = { - // Singular forms - error: "errors", - session: "sessions", - metric: "metrics", - span: "spans", - log: "logs", - // Dashboard terminology for the error dataset - "error-events": "errors", -}; - /** Deprecated dataset names that now route to `spans` with `is_transaction:true`. */ const TRANSACTION_DATASET_NAMES = new Set(["transaction", "transactions"]); @@ -225,7 +189,7 @@ export function normalizeProjectList( */ export function normalizeMetricDataset(dataset: string): string { const lower = dataset.trim().toLowerCase(); - return METRIC_DATASET_ALIASES[lower] ?? lower; + return METRIC_ALERT_DATASET_ALIASES[lower] ?? lower; } /** @@ -281,12 +245,11 @@ export function resolveMetricDataset( /** Validate that `dataset` is one of the allowed Sentry metric alert dataset values. */ export function validateMetricDataset(dataset: string): void { - const normalized = normalizeMetricDataset(dataset); - if (METRIC_DATASET_VALUES.has(normalized)) { + if (metricAlertSource(normalizeMetricDataset(dataset))) { return; } throw new ValidationError( - `dataset must be one of: ${[...METRIC_DATASET_VALUES].join(", ")}.`, + `dataset must be one of: ${METRIC_ALERT_DATASET_NAMES.join(", ")}.`, "dataset", ); } @@ -302,95 +265,6 @@ export function validateMetricTimeWindow(timeWindow: number): void { ); } -const METRIC_TRIGGER_LABELS = new Set(["critical", "warning"]); - -/** Whether a trigger is a native detector condition rather than an `alertThreshold` trigger. */ -function isDetectorCondition(trigger: Record): boolean { - return typeof trigger.type === "string"; -} - -/** - * Validate one native detector condition (`{type, comparison, conditionResult}`). - * The backend checks the operator and priority values; only presence is checked here. - */ -function validateNativeMetricCondition(trigger: Record): void { - if ( - trigger.comparison === undefined || - trigger.conditionResult === undefined - ) { - throw new ValidationError( - "Detector conditions with 'type' must include comparison and conditionResult.", - "trigger", - ); - } -} - -/** Validate one `alertThreshold`-style trigger. */ -function validateThresholdTrigger(trigger: Record): void { - const threshold = trigger.alertThreshold; - if ( - (typeof threshold !== "number" && typeof threshold !== "string") || - !Number.isFinite(Number(threshold)) - ) { - throw new ValidationError( - "Each trigger must include a numeric alertThreshold.", - "trigger", - ); - } - if ( - trigger.label !== undefined && - !METRIC_TRIGGER_LABELS.has(String(trigger.label).toLowerCase()) - ) { - throw new ValidationError( - `Trigger label must be 'critical' or 'warning' (got ${JSON.stringify(trigger.label)}).`, - "trigger", - ); - } - if (Array.isArray(trigger.actions) && trigger.actions.length > 0) { - throw new ValidationError( - "Metric alerts no longer carry notification actions on triggers. " + - "Remove 'actions' and connect an existing alert with --workflow .", - "trigger", - ); - } -} - -/** - * Validate metric alert triggers. - * - * Each trigger is either a threshold (`alertThreshold`, optional `label` - * critical|warning, `resolveThreshold`, `thresholdType` 0 above / 1 below) or - * a native detector condition (`type`, `comparison`, `conditionResult`). - * Notification actions are rejected: the detectors API does not accept them, - * so they would otherwise be dropped silently. - */ -export function validateMetricTriggers( - triggers: readonly Record[] | undefined, -): void { - if (!triggers || triggers.length === 0) { - throw new ValidationError( - "Pass at least one --trigger JSON object.", - "trigger", - ); - } - - const nativeCount = triggers.filter(isDetectorCondition).length; - if (nativeCount > 0 && nativeCount < triggers.length) { - throw new ValidationError( - "Do not mix detector conditions (with 'type') and alertThreshold triggers.", - "trigger", - ); - } - - for (const trigger of triggers) { - if (isDetectorCondition(trigger)) { - validateNativeMetricCondition(trigger); - } else { - validateThresholdTrigger(trigger); - } - } -} - /** Parse `--workflow` values (repeatable or comma-separated) into numeric workflow ids. */ export function parseWorkflowIds( values: readonly string[] | undefined, diff --git a/packages/cli/src/lib/api/alerts.ts b/packages/cli/src/lib/api/alerts.ts index af5d5764c..e6619a3ad 100644 --- a/packages/cli/src/lib/api/alerts.ts +++ b/packages/cli/src/lib/api/alerts.ts @@ -16,6 +16,17 @@ import { updateOrganizationDetector, updateOrganizationWorkflow, } from "@sentry/api"; +import { + METRIC_MONITOR_PRIORITY, + METRIC_MONITOR_SOURCES, + type MetricMonitorSource, +} from "@sentry/toolkit-core/metric-monitor"; +import { + isDetectorConditionList, + type MetricTriggers, + metricAlertDatasetName, + metricAlertSource, +} from "../../types/metric-alert.js"; import { ApiError, ValidationError } from "../errors.js"; import { resolveOrgRegion } from "../region.js"; import { @@ -205,9 +216,9 @@ function mapDetectorToMetricAlertRule( status: detector.enabled === false ? 1 : 0, query: pickDetectorString(nested.query), aggregate: pickDetectorString(nested.aggregate), - dataset: cliDatasetName( + dataset: metricAlertDatasetName( pickDetectorString(nested.dataset), - nested.eventTypes, + Array.isArray(nested.eventTypes) ? nested.eventTypes : [], ), timeWindow: timeWindowSeconds > 0 ? Math.round(timeWindowSeconds / 60) : 0, environment, @@ -557,155 +568,67 @@ export async function createIssueAlertRule( // command layer unchanged while the transport moves off the retiring // `/alert-rules/` endpoint (getsentry/cli#1274, #1277). -/** - * How a CLI metric alert dataset maps onto a detector data source. - * - * The CLI keeps the friendly legacy dataset names (`errors`, `spans`, `logs`, - * …), but the detector `SnubaQueryValidator` only accepts native `Dataset` - * values and checks them against `queryType` and `eventTypes`: errors live in - * `events` (type 0), spans and logs in `events_analytics_platform` (type 1) - * distinguished by `eventTypes`, and crash-rate alerts in `metrics` (type 2). - */ -type DetectorDatasetSource = { +/** Native data source fields sent on a detector write. */ +type DetectorSourceFields = Omit & { readonly dataset: string; - readonly queryType: 0 | 1 | 2; - readonly eventTypes: readonly string[]; -}; - -const ERROR_SOURCE: DetectorDatasetSource = { - dataset: "events", - queryType: 0, - eventTypes: ["error", "default"], -}; -const SPAN_SOURCE: DetectorDatasetSource = { - dataset: "events_analytics_platform", - queryType: 1, - eventTypes: ["trace_item_span"], -}; -const LOG_SOURCE: DetectorDatasetSource = { - dataset: "events_analytics_platform", - queryType: 1, - eventTypes: ["trace_item_log"], -}; -const CRASH_RATE_SOURCE: DetectorDatasetSource = { - dataset: "metrics", - queryType: 2, - eventTypes: [], -}; - -const DETECTOR_DATASET_SOURCES: Record = { - errors: ERROR_SOURCE, - events: ERROR_SOURCE, - spans: SPAN_SOURCE, - logs: LOG_SOURCE, - sessions: CRASH_RATE_SOURCE, - metrics: CRASH_RATE_SOURCE, }; /** - * Resolve the native detector dataset fields for a flat metric alert body. + * Resolve the native data source fields for a flat metric alert body. * - * CLI dataset names use the mapping table. Other (native) datasets come from - * an edit baseline, so they keep the stored `eventTypes`/`queryType`, falling - * back to the span defaults only when the baseline carries none. + * CLI dataset names come from the shared dataset table. Any other (native) + * dataset comes from the edit baseline built by `getMetricAlertRuleDocument`, + * which carries the stored `eventTypes`/`queryType`; span defaults apply only + * when the baseline has none. */ -function detectorDatasetSource( +function detectorSourceFields( body: Record, -): DetectorDatasetSource { - const dataset = typeof body.dataset === "string" ? body.dataset : ""; - const modelled = DETECTOR_DATASET_SOURCES[dataset.trim().toLowerCase()]; - if (modelled) { - return modelled; +): DetectorSourceFields { + const dataset = String(body.dataset ?? ""); + const named = metricAlertSource(dataset); + if (named) { + return named; } + const stored = body as Partial; return { + queryType: stored.queryType ?? METRIC_MONITOR_SOURCES.spans.queryType, + eventTypes: stored.eventTypes ?? METRIC_MONITOR_SOURCES.spans.eventTypes, dataset, - queryType: - body.queryType === 0 || body.queryType === 1 || body.queryType === 2 - ? body.queryType - : SPAN_SOURCE.queryType, - eventTypes: Array.isArray(body.eventTypes) - ? body.eventTypes.filter((t): t is string => typeof t === "string") - : SPAN_SOURCE.eventTypes, }; } /** - * Map a native detector dataset back onto the CLI dataset name. - * - * Inverse of {@link detectorDatasetSource}, so `view`/`list` show the same - * names `create` accepts and an `edit` round-trip keeps log alerts on - * `trace_item_log`. Unmodelled combinations are returned verbatim. - */ -function cliDatasetName(dataset: string, eventTypes: unknown): string { - const types = Array.isArray(eventTypes) ? eventTypes : []; - if (dataset === "events") { - return "errors"; - } - if (dataset === "events_analytics_platform") { - if (types.includes("trace_item_log")) { - return "logs"; - } - if (types.includes("trace_item_span")) { - return "spans"; - } - } - return dataset; -} - -/** Detector priority levels used as metric condition results. */ -const PRIORITY_BY_LABEL: Record = { critical: 75, warning: 50 }; -const PRIORITY_OK = 0; - -/** Trigger severity label, defaulting to `critical` like the legacy alert-rule API. */ -function triggerLabel(trigger: Record): string { - return String(trigger.label ?? "critical").toLowerCase(); -} - -/** Whether a trigger is already a native detector condition (`{type, comparison, conditionResult}`). */ -function isNativeCondition(trigger: Record): boolean { - return typeof trigger.type === "string"; -} - -/** - * Translate CLI triggers into detector condition-group conditions. + * Translate parsed triggers into detector condition-group conditions. * - * Legacy-shaped triggers (`alertThreshold`, optional `label`, - * `resolveThreshold`, `thresholdType` 0 above / 1 below) become a threshold - * condition per trigger plus the single resolution condition the - * metric detector validator requires. Resolution uses the first - * `resolveThreshold` given, else the critical (or first) alert threshold. - * Native conditions (with a `type`) pass through untouched. + * Threshold triggers become one threshold condition each plus the single + * resolution condition the metric detector validator requires. Resolution uses + * the first `resolveThreshold` given, else the critical (or first) alert + * threshold. Native conditions pass through untouched. */ function buildDetectorConditions( - triggers: readonly Record[], -): Record[] { - // validateMetricTriggers rejects mixing the two shapes. - if (triggers.some(isNativeCondition)) { - return [...triggers]; + triggers: MetricTriggers, +): readonly Record[] { + if (isDetectorConditionList(triggers)) { + return triggers; } - const below = triggers.some((trigger) => Number(trigger.thresholdType) === 1); + const below = triggers.some((trigger) => trigger.thresholdType === 1); const conditions: Record[] = triggers.map((trigger) => ({ type: below ? "lt" : "gt", - comparison: Number(trigger.alertThreshold), - conditionResult: - PRIORITY_BY_LABEL[triggerLabel(trigger)] ?? PRIORITY_BY_LABEL.critical, + comparison: trigger.alertThreshold, + conditionResult: METRIC_MONITOR_PRIORITY[trigger.label ?? "critical"], })); - const explicitResolve = triggers.find( - (trigger) => - trigger.resolveThreshold !== undefined && - trigger.resolveThreshold !== null, - ); const critical = - triggers.find((trigger) => triggerLabel(trigger) === "critical") ?? + triggers.find((trigger) => (trigger.label ?? "critical") === "critical") ?? triggers[0]; + const resolveThreshold = + triggers.find((trigger) => trigger.resolveThreshold != null) + ?.resolveThreshold ?? critical?.alertThreshold; conditions.push({ type: below ? "gte" : "lte", - comparison: Number( - explicitResolve?.resolveThreshold ?? critical?.alertThreshold, - ), - conditionResult: PRIORITY_OK, + comparison: resolveThreshold, + conditionResult: METRIC_MONITOR_PRIORITY.resolved, }); return conditions; } @@ -720,7 +643,7 @@ function buildDetectorConditions( function buildDetectorDataSource( body: Record, ): Record { - const native = detectorDatasetSource(body); + const native = detectorSourceFields(body); const timeWindowMinutes = typeof body.timeWindow === "number" ? body.timeWindow : 0; const source: Record = { @@ -756,7 +679,8 @@ function buildDetectorBody( config: { detectionType: "static" }, }; - const triggers = Array.isArray(body.triggers) ? body.triggers : undefined; + // create/edit store triggers already parsed by parseMetricTriggers. + const triggers = body.triggers as MetricTriggers | undefined; if (triggers) { detectorBody.conditionGroup = { logicType: "any", diff --git a/packages/cli/src/types/metric-alert.ts b/packages/cli/src/types/metric-alert.ts new file mode 100644 index 000000000..93240abf0 --- /dev/null +++ b/packages/cli/src/types/metric-alert.ts @@ -0,0 +1,205 @@ +/** + * Metric alert types and schemas + * + * The CLI `--dataset` vocabulary for `sentry alert metrics`, mapped onto the + * shared metric monitor data sources, plus Valibot schemas for `--trigger`. + * Every dataset list, help string, and lookup is derived from + * {@link METRIC_ALERT_DATASETS} so they cannot diverge. + */ + +import { + METRIC_MONITOR_PRIORITY, + METRIC_MONITOR_SOURCES, + METRIC_MONITOR_THRESHOLD_TYPES, + type MetricMonitorSource, +} from "@sentry/toolkit-core/metric-monitor"; +import { + array, + finite, + getDotPath, + type InferOutput, + looseObject, + maxLength, + nullable, + number, + object, + optional, + picklist, + pipe, + record, + safeParse, + string, + transform, + union, + unknown, +} from "valibot"; + +import { ValidationError } from "../lib/errors.js"; + +// --------------------------------------------------------------------------- +// Datasets +// --------------------------------------------------------------------------- + +/** + * CLI dataset names and the detector data source each selects. + * + * Key order drives help text and, for shared sources, which name a stored + * monitor reads back as (`errors` over `events`, `metrics` over `sessions`). + */ +export const METRIC_ALERT_DATASETS = { + errors: METRIC_MONITOR_SOURCES.errors, + events: METRIC_MONITOR_SOURCES.errors, + spans: METRIC_MONITOR_SOURCES.spans, + logs: METRIC_MONITOR_SOURCES.logs, + metrics: METRIC_MONITOR_SOURCES.crashRate, + sessions: METRIC_MONITOR_SOURCES.crashRate, +} as const satisfies Record; + +export type MetricAlertDataset = keyof typeof METRIC_ALERT_DATASETS; + +export const METRIC_ALERT_DATASET_NAMES = Object.keys( + METRIC_ALERT_DATASETS, +) as MetricAlertDataset[]; + +/** + * Aliases for {@link METRIC_ALERT_DATASETS} names. + * + * Only singular forms and the dashboard name for errors (`error-events`) are + * aliased. Names of *distinct* datasets — `tracemetrics`, `eap`, + * `events_analytics_platform` — are deliberately not, because rewriting them + * onto `metrics`/`spans` would silently send the wrong data source (the CLI's + * `metrics` is crash rate, not trace metrics). `transaction(s)` is routed to + * `spans` with an `is_transaction:true` filter by `resolveMetricDataset`. + */ +export const METRIC_ALERT_DATASET_ALIASES: Record = + { + error: "errors", + "error-events": "errors", + span: "spans", + log: "logs", + metric: "metrics", + session: "sessions", + }; + +/** `--dataset` flag help, derived from the dataset table. */ +export const METRIC_ALERT_DATASET_BRIEF = `Dataset: ${METRIC_ALERT_DATASET_NAMES.join(", ")} (error-events is errors; transaction(s) routes to spans)`; + +/** Data source for a CLI dataset name, or `undefined` for other names. */ +export function metricAlertSource( + dataset: string, +): MetricMonitorSource | undefined { + const name = dataset.trim().toLowerCase(); + return Object.hasOwn(METRIC_ALERT_DATASETS, name) + ? METRIC_ALERT_DATASETS[name as MetricAlertDataset] + : undefined; +} + +/** + * CLI dataset name for a stored data source, or the native dataset when no + * CLI name selects it (e.g. EAP `trace_item_metric`). + */ +export function metricAlertDatasetName( + dataset: string, + eventTypes: readonly string[], +): string { + const match = METRIC_ALERT_DATASET_NAMES.find((name) => { + const source: MetricMonitorSource = METRIC_ALERT_DATASETS[name]; + return ( + source.dataset === dataset && + eventTypes.every((type) => source.eventTypes.includes(type)) + ); + }); + return match ?? dataset; +} + +// --------------------------------------------------------------------------- +// Triggers +// --------------------------------------------------------------------------- + +/** A threshold given as a number or numeric string. */ +const ThresholdSchema = pipe( + union([number(), string()]), + transform(Number), + number(), + finite(), +); + +const ACTIONS_MESSAGE = + "Metric alerts no longer carry notification actions on triggers. Remove 'actions' and connect an existing alert with --workflow ."; + +/** + * Legacy-style trigger: alert above (`thresholdType` 0, default) or below (1) + * `alertThreshold`. Notification actions are rejected because detectors do + * not accept them and would drop them silently. + */ +const ThresholdTriggerSchema = object({ + alertThreshold: ThresholdSchema, + label: optional(picklist(["critical", "warning"])), + resolveThreshold: optional(nullable(ThresholdSchema)), + thresholdType: optional(picklist([0, 1])), + actions: optional(pipe(array(unknown()), maxLength(0, ACTIONS_MESSAGE))), +}); + +/** Native detector condition, passed to the API as-is. */ +const DetectorConditionSchema = looseObject({ + type: picklist([...METRIC_MONITOR_THRESHOLD_TYPES, "anomaly_detection"]), + comparison: union([number(), record(string(), unknown())]), + conditionResult: picklist(Object.values(METRIC_MONITOR_PRIORITY)), +}); + +const ThresholdTriggersSchema = array(ThresholdTriggerSchema); +const DetectorConditionsSchema = array(DetectorConditionSchema); + +export type ThresholdTrigger = InferOutput; +export type DetectorCondition = InferOutput; + +/** Parsed `--trigger` values: all thresholds or all native conditions. */ +export type MetricTriggers = ThresholdTrigger[] | DetectorCondition[]; + +/** Whether parsed triggers are native detector conditions. */ +export function isDetectorConditionList( + triggers: MetricTriggers, +): triggers is DetectorCondition[] { + return triggers.some((trigger) => "type" in trigger); +} + +/** + * Parse and validate `--trigger` JSON objects. + * + * Triggers must be all threshold triggers or all native detector conditions + * (shape chosen by the presence of `type`). + * + * @throws {ValidationError} with the offending path and allowed values + */ +export function parseMetricTriggers( + raw: readonly Record[] | undefined, +): MetricTriggers { + if (!raw || raw.length === 0) { + throw new ValidationError( + "Pass at least one --trigger JSON object.", + "trigger", + ); + } + const nativeCount = raw.filter((trigger) => "type" in trigger).length; + if (nativeCount > 0 && nativeCount < raw.length) { + throw new ValidationError( + "Do not mix detector conditions (with 'type') and alertThreshold triggers.", + "trigger", + ); + } + + const result = + nativeCount > 0 + ? safeParse(DetectorConditionsSchema, raw) + : safeParse(ThresholdTriggersSchema, raw); + if (result.success) { + return result.output; + } + const issues = result.issues.map( + (issue) => `${getDotPath(issue) ?? "trigger"}: ${issue.message}`, + ); + throw new ValidationError( + `Invalid --trigger:\n${issues.join("\n")}`, + "trigger", + ); +} diff --git a/packages/cli/test/commands/alert/mutation-utils.test.ts b/packages/cli/test/commands/alert/mutation-utils.test.ts index 682b64185..7089b3060 100644 --- a/packages/cli/test/commands/alert/mutation-utils.test.ts +++ b/packages/cli/test/commands/alert/mutation-utils.test.ts @@ -13,7 +13,6 @@ import { validateMetricDataset, validateMetricTimeWindow, parseWorkflowIds, - validateMetricTriggers, } from "../../../src/commands/alert/mutation-utils.js"; import { ValidationError } from "../../../src/lib/errors.js"; @@ -346,81 +345,6 @@ describe("validateMetricTimeWindow", () => { }); }); -describe("validateMetricTriggers", () => { - test("throws for undefined triggers", () => { - try { - validateMetricTriggers(undefined); - expect.unreachable("should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ValidationError); - } - }); - - test("throws for empty triggers", () => { - try { - validateMetricTriggers([]); - expect.unreachable("should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ValidationError); - } - }); - - test("throws for missing alertThreshold", () => { - try { - validateMetricTriggers([{ actions: [{ id: "a" }] }]); - expect.unreachable("should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(ValidationError); - } - }); - - test("passes for a threshold trigger without actions", () => { - expect(() => - validateMetricTriggers([ - { alertThreshold: 100, label: "warning", resolveThreshold: 50 }, - ]), - ).not.toThrow(); - }); - - test("rejects trigger actions, pointing at --workflow", () => { - expect(() => - validateMetricTriggers([ - { alertThreshold: 100, actions: [{ type: "email" }] }, - ]), - ).toThrow(/--workflow/); - }); - - test("rejects an unknown label", () => { - expect(() => - validateMetricTriggers([{ alertThreshold: 100, label: "info" }]), - ).toThrow(ValidationError); - }); - - test("passes native detector conditions", () => { - expect(() => - validateMetricTriggers([ - { type: "gt", comparison: 0, conditionResult: 75 }, - { type: "lte", comparison: 0, conditionResult: 0 }, - ]), - ).not.toThrow(); - }); - - test("rejects native conditions missing comparison", () => { - expect(() => - validateMetricTriggers([{ type: "gt", conditionResult: 75 }]), - ).toThrow(ValidationError); - }); - - test("rejects mixing native conditions and threshold triggers", () => { - expect(() => - validateMetricTriggers([ - { alertThreshold: 100 }, - { type: "lte", comparison: 0, conditionResult: 0 }, - ]), - ).toThrow(ValidationError); - }); -}); - describe("parseWorkflowIds", () => { test("returns undefined when absent", () => { expect(parseWorkflowIds(undefined)).toBeUndefined(); diff --git a/packages/cli/test/types/metric-alert.test.ts b/packages/cli/test/types/metric-alert.test.ts new file mode 100644 index 000000000..906078d42 --- /dev/null +++ b/packages/cli/test/types/metric-alert.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, test } from "vitest"; +import { ValidationError } from "../../src/lib/errors.js"; +import { + METRIC_ALERT_DATASET_BRIEF, + METRIC_ALERT_DATASET_NAMES, + metricAlertDatasetName, + metricAlertSource, + parseMetricTriggers, +} from "../../src/types/metric-alert.js"; + +describe("metric alert datasets", () => { + test("help text lists every dataset name", () => { + for (const name of METRIC_ALERT_DATASET_NAMES) { + expect(METRIC_ALERT_DATASET_BRIEF).toContain(name); + } + }); + + test("every dataset name reads back as a dataset name", () => { + for (const name of METRIC_ALERT_DATASET_NAMES) { + const source = metricAlertSource(name); + expect(source).toBeDefined(); + const readBack = metricAlertDatasetName( + source!.dataset, + source!.eventTypes, + ); + expect(metricAlertSource(readBack)).toEqual(source); + } + }); + + test("ignores prototype keys", () => { + expect(metricAlertSource("constructor")).toBeUndefined(); + }); +}); + +describe("parseMetricTriggers", () => { + test("throws for undefined triggers", () => { + try { + parseMetricTriggers(undefined); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(ValidationError); + } + }); + + test("throws for empty triggers", () => { + try { + parseMetricTriggers([]); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(ValidationError); + } + }); + + test("throws for missing alertThreshold", () => { + try { + parseMetricTriggers([{ actions: [{ id: "a" }] }]); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(ValidationError); + } + }); + + test("passes for a threshold trigger without actions", () => { + expect(() => + parseMetricTriggers([ + { alertThreshold: 100, label: "warning", resolveThreshold: 50 }, + ]), + ).not.toThrow(); + }); + + test("rejects trigger actions, pointing at --workflow", () => { + expect(() => + parseMetricTriggers([ + { alertThreshold: 100, actions: [{ type: "email" }] }, + ]), + ).toThrow(/--workflow/); + }); + + test("rejects an unknown label", () => { + expect(() => + parseMetricTriggers([{ alertThreshold: 100, label: "info" }]), + ).toThrow(ValidationError); + }); + + test("passes native detector conditions", () => { + expect(() => + parseMetricTriggers([ + { type: "gt", comparison: 0, conditionResult: 75 }, + { type: "lte", comparison: 0, conditionResult: 0 }, + ]), + ).not.toThrow(); + }); + + test("rejects native conditions missing comparison", () => { + expect(() => + parseMetricTriggers([{ type: "gt", conditionResult: 75 }]), + ).toThrow(ValidationError); + }); + + test("rejects mixing native conditions and threshold triggers", () => { + expect(() => + parseMetricTriggers([ + { alertThreshold: 100 }, + { type: "lte", comparison: 0, conditionResult: 0 }, + ]), + ).toThrow(ValidationError); + }); +}); + +describe("parseMetricTriggers output", () => { + test("coerces numeric-string thresholds", () => { + expect( + parseMetricTriggers([{ alertThreshold: "10", resolveThreshold: "5" }]), + ).toEqual([{ alertThreshold: 10, resolveThreshold: 5 }]); + }); + + test("rejects non-numeric thresholds", () => { + expect(() => parseMetricTriggers([{ alertThreshold: "high" }])).toThrow( + ValidationError, + ); + }); + + test("rejects unknown condition results", () => { + expect(() => + parseMetricTriggers([{ type: "gt", comparison: 1, conditionResult: 99 }]), + ).toThrow(ValidationError); + }); +}); diff --git a/packages/mcp-core/src/tools/support/metric-monitor-config.ts b/packages/mcp-core/src/tools/support/metric-monitor-config.ts index 2ed94cc8f..0534fb865 100644 --- a/packages/mcp-core/src/tools/support/metric-monitor-config.ts +++ b/packages/mcp-core/src/tools/support/metric-monitor-config.ts @@ -1,3 +1,8 @@ +import { + METRIC_MONITOR_DATASETS, + METRIC_MONITOR_PRIORITY, + METRIC_MONITOR_THRESHOLD_TYPES, +} from "@sentry/toolkit-core/metric-monitor"; import { z } from "zod"; import type { Detector, MetricMonitorUpdate } from "../../api-client/types"; import { UserInputError } from "../../errors"; @@ -8,10 +13,15 @@ import { } from "./detector-details"; const metricConditionSchema = conditionGroupSchema.shape.conditions.element; +const { critical, warning, resolved } = METRIC_MONITOR_PRIORITY; const thresholdCondition = metricConditionSchema.extend({ - type: z.enum(["gt", "lt", "gte", "lte"]), + type: z.enum(METRIC_MONITOR_THRESHOLD_TYPES), comparison: z.number(), - conditionResult: z.union([z.literal(75), z.literal(50), z.literal(0)]), + conditionResult: z.union([ + z.literal(critical), + z.literal(warning), + z.literal(resolved), + ]), }); const anomalyCondition = metricConditionSchema.extend({ type: z.literal("anomaly_detection"), @@ -20,17 +30,11 @@ const anomalyCondition = metricConditionSchema.extend({ seasonality: z.literal("auto"), thresholdType: z.union([z.literal(0), z.literal(1), z.literal(2)]), }), - conditionResult: z.union([z.literal(75), z.literal(50)]), + conditionResult: z.union([z.literal(critical), z.literal(warning)]), }); const metricMonitorQuerySchema = metricQueryDetailsSchema.extend({ - dataset: z.enum([ - "events", - "transactions", - "generic_metrics", - "metrics", - "events_analytics_platform", - ]), + dataset: z.enum(METRIC_MONITOR_DATASETS), aggregate: z.string().min(1), timeWindowSeconds: z.number().int().positive(), }); diff --git a/packages/toolkit-core/package.json b/packages/toolkit-core/package.json index 3f4ff17c7..0c0a64fe1 100644 --- a/packages/toolkit-core/package.json +++ b/packages/toolkit-core/package.json @@ -27,6 +27,10 @@ "types": "./src/events-sort.ts", "default": "./src/events-sort.ts" }, + "./metric-monitor": { + "types": "./src/metric-monitor.ts", + "default": "./src/metric-monitor.ts" + }, "./oauth-device": { "types": "./src/oauth-device.ts", "default": "./src/oauth-device.ts" diff --git a/packages/toolkit-core/src/metric-monitor.ts b/packages/toolkit-core/src/metric-monitor.ts new file mode 100644 index 000000000..d3a0880c9 --- /dev/null +++ b/packages/toolkit-core/src/metric-monitor.ts @@ -0,0 +1,65 @@ +/** + * Metric monitor (metric-issue detector) constants shared by the CLI and MCP. + * + * `@sentry/api` types detector `data_sources` and `condition_group.conditions` + * as `Array`, so these values come from the Sentry source instead: + * - Datasets: `sentry.snuba.snuba_query_validator.QUERY_TYPE_VALID_DATASETS` + * - Event types: `QUERY_TYPE_VALID_EVENT_TYPES` in the same module + * - Conditions and priorities: + * `sentry.incidents.metric_issue_detector.MetricIssueComparisonConditionValidator` + */ + +/** Native `Dataset` values the detector validator accepts. */ +export const METRIC_MONITOR_DATASETS = [ + "events", + "transactions", + "generic_metrics", + "metrics", + "events_analytics_platform", +] as const; + +export type MetricMonitorDataset = (typeof METRIC_MONITOR_DATASETS)[number]; + +/** Static/percent threshold operators (`anomaly_detection` is separate). */ +export const METRIC_MONITOR_THRESHOLD_TYPES = [ + "gt", + "lt", + "gte", + "lte", +] as const; + +export type MetricMonitorThresholdType = + (typeof METRIC_MONITOR_THRESHOLD_TYPES)[number]; + +/** `DetectorPriorityLevel` values a metric condition can resolve to. */ +export const METRIC_MONITOR_PRIORITY = { + critical: 75, + warning: 50, + resolved: 0, +} as const; + +/** `SnubaQuery.Type`: 0 error, 1 performance (incl. EAP), 2 crash rate. */ +export type MetricMonitorQueryType = 0 | 1 | 2; + +/** The dataset, query type, and event types that select one signal. */ +export type MetricMonitorSource = { + readonly dataset: MetricMonitorDataset; + readonly queryType: MetricMonitorQueryType; + readonly eventTypes: readonly string[]; +}; + +/** Data sources for the signals clients expose by name. */ +export const METRIC_MONITOR_SOURCES = { + errors: { dataset: "events", queryType: 0, eventTypes: ["error", "default"] }, + spans: { + dataset: "events_analytics_platform", + queryType: 1, + eventTypes: ["trace_item_span"], + }, + logs: { + dataset: "events_analytics_platform", + queryType: 1, + eventTypes: ["trace_item_log"], + }, + crashRate: { dataset: "metrics", queryType: 2, eventTypes: [] }, +} as const satisfies Record; From 1e4ce22e1465372ad702fa9e6ab5d81224d1cf6e Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:48:13 +0000 Subject: [PATCH 4/6] fix(cli): Keep EAP metric monitors without event types native An empty eventTypes list matched any CLI dataset on the same native dataset, so EAP monitors without event types read back as spans and an edit rewrote them onto trace_item_span. Only resolve a CLI name for empty event types when the native dataset is unambiguous, and keep missing event types empty on edit instead of defaulting to spans. --- packages/cli/src/lib/api/alerts.ts | 7 ++-- packages/cli/src/types/metric-alert.ts | 20 ++++++--- packages/cli/test/lib/api/alerts.test.ts | 44 ++++++++++++++++++++ packages/cli/test/types/metric-alert.test.ts | 19 +++++++++ 4 files changed, 82 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/lib/api/alerts.ts b/packages/cli/src/lib/api/alerts.ts index e6619a3ad..626c4c177 100644 --- a/packages/cli/src/lib/api/alerts.ts +++ b/packages/cli/src/lib/api/alerts.ts @@ -578,8 +578,9 @@ type DetectorSourceFields = Omit & { * * CLI dataset names come from the shared dataset table. Any other (native) * dataset comes from the edit baseline built by `getMetricAlertRuleDocument`, - * which carries the stored `eventTypes`/`queryType`; span defaults apply only - * when the baseline has none. + * which carries the stored `eventTypes`/`queryType`. Missing event types stay + * empty rather than defaulting to spans, so an edit never re-targets an + * ambiguous EAP monitor onto `trace_item_span`. */ function detectorSourceFields( body: Record, @@ -592,7 +593,7 @@ function detectorSourceFields( const stored = body as Partial; return { queryType: stored.queryType ?? METRIC_MONITOR_SOURCES.spans.queryType, - eventTypes: stored.eventTypes ?? METRIC_MONITOR_SOURCES.spans.eventTypes, + eventTypes: stored.eventTypes ?? [], dataset, }; } diff --git a/packages/cli/src/types/metric-alert.ts b/packages/cli/src/types/metric-alert.ts index 93240abf0..4c1d9a4a2 100644 --- a/packages/cli/src/types/metric-alert.ts +++ b/packages/cli/src/types/metric-alert.ts @@ -97,17 +97,27 @@ export function metricAlertSource( /** * CLI dataset name for a stored data source, or the native dataset when no * CLI name selects it (e.g. EAP `trace_item_metric`). + * + * Missing or empty event types only resolve to a CLI name when every CLI name + * on that native dataset selects the same source; EAP is shared by spans, logs, + * and trace metrics, so it stays native rather than being guessed as `spans`. */ export function metricAlertDatasetName( dataset: string, eventTypes: readonly string[], ): string { - const match = METRIC_ALERT_DATASET_NAMES.find((name) => { - const source: MetricMonitorSource = METRIC_ALERT_DATASETS[name]; - return ( - source.dataset === dataset && - eventTypes.every((type) => source.eventTypes.includes(type)) + const candidates = METRIC_ALERT_DATASET_NAMES.filter( + (name) => METRIC_ALERT_DATASETS[name].dataset === dataset, + ); + if (eventTypes.length === 0) { + const sources = new Set( + candidates.map((name) => METRIC_ALERT_DATASETS[name]), ); + return sources.size === 1 && candidates[0] ? candidates[0] : dataset; + } + const match = candidates.find((name) => { + const source: MetricMonitorSource = METRIC_ALERT_DATASETS[name]; + return eventTypes.every((type) => source.eventTypes.includes(type)); }); return match ?? dataset; } diff --git a/packages/cli/test/lib/api/alerts.test.ts b/packages/cli/test/lib/api/alerts.test.ts index 573840007..e00d76303 100644 --- a/packages/cli/test/lib/api/alerts.test.ts +++ b/packages/cli/test/lib/api/alerts.test.ts @@ -248,6 +248,7 @@ describe("getMetricAlertRule", () => { ["trace_item_metric"], "events_analytics_platform", ], + ["events_analytics_platform", [], "events_analytics_platform"], ["metrics", [], "metrics"], ])( "maps native dataset %s with eventTypes %j to CLI dataset %s", @@ -696,6 +697,49 @@ describe("metric alert edit round-trip", () => { }); }); +describe("metric alert edit round-trip without event types", () => { + test.each([ + ["empty", { eventTypes: [] }], + ["missing", {}], + ])( + "does not rewrite an EAP monitor with %s eventTypes onto spans", + async (_label, eventTypes) => { + const storedSource = { + type: "snuba_query_subscription", + queryObj: { + snubaQuery: { + aggregate: "count()", + dataset: "events_analytics_platform", + ...eventTypes, + queryType: 1, + query: "", + timeWindow: 300, + }, + }, + }; + let putSource: Record | undefined; + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + if (req.method === "PUT") { + const body = (await req.json()) as Record; + putSource = (body.dataSources as Record[])[0]; + } + return Response.json(metricDetector({ dataSources: [storedSource] })); + }); + + const doc = await getMetricAlertRuleDocument("test-org", "9"); + expect(doc.dataset).toBe("events_analytics_platform"); + await putMetricAlertRule("test-org", "9", { ...doc, name: "Renamed" }); + + expect(putSource).toMatchObject({ + dataset: "events_analytics_platform", + eventTypes: [], + queryType: 1, + }); + }, + ); +}); + describe("putMetricAlertRule", () => { test("PUTs a nested detector body to the /detectors/{id}/ endpoint", async () => { globalThis.fetch = mockFetch(async (input, init) => { diff --git a/packages/cli/test/types/metric-alert.test.ts b/packages/cli/test/types/metric-alert.test.ts index 906078d42..f918ed4d3 100644 --- a/packages/cli/test/types/metric-alert.test.ts +++ b/packages/cli/test/types/metric-alert.test.ts @@ -27,6 +27,25 @@ describe("metric alert datasets", () => { } }); + test("keeps EAP native when event types are missing or empty", () => { + expect(metricAlertDatasetName("events_analytics_platform", [])).toBe( + "events_analytics_platform", + ); + }); + + test("resolves unambiguous datasets without event types", () => { + expect(metricAlertDatasetName("events", [])).toBe("errors"); + expect(metricAlertDatasetName("metrics", [])).toBe("metrics"); + }); + + test("keeps unmodelled EAP event types native", () => { + expect( + metricAlertDatasetName("events_analytics_platform", [ + "trace_item_metric", + ]), + ).toBe("events_analytics_platform"); + }); + test("ignores prototype keys", () => { expect(metricAlertSource("constructor")).toBeUndefined(); }); From 77bd980ef442bbb0da2803d699bdf4cfe4517afb Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 12:05:17 +0000 Subject: [PATCH 5/6] fix(cli): Keep detector config when editing metric alerts Edits always sent config { detectionType: static }, which replaced the stored config and turned percent and dynamic detectors into static ones (and deletes Seer data for dynamic detectors). Carry the stored detector config through the edit baseline and send it back; creates still default to static detection. --- packages/cli/src/lib/api/alerts.ts | 19 +++++++++++-- packages/cli/test/lib/api/alerts.test.ts | 36 ++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/lib/api/alerts.ts b/packages/cli/src/lib/api/alerts.ts index 626c4c177..5a072b45d 100644 --- a/packages/cli/src/lib/api/alerts.ts +++ b/packages/cli/src/lib/api/alerts.ts @@ -100,6 +100,8 @@ type MetricDetector = { owner?: { type: string; name?: string; id?: string } | string | null; dateCreated?: string; dataSources?: Record[] | null; + /** Detection settings (`detectionType`, `comparisonDelta`, …), kept on edit. */ + config?: Record | null; }; /** @@ -661,6 +663,10 @@ function buildDetectorDataSource( return source; } +function isDetectorConfig(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + /** * Translate a flat metric alert body into the nested detector write payload. * @@ -677,7 +683,11 @@ function buildDetectorBody( name: typeof body.name === "string" ? body.name : "", type: "metric_issue", dataSources: [buildDetectorDataSource(body)], - config: { detectionType: "static" }, + // Edits carry the stored config so percent and dynamic detectors keep + // their detection mode; creates default to static thresholds. + config: isDetectorConfig(body.detectorConfig) + ? body.detectorConfig + : { detectionType: "static" }, }; // create/edit store triggers already parsed by parseMetricTriggers. @@ -760,7 +770,9 @@ export async function deleteMetricAlertRule( * which translates it into the nested detector document. Returning the flat * shape (rather than the raw detector) keeps the command-side merge coherent. * The stored `eventTypes`/`queryType` ride along so an edit of a data source - * the CLI does not model (e.g. `trace_item_metric`) keeps its signal. + * the CLI does not model (e.g. `trace_item_metric`) keeps its signal, and the + * stored detector `config` rides along as `detectorConfig` so an edit keeps a + * percent or dynamic detector's detection mode. */ export async function getMetricAlertRuleDocument( orgSlug: string, @@ -777,6 +789,9 @@ export async function getMetricAlertRuleDocument( if (typeof source.queryType === "number") { document.queryType = source.queryType; } + if (isDetectorConfig(detector.config)) { + document.detectorConfig = detector.config; + } return document; } diff --git a/packages/cli/test/lib/api/alerts.test.ts b/packages/cli/test/lib/api/alerts.test.ts index e00d76303..35f70e906 100644 --- a/packages/cli/test/lib/api/alerts.test.ts +++ b/packages/cli/test/lib/api/alerts.test.ts @@ -740,6 +740,42 @@ describe("metric alert edit round-trip without event types", () => { ); }); +describe("metric alert edit round-trip of detector config", () => { + test("keeps a percent detector's config on a rename", async () => { + const storedConfig = { detectionType: "percent", comparisonDelta: 3600 }; + let putConfig: unknown; + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + if (req.method === "PUT") { + const body = (await req.json()) as Record; + putConfig = body.config; + } + return Response.json(metricDetector({ config: storedConfig })); + }); + + const doc = await getMetricAlertRuleDocument("test-org", "9"); + await putMetricAlertRule("test-org", "9", { ...doc, name: "Renamed" }); + + expect(putConfig).toEqual(storedConfig); + }); + + test("defaults to static detection without a stored config", async () => { + let putConfig: unknown; + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + if (req.method === "PUT") { + const body = (await req.json()) as Record; + putConfig = body.config; + } + return Response.json(metricDetector({})); + }); + + await putMetricAlertRule("test-org", "9", { name: "Renamed" }); + + expect(putConfig).toEqual({ detectionType: "static" }); + }); +}); + describe("putMetricAlertRule", () => { test("PUTs a nested detector body to the /detectors/{id}/ endpoint", async () => { globalThis.fetch = mockFetch(async (input, init) => { From ac111f135b3d944f8f862b123cf13058df4bd09b Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 12:13:23 +0000 Subject: [PATCH 6/6] fix(cli): Send static config with threshold triggers on metric edits Edits resent the stored detector config even when --trigger supplied alertThreshold triggers, so percent detectors read the thresholds as percent comparisons and dynamic detectors got static conditions the backend rejects. Threshold triggers are static thresholds, so send a static config with them; native conditions and trigger-less edits keep the stored config. --- packages/cli/src/lib/api/alerts.ts | 14 ++++--- packages/cli/test/lib/api/alerts.test.ts | 50 ++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/lib/api/alerts.ts b/packages/cli/src/lib/api/alerts.ts index 5a072b45d..74b0fdc14 100644 --- a/packages/cli/src/lib/api/alerts.ts +++ b/packages/cli/src/lib/api/alerts.ts @@ -679,19 +679,23 @@ function isDetectorConfig(value: unknown): value is Record { function buildDetectorBody( body: Record, ): Record { + // create/edit store triggers already parsed by parseMetricTriggers. + const triggers = body.triggers as MetricTriggers | undefined; + // Threshold triggers are static thresholds. Otherwise edits carry the stored + // config so percent and dynamic detectors keep their detection mode, and + // creates default to static. + const keepStoredConfig = + isDetectorConfig(body.detectorConfig) && + !(triggers && !isDetectorConditionList(triggers)); const detectorBody: Record = { name: typeof body.name === "string" ? body.name : "", type: "metric_issue", dataSources: [buildDetectorDataSource(body)], - // Edits carry the stored config so percent and dynamic detectors keep - // their detection mode; creates default to static thresholds. - config: isDetectorConfig(body.detectorConfig) + config: keepStoredConfig ? body.detectorConfig : { detectionType: "static" }, }; - // create/edit store triggers already parsed by parseMetricTriggers. - const triggers = body.triggers as MetricTriggers | undefined; if (triggers) { detectorBody.conditionGroup = { logicType: "any", diff --git a/packages/cli/test/lib/api/alerts.test.ts b/packages/cli/test/lib/api/alerts.test.ts index 35f70e906..2b226d90b 100644 --- a/packages/cli/test/lib/api/alerts.test.ts +++ b/packages/cli/test/lib/api/alerts.test.ts @@ -759,6 +759,56 @@ describe("metric alert edit round-trip of detector config", () => { expect(putConfig).toEqual(storedConfig); }); + test.each([ + ["percent", { detectionType: "percent", comparisonDelta: 3600 }], + ["dynamic", { detectionType: "dynamic" }], + ])( + "switches a %s detector to static for threshold triggers", + async (_label, storedConfig) => { + let putConfig: unknown; + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + if (req.method === "PUT") { + const body = (await req.json()) as Record; + putConfig = body.config; + } + return Response.json(metricDetector({ config: storedConfig })); + }); + + const doc = await getMetricAlertRuleDocument("test-org", "9"); + await putMetricAlertRule("test-org", "9", { + ...doc, + triggers: [{ alertThreshold: 100 }], + }); + + expect(putConfig).toEqual({ detectionType: "static" }); + }, + ); + + test("keeps the stored config for native detector conditions", async () => { + const storedConfig = { detectionType: "percent", comparisonDelta: 3600 }; + let putConfig: unknown; + globalThis.fetch = mockFetch(async (input, init) => { + const req = new Request(input!, init); + if (req.method === "PUT") { + const body = (await req.json()) as Record; + putConfig = body.config; + } + return Response.json(metricDetector({ config: storedConfig })); + }); + + const doc = await getMetricAlertRuleDocument("test-org", "9"); + await putMetricAlertRule("test-org", "9", { + ...doc, + triggers: [ + { type: "gt", comparison: 110, conditionResult: 75 }, + { type: "lte", comparison: 105, conditionResult: 0 }, + ], + }); + + expect(putConfig).toEqual(storedConfig); + }); + test("defaults to static detection without a stored config", async () => { let putConfig: unknown; globalThis.fetch = mockFetch(async (input, init) => {