fix(alerts): a series that comes back gets one history event, not two at one instant (#5442)
When a silent series' sample also moved the rule (fired, resolved), the
resume and the transition were two events: the resume from the
pre-evaluation state at ingest time, the transition at the sample's
time. Ties sort by a random id, and the resume's "→ inactive" landed
after "→ firing", so the history read "silent, firing, back to normal"
at once and the episode closed while the alert was firing.
_track_samples now only clears the mark and returns its note; the first
sample's evaluation records one event from no_data to wherever that
sample took the rule ("firing" or "resolved" notification), or back to
the state it was in ("data_resumed", still only when notify_no_data).
Also fixes the #5441 integration test: its rule needs a recorded
resource to target (CI 8878).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+32
-25
@@ -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)],
|
samples = sorted(by_key[(rule.source_module, rule.resource_name, rule.metric_name)],
|
||||||
key=lambda r: r.recorded_at)
|
key=lambda r: r.recorded_at)
|
||||||
muted = _muted_by(windows, rule)
|
muted = _muted_by(windows, rule)
|
||||||
resumed = _track_samples(session, rule, alert_state, samples, now, muted)
|
resumed = _track_samples(alert_state, samples, now)
|
||||||
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)
|
|
||||||
for sample in samples:
|
for sample in samples:
|
||||||
notifications = await _evaluate_rule(session, rule, alert_state, sample.value,
|
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:
|
if muted:
|
||||||
continue
|
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,
|
_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.
|
# Admin-configured Ansible action runs only on transition-to-firing.
|
||||||
if notif_type == "firing" and rule.ansible_action:
|
if notif_type == "firing" and rule.ansible_action:
|
||||||
_after_commit(session, _run_ansible_action, rule, sample.value, event_id)
|
_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
|
return last + limit if now - last > limit else None
|
||||||
|
|
||||||
|
|
||||||
def _track_samples(session: AsyncSession, rule: AlertRule, state: AlertState,
|
def _track_samples(state: AlertState, samples: list[PluginMetric], now: datetime) -> str | None:
|
||||||
samples: list[PluginMetric], now: datetime, muted: str | None = None
|
|
||||||
) -> tuple[str, float, str] | None:
|
|
||||||
"""Note a rule's new samples (oldest first) for silence tracking. When the
|
"""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,
|
series had been marked no_data, clear the mark and return the note saying
|
||||||
returned as (event_id, value, note) for the caller to notify. `muted` is
|
so ("data resumed after 15m silent"); the first sample's evaluation
|
||||||
added to the event's note when a maintenance window holds it back."""
|
records it, in one event with whatever that sample does to the rule
|
||||||
|
(#5442)."""
|
||||||
for sample in samples:
|
for sample in samples:
|
||||||
if state.last_sample_at is not None:
|
if state.last_sample_at is not None:
|
||||||
if sample.recorded_at <= state.last_sample_at:
|
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:
|
if state.no_data_since is None or not samples:
|
||||||
return None
|
return None
|
||||||
note = f"data resumed after {human_duration((now - state.no_data_since).total_seconds())} silent"
|
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
|
state.no_data_since = None
|
||||||
return event_id, value, note
|
return note
|
||||||
|
|
||||||
|
|
||||||
def _is_breached(value: float, operator: AlertOperator, threshold: float) -> bool:
|
def _is_breached(value: float, operator: AlertOperator, threshold: float) -> bool:
|
||||||
@@ -401,26 +396,38 @@ async def _evaluate_rule(
|
|||||||
value: float,
|
value: float,
|
||||||
now: datetime,
|
now: datetime,
|
||||||
note: str | None = None,
|
note: str | None = None,
|
||||||
) -> list[tuple[str, str]]:
|
resumed: str | None = None,
|
||||||
"""Apply one sample to a rule's state. Returns [(notif_type, event_id)];
|
) -> list[tuple[str, str, str | None]]:
|
||||||
an event it records carries `note`."""
|
"""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
|
from_state = state.state
|
||||||
step = next_step(from_state, state.consecutive_failure_count, state.consecutive_ok_count,
|
step = next_step(from_state, state.consecutive_failure_count, state.consecutive_ok_count,
|
||||||
_is_breached(value, rule.operator, rule.threshold),
|
_is_breached(value, rule.operator, rule.threshold),
|
||||||
rule.consecutive_failures_required, rule.consecutive_ok_required)
|
rule.consecutive_failures_required, rule.consecutive_ok_required)
|
||||||
state.consecutive_failure_count = step.fail_count
|
state.consecutive_failure_count = step.fail_count
|
||||||
state.consecutive_ok_count = step.ok_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:
|
if step.state != from_state:
|
||||||
state.state = step.state
|
state.state = step.state
|
||||||
state.last_transition_at = now
|
state.last_transition_at = now
|
||||||
if step.state == AlertStateEnum.inactive:
|
if step.state == AlertStateEnum.inactive:
|
||||||
state.acknowledged_by = None
|
state.acknowledged_by = None
|
||||||
state.acknowledged_at = None
|
state.acknowledged_at = None
|
||||||
|
said = "; ".join(n for n in (resumed, note) if n) or None
|
||||||
if step.event:
|
if step.event:
|
||||||
event_id = _add_event(session, rule.id, from_state.value, step.event, value, now,
|
event_id = _add_event(session, rule.id, "no_data" if resumed else from_state.value,
|
||||||
note=note)
|
step.event, value, now, note=said)
|
||||||
notifications.append((step.event, event_id))
|
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()
|
await session.flush()
|
||||||
return notifications
|
return notifications
|
||||||
|
|
||||||
|
|||||||
@@ -2,15 +2,17 @@
|
|||||||
samples, an outage does not become its cadence, and a series counts as silent
|
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
|
after three usual gaps (at least five minutes; an hour while its cadence is
|
||||||
unknown)."""
|
unknown)."""
|
||||||
|
import asyncio
|
||||||
from datetime import datetime, timedelta, timezone
|
from datetime import datetime, timedelta, timezone
|
||||||
from types import SimpleNamespace as NS
|
from types import SimpleNamespace as NS
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
from steward.core.alerts import (
|
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)
|
NOW = datetime(2026, 10, 6, 12, 0, tzinfo=timezone.utc)
|
||||||
|
|
||||||
@@ -80,10 +82,13 @@ class _Session:
|
|||||||
def add(self, row):
|
def add(self, row):
|
||||||
self.added.append(row)
|
self.added.append(row)
|
||||||
|
|
||||||
|
async def flush(self):
|
||||||
|
pass
|
||||||
|
|
||||||
|
|
||||||
def _state(**kw):
|
def _state(**kw):
|
||||||
base = dict(state=AlertStateEnum.firing, last_sample_at=None, sample_interval_s=None,
|
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})
|
return NS(**{**base, **kw})
|
||||||
|
|
||||||
|
|
||||||
@@ -92,23 +97,49 @@ def _samples(*ago_values):
|
|||||||
|
|
||||||
|
|
||||||
def test_tracking_learns_the_gap_and_skips_late_samples():
|
def test_tracking_learns_the_gap_and_skips_late_samples():
|
||||||
st, session = _state(), _Session()
|
st = _state()
|
||||||
assert _track_samples(session, NS(id="r"), st, _samples((180, 1), (120, 1), (60, 1)), NOW) is None
|
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)
|
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 (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():
|
def test_a_sample_after_silence_clears_it_and_says_so():
|
||||||
st, session = _state(last_sample_at=NOW - timedelta(minutes=20), sample_interval_s=60,
|
st = _state(last_sample_at=NOW - timedelta(minutes=20), sample_interval_s=60,
|
||||||
no_data_since=NOW - timedelta(minutes=15)), _Session()
|
no_data_since=NOW - timedelta(minutes=15))
|
||||||
event_id, value, note = _track_samples(session, NS(id="r"), st, _samples((0, 42.0)), NOW)
|
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.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 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
|
[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():
|
def test_muted_by_names_the_window_that_covers_the_rule():
|
||||||
|
|||||||
@@ -104,13 +104,14 @@ def test_silence_is_marked_once_and_cleared_by_data(app):
|
|||||||
await asyncio.sleep(0)
|
await asyncio.sleep(0)
|
||||||
st = await state("loud")
|
st = await state("loud")
|
||||||
assert st.no_data_since is None and st.state == AlertStateEnum.firing
|
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")
|
assert first[:2] == ("inactive", "no_data")
|
||||||
# The resume and the firing it leads to share the sample's timestamp.
|
assert back[:2] == ("no_data", "firing") and back[2].startswith("data resumed after ")
|
||||||
assert sorted(e[:2] for e in same_instant) == [("inactive", "firing"),
|
assert [t for n, t, _ in sent if n == f"loud {tag}"] == ["no_data", "firing"]
|
||||||
("no_data", "inactive")]
|
# The hushed rule says nothing of silence, but its firing is still news.
|
||||||
assert any((e[2] or "").startswith("data resumed after ") for e in same_instant)
|
assert [t for n, t, _ in sent if n == f"hushed {tag}"] == ["firing"]
|
||||||
assert [t for _, t, _ in sent if t != "firing"] == ["no_data", "data_resumed"]
|
|
||||||
|
|
||||||
# A firing alert that goes silent stays firing, marked.
|
# A firing alert that goes silent stays firing, marked.
|
||||||
async with app.db_sessionmaker() as s:
|
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")
|
st = await state("loud")
|
||||||
assert st.state == AlertStateEnum.firing and st.no_data_since is not None
|
assert st.state == AlertStateEnum.firing and st.no_data_since is not None
|
||||||
assert (await events("loud"))[-1][:2] == ("firing", "no_data")
|
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):
|
with patch.object(alerts, "_dispatch_notification", fake_dispatch):
|
||||||
asyncio.run(_go())
|
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 import alerts
|
||||||
from steward.core.watch import watch
|
from steward.core.watch import watch
|
||||||
from steward.models.alerts import AlertEvent, AlertState
|
from steward.models.alerts import AlertEvent, AlertState
|
||||||
|
from steward.models.metrics import PluginMetric
|
||||||
from steward.models.users import User, UserRole
|
from steward.models.users import User, UserRole
|
||||||
|
|
||||||
sent = []
|
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",
|
user = User(username=f"bk-{tag}", email=f"bk-{tag}@test", password_hash="x",
|
||||||
role=UserRole.admin)
|
role=UserRole.admin)
|
||||||
s.add(user)
|
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()
|
await s.flush()
|
||||||
fields = await alert_svc.rule_fields(
|
fields = await alert_svc.rule_fields(
|
||||||
s, name=f"back {tag}", source_module="host_agent", resource_name=res,
|
s, name=f"back {tag}", source_module="host_agent", resource_name=res,
|
||||||
|
|||||||
Reference in New Issue
Block a user