From 1f5d03925f35daf504743ad2dc1b5b12c92eee74 Mon Sep 17 00:00:00 2001 From: Ivan Vydrin Date: Wed, 29 Jul 2026 22:11:42 +0300 Subject: [PATCH] Ask the narrower question once, instead of in every handler StateChanged has been listed as a known issue since the event existed: it is raised on every check rather than only when something meaningful changes, because a stored result always moves the last execution time and that counts towards state equality. It is not a defect. The dashboard redraws from exactly those events -- every check updates a row's last-run time and pushes a blip onto its pulse strip -- so raising it less often would break the thing the event mainly exists for. What was missing was a way to ask the narrower question, and without one, every handler that only cared about a component going down reached into OldState and NewState.LastResult?.Health and worked it out for itself. The alerting and uptime packages were doing that identically, three lines each, which is the point at which a concept belongs to the type rather than to its readers. PulseCheckerStateChangedEventArgs now carries PreviousHealth, CurrentHealth and HealthChanged, and both packages read them. Two edges are deliberate. A first result is a change: going from nothing known to Healthy is the first thing anyone learns about the component, and a rule that ignored it would stay silent through a cold start into an outage. Losing a result is not: a state with no result says nothing about health, so treating it as a transition would raise an alert whose current health is nothing at all. The changelog and the README roadmap both described this as an outstanding problem. They now describe what the event does and how to ask for less. --- CHANGELOG.md | 13 +-- README.md | 18 +++-- .../PulseCheckerStateChangedEventArgs.cs | 31 +++++++ src/Healthie.Alerting/AlertDispatcher.cs | 6 +- src/Healthie.Uptime/UptimeRecorder.cs | 7 +- tests/Healthie.Tests.Unit/AlertingTests.cs | 28 +++++++ .../Healthie.Tests.Unit/HealthChangedTests.cs | 80 +++++++++++++++++++ tests/Healthie.Tests.Unit/TestDoubles.cs | 11 +++ 8 files changed, 172 insertions(+), 22 deletions(-) create mode 100644 tests/Healthie.Tests.Unit/HealthChangedTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 13b03dc..ae3cff4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,13 @@ its code behaves exactly as it did. reports `SupportsOptimisticConcurrency == false` rather than pretending. CosmosDB uses ETags, the relational providers a version column added to existing tables on startup, and the in-memory provider a compare-and-swap. +- **`HealthChanged` on `PulseCheckerStateChangedEventArgs`**, with `PreviousHealth` and + `CurrentHealth` beside it. `StateChanged` fires on every check -- a stored result always moves the + execution time -- which was listed as a known issue and is not one: the dashboard redraws from + exactly those events. What was missing was a way to ask the narrower question, so every handler + that only cared about a component going down reached into `OldState`/`NewState.LastResult?.Health` + and worked it out again. The alerting and uptime packages were doing that identically; both now + read `PreviousHealth` and `CurrentHealth` from the event instead of digging for them. - **Schedules.** `PulseSchedule` says either "every this long" or "on this cron expression", and sits alongside `PulseInterval` rather than replacing it. The enum stopped at five minutes, which is short of what a certificate-expiry or disk-space check wants. Cron is standard Unix syntax and @@ -89,12 +96,6 @@ its code behaves exactly as it did. `Newtonsoft.Json` reached through Hangfire (GHSA-5crp-9r3c-p9vr), and `System.Text.Json` in the console sample (CVE-2024-30105 and CVE-2024-43485). The solution reports no vulnerable packages. -### Known issues - -- `StateChanged` is raised on **every** check rather than only when state changes, because - `PulseCheckerState` equality includes the last execution time. Anything reacting to it should - compare the health itself, which the alerting and uptime packages do. - ## [3.1.4] - 2026-07-19 ### Changed diff --git a/README.md b/README.md index 368d396..c483be8 100644 --- a/README.md +++ b/README.md @@ -252,9 +252,14 @@ Subscribe to state transitions on any pulse checker: ```csharp checker.StateChanged += (sender, args) => { - Console.WriteLine( - $"Health changed from {args.OldState.LastResult?.Health} " + - $"to {args.NewState.LastResult?.Health}"); + // Fires on every check, because a stored result always moves the execution time. + // HealthChanged is the narrower question: did the component itself move? + if (!args.HealthChanged) + { + return; + } + + Console.WriteLine($"Health changed from {args.PreviousHealth} to {args.CurrentHealth}"); }; ``` @@ -818,12 +823,9 @@ Upgrading from v1.x? See the [v1 to v2 migration guide](https://github.com/ivanv Shipped since 3.1.4: alerting on transitions, OpenTelemetry metrics and traces, arbitrary intervals and cron, PostgreSQL / SQL Server / SQLite state providers, Hangfire / Coravel / Temporal -scheduling, ready-made checkers, uptime reporting, leader election, and optimistic concurrency on -`IStateProvider`. What is left: +scheduling, ready-made checkers, uptime reporting, leader election, optimistic concurrency +on `IStateProvider`, and `HealthChanged` on the state-changed event. What is left: -- **`StateChanged` fires on every check** rather than only when state changes, because state - equality includes the last execution time. Anything reacting to it should compare the health - itself, which the alerting and uptime packages do. - **A Redis state provider** -- the fastest option for state written on every tick, and a natural lease store for leader election. - **Alert sinks beyond the webhook** -- Slack, Teams and PagerDuty as packages rather than as a diff --git a/src/Healthie.Abstractions/Models/PulseCheckerStateChangedEventArgs.cs b/src/Healthie.Abstractions/Models/PulseCheckerStateChangedEventArgs.cs index b864a2e..996abeb 100644 --- a/src/Healthie.Abstractions/Models/PulseCheckerStateChangedEventArgs.cs +++ b/src/Healthie.Abstractions/Models/PulseCheckerStateChangedEventArgs.cs @@ -1,3 +1,5 @@ +using Healthie.Abstractions.Enums; + namespace Healthie.Abstractions.Models; /// @@ -18,4 +20,33 @@ public class PulseCheckerStateChangedEventArgs( /// Gets the state after the change. /// public PulseCheckerState NewState { get; } = newState; + + /// + /// Gets the health before this change, or null if no check had produced one yet. + /// + public PulseCheckerHealth? PreviousHealth => OldState.LastResult?.Health; + + /// + /// Gets the health after this change, or null if there is still no result -- which is + /// what a setting changed before the first check ever ran looks like. + /// + public PulseCheckerHealth? CurrentHealth => NewState.LastResult?.Health; + + /// + /// Gets whether this change was a change of health, rather than only of settings or of when the + /// check last ran. + /// + /// + /// + /// fires whenever the stored state differs, and a check + /// storing its result always changes it -- the execution time alone is enough. So a handler that + /// cares about a component going down, rather than about it having been looked at, wants this + /// and not the event itself. + /// + /// + /// A first result counts: going from nothing known to Healthy is a change. Losing a + /// result does not, because a state with no result says nothing about health. + /// + /// + public bool HealthChanged => CurrentHealth is not null && PreviousHealth != CurrentHealth; } diff --git a/src/Healthie.Alerting/AlertDispatcher.cs b/src/Healthie.Alerting/AlertDispatcher.cs index dc0442b..9aee9d6 100644 --- a/src/Healthie.Alerting/AlertDispatcher.cs +++ b/src/Healthie.Alerting/AlertDispatcher.cs @@ -141,10 +141,10 @@ private void Unsubscribe() /// private void OnStateChanged(IPulseChecker checker, PulseCheckerStateChangedEventArgs args) { - var previous = args.OldState.LastResult?.Health; - var current = args.NewState.LastResult?.Health; + // StateChanged fires on every check; only a change of health is worth waking somebody for. + var previous = args.PreviousHealth; - if (current is not { } health || previous == health || !ShouldAlert(previous, health)) + if (args.CurrentHealth is not { } health || previous == health || !ShouldAlert(previous, health)) { return; } diff --git a/src/Healthie.Uptime/UptimeRecorder.cs b/src/Healthie.Uptime/UptimeRecorder.cs index d043f65..d33b0a7 100644 --- a/src/Healthie.Uptime/UptimeRecorder.cs +++ b/src/Healthie.Uptime/UptimeRecorder.cs @@ -122,12 +122,9 @@ private void Unsubscribe() private void OnStateChanged(IPulseChecker checker, PulseCheckerStateChangedEventArgs args) { // Only a change of health starts a new segment. StateChanged fires on every check, because - // state equality includes the last execution time, so recording every one of them would + // a stored result always moves the execution time, so recording every one of them would // turn a day into 86,400 segments that all say the same thing. - var previous = args.OldState.LastResult?.Health; - var current = args.NewState.LastResult?.Health; - - if (current is not { } health || previous == health) + if (args.CurrentHealth is not { } health || args.PreviousHealth == health) { return; } diff --git a/tests/Healthie.Tests.Unit/AlertingTests.cs b/tests/Healthie.Tests.Unit/AlertingTests.cs index 1ae9eb8..6a223d9 100644 --- a/tests/Healthie.Tests.Unit/AlertingTests.cs +++ b/tests/Healthie.Tests.Unit/AlertingTests.cs @@ -98,6 +98,34 @@ public async Task AFailure_ReachesTheSink() } } + /// + /// A change carrying no result says nothing about health, so it must not alert. This is the one + /// branch of the health-change test that the other tests here cannot reach: every other way of + /// raising the event puts a result on both sides. + /// + [Fact] + public async Task ASettingChangedBeforeTheFirstCheck_ReachesNoSink() + { + var sink = new RecordingSink(); + var (dispatcher, checker) = await StartAsync(sink); + + try + { + Assert.Equal(1, checker.SubscriberCount); + checker.RaiseSettingChanged("Tier 1"); + + // A failure raised straight after must be the first thing the sink sees. + checker.RaiseStateChanged(PulseCheckerHealth.Unhealthy); + + Assert.True(await WaitUntilAsync(() => sink.Received.Count == 1, TimeSpan.FromSeconds(5))); + Assert.Equal(PulseCheckerHealth.Unhealthy, sink.Received[0].CurrentHealth); + } + finally + { + await ((IHostedService)dispatcher).StopAsync(CancellationToken.None); + } + } + /// /// Suspicious is the state a checker passes through on its way to unhealthy, so alerting on it /// by default would page somebody for every blip the threshold exists to absorb. diff --git a/tests/Healthie.Tests.Unit/HealthChangedTests.cs b/tests/Healthie.Tests.Unit/HealthChangedTests.cs new file mode 100644 index 0000000..71c3b68 --- /dev/null +++ b/tests/Healthie.Tests.Unit/HealthChangedTests.cs @@ -0,0 +1,80 @@ +using Healthie.Abstractions.Enums; +using Healthie.Abstractions.Models; + +namespace Healthie.Tests.Unit; + +/// +/// StateChanged fires on every check, because a stored result always moves the execution +/// time. That is not a defect -- the dashboard redraws from it -- but it means every handler that +/// only cares about a component going down has to work out for itself whether the health moved. +/// Two packages were doing exactly that, identically, so the question now has one answer. +/// +public class HealthChangedTests +{ + private static PulseCheckerState With(PulseCheckerHealth? health) => + health is { } value + ? new PulseCheckerState { LastResult = new PulseCheckerResult(value, string.Empty) } + : new PulseCheckerState(); + + private static PulseCheckerStateChangedEventArgs Change( + PulseCheckerHealth? from, + PulseCheckerHealth? to) => new(With(from), With(to)); + + [Fact] + public void ACheckRepeatingItsResult_IsNotAHealthChange() + { + var args = Change(PulseCheckerHealth.Healthy, PulseCheckerHealth.Healthy); + + Assert.False(args.HealthChanged); + Assert.Equal(PulseCheckerHealth.Healthy, args.PreviousHealth); + Assert.Equal(PulseCheckerHealth.Healthy, args.CurrentHealth); + } + + [Fact] + public void GoingUnhealthy_IsAHealthChange() + { + var args = Change(PulseCheckerHealth.Healthy, PulseCheckerHealth.Unhealthy); + + Assert.True(args.HealthChanged); + Assert.Equal(PulseCheckerHealth.Healthy, args.PreviousHealth); + Assert.Equal(PulseCheckerHealth.Unhealthy, args.CurrentHealth); + } + + /// + /// Nothing known to something known is a change: it is the first thing anyone learns about the + /// component, and an alerting rule that ignored it would stay silent through a cold start into + /// an outage. + /// + [Fact] + public void TheFirstResult_IsAHealthChange() + { + var args = Change(null, PulseCheckerHealth.Unhealthy); + + Assert.True(args.HealthChanged); + Assert.Null(args.PreviousHealth); + } + + /// + /// A state with no result is not a report of good health, so losing one is not a transition to + /// anything. Treating it as one would fire an alert whose "current health" is nothing at all. + /// + [Fact] + public void LosingAResult_IsNotAHealthChange() + { + var args = Change(PulseCheckerHealth.Unhealthy, null); + + Assert.False(args.HealthChanged); + } + + [Fact] + public void ASettingChangeThatKeepsTheResult_IsNotAHealthChange() + { + var result = new PulseCheckerResult(PulseCheckerHealth.Suspicious, "flapping"); + + var args = new PulseCheckerStateChangedEventArgs( + new PulseCheckerState { LastResult = result, Group = "before" }, + new PulseCheckerState { LastResult = result, Group = "after" }); + + Assert.False(args.HealthChanged); + } +} diff --git a/tests/Healthie.Tests.Unit/TestDoubles.cs b/tests/Healthie.Tests.Unit/TestDoubles.cs index 2744644..ca58e9e 100644 --- a/tests/Healthie.Tests.Unit/TestDoubles.cs +++ b/tests/Healthie.Tests.Unit/TestDoubles.cs @@ -65,6 +65,17 @@ public void RaiseStateChanged(PulseCheckerHealth health) StateChanged?.Invoke(this, new PulseCheckerStateChangedEventArgs(oldState, _state)); } + /// + /// Raises for a change that carries no result at all, which is what a + /// setting changed before the checker has ever run looks like. + /// + public void RaiseSettingChanged(string group) + { + var oldState = _state; + _state = new PulseCheckerState(PulseInterval.EveryMinute, 0) { Group = group }; + StateChanged?.Invoke(this, new PulseCheckerStateChangedEventArgs(oldState, _state)); + } + public Task TriggerAsync(CancellationToken cancellationToken = default) { TriggerCount++;