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++;