diff --git a/docs/requirements/Component-HealthMonitoring.md b/docs/requirements/Component-HealthMonitoring.md index 26628bc5..6b612d4c 100644 --- a/docs/requirements/Component-HealthMonitoring.md +++ b/docs/requirements/Component-HealthMonitoring.md @@ -42,8 +42,11 @@ Site clusters (metric collection and reporting). Central cluster (aggregation an - Sites send a **health report message** to central at a configurable interval (default: **30 seconds**). - Each report is a **flat snapshot** containing the current values of all monitored metrics, a **monotonic sequence number**, and the **report timestamp** from the site. Central replaces the previous state for that site only if the incoming sequence number is higher than the last received — this prevents stale reports (e.g., delayed in transit or from a pre-failover node) from overwriting newer state. -- **Offline detection**: If central does not receive a report within a configurable timeout window (default: **60 seconds** — 2x the report interval), the site is marked as **offline**. This gives one missed report as grace before marking offline. -- **Online recovery**: When central receives a health report from a site that was marked offline, the site is automatically marked **online**. No manual acknowledgment required — the metrics in the report provide immediate visibility into the site's condition. +- Central tracks two **distinct** signals per site — do not conflate them: + - **Liveness (online/offline)**: driven by the **last signal of any kind** (a full report *or* a transport heartbeat, the latter arriving every ~5s from any reachable site node). If central receives no signal within `OfflineTimeout` (default: **60 seconds**), the site is marked **offline**. Because heartbeats are frequent, this only fires on genuine total loss of contact — not during a single-node failover, when the surviving node keeps heartbeating. The synthetic *central* self-report site has no heartbeat source and uses the longer `CentralOfflineTimeout` (default: **3 minutes**) so a single missed self-report does not flap it offline. + - **Metrics staleness**: an **online** site (heartbeats still arriving) that has produced no full **report** within `MetricsStaleTimeout` (default: **2 minutes**) is flagged **metrics stale**. This surfaces the failure mode where a site's report pipeline (`HealthReportSender`) has died but the node is otherwise reachable — previously such a site showed "online with frozen metrics forever". `MetricsStaleTimeout` must be positive and no shorter than the report interval. +- **Online recovery**: when central receives a health report from a site that was marked offline, the site is automatically marked **online** and the metrics-stale flag cleared (a fresh report proves the pipeline is alive); a heartbeat alone also restores **online** but leaves the metrics-stale flag untouched. No manual acknowledgment required. +- **Status-transition timestamp**: each online↔offline flip stamps `LastStatusChangeAt`, so the UI can answer "since when has this site been offline/online" rather than only "is it offline now". Report/heartbeat traffic that leaves the status unchanged does not move it. ## Error Rate Metrics diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/CentralHealthAggregator.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/CentralHealthAggregator.cs index 20952c12..3f5a2da1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/CentralHealthAggregator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/CentralHealthAggregator.cs @@ -48,7 +48,9 @@ public class CentralHealthAggregator : BackgroundService, ICentralHealthAggregat LastReportReceivedAt = now, LastHeartbeatAt = now, LastSequenceNumber = report.SequenceNumber, - IsOnline = true + IsOnline = true, + IsMetricsStale = false + // LastStatusChangeAt stays null: first observation is not a flip. }; if (_siteStates.TryAdd(report.SiteId, registered)) @@ -76,7 +78,12 @@ public class CentralHealthAggregator : BackgroundService, ICentralHealthAggregat LastReportReceivedAt = now, LastHeartbeatAt = now, LastSequenceNumber = report.SequenceNumber, - IsOnline = true + IsOnline = true, + // A fresh report is proof the metrics pipeline is alive again. + IsMetricsStale = false, + // Stamp a flip only when this report brings the site back online; + // a report from an already-online site leaves the timestamp be. + LastStatusChangeAt = existing.IsOnline ? existing.LastStatusChangeAt : now }; if (_siteStates.TryUpdate(report.SiteId, updated, existing)) @@ -155,7 +162,14 @@ public class CentralHealthAggregator : BackgroundService, ICentralHealthAggregat var updated = existing with { LastHeartbeatAt = newHeartbeat, - IsOnline = true + IsOnline = true, + // Stamp the flip when this heartbeat promotes an offline site back + // online. IsMetricsStale is deliberately left untouched — a + // heartbeat says nothing about whether the metrics pipeline + // recovered; only a full report clears staleness. + LastStatusChangeAt = existing.IsOnline + ? existing.LastStatusChangeAt + : _timeProvider.GetUtcNow() }; if (_siteStates.TryUpdate(siteId, updated, existing)) @@ -246,18 +260,41 @@ public class CentralHealthAggregator : BackgroundService, ICentralHealthAggregat : _options.OfflineTimeout; var elapsed = now - state.LastHeartbeatAt; - if (elapsed <= timeout) - continue; - - // Atomically swap to an offline copy. If the CAS loses to a - // concurrent report/heartbeat the site was just heard from, so - // leaving it online is the correct outcome — no retry needed. - var offline = state with { IsOnline = false }; - if (_siteStates.TryUpdate(kvp.Key, offline, state)) + if (elapsed > timeout) { - _logger.LogWarning( - "Site {SiteId} marked offline — no signal for {Elapsed}s (timeout: {Timeout}s)", - state.SiteId, elapsed.TotalSeconds, timeout.TotalSeconds); + // Atomically swap to an offline copy, stamping the flip. If the + // CAS loses to a concurrent report/heartbeat the site was just + // heard from, so leaving it online is correct — no retry needed. + var offline = state with { IsOnline = false, LastStatusChangeAt = now }; + if (_siteStates.TryUpdate(kvp.Key, offline, state)) + { + _logger.LogWarning( + "Site {SiteId} marked offline — no signal for {Elapsed}s (timeout: {Timeout}s)", + state.SiteId, elapsed.TotalSeconds, timeout.TotalSeconds); + } + + // Whether the CAS won or lost, do not also evaluate staleness on + // this stale snapshot — offline supersedes metrics-stale. + continue; + } + + // Second signal: the site is still live (heartbeats within timeout) + // but its full-report pipeline may have died. Flag it metrics-stale + // when no report has arrived within MetricsStaleTimeout. Distinct from + // offline — a site whose HealthReportSender crashed keeps heartbeating + // and would otherwise show "online with frozen metrics forever". + if (state.LastReportReceivedAt is { } lastReport + && now - lastReport > _options.MetricsStaleTimeout + && !state.IsMetricsStale) + { + var stale = state with { IsMetricsStale = true }; + if (_siteStates.TryUpdate(kvp.Key, stale, state)) + { + _logger.LogWarning( + "Site {SiteId} metrics are stale — online (heartbeats) but no report for {Elapsed}s (timeout: {Timeout}s)", + state.SiteId, (now - lastReport).TotalSeconds, _options.MetricsStaleTimeout.TotalSeconds); + } + // CAS loss ⇒ a fresh report/heartbeat swapped in ⇒ correct to skip. } } } diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/HealthMonitoringOptions.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/HealthMonitoringOptions.cs index 51df1432..645da52d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/HealthMonitoringOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/HealthMonitoringOptions.cs @@ -19,4 +19,17 @@ public class HealthMonitoringOptions /// the design doc grants real sites. Default: 3x the report interval. /// public TimeSpan CentralOfflineTimeout { get; set; } = TimeSpan.FromMinutes(3); + + /// + /// Duration an online site may go without a full health report + /// (heartbeats notwithstanding) before it is flagged + /// . Distinct from + /// , which is heartbeat-based liveness: a site + /// whose HealthReportSender has died keeps heartbeating and stays "online", + /// but its metrics freeze — this window surfaces that. Must be positive and + /// no shorter than (a window shorter than one + /// report interval would flag every healthy site). Default: 2 minutes + /// (~4 missed 30s reports). + /// + public TimeSpan MetricsStaleTimeout { get; set; } = TimeSpan.FromMinutes(2); } diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/HealthMonitoringOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/HealthMonitoringOptionsValidator.cs index a119c56d..64df0047 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/HealthMonitoringOptionsValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/HealthMonitoringOptionsValidator.cs @@ -45,5 +45,24 @@ public sealed class HealthMonitoringOptionsValidator : OptionsValidatorBase= OfflineTimeout ({options.OfflineTimeout}): the synthetic 'central' site has " + "no heartbeat source and is fed only by the slower self-report loop, so it needs at " + "least as much offline grace as a real site."); + + builder.RequireThat(options.MetricsStaleTimeout > TimeSpan.Zero, + $"ScadaBridge:HealthMonitoring:MetricsStaleTimeout must be a positive duration " + + $"(was {options.MetricsStaleTimeout}); it is the window an online site may go without " + + "a full report before being flagged metrics-stale."); + + // A stale window shorter than one report interval flags every healthy + // site (reports simply can't arrive faster than they are emitted). Same + // De Morgan guard shape as the Central/Offline pair above: stay silent + // when either field is non-positive so the dedicated positive-duration + // checks own that failure rather than double-firing here. + builder.RequireThat( + !(options.ReportInterval > TimeSpan.Zero + && options.MetricsStaleTimeout > TimeSpan.Zero + && options.MetricsStaleTimeout < options.ReportInterval), + $"ScadaBridge:HealthMonitoring:MetricsStaleTimeout ({options.MetricsStaleTimeout}) " + + $"must be >= ReportInterval ({options.ReportInterval}): a stale window shorter than one " + + "report interval would flag every healthy site, since reports cannot arrive faster than " + + "they are emitted."); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteHealthState.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteHealthState.cs index 123ee003..3ef9c184 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteHealthState.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteHealthState.cs @@ -44,4 +44,23 @@ public sealed record SiteHealthState public long LastSequenceNumber { get; init; } /// Gets a value indicating whether the site is currently considered online. public bool IsOnline { get; init; } + + /// + /// Gets a value indicating whether the site is online (heartbeats still + /// arriving) but has sent no full within + /// . This is a signal + /// distinct from liveness: a site whose HealthReportSender died keeps + /// heartbeating and would otherwise show "online with frozen metrics forever". + /// Cleared whenever a fresh report is processed. Heartbeats never set or clear + /// it — they say nothing about the metrics pipeline. + /// + public bool IsMetricsStale { get; init; } + + /// + /// Gets the instant of the last online↔offline flip, or null if the + /// site has never changed status since it was first observed. Answers "when + /// did this site drop / come back". Not moved by report/heartbeat traffic that + /// leaves the online/offline status unchanged, nor by the metrics-stale flag. + /// + public DateTimeOffset? LastStatusChangeAt { get; init; } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs index b1b93e26..c2ca7aeb 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs @@ -40,6 +40,23 @@ public class CentralHealthAggregatorTests _timeProvider); } + /// + /// Builds a fresh aggregator + manual time provider with an explicit + /// so the metrics-staleness tests can + /// pick a window independent of the shared 60s OfflineTimeout. + /// + private static (CentralHealthAggregator Aggregator, TestTimeProvider Time) NewAggregator( + TimeSpan? metricsStaleTimeout = null) + { + var time = new TestTimeProvider(DateTimeOffset.UtcNow); + var opts = new HealthMonitoringOptions { OfflineTimeout = TimeSpan.FromSeconds(60) }; + if (metricsStaleTimeout is { } mst) + opts.MetricsStaleTimeout = mst; + var aggregator = new CentralHealthAggregator( + Options.Create(opts), NullLogger.Instance, time); + return (aggregator, time); + } + private static SiteHealthReport MakeReport(string siteId, long seq) => new( SiteId: siteId, @@ -423,4 +440,56 @@ public class CentralHealthAggregatorTests _aggregator.ProcessReport(MakeReport("site-1", 11)); Assert.Equal(11, _aggregator.GetSiteState("site-1")!.LastSequenceNumber); } + + /// + /// Review 01 [Medium]: a site whose HealthReportSender died keeps + /// heartbeating and previously showed "online with frozen metrics forever". + /// Metrics staleness is a distinct signal from liveness: heartbeats keep the + /// site online, but the absence of a full report for + /// flags it stale. + /// + [Fact] + public void HeartbeatingSiteWithStaleReports_IsFlaggedMetricsStale_ButStaysOnline() + { + var (aggregator, time) = NewAggregator(metricsStaleTimeout: TimeSpan.FromMinutes(2)); + aggregator.ProcessReport(MakeReport("site-a", 1)); + time.Advance(TimeSpan.FromMinutes(3)); + aggregator.MarkHeartbeat("site-a", time.GetUtcNow()); // heartbeats keep flowing + aggregator.CheckForOfflineSites(); + var state = aggregator.GetSiteState("site-a")!; + Assert.True(state.IsOnline); // heartbeat-based liveness unchanged + Assert.True(state.IsMetricsStale); // NEW distinct signal + } + + /// A fresh full report clears the metrics-stale flag. + [Fact] + public void FreshReport_ClearsMetricsStale() + { + var (aggregator, time) = NewAggregator(metricsStaleTimeout: TimeSpan.FromMinutes(2)); + aggregator.ProcessReport(MakeReport("site-a", 1)); + time.Advance(TimeSpan.FromMinutes(3)); + aggregator.CheckForOfflineSites(); + aggregator.ProcessReport(MakeReport("site-a", 2)); + Assert.False(aggregator.GetSiteState("site-a")!.IsMetricsStale); + } + + /// + /// Review 01 underdeveloped #7: "when did the site drop" was unanswerable. + /// Every online↔offline flip stamps . + /// + [Fact] + public void OnlineOfflineTransitions_RecordLastStatusChangeAt() + { + var (aggregator, time) = NewAggregator(); + aggregator.ProcessReport(MakeReport("site-a", 1)); + time.Advance(TimeSpan.FromMinutes(5)); // > OfflineTimeout + aggregator.CheckForOfflineSites(); + var offlineAt = aggregator.GetSiteState("site-a")!.LastStatusChangeAt; + Assert.Equal(time.GetUtcNow(), offlineAt); + // Advance so the recovery flip lands at a demonstrably later instant than + // the offline flip — proves LastStatusChangeAt is re-stamped on each flip. + time.Advance(TimeSpan.FromSeconds(10)); + aggregator.MarkHeartbeat("site-a", time.GetUtcNow()); + Assert.NotEqual(offlineAt, aggregator.GetSiteState("site-a")!.LastStatusChangeAt); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/HealthMonitoringOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/HealthMonitoringOptionsValidatorTests.cs index 089c9137..c4e7fbb5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/HealthMonitoringOptionsValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/HealthMonitoringOptionsValidatorTests.cs @@ -70,4 +70,27 @@ public class HealthMonitoringOptionsValidatorTests Assert.True(result.Failed); Assert.Contains("CentralOfflineTimeout", result.FailureMessage); } + + [Fact] + public void ZeroMetricsStaleTimeout_IsRejected() + { + var result = Validate(new HealthMonitoringOptions { MetricsStaleTimeout = TimeSpan.Zero }); + + Assert.True(result.Failed); + Assert.Contains("MetricsStaleTimeout", result.FailureMessage); + } + + [Fact] + public void MetricsStaleTimeout_ShorterThanReportInterval_IsRejected() + { + // A stale window shorter than one report interval flags every site. + var result = Validate(new HealthMonitoringOptions + { + ReportInterval = TimeSpan.FromSeconds(30), + MetricsStaleTimeout = TimeSpan.FromSeconds(10) + }); + + Assert.True(result.Failed); + Assert.Contains("MetricsStaleTimeout", result.FailureMessage); + } }