feat(incidents): ingest alerts from external systems over a webhook - #13501
feat(incidents): ingest alerts from external systems over a webhook#13501prabhatsharma wants to merge 9 commits into
Conversation
|
Failed to generate code suggestions for PR |
prabhatsharma
added
the
e2e
⏭️ Playwright E2E Crosscheck — handled by the sister ENT PRThis OSS PR has a matching enterprise PR on branch |
AI Code Review (DeepSeek-V4-Pro)Decision: minor_issuesBoth warnings from the prior review remain unfixed: injecting Findings: 🔴 0 blocker · 🟡 2 warnings · 🔵 3 suggestions DetailsShow findings (5)🟡 Warnings
🔵 Suggestions
|
Closes openobserve/o2-enterprise#2286. Incident correlation could only ever see alerts OpenObserve evaluated itself, so an org running Alertmanager or Datadog got a partial picture of an incident — which is worse than none, because it looks complete. Adds `POST /api/v2/{org_id}/alerts/incidents/ingest`. The payload is turned into a synthetic in-memory Alert (never persisted) and handed to the existing `correlate_alert_to_incident` path, so external and native alerts land in the same incident whenever their identity labels agree, and in separate incidents when they do not. Only `labels` drives correlation; the semantic field groups already normalize vendor-specific names, so no per-vendor adapters are needed. Three things the correlation engine silently depends on, each pinned by a test: - The synthetic alert's id is derived from (org, source, alert_name), never generated fresh. Correlation distinguishes "this alert type is already here" from "a new alert type joined" purely by alert id — a fresh id per delivery would notify on every repeat, and `Alert::get_unique_key()` returning "" for a `None` id would collapse every external alert into one identity and suppress notifications that should fire. - The id is KSUID-shaped, because `get_incident_with_alerts` parses alert_id as a Ksuid and silently skips anything that fails. - `destinations` is empty. Routing belongs to the incident's native alerts; an external sender does not get to choose who is paged. Also: - `status: resolved` closes the alert's contribution and resolves the incident once nothing in it is still firing. Without it, external alerts would sit open until O2_INCIDENTS_AUTO_RESOLVE_AFTER_MINUTES (3 days by default). - `dedup_key` makes redelivery idempotent, reusing the existing alert_dedup_state table rather than adding a parallel store. - `get_incident_with_alerts` renders external alerts from the junction row instead of looking them up and logging a miss. Migration adds `source`, `external_url`, `annotations` and `resolved_at` to alert_incident_alerts; `source` NULL means native, so every existing row is unaffected. UI: new Alerts tab under Data Sources > Custom, alongside Logs/Metrics/Traces, with firing, resolve and Alertmanager-forwarding examples. It does not use the rich DataSourceSetupCard — that card's detect step counts rows on a stream, and these alerts never land in one. The Alertmanager example shows the payload transform rather than a bare webhook_configs block, which would look correct and 400 at runtime. Requires the companion o2-enterprise change for the OpenFGA route permission.
The placeholder URL did not match where the page actually lands (user-guide/analytics/incidents/incident-webhook).
db_schema_version_check requires the constant to advance whenever a migration file is added; 53 -> 54.
…path Two defects found reviewing this branch against its own documentation. **Severity was accepted but never applied.** `ExternalAlertPayload.severity` was parsed, length-validated, mapped by `map_external_severity`, and documented with a full vocabulary table — and then dropped on the floor. `create_new_incident` called `determine_severity(None)` unconditionally, so every externally-ingested alert opened its incident at the default severity no matter what the sender reported. A caller sending `"severity": "critical"` got a P3. `map_external_severity` had no non-test callers at all, which is what gave it away. It now normalizes at the ingest boundary and rides to incident creation on `ExternalAlertMeta`, which was already threaded down that path. An unrecognized value still maps to `None` so the default stands rather than being replaced by a guess. **Resolve bypassed the crate-level `update_status`.** `resolve_external_alert` called `infra::table::alert_incidents::update_status` directly, which only writes the row. The crate-level function is what also emits the `Resolved` timeline event and publishes the status change to the super cluster — so an incident resolved by an upstream system showed no resolution on its timeline and stayed open forever on every peer cluster. Both are covered by tests that fail against the previous code.
dd073bd to
d2e5982
Compare
Self-review pass + rebase onto mainRebased onto Reviewing the branch against its own documentation turned up two real defects, both now fixed with regression tests: 1.
|
Second review pass over the ingest payload validation. **external_url accepted any scheme.** The field is stored and handed back to clients as a link to render — the payload reference literally calls it a deep link. Only its length was checked, so `javascript:alert(1)` or a `data:` URL would round-trip intact and become stored XSS the moment the incident detail view renders it as an anchor. Now an allowlist: `http://` or `https://` only. Nothing renders it yet, which is exactly why this is worth fixing now — the trap would otherwise be sprung by whoever wires up that view. **timestamp was unbounded.** It is documented as microseconds, but seconds and milliseconds are what most senders have to hand. A seconds value silently placed the incident in 1970, where the auto-resolve sweep treats it as long stale and closes it on the next tick: the caller sees 200 and the alert disappears. A far-future value pinned last_alert_at ahead of now so the incident could never age out at all. Now rejected outside 2000..2100 with an error naming the likely unit mistake. Bounds are constants rather than clock-relative so validation stays pure. Docs updated to match, including one imprecision the review surfaced: an incident that also holds natively-evaluated alerts never auto-closes on resolve, because native alerts have no upstream resolved signal. That is the correct behaviour — closing on the external half would hide a native alert still firing — but the page claimed otherwise.
Second review pass — two more findings1.
|
Third review pass. `dedup_key` was advertised in the API, documented with its
own section, and had never worked once.
Idempotency state was being written to `alert_dedup_state`, whose `alert_id`
column carries a foreign key to `alerts` (`fk_alert_dedup_alert`, added in
m20251024_000001). An externally-ingested alert has no row in `alerts` by
design — that is the whole premise of this feature — so every write violated
the constraint. The write was wrapped in a warn-only handler, so the request
still returned 200 while:
- the insert failed on every single ingest, logging a warning each time;
- the paired read therefore always missed, making the dedup check dead code;
- retries were recorded as distinct firings, inflating `alert_count`.
Idempotency now lives on the `alert_incident_alerts` junction row, which has
no foreign key to `alerts`. The key is written by the same insert that records
the alert, so there is no window where a firing is stored but its dedup key is
not. Adds a `dedup_key` column and an `(alert_id, dedup_key)` index to the
migration this branch already ships.
Behaviour change worth noting: a payload with no `dedup_key` is no longer
deduplicated at all. The previous fallback keyed on `(source, alert_name)`,
which would have collapsed two genuine firings of the same rule minutes apart
into one. Without a key nothing distinguishes a retry from a real re-fire, and
losing a real alert is worse than recording a duplicate. Docs updated.
Also in this pass:
- `find_open_incidents_containing_alert` loaded full junction models for
every firing the alert had ever had, across all orgs, just to collect
incident ids. Now selects the id column only, distinct.
- Removed `dedup_identity`, which after the rewrite had only test callers
and described a fallback the system no longer has — the same dead-code
shape that hid the severity bug two passes ago. Replaced with
`effective_dedup_key` and tests that exercise the real behaviour.
Third review pass —
|
The schemathesis fuzz job flagged this endpoint under "schema constraints don't match API validation" — it generated payloads that satisfied the declared OpenAPI schema and got 400s back, because the schema said nothing about the rules `validate()` actually applies. Declares them: min/max length on `source` and `alert_name`, max length on `dedup_key`, the `^https?://` pattern on `external_url`, and the microsecond bounds on `timestamp`. Generated clients now see the real contract, and the timestamp bound in particular documents itself at the schema level rather than only failing at runtime. No behaviour change — `validate()` was already enforcing all of this.
Adds 20 tests in tests/api-testing against a live server, plus a `client.incidents` wrapper. These exercise what unit tests structurally cannot: the migration having applied, correlation actually grouping alerts, idempotency resolving against the database, and resolve closing an incident. Enables O2_INCIDENTS_ENABLED in the api-testing workflow — without it the endpoint 403s and none of this runs. The tests skip cleanly via /config's `incidents_enabled` when pointed at a non-enterprise build. Running them immediately found a correlation defect that every prior review pass missed, because it is invisible in the diff. `DimensionRelationship::check` returns `NewIsSuperset` when the EXISTING incident has no dimensions — "compatible with anything". So the first alert that fails to correlate opens a dimensionless incident, and every unrelated alert afterwards joins it. On a default install, where no semantic field groups are configured, that is not a corner case: it is the common path, and it silently collapses every external alert into one incident. The documented promise that an alert with no matching labels gets an incident of its own was false. `find_or_create_incident` now skips dimensionless incidents when evaluating join candidates. Empty means "identity unknown", not "matches everything". Note for reviewers: this touches correlation shared with natively-evaluated alerts, not just the ingest path. The previous behaviour looks unintentional — nothing else treats an empty dimension set as a wildcard — but it predates this branch and deserves a second opinion. Two unit tests pin the relationship semantics, including the empty-existing case, so the trap is documented where the next reader will meet it.
Review pass over the tests themselves. The previous set passed, but several assertions were weaker than they looked and the most important behaviour — the correlation fix in the preceding commit — had no test of its own. Added: - **Dimensionless-magnet regression.** The defect the suite found was only caught incidentally, by an unrelated test's expectation shifting. Now explicit: ingest an alert whose labels yield no dimension (on a default install `service` maps to no semantic group, `k8s_namespace_name` does), then ingest an unrelated labelled alert and assert they do NOT merge. Verified it actually catches the bug by reverting the guard, rebuilding, and confirming this test fails — a regression test that passes against the broken code would have been worthless. - **Severity mapping** parameterised across critical/P1/error/warning/info, plus the unrecognised-value fallback. Previously only critical -> P1. - **external_url and annotations round-trip.** These columns exist so an external alert can render without a row in `alerts`; nothing checked they came back. - **Refire after resolve** opens a new incident rather than reopening the closed one. Previously reasoned about, never executed. - **Microsecond timestamp is honoured** — the rejection cases were covered, the accepted case was not. - **`_o2_external_source` does not leak into group_values.** Had it, every alert from one source would correlate together regardless of identity. - **alert_count advances** on repeat firings. Strengthened: the external_url acceptance test asserted only that an incident came back; it now checks the URL round-trips, and covers uppercase scheme. Documented gap, deliberately not faked: external + native alert correlating into one incident is the feature's headline claim and is still uncovered. It needs a native alert whose evaluated row carries matching dimensions — a stream, ingested data, an alert definition and a scheduler tick. The manual-trigger shortcut does not work, since its synthetic row carries none of the identity labels correlation groups on.
2 participants
Implements the external alert ingest webhook. Pairs with openobserve/o2-enterprise#2287 (OpenFGA route permission — required, the endpoint is unauthorized without it).
Design and rationale: openobserve/o2-enterprise#2286.
Why
Incident correlation could only ever see alerts OpenObserve evaluated itself. An org running Alertmanager or Datadog — i.e. most of them — got a partial picture of an incident, which is worse than no picture because it looks complete.
What
POST /api/v2/{org_id}/alerts/incidents/ingest{ "source": "alertmanager", // required "alert_name": "HighErrorRate", // required "dedup_key": "a1b2c3", // idempotency across retries "severity": "critical", // mapped onto IncidentSeverity "status": "firing", // firing | resolved "timestamp": 1753612800000000, // epoch micros, defaults to now "labels": { "service": "checkout", "k8s_namespace_name": "prod" }, "annotations": { "summary": "…" }, "external_url": "https://…" }The payload becomes a synthetic in-memory
Alert— never persisted — and goes through the existingcorrelate_alert_to_incidentpath. External and native alerts land in the same incident when their identity labels agree, and separate incidents when they don't. Onlylabelsdrives correlation, and the semantic field groups already normalize vendor-specific names (host/instance/k8s.node.name), so no per-vendor adapters are needed.The junction table never had an FK to
alerts, so external alerts join incidents without a row there. That's what makes this cheap.The three sharp edges, each pinned by a test
These are the ones that fail silently rather than loudly:
Alert::get_unique_key()returns""whenidisNone. Correlation tells "this alert type is already here" from "a new alert type joined" purely by alert id. ANoneid would give every external alert the same identity and suppress notifications that should fire; a fresh id per delivery would notify on every repeat. The id is derived from(org, source, alert_name)— stable across deliveries, distinct per rule.get_incident_with_alertsparsesalert_idas a Ksuid and skips anything that fails. The derived id is KSUID-shaped, with a fixed timestamp prefix since it encodes identity, not creation time.destinationsis empty. Routing belongs to the incident's native alerts — an external sender doesn't get to choose who gets paged. Deliberate, not an oversight.Also in scope
status: resolvedcloses the alert's contribution; the incident resolves once nothing in it is still firing. Without this, external alerts sit open untilO2_INCIDENTS_AUTO_RESOLVE_AFTER_MINUTES(3 days by default).dedup_keyreuses the existingalert_dedup_statetable and its cleanup job rather than adding a parallel store. 30-minute window — long enough for retries, short enough that a genuine re-fire hours later isn't swallowed.get_incident_with_alertsnow builds external alerts from the junction row instead of looking them up and logging a miss.source,external_url,annotations,resolved_atonalert_incident_alerts.sourceNULL means native, so every existing row is untouched.UI — Data Sources › Custom › Alerts
New tab alongside Logs / Metrics / Traces, with firing, resolve, and Alertmanager-forwarding examples.
Two deliberate departures worth flagging:
DataSourceSetupCard. That card'sdetectstep counts rows on a stream to confirm the integration works. These alerts never land in a stream, so there's nothing to count — the card's core affordance doesn't apply. It follows the simplerIngestionContent+CopyContentsibling pattern (metrics/VMagentConfig.vue) instead.webhook_configs. Alertmanager posts its own fixed shape, which this endpoint rejects. A barewebhook_configsblock would look right and 400 at runtime. A test asserts the example does not containwebhook_configs.Verification
cargo test -p config --lib meta::alerts::incidentscargo test -p openobserve-core --features enterprise --lib alerts::incidents::testscargo clippy(core, infra, config, api-management, super_cluster_queue)-- -D warningsnpm run type-checknpm run lint:design:strictnpx vitest run src/components/ingestion/alertsCustom.spec.ts+useIngestionRoutes.spec.tscargo fmt --allThe workspace build couldn't be run end to end locally —
vectorscan-rs-sysfails to build in this environment (native dep, unrelated to this change). Every crate this PR touches was checked individually with--features enterprise, andopenobserve-api-httpwas checked separately for the router and OpenAPI registration.Not in scope