diff --git a/docs/core/alerting.md b/docs/core/alerting.md index 45add95..55872d9 100644 --- a/docs/core/alerting.md +++ b/docs/core/alerting.md @@ -32,18 +32,25 @@ Notifications are sent via `asyncio.create_task()` after the function returns, s Each `AlertRule` has one associated `AlertState` row. The state transitions are: ``` -inactive ──(breached)──► pending ──(consecutive count met)──► FIRING - │ │ - └──(recovered)──► inactive (no notification) │ - │ - FIRING ──(recovered)──► RESOLVED ──► inactive - FIRING ──(acknowledged)──► ACKNOWLEDGED - ACKNOWLEDGED ──(recovered)──► RESOLVED ──► inactive - ACKNOWLEDGED ──(re-breached)──► FIRING +inactive ──(breached)──► pending ──(N breaching checks in a row)──► FIRING + ▲ │ │ + └──(an OK check)───────┘ │ + FIRING ──(acknowledged)──► ACKNOWLEDGED + FIRING / ACKNOWLEDGED ──(M OK checks in a row)──► RESOLVED ──► inactive ``` +N is the rule's consecutive checks, M its checks to clear. An acknowledgement +holds for as long as the breach lasts: only a resolve clears it, and a breaching +check never sends an acknowledged alert back to firing. + `RESOLVED` is transient — the evaluator writes the event and immediately sets state back to `inactive` within the same transaction. `RESOLVED` never persists as a final state in the DB. +Every transition is an `AlertEvent`, the history the Alerts → History page and +MCP `get_alert_history` read. A transition a person made carries them in +`actor_id`: an acknowledgement (firing → acknowledged), or a rule edit, disable +or enable that reset the alert (→ resolved, with a note such as "rule +disabled"). Transitions caused by readings, or by Steward itself, have none. + ### State Definitions | State | Meaning | @@ -51,7 +58,7 @@ inactive ──(breached)──► pending ──(consecutive count met)── | `inactive` | Threshold not breached | | `pending` | Threshold breached but consecutive count not yet met; no notification sent | | `firing` | Consecutive count met; FIRING notification sent | -| `acknowledged` | Operator acknowledged; suppresses repeat notifications; auto-clears on recovery | +| `acknowledged` | Someone has seen it; no further notifications; clears when the readings recover | | `resolved` | Transient — notification sent, immediately transitions to `inactive` | --- diff --git a/steward/alerts/routes.py b/steward/alerts/routes.py index 18ae454..e1712ae 100644 --- a/steward/alerts/routes.py +++ b/steward/alerts/routes.py @@ -144,8 +144,9 @@ async def active_fragment(): rules_off=sum(1 for r in rules if not r.enabled)) -def _history_row(event, rule) -> dict: - """One transition as the history page shows it.""" +def _history_row(event, rule, names: dict | None = None) -> dict: + """One transition as the history page shows it. `names` maps user ids to + usernames: a transition a person made says who (#5463).""" from steward.core.alerts import MUTED, RELEASED from steward.core.formatting import metric_label, metric_value notes, muted_by, released = [], None, False @@ -156,6 +157,12 @@ def _history_row(event, rule) -> dict: released = True elif part: notes.append(part) + actor = getattr(event, "actor_id", None) + by = (names or {}).get(actor) or ("a deleted user" if actor else None) + if event.to_state == "acknowledged": + notes.insert(0, f"acknowledged by {by}" if by else "acknowledged") + elif by and notes: + notes[0] = f"{notes[0]} by {by}" metric = rule.metric_name if event.metric_value is None: value = f"No {metric_label(metric)} reading" @@ -193,14 +200,15 @@ async def alert_history(): rule_id = chosen.id if chosen else None rows = await alert_svc.history(db, since, rule_id, PAGE_SIZE, before) show_notified, show_action = await alert_svc.history_columns(db, since, rule_id) + names = await alert_svc.usernames(db, (e.actor_id for e, _ in rows)) verdict = None if chosen and not before: every = await alert_svc.history(db, since, rule_id, HISTORY_SUMMARY_ROWS) verdict = summary(episodes([_history_row(e, r) for e, r in every], newest_page=True, now=now, since=since)) rows, after = next_page(rows, lambda row: (row[0].transitioned_at, row[0].id)) - ctx = {"episodes": episodes([_history_row(e, r) for e, r in rows], newest_page=not before, - now=now, since=since), + ctx = {"episodes": episodes([_history_row(e, r, names) for e, r in rows], + newest_page=not before, now=now, since=since), "after": after, "rules": rules, "chosen": chosen, "current_range": range_key, "show_notified": show_notified, "show_action": show_action, "verdict": verdict} if before: @@ -327,7 +335,7 @@ async def update_rule(rule_id: str): fields = await alert_svc.rule_fields(db, **_rule_form(form)) except ValueError as exc: return str(exc), 400 - await alert_svc.apply_rule_change(db, rule, fields) + await alert_svc.apply_rule_change(db, rule, fields, actor_id=session.get("user_id")) if apply_action: rule.ansible_action = ansible_action await log_audit(current_app, session.get("user_id"), session.get("username", ""), @@ -344,7 +352,8 @@ async def toggle_rule(rule_id: str): rule = result.scalar_one_or_none() if rule is None: return "Not found", 404 - await alert_svc.apply_rule_change(db, rule, {}, enabled=not rule.enabled) + await alert_svc.apply_rule_change(db, rule, {}, enabled=not rule.enabled, + actor_id=session.get("user_id")) action = "alert_rule.enabled" if rule.enabled else "alert_rule.disabled" await log_audit(current_app, session.get("user_id"), session.get("username", ""), action, entity_type="alert_rule", entity_id=rule.name) diff --git a/steward/alerts/service.py b/steward/alerts/service.py index 523438c..cdd1e62 100644 --- a/steward/alerts/service.py +++ b/steward/alerts/service.py @@ -203,18 +203,24 @@ async def breach_started(db, rule_ids) -> dict[str, datetime]: )).all()) +async def usernames(db, ids) -> dict[str, str]: + """{user id: username} for the given ids (None and repeats ignored).""" + from steward.models.users import User + ids = {i for i in ids if i} + if not ids: + return {} + return dict((await db.execute(select(User.id, User.username).where(User.id.in_(ids)))).all()) + + async def active_rows(db, now: datetime) -> list[dict]: """The alerts page's active list, firing first, then no data, then acknowledged, then pending; within each the longest-running first. Each row: its state, rule, label, sentence, the link to what it watches, when the breach began, and who acknowledged it.""" - from steward.models.users import User alerts = await active_alerts(db) started = await breach_started(db, {rule.id for state, rule in alerts if state.state.value in _BREACHING}) - ackers = {s.acknowledged_by for s, _ in alerts if s.acknowledged_by} - names = dict((await db.execute( - select(User.id, User.username).where(User.id.in_(ackers)))).all()) if ackers else {} + names = await usernames(db, (s.acknowledged_by for s, _ in alerts)) rows = [] for state, rule in alerts: label = alert_label(state) @@ -430,11 +436,12 @@ async def rule_fields(db, *, name: str, source_module: str, resource_name: str, WATCHED_FIELDS = ("source_module", "resource_name", "metric_name", "operator", "threshold") -async def apply_rule_change(db, rule: AlertRule, fields: dict, *, enabled: bool | None = None) -> None: +async def apply_rule_change(db, rule: AlertRule, fields: dict, *, enabled: bool | None = None, + actor_id: str | None = None) -> None: """Apply validated fields (and optionally enabled) to a rule, in the caller's transaction. A change to what it watches, or disabling or re-enabling it, puts its alert back to inactive and starts its silence - watch over (core.alerts.reset_rule_state).""" + watch over (core.alerts.reset_rule_state); the event says who did it.""" from steward.core.alerts import reset_rule_state changed = [k for k in WATCHED_FIELDS if k in fields and getattr(rule, k) != fields[k]] for k, v in fields.items(): @@ -444,11 +451,11 @@ async def apply_rule_change(db, rule: AlertRule, fields: dict, *, enabled: bool if enabled is not None: rule.enabled = bool(enabled) if disabling: - await reset_rule_state(db, rule, "rule disabled") + await reset_rule_state(db, rule, "rule disabled", actor_id) elif enabling: - await reset_rule_state(db, rule, "rule enabled") + await reset_rule_state(db, rule, "rule enabled", actor_id) elif changed: - await reset_rule_state(db, rule, "rule changed: " + ", ".join(changed)) + await reset_rule_state(db, rule, "rule changed: " + ", ".join(changed), actor_id) async def add_rule(db, fields: dict, *, created_by: str, enabled: bool = True, @@ -462,11 +469,14 @@ async def add_rule(db, fields: dict, *, created_by: str, enabled: bool = True, return rule -async def acknowledge(db, rule_id: str, user_id: str) -> AlertState: - """Acknowledge a firing alert, in the caller's transaction. +async def acknowledge(db, rule_id: str, user_id: str | None) -> AlertState: + """Acknowledge a firing alert, in the caller's transaction, and record it + in the alert's history as a firing → acknowledged event by `user_id`, so + who saw it outlives the alert (#5463). LookupError when the rule has no state; ValueError when it is not firing. """ + from steward.core.alerts import _add_event state = (await db.execute( select(AlertState).where(AlertState.rule_id == rule_id))).scalar_one_or_none() if state is None: @@ -479,6 +489,8 @@ async def acknowledge(db, rule_id: str, user_id: str) -> AlertState: state.acknowledged_by = user_id state.acknowledged_at = now state.last_transition_at = now + _add_event(db, rule_id, AlertStateEnum.firing.value, AlertStateEnum.acknowledged.value, + None, now, actor_id=user_id) return state diff --git a/steward/core/alerts.py b/steward/core/alerts.py index e4a329b..2e01dc1 100644 --- a/steward/core/alerts.py +++ b/steward/core/alerts.py @@ -432,19 +432,22 @@ async def _evaluate_rule( return notifications -async def reset_rule_state(session: AsyncSession, rule: AlertRule, note: str) -> None: +async def reset_rule_state(session: AsyncSession, rule: AlertRule, note: str, + actor_id: str | None = None) -> None: """Put a rule's alert back to inactive because the rule itself changed (what it watches, its threshold, or it was disabled or re-enabled), and start its silence watch over. An alert that was - firing or acknowledged gets a resolved event carrying `note`; nobody is - notified, since nothing about the watched resource changed.""" + firing or acknowledged gets a resolved event carrying `note` and the + person who changed the rule (`actor_id`); nobody is notified, since + nothing about the watched resource changed.""" state = (await session.execute( select(AlertState).where(AlertState.rule_id == rule.id))).scalar_one_or_none() if state is None: return now = datetime.now(timezone.utc) if state.state in (AlertStateEnum.firing, AlertStateEnum.acknowledged): - _add_event(session, rule.id, state.state.value, "resolved", None, now, note=note) + _add_event(session, rule.id, state.state.value, "resolved", None, now, note=note, + actor_id=actor_id) # The watch starts over now: silence is measured from here, against the # cadence of whatever the rule watches from now on. state.last_transition_at = now @@ -466,6 +469,7 @@ def _add_event( value: float | None, now: datetime, note: str | None = None, + actor_id: str | None = None, ) -> str: """Add an AlertEvent row and return its ID.""" event_id = str(uuid.uuid4()) @@ -477,6 +481,7 @@ def _add_event( metric_value=value, transitioned_at=now, note=note, + actor_id=actor_id, ) session.add(event) return event_id diff --git a/steward/mcp/tools/alerts.py b/steward/mcp/tools/alerts.py index 0eb78cb..3a3ceec 100644 --- a/steward/mcp/tools/alerts.py +++ b/steward/mcp/tools/alerts.py @@ -8,7 +8,7 @@ from sqlalchemy import select from steward.alerts import service as alert_svc from steward.mcp._context import current_caller from steward.models.alerts import AlertOperator, AlertRule, AlertState, MaintenanceWindow -from steward.models.users import User, UserRole +from steward.models.users import UserRole from ._audit import audit_mcp from ._shape import ( @@ -33,13 +33,6 @@ def _action(rule: AlertRule) -> dict | None: "extra_vars", "check") if a.get(k) is not None} -async def _usernames(db, ids) -> dict[str, str]: - ids = {i for i in ids if i} - if not ids: - return {} - return dict((await db.execute(select(User.id, User.username).where(User.id.in_(ids)))).all()) - - async def list_alerts() -> dict: """Alerts that need attention now: firing, pending, acknowledged but still breaching, or gone silent (no_data). @@ -56,7 +49,7 @@ async def list_alerts() -> dict: """ async with db_session() as db: active = await alert_svc.active_alerts(db) - names = await _usernames(db, [s.acknowledged_by for s, _ in active]) + names = await alert_svc.usernames(db, (s.acknowledged_by for s, _ in active)) order = {"firing": 0, "no_data": 1, "pending": 2, "acknowledged": 3} rows = [ { @@ -89,7 +82,9 @@ async def get_alert_history(rule: str = "", range: str = "7d", limit: int = 50) Each transition carries the metric value that caused it and whether the email/webhook notification went out (with the error if not), plus the id - of any Ansible run the rule's action triggered. + of any Ansible run the rule's action triggered. `by` names the person who + made it happen (an acknowledgement, or a rule edit that reset the alert); + null when a reading or Steward itself did. """ since, key = lookback(range) limit = clamp_limit(limit, 50, 500) @@ -97,6 +92,7 @@ async def get_alert_history(rule: str = "", range: str = "7d", limit: int = 50) rules = await alert_svc.list_rules(db) chosen = pick(rule, rules, kind="alert rule", list_tool="list_alert_rules") if rule else None rows = await alert_svc.history(db, since, chosen.id if chosen else None, limit) + names = await alert_svc.usernames(db, (e.actor_id for e, _ in rows)) events = [ { "rule_id": r.id, @@ -105,6 +101,7 @@ async def get_alert_history(rule: str = "", range: str = "7d", limit: int = 50) "to_state": e.to_state, "metric_value": rnd(e.metric_value), "note": e.note, + "by": names.get(e.actor_id, e.actor_id), "at": iso(e.transitioned_at), "age_s": age_s(e.transitioned_at), "email_sent": e.email_sent, @@ -296,7 +293,8 @@ async def update_alert_rule(rule: str, name: str | None = None, source_module: s consecutive_ok_required=(r.consecutive_ok_required if clear_after_checks is None else clear_after_checks), notify_no_data=r.notify_no_data if notify_no_data is None else notify_no_data) - await alert_svc.apply_rule_change(db, r, fields, enabled=enabled) + await alert_svc.apply_rule_change(db, r, fields, enabled=enabled, + actor_id=current_caller().user_id) changed = sorted(k for k, v in {"name": name, "source_module": source_module, "resource": resource, "metric": metric, "operator": operator, "threshold": threshold, "consecutive_checks": consecutive_checks, diff --git a/steward/migrations/versions/0040_alert_event_actor.py b/steward/migrations/versions/0040_alert_event_actor.py new file mode 100644 index 0000000..389e9d5 --- /dev/null +++ b/steward/migrations/versions/0040_alert_event_actor.py @@ -0,0 +1,24 @@ +"""Who made an alert transition happen (#5463): an acknowledgement, or a rule +edit, disable or enable that reset the alert, records the person on its event. + +Revision ID: 0040_alert_event_actor +Revises: 0039_password_resets +Create Date: 2026-10-09 +""" +from typing import Sequence, Union +from alembic import op +import sqlalchemy as sa + +revision: str = "0040_alert_event_actor" +down_revision: Union[str, None] = "0039_password_resets" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.add_column("alert_events", sa.Column( + "actor_id", sa.String(36), sa.ForeignKey("users.id", ondelete="SET NULL"), nullable=True)) + + +def downgrade() -> None: + op.drop_column("alert_events", "actor_id") diff --git a/steward/models/alerts.py b/steward/models/alerts.py index 8141259..6bf5702 100644 --- a/steward/models/alerts.py +++ b/steward/models/alerts.py @@ -124,6 +124,12 @@ class AlertEvent(Base): email_error: Mapped[str | None] = mapped_column(Text, nullable=True) webhook_sent: Mapped[bool | None] = mapped_column(Boolean, nullable=True) webhook_error: Mapped[str | None] = mapped_column(Text, nullable=True) + # Who made this transition happen, when a person did: an acknowledgement, + # or a rule edit, disable or enable that reset the alert (#5463). None for + # every transition a sample or Steward itself caused. + actor_id: Mapped[str | None] = mapped_column( + String(36), ForeignKey("users.id", ondelete="SET NULL"), nullable=True + ) # Set when this firing event triggered an Ansible action run (audit link). ansible_run_id: Mapped[str | None] = mapped_column( String(36), ForeignKey("ansible_runs.id", ondelete="SET NULL"), nullable=True diff --git a/tests/core/test_alert_history_row.py b/tests/core/test_alert_history_row.py index ede2e38..4bbd54f 100644 --- a/tests/core/test_alert_history_row.py +++ b/tests/core/test_alert_history_row.py @@ -16,7 +16,7 @@ RULE = SimpleNamespace(metric_name="cpu_pct", operator=AlertOperator(">"), thres def _event(**kw): base = dict(transitioned_at=datetime(2026, 10, 6, tzinfo=timezone.utc), from_state="inactive", to_state="firing", metric_value=97.5, note=None, email_sent=None, email_error=None, - webhook_sent=None, webhook_error=None, ansible_run_id=None) + webhook_sent=None, webhook_error=None, ansible_run_id=None, actor_id=None) return SimpleNamespace(**{**base, **kw}) @@ -48,3 +48,16 @@ def test_only_attempted_deliveries_are_listed(): row = _history_row(_event(email_sent=True, webhook_sent=True), RULE) assert [(label, sent) for label, sent, _ in row["deliveries"]] == [("email", True), ("webhook", True)] assert _history_row(_event(), RULE)["deliveries"] == [] + + +def test_a_transition_a_person_made_says_who(): + """#5463: an acknowledgement, and a rule change that reset the alert, + name the person; one whose account is gone still reads as a person's.""" + names = {"u1": "bob"} + ack = _event(from_state="firing", to_state="acknowledged", metric_value=None, actor_id="u1") + assert _history_row(ack, RULE, names)["notes"] == ["acknowledged by bob"] + off = _event(from_state="firing", to_state="resolved", note="rule disabled", actor_id="u1") + assert _history_row(off, RULE, names)["notes"] == ["rule disabled by bob"] + gone = _event(from_state="firing", to_state="acknowledged", actor_id="u9") + assert _history_row(gone, RULE, names)["notes"] == ["acknowledged by a deleted user"] + assert _history_row(_event(note="rule enabled"), RULE, names)["notes"] == ["rule enabled"] diff --git a/tests/integration/test_alert_states.py b/tests/integration/test_alert_states.py index c2efcdb..6ac02a1 100644 --- a/tests/integration/test_alert_states.py +++ b/tests/integration/test_alert_states.py @@ -1,6 +1,7 @@ """Integration: alert state against real Postgres. An acknowledgement holds -while the breach lasts, an alert clears after its OK run, and changing what a -rule watches (or disabling it) resets its state with a note on the event.""" +while the breach lasts and is on the record with who made it (#5463), an alert +clears after its OK run, and changing what a rule watches (or disabling it) +resets its state with a note and the person on the event.""" from __future__ import annotations import asyncio @@ -44,7 +45,7 @@ def test_acknowledge_holds_clear_after_and_reset(app): metric_name="cpu_pct", operator=">", threshold=90, consecutive_failures_required=1, consecutive_ok_required=2) rule = await alert_svc.add_rule(s, fields, created_by=user.id) - rule_id = rule.id + rule_id, user_id = rule.id, user.id async def sample(v): async with app.db_sessionmaker() as s: @@ -62,11 +63,18 @@ def test_acknowledge_holds_clear_after_and_reset(app): .order_by(AlertEvent.transitioned_at))).scalars().all() return [(e.from_state, e.to_state, e.note) for e in rows] + async def last_actor(): + async with app.db_sessionmaker() as s: + return (await s.execute(select(AlertEvent.actor_id).where( + AlertEvent.rule_id == rule_id).order_by( + AlertEvent.transitioned_at.desc()).limit(1))).scalar_one() + await sample(95) assert (await state()).state == AlertStateEnum.firing async with app.db_sessionmaker() as s: async with s.begin(): - await alert_svc.acknowledge(s, rule_id, None) + await alert_svc.acknowledge(s, rule_id, user_id) + assert await last_actor() == user_id for _ in range(3): await sample(97) # still breached: the acknowledgement holds assert (await state()).state == AlertStateEnum.acknowledged @@ -75,7 +83,9 @@ def test_acknowledge_holds_clear_after_and_reset(app): await sample(10) st = await state() assert st.state == AlertStateEnum.inactive and st.acknowledged_at is None - assert await events() == [("inactive", "firing", None), ("acknowledged", "resolved", None)] + assert await events() == [("inactive", "firing", None), ("firing", "acknowledged", None), + ("acknowledged", "resolved", None)] + assert await last_actor() is None # the readings resolved it, not a person # Changing the threshold of a firing rule resets it, saying why. await sample(95) @@ -100,9 +110,10 @@ def test_acknowledge_holds_clear_after_and_reset(app): async with app.db_sessionmaker() as s: async with s.begin(): r = await s.get(AlertRule, rule_id) - await alert_svc.apply_rule_change(s, r, {}, enabled=False) + await alert_svc.apply_rule_change(s, r, {}, enabled=False, actor_id=user_id) assert (await state()).state == AlertStateEnum.inactive assert (await events())[-1] == ("firing", "resolved", "rule disabled") + assert await last_actor() == user_id # Re-enabling starts the watch over: silence is measured from now. async with app.db_sessionmaker() as s: