diff --git a/steward/core/alerts.py b/steward/core/alerts.py index f6589cc..e4a329b 100644 --- a/steward/core/alerts.py +++ b/steward/core/alerts.py @@ -249,19 +249,18 @@ async def record_metrics(session: AsyncSession, rows: list[PluginMetric], phases samples = sorted(by_key[(rule.source_module, rule.resource_name, rule.metric_name)], key=lambda r: r.recorded_at) muted = _muted_by(windows, rule) - resumed = _track_samples(session, rule, alert_state, samples, now, muted) - if resumed and rule.notify_no_data and not muted: - event_id, value, note = resumed - _after_commit(session, _dispatch_notification, "data_resumed", rule, value, - event_id, note) + resumed = _track_samples(alert_state, samples, now) for sample in samples: notifications = await _evaluate_rule(session, rule, alert_state, sample.value, - sample.recorded_at, note=muted) + sample.recorded_at, note=muted, resumed=resumed) + resumed = None # said once, with the first sample back if muted: continue - for notif_type, event_id in notifications: + for notif_type, event_id, note in notifications: + if notif_type == "data_resumed" and not rule.notify_no_data: + continue _after_commit(session, _dispatch_notification, notif_type, rule, sample.value, - event_id) + event_id, note) # Admin-configured Ansible action runs only on transition-to-firing. if notif_type == "firing" and rule.ansible_action: _after_commit(session, _run_ansible_action, rule, sample.value, event_id) @@ -315,13 +314,12 @@ def silent_since(last_sample_at: datetime | None, interval: float | None, return last + limit if now - last > limit else None -def _track_samples(session: AsyncSession, rule: AlertRule, state: AlertState, - samples: list[PluginMetric], now: datetime, muted: str | None = None - ) -> tuple[str, float, str] | None: +def _track_samples(state: AlertState, samples: list[PluginMetric], now: datetime) -> str | None: """Note a rule's new samples (oldest first) for silence tracking. When the - series had been marked no_data, clear it and record a resume event, - returned as (event_id, value, note) for the caller to notify. `muted` is - added to the event's note when a maintenance window holds it back.""" + series had been marked no_data, clear the mark and return the note saying + so ("data resumed after 15m silent"); the first sample's evaluation + records it, in one event with whatever that sample does to the rule + (#5442).""" for sample in samples: if state.last_sample_at is not None: if sample.recorded_at <= state.last_sample_at: @@ -332,11 +330,8 @@ def _track_samples(session: AsyncSession, rule: AlertRule, state: AlertState, if state.no_data_since is None or not samples: return None note = f"data resumed after {human_duration((now - state.no_data_since).total_seconds())} silent" - value = samples[-1].value - event_id = _add_event(session, rule.id, "no_data", state.state.value, value, now, - note=f"{note}; {muted}" if muted else note) state.no_data_since = None - return event_id, value, note + return note def _is_breached(value: float, operator: AlertOperator, threshold: float) -> bool: @@ -401,26 +396,38 @@ async def _evaluate_rule( value: float, now: datetime, note: str | None = None, -) -> list[tuple[str, str]]: - """Apply one sample to a rule's state. Returns [(notif_type, event_id)]; - an event it records carries `note`.""" + resumed: str | None = None, +) -> list[tuple[str, str, str | None]]: + """Apply one sample to a rule's state. Returns [(notif_type, event_id, + note)]; an event it records carries `note`. + + `resumed` says this sample ends a silence. The rule then gets one event + from no_data: to the state the sample moved it to (a "firing" + notification), or back to the state it was in ("data_resumed"). Two + events at one instant used to sort either way round, so the history + could read "back to inactive" right after "firing" (#5442). + """ from_state = state.state step = next_step(from_state, state.consecutive_failure_count, state.consecutive_ok_count, _is_breached(value, rule.operator, rule.threshold), rule.consecutive_failures_required, rule.consecutive_ok_required) state.consecutive_failure_count = step.fail_count state.consecutive_ok_count = step.ok_count - notifications: list[tuple[str, str]] = [] + notifications: list[tuple[str, str, str | None]] = [] if step.state != from_state: state.state = step.state state.last_transition_at = now if step.state == AlertStateEnum.inactive: state.acknowledged_by = None state.acknowledged_at = None + said = "; ".join(n for n in (resumed, note) if n) or None if step.event: - event_id = _add_event(session, rule.id, from_state.value, step.event, value, now, - note=note) - notifications.append((step.event, event_id)) + event_id = _add_event(session, rule.id, "no_data" if resumed else from_state.value, + step.event, value, now, note=said) + notifications.append((step.event, event_id, None)) + elif resumed: + event_id = _add_event(session, rule.id, "no_data", state.state.value, value, now, note=said) + notifications.append(("data_resumed", event_id, resumed)) await session.flush() return notifications diff --git a/tests/core/test_alert_no_data.py b/tests/core/test_alert_no_data.py index 6360b2d..d3bc325 100644 --- a/tests/core/test_alert_no_data.py +++ b/tests/core/test_alert_no_data.py @@ -2,15 +2,17 @@ samples, an outage does not become its cadence, and a series counts as silent after three usual gaps (at least five minutes; an hour while its cadence is unknown).""" +import asyncio from datetime import datetime, timedelta, timezone from types import SimpleNamespace as NS import pytest from steward.core.alerts import ( - NO_DATA_FLOOR_S, NO_DATA_UNKNOWN_S, _track_samples, next_interval, no_data_after, silent_since, + NO_DATA_FLOOR_S, NO_DATA_UNKNOWN_S, _evaluate_rule, _track_samples, next_interval, + no_data_after, silent_since, ) -from steward.models.alerts import AlertStateEnum +from steward.models.alerts import AlertOperator, AlertStateEnum NOW = datetime(2026, 10, 6, 12, 0, tzinfo=timezone.utc) @@ -80,10 +82,13 @@ class _Session: def add(self, row): self.added.append(row) + async def flush(self): + pass + def _state(**kw): base = dict(state=AlertStateEnum.firing, last_sample_at=None, sample_interval_s=None, - no_data_since=None) + no_data_since=None, consecutive_failure_count=0, consecutive_ok_count=0) return NS(**{**base, **kw}) @@ -92,23 +97,49 @@ def _samples(*ago_values): def test_tracking_learns_the_gap_and_skips_late_samples(): - st, session = _state(), _Session() - assert _track_samples(session, NS(id="r"), st, _samples((180, 1), (120, 1), (60, 1)), NOW) is None + st = _state() + assert _track_samples(st, _samples((180, 1), (120, 1), (60, 1)), NOW) is None assert (st.last_sample_at, st.sample_interval_s) == (NOW - timedelta(seconds=60), 60) - _track_samples(session, NS(id="r"), st, _samples((90, 1)), NOW) # older than the last + _track_samples(st, _samples((90, 1)), NOW) # older than the last assert (st.last_sample_at, st.sample_interval_s) == (NOW - timedelta(seconds=60), 60) - assert session.added == [] -def test_a_sample_after_silence_clears_it_and_records_the_resume(): - st, session = _state(last_sample_at=NOW - timedelta(minutes=20), sample_interval_s=60, - no_data_since=NOW - timedelta(minutes=15)), _Session() - event_id, value, note = _track_samples(session, NS(id="r"), st, _samples((0, 42.0)), NOW) +def test_a_sample_after_silence_clears_it_and_says_so(): + st = _state(last_sample_at=NOW - timedelta(minutes=20), sample_interval_s=60, + no_data_since=NOW - timedelta(minutes=15)) + note = _track_samples(st, _samples((0, 42.0)), NOW) assert st.no_data_since is None and st.state == AlertStateEnum.firing # still firing assert st.sample_interval_s == 90 # the outage counted as three gaps, at most - assert value == 42.0 and note == "data resumed after 15m silent" + assert note == "data resumed after 15m silent" + + +_RULE = NS(id="r", operator=AlertOperator.gt, threshold=90.0, + consecutive_failures_required=1, consecutive_ok_required=1) + + +@pytest.mark.parametrize("before,value,want_event,want_notice", [ + # Back, and breaching: one event, silent to firing (#5442). + (AlertStateEnum.inactive, 95.0, ("no_data", "firing"), "firing"), + # Back, and fine: one event, silent back to how it was. + (AlertStateEnum.inactive, 10.0, ("no_data", "inactive"), "data_resumed"), + # A firing alert that went silent comes back fine: it resolves, from silent. + (AlertStateEnum.firing, 10.0, ("no_data", "resolved"), "resolved"), + # ... or still breaching: it stays firing, and only the resume is news. + (AlertStateEnum.firing, 95.0, ("no_data", "firing"), "data_resumed"), +]) +def test_a_resumed_series_gets_one_event_for_what_its_first_sample_did( + before, value, want_event, want_notice): + session, st = _Session(), _state(state=before) + said = "data resumed after 15m silent" + out = asyncio.run(_evaluate_rule(session, _RULE, st, value, NOW, resumed=said)) [event] = session.added - assert (event.id, event.from_state, event.to_state) == (event_id, "no_data", "firing") + assert (event.from_state, event.to_state) == want_event + assert event.note == said and event.transitioned_at == NOW + assert [(n, e) for n, e, _ in out] == [(want_notice, event.id)] + # Without a resume, a sample that changes nothing records nothing. + session = _Session() + assert asyncio.run(_evaluate_rule(session, _RULE, _state(state=st.state), value, NOW)) == [] + assert session.added == [] def test_muted_by_names_the_window_that_covers_the_rule(): diff --git a/tests/integration/test_alert_no_data.py b/tests/integration/test_alert_no_data.py index 8db3901..922e308 100644 --- a/tests/integration/test_alert_no_data.py +++ b/tests/integration/test_alert_no_data.py @@ -104,13 +104,14 @@ def test_silence_is_marked_once_and_cleared_by_data(app): await asyncio.sleep(0) st = await state("loud") assert st.no_data_since is None and st.state == AlertStateEnum.firing - first, *same_instant = await events("loud") + # The resume and the firing it leads to are one event, silent to + # firing: two events at one instant used to sort either way (#5442). + first, back = await events("loud") assert first[:2] == ("inactive", "no_data") - # The resume and the firing it leads to share the sample's timestamp. - assert sorted(e[:2] for e in same_instant) == [("inactive", "firing"), - ("no_data", "inactive")] - assert any((e[2] or "").startswith("data resumed after ") for e in same_instant) - assert [t for _, t, _ in sent if t != "firing"] == ["no_data", "data_resumed"] + assert back[:2] == ("no_data", "firing") and back[2].startswith("data resumed after ") + assert [t for n, t, _ in sent if n == f"loud {tag}"] == ["no_data", "firing"] + # The hushed rule says nothing of silence, but its firing is still news. + assert [t for n, t, _ in sent if n == f"hushed {tag}"] == ["firing"] # A firing alert that goes silent stays firing, marked. async with app.db_sessionmaker() as s: @@ -123,7 +124,7 @@ def test_silence_is_marked_once_and_cleared_by_data(app): st = await state("loud") assert st.state == AlertStateEnum.firing and st.no_data_since is not None assert (await events("loud"))[-1][:2] == ("firing", "no_data") - assert len(await events("hushed")) == 3 # disabled: not watched + assert len(await events("hushed")) == 2 # disabled: not watched with patch.object(alerts, "_dispatch_notification", fake_dispatch): asyncio.run(_go()) @@ -138,6 +139,7 @@ def test_a_series_quiet_only_while_steward_was_down_is_not_silent(app): from steward.core import alerts from steward.core.watch import watch from steward.models.alerts import AlertEvent, AlertState + from steward.models.metrics import PluginMetric from steward.models.users import User, UserRole sent = [] @@ -154,6 +156,9 @@ def test_a_series_quiet_only_while_steward_was_down_is_not_silent(app): user = User(username=f"bk-{tag}", email=f"bk-{tag}@test", password_hash="x", role=UserRole.admin) s.add(user) + s.add(PluginMetric(source_module="host_agent", resource_name=res, + metric_name="cpu_pct", value=1.0, + recorded_at=now - timedelta(minutes=18))) await s.flush() fields = await alert_svc.rule_fields( s, name=f"back {tag}", source_module="host_agent", resource_name=res,