diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs index 4f16d50c..a357b830 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs @@ -166,6 +166,19 @@ public sealed class KpiHistoryRepository : IKpiHistoryRepository // Per-metric aggregation intent: Rate metrics sum the hour (last-value would // discard 59 of 60 intra-hour deltas); Gauge metrics keep the last (latest // timestamp) value in the hour. Min/Max/Count preserve the fold's fidelity. + // + // This Sum is only correct if every sample in the group is a non-overlapping + // delta covering exactly one recorder tick (KpiHistoryOptions.SampleInterval). + // That invariant used to be an unenforced assumption across three + // independently-owned config keys (KpiHistoryOptions.SampleInterval, + // NotificationOutboxOptions.DeliveredKpiWindow, SiteCallAuditOptions.KpiInterval) + // — widening one silently over/under-counted the rollup (arch-review P5-5, Low). + // As of P5-5, NotificationOutboxKpiSampleSource / SiteCallAuditKpiSampleSource + // rescale their Rate-classified samples onto SampleInterval before recording, so + // this Sum is correct here regardless of how the other two keys are tuned; a + // boot-time cross-validator (Host's KpiRateFoldIntervalCrossValidator) still + // enforces all three being equal so the recorded/charted samples never silently + // diverge from what the live dashboard tile shows at the same instant. var aggregation = KpiMetricAggregationCatalog.Resolve(key.Source, key.Metric); var value = aggregation == KpiRollupAggregation.Rate ? group.Sum(s => s.Value) diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/KpiRateFoldIntervalCrossValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/KpiRateFoldIntervalCrossValidator.cs new file mode 100644 index 00000000..2eee28e7 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Host/KpiRateFoldIntervalCrossValidator.cs @@ -0,0 +1,87 @@ +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Configuration; +using ZB.MOM.WW.ScadaBridge.KpiHistory; +using ZB.MOM.WW.ScadaBridge.NotificationOutbox; +using ZB.MOM.WW.ScadaBridge.SiteCallAudit; + +namespace ZB.MOM.WW.ScadaBridge.Host; + +/// +/// Boot-time cross-validation of the "rate-fold interval triple" (arch-review P5-5, +/// Low). , +/// , and +/// are three independently-owned config +/// keys, bound in three different component projects with no compile-time relationship +/// to one another. +/// +/// +/// +/// What no longer depends on this equality. NotificationOutboxKpiSampleSource +/// and SiteCallAuditKpiSampleSource now rescale their Rate-classified KPI-history +/// samples (deliveredLastInterval, failedLastInterval) by +/// SampleInterval / (DeliveredKpiWindow or KpiInterval) before recording them, so +/// KpiHistoryRepository.FoldHourlyRollupsAsync's per-hour Sum over those +/// samples is correct for any value of the other two keys — a widened +/// DeliveredKpiWindow can no longer silently over-count the hourly rollup the way +/// it did before P5-5. +/// +/// +/// What still depends on it. The rescale trades exact per-sample fidelity for +/// rollup correctness: whenever the scale factor is not 1, the recorded/charted +/// deliveredLastInterval/failedLastInterval sample no longer numerically equals what the +/// live Health-dashboard tile shows at the same instant (the live tile computation is +/// untouched and still uses the wider/narrower window directly) — a documented invariant +/// both sample sources' XML docs otherwise promise. This validator keeps that promise true +/// by construction: it fails the host at boot, naming all three configuration keys, unless +/// they are all equal (scale factor exactly 1 on both sides). +/// +/// +/// Registered against IValidateOptions<KpiHistoryOptions> — that options type +/// already calls ValidateOnStart() (see KpiHistory.ServiceCollectionExtensions), +/// so this cross-validator rides the same eager trigger without either +/// or needing +/// their own ValidateOnStart() call for this specific check to fire. It lives in Host +/// — the one composition root that references all three component projects — rather than in +/// any of the three (which deliberately do not reference each other). +/// +/// +public sealed class KpiRateFoldIntervalCrossValidator : OptionsValidatorBase +{ + private readonly IOptions _notificationOutboxOptions; + private readonly IOptions _siteCallAuditOptions; + + /// + /// Creates the validator with the two sibling options it cross-checks against + /// . + /// + public KpiRateFoldIntervalCrossValidator( + IOptions notificationOutboxOptions, + IOptions siteCallAuditOptions) + { + ArgumentNullException.ThrowIfNull(notificationOutboxOptions); + ArgumentNullException.ThrowIfNull(siteCallAuditOptions); + + _notificationOutboxOptions = notificationOutboxOptions; + _siteCallAuditOptions = siteCallAuditOptions; + } + + /// + protected override void Validate(ValidationBuilder builder, KpiHistoryOptions options) + { + var sampleInterval = options.SampleInterval; + var deliveredKpiWindow = _notificationOutboxOptions.Value.DeliveredKpiWindow; + var kpiInterval = _siteCallAuditOptions.Value.KpiInterval; + + builder.RequireThat( + deliveredKpiWindow == sampleInterval && kpiInterval == sampleInterval, + "KPI rate-fold interval mismatch: " + + $"ScadaBridge:KpiHistory:{nameof(KpiHistoryOptions.SampleInterval)} ({sampleInterval}), " + + $"ScadaBridge:NotificationOutbox:{nameof(NotificationOutboxOptions.DeliveredKpiWindow)} " + + $"({deliveredKpiWindow}), and " + + $"ScadaBridge:SiteCallAudit:{nameof(SiteCallAuditOptions.KpiInterval)} ({kpiInterval}) " + + "must all be equal. The KPI-history sample sources rescale their delivered/failed-last-interval " + + "counts onto SampleInterval before recording so the hourly rollup fold's sum stays correct " + + "regardless of this divergence, but a mismatch here still means the recorded/charted KPI-history " + + "values would silently stop matching the live Health-dashboard tile at the same instant."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index ad0ec231..a4b36581 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -346,6 +346,12 @@ try // the KpiHistoryRecorderActor cluster singleton (started in // AkkaHostedService). Observability/best-effort: NOT readiness-gated. builder.Services.AddKpiHistory(builder.Configuration); + // P5-5 (arch-review Low): cross-validates the rate-fold interval triple + // (KpiHistoryOptions.SampleInterval, NotificationOutboxOptions.DeliveredKpiWindow, + // SiteCallAuditOptions.KpiInterval) at boot. Rides KpiHistoryOptions' existing + // ValidateOnStart trigger set by AddKpiHistory above. + builder.Services.TryAddEnumerable( + ServiceDescriptor.Singleton, KpiRateFoldIntervalCrossValidator>()); // The Audit Log's IKpiSampleSource, enumerated by the recorder registered just // above. Central-only (its IAuditLogRepository is central-only); split out of // AddAuditLog by deferred register row 50 for the same reason as diff --git a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Kpi/NotificationOutboxKpiSampleSource.cs b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Kpi/NotificationOutboxKpiSampleSource.cs index 35ce1da8..ab533dc6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Kpi/NotificationOutboxKpiSampleSource.cs +++ b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Kpi/NotificationOutboxKpiSampleSource.cs @@ -4,6 +4,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Kpi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Types.Kpi; using ZB.MOM.WW.ScadaBridge.Commons.Types.Notifications; +using ZB.MOM.WW.ScadaBridge.KpiHistory; namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox.Kpi; @@ -20,9 +21,12 @@ namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox.Kpi; /// capturedAtUtc rather than wall-clock now: the stuck cutoff is /// capturedAtUtc - and the /// delivered window is capturedAtUtc - . -/// The COUNT metrics (queueDepth, stuckCount, parkedCount, deliveredLastInterval) equal the -/// live tile at the same instant; the oldestPendingAgeSeconds metric is computed against -/// the repository's internal clock and may differ from the live tile by the query-execution latency. +/// The COUNT metrics (queueDepth, stuckCount, parkedCount) equal the live tile at the same +/// instant; deliveredLastInterval is rescaled onto KpiHistoryOptions.SampleInterval +/// before being recorded (see the rescale note below) and equals the live tile only when +/// that rescale factor is 1, which the boot-time cross-validator guarantees; the +/// oldestPendingAgeSeconds metric is computed against the repository's internal clock +/// and may differ from the live tile by the query-execution latency. /// /// /// Emits Global (ScopeKey == null), per-Site (ScopeKey == SourceSiteId), and @@ -31,6 +35,28 @@ namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox.Kpi; /// oldestPendingAgeSeconds via ; that one metric is /// omitted when the age is null. /// +/// +/// deliveredLastInterval rescale (P5-5, arch-review Low). +/// classifies this metric as a +/// : the central hourly rollup fold sums every +/// sample recorded in the hour, which is only correct if each sample's value is a +/// non-overlapping delta covering exactly one recorder tick +/// (KpiHistoryOptions.SampleInterval). The live cutoff this class queries the +/// repository with is still anchored on the independently-configurable +/// — unchanged, so the live +/// dashboard tile keeps its own smoothing window — but the COUNT that gets written into +/// is rescaled by +/// SampleInterval / DeliveredKpiWindow before it is recorded, so the rollup's sum +/// is correct for the hour regardless of how DeliveredKpiWindow is tuned (e.g. +/// widened to smooth a noisy live tile). At the documented defaults (both 1 minute) the +/// scale factor is exactly 1 and this is a no-op. A configuration where the scale factor +/// is not 1 — i.e. where the live tile's window no longer equals the recorder's sampling +/// cadence — still boots and rolls up correctly, but the recorded/charted +/// deliveredLastInterval sample then no longer numerically equals the live tile at the +/// same instant (only the rollup total is guaranteed correct); the boot-time +/// cross-validator (Host's KpiRateFoldIntervalCrossValidator) enforces the equality +/// so that divergence can never actually happen in a config that boots. +/// /// public sealed class NotificationOutboxKpiSampleSource : IKpiSampleSource { @@ -44,23 +70,51 @@ public sealed class NotificationOutboxKpiSampleSource : IKpiSampleSource private readonly INotificationOutboxRepository _repository; private readonly NotificationOutboxOptions _options; + private readonly KpiHistoryOptions _kpiHistoryOptions; /// /// Creates the sample source. /// /// Outbox repository providing the KPI computation methods. /// Outbox options carrying the stuck-age and delivered-window cutoffs. + /// + /// KPI History options carrying — the + /// recorder's sampling cadence the deliveredLastInterval sample is rescaled onto (P5-5), + /// so the hourly rollup fold's per-sample sum stays correct independent of + /// . + /// public NotificationOutboxKpiSampleSource( INotificationOutboxRepository repository, - IOptions options) + IOptions options, + IOptions kpiHistoryOptions) { ArgumentNullException.ThrowIfNull(repository); ArgumentNullException.ThrowIfNull(options); + ArgumentNullException.ThrowIfNull(kpiHistoryOptions); _repository = repository; _options = options.Value; + _kpiHistoryOptions = kpiHistoryOptions.Value; } + /// + /// Rescale factor applied to the raw deliveredLastInterval count before it is + /// recorded, so the value written into represents a + /// non-overlapping per- delta regardless of + /// how wide is configured — the + /// hourly rollup fold (KpiHistoryRepository.FoldHourlyRollupsAsync) sums this value + /// across the hour's samples, which is only correct for non-overlapping deltas. Exactly 1 + /// at the documented defaults (both 1 minute). Guards against a non-positive + /// (validated > 0 at boot by + /// , but this defends the arithmetic even + /// if that validation is ever bypassed) by falling back to the unscaled 1:1 factor rather + /// than dividing by zero. + /// + private double DeliveredIntervalScaleFactor => + _options.DeliveredKpiWindow > TimeSpan.Zero + ? _kpiHistoryOptions.SampleInterval.TotalSeconds / _options.DeliveredKpiWindow.TotalSeconds + : 1.0; + /// public string Source => KpiSources.NotificationOutbox; @@ -75,13 +129,14 @@ public sealed class NotificationOutboxKpiSampleSource : IKpiSampleSource var deliveredSince = capturedAt - _options.DeliveredKpiWindow; var samples = new List(); + var scaleFactor = DeliveredIntervalScaleFactor; var global = await _repository.ComputeKpisAsync(stuckCutoff, deliveredSince, cancellationToken) .ConfigureAwait(false); AddSnapshot( samples, capturedAtUtc, KpiScopes.Global, scopeKey: null, global.QueueDepth, global.StuckCount, global.ParkedCount, - global.DeliveredLastInterval, global.OldestPendingAge); + global.DeliveredLastInterval * scaleFactor, global.OldestPendingAge); var perSite = await _repository.ComputePerSiteKpisAsync(stuckCutoff, deliveredSince, cancellationToken) .ConfigureAwait(false); @@ -90,7 +145,7 @@ public sealed class NotificationOutboxKpiSampleSource : IKpiSampleSource AddSnapshot( samples, capturedAtUtc, KpiScopes.Site, site.SourceSiteId, site.QueueDepth, site.StuckCount, site.ParkedCount, - site.DeliveredLastInterval, site.OldestPendingAge); + site.DeliveredLastInterval * scaleFactor, site.OldestPendingAge); } var perNode = await _repository.ComputePerNodeKpisAsync(stuckCutoff, deliveredSince, cancellationToken) @@ -100,7 +155,7 @@ public sealed class NotificationOutboxKpiSampleSource : IKpiSampleSource AddSnapshot( samples, capturedAtUtc, KpiScopes.Node, node.SourceNode, node.QueueDepth, node.StuckCount, node.ParkedCount, - node.DeliveredLastInterval, node.OldestPendingAge); + node.DeliveredLastInterval * scaleFactor, node.OldestPendingAge); } return samples; @@ -109,6 +164,9 @@ public sealed class NotificationOutboxKpiSampleSource : IKpiSampleSource /// /// Appends the five outbox metrics for one snapshot at the given scope, omitting /// oldestPendingAgeSeconds when is null. + /// is already rescaled by + /// (a double, not the raw repository + /// int count) by the caller. /// private void AddSnapshot( List samples, @@ -118,7 +176,7 @@ public sealed class NotificationOutboxKpiSampleSource : IKpiSampleSource int queueDepth, int stuckCount, int parkedCount, - int deliveredLastInterval, + double deliveredLastInterval, TimeSpan? oldestPendingAge) { samples.Add(Sample(capturedAtUtc, MetricQueueDepth, scope, scopeKey, queueDepth)); diff --git a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxOptionsValidator.cs new file mode 100644 index 00000000..1cb1d9ee --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxOptionsValidator.cs @@ -0,0 +1,58 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox; + +/// +/// Validates at host startup (arch-review +/// §1.5 eager-options-validation pattern — this options class had no registered +/// validator before P5-5). The durations feed the dispatch sweep, stuck-call +/// detection, terminal-row purge, and the delivered-throughput KPI window; a +/// zero/negative cadence would spin an Akka scheduler, and a non-positive +/// would collapse the +/// KPI window and — because +/// divides by it to rescale the KPI-history sample onto +/// KpiHistoryOptions.SampleInterval — divide by zero. +/// is deliberately +/// validated on its BASE property, not the clamped +/// , so a +/// misconfigured non-positive value fails fast instead of silently clamping to 1 +/// (mirrors SiteCallAuditOptionsValidator's cadence-field convention). +/// +public sealed class NotificationOutboxOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, NotificationOutboxOptions options) + { + builder.RequireThat(options.DispatchInterval > TimeSpan.Zero, + $"ScadaBridge:NotificationOutbox:{nameof(NotificationOutboxOptions.DispatchInterval)} " + + $"({options.DispatchInterval}) must be a positive duration; it is the dispatch-sweep cadence."); + + builder.RequireThat(options.DispatchBatchSize > 0, + $"ScadaBridge:NotificationOutbox:{nameof(NotificationOutboxOptions.DispatchBatchSize)} " + + $"({options.DispatchBatchSize}) must be positive; it is the max rows claimed per sweep."); + + builder.RequireThat(options.StuckAgeThreshold > TimeSpan.Zero, + $"ScadaBridge:NotificationOutbox:{nameof(NotificationOutboxOptions.StuckAgeThreshold)} " + + $"({options.StuckAgeThreshold}) must be a positive duration; it is the age past which a " + + "pending/retrying notification is flagged stuck."); + + builder.RequireThat(options.TerminalRetention > TimeSpan.Zero, + $"ScadaBridge:NotificationOutbox:{nameof(NotificationOutboxOptions.TerminalRetention)} " + + $"({options.TerminalRetention}) must be a positive duration; it is the terminal-row retention window."); + + builder.RequireThat(options.PurgeInterval > TimeSpan.Zero, + $"ScadaBridge:NotificationOutbox:{nameof(NotificationOutboxOptions.PurgeInterval)} " + + $"({options.PurgeInterval}) must be a positive duration; it is the terminal-row purge-sweep cadence."); + + builder.RequireThat(options.DeliveredKpiWindow > TimeSpan.Zero, + $"ScadaBridge:NotificationOutbox:{nameof(NotificationOutboxOptions.DeliveredKpiWindow)} " + + $"({options.DeliveredKpiWindow}) must be a positive duration; it is the trailing window for the " + + "delivered-throughput KPI and the divisor the KPI-history sample source rescales onto " + + "KpiHistoryOptions.SampleInterval."); + + builder.RequireThat(options.MaxParallelDeliveries > 0, + $"ScadaBridge:NotificationOutbox:{nameof(NotificationOutboxOptions.MaxParallelDeliveries)} " + + $"({options.MaxParallelDeliveries}) must be positive; a non-positive value silently clamps to 1 " + + $"via {nameof(NotificationOutboxOptions.ResolvedMaxParallelDeliveries)}, hiding the misconfiguration."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ServiceCollectionExtensions.cs index 41b321a7..bd4c599d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ServiceCollectionExtensions.cs @@ -56,8 +56,16 @@ public static class ServiceCollectionExtensions { ArgumentNullException.ThrowIfNull(services); + // Eager startup validation (arch-review §1.5 / P5-5): this options class had no + // registered validator before — ValidateOnStart makes a bad + // ScadaBridge:NotificationOutbox section fail fast at boot with a clear, + // key-naming message instead of spinning a scheduler or, for DeliveredKpiWindow, + // dividing by zero in NotificationOutboxKpiSampleSource's KpiHistory rescale. services.AddOptions() - .BindConfiguration(OptionsSection); + .BindConfiguration(OptionsSection) + .ValidateOnStart(); + services.TryAddEnumerable( + ServiceDescriptor.Singleton, NotificationOutboxOptionsValidator>()); // Bind + validate SmsOptions (ScadaBridge:Sms): the SMS adapter's body cap and // per-request timeout fallback. ValidateOnStart surfaces a bad option at host diff --git a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ZB.MOM.WW.ScadaBridge.NotificationOutbox.csproj b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ZB.MOM.WW.ScadaBridge.NotificationOutbox.csproj index c9a02e54..8dd9dcb5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ZB.MOM.WW.ScadaBridge.NotificationOutbox.csproj +++ b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/ZB.MOM.WW.ScadaBridge.NotificationOutbox.csproj @@ -21,6 +21,11 @@ (ISmtpClientWrapper, SmtpPermanentException, SmtpTlsModeParser, OAuth2TokenService). --> + + diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/Kpi/SiteCallAuditKpiSampleSource.cs b/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/Kpi/SiteCallAuditKpiSampleSource.cs index f7a2d994..a7b28087 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/Kpi/SiteCallAuditKpiSampleSource.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/Kpi/SiteCallAuditKpiSampleSource.cs @@ -4,6 +4,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Kpi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; using ZB.MOM.WW.ScadaBridge.Commons.Types.Kpi; +using ZB.MOM.WW.ScadaBridge.KpiHistory; namespace ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi; @@ -21,8 +22,10 @@ namespace ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi; /// live SiteCallAuditActor KPI handlers derive them /// (stuckCutoff = capturedAtUtc - StuckAgeThreshold, /// intervalSince = capturedAtUtc - KpiInterval). The COUNT metrics -/// (buffered, parked, failedLastInterval, deliveredLastInterval, stuck) equal the -/// live tile at the same instant; the oldestPendingAgeSeconds metric is +/// (buffered, parked, stuck) equal the live tile at the same instant; +/// failedLastInterval and deliveredLastInterval are rescaled before recording (see +/// below) and equal the live tile only when that rescale factor is 1, which the +/// boot-time cross-validator guarantees; the oldestPendingAgeSeconds metric is /// computed against the repository's internal clock and may differ from the live /// tile by the query-execution latency. The recorder's capturedAtUtc is /// the single anchor for both cutoffs. @@ -32,6 +35,25 @@ namespace ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi; /// each sampling pass resolves a fresh repository scope, mirroring the actor's /// scope-per-message repository access. /// +/// +/// failedLastInterval / deliveredLastInterval rescale (P5-5, arch-review Low). +/// classifies both as +/// : the central hourly rollup fold sums every +/// sample recorded in the hour, which is only correct if each sample's value is a +/// non-overlapping delta covering exactly one recorder tick +/// (KpiHistoryOptions.SampleInterval). The live cutoff this class queries the +/// repository with is still anchored on the independently-configurable +/// — unchanged, so the live dashboard +/// tile keeps its own smoothing window — but the COUNTS that get written into +/// are rescaled by SampleInterval / KpiInterval +/// before they are recorded, so the rollup's sum is correct for the hour regardless +/// of how KpiInterval is tuned. At the documented defaults (both 1 minute) the +/// scale factor is exactly 1 and this is a no-op. The boot-time cross-validator +/// (Host's KpiRateFoldIntervalCrossValidator) enforces +/// KpiInterval == SampleInterval (alongside the outbox's +/// DeliveredKpiWindow) so a config where the recorded/charted samples no +/// longer equal the live tile can never actually boot. +/// /// public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource { @@ -48,6 +70,7 @@ public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource private readonly ISiteCallAuditRepository _repository; private readonly SiteCallAuditOptions _options; + private readonly KpiHistoryOptions _kpiHistoryOptions; /// /// Creates the sample source over the central SiteCalls repository and @@ -56,17 +79,43 @@ public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource /// /// The central SiteCalls operational-state repository. /// Site Call Audit windowing options (stuck-age + KPI interval). + /// + /// KPI History options carrying — the + /// recorder's sampling cadence the failedLastInterval/deliveredLastInterval samples are + /// rescaled onto (P5-5), so the hourly rollup fold's per-sample sum stays correct + /// independent of . + /// public SiteCallAuditKpiSampleSource( ISiteCallAuditRepository repository, - IOptions options) + IOptions options, + IOptions kpiHistoryOptions) { ArgumentNullException.ThrowIfNull(repository); ArgumentNullException.ThrowIfNull(options); + ArgumentNullException.ThrowIfNull(kpiHistoryOptions); _repository = repository; _options = options.Value; + _kpiHistoryOptions = kpiHistoryOptions.Value; } + /// + /// Rescale factor applied to the raw failedLastInterval/deliveredLastInterval + /// counts before they are recorded, so the values written into + /// represent non-overlapping + /// per- deltas regardless of how wide + /// is configured. Exactly 1 at the + /// documented defaults (both 1 minute). Guards against a non-positive + /// (validated > 0 at boot by + /// , but this defends the arithmetic even if + /// that validation is ever bypassed) by falling back to the unscaled 1:1 factor rather + /// than dividing by zero. + /// + private double IntervalScaleFactor => + _options.KpiInterval > TimeSpan.Zero + ? _kpiHistoryOptions.SampleInterval.TotalSeconds / _options.KpiInterval.TotalSeconds + : 1.0; + /// public string Source => KpiSources.SiteCallAudit; @@ -90,12 +139,13 @@ public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource .ConfigureAwait(false); var samples = new List(); + var scaleFactor = IntervalScaleFactor; // Global scope (null ScopeKey). AddSnapshot( samples, capturedAtUtc, KpiScopes.Global, scopeKey: null, global.BufferedCount, global.ParkedCount, - global.FailedLastInterval, global.DeliveredLastInterval, + global.FailedLastInterval * scaleFactor, global.DeliveredLastInterval * scaleFactor, global.OldestPendingAge, global.StuckCount); // Per-site scope (ScopeKey = source site id). @@ -104,7 +154,7 @@ public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource AddSnapshot( samples, capturedAtUtc, KpiScopes.Site, scopeKey: site.SourceSite, site.BufferedCount, site.ParkedCount, - site.FailedLastInterval, site.DeliveredLastInterval, + site.FailedLastInterval * scaleFactor, site.DeliveredLastInterval * scaleFactor, site.OldestPendingAge, site.StuckCount); } @@ -114,7 +164,7 @@ public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource AddSnapshot( samples, capturedAtUtc, KpiScopes.Node, scopeKey: node.SourceNode, node.BufferedCount, node.ParkedCount, - node.FailedLastInterval, node.DeliveredLastInterval, + node.FailedLastInterval * scaleFactor, node.DeliveredLastInterval * scaleFactor, node.OldestPendingAge, node.StuckCount); } @@ -125,6 +175,9 @@ public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource /// Appends the six-metric catalog for one snapshot at the given scope. The /// oldest-pending-age metric is omitted when the snapshot's age is /// null (no non-terminal rows) rather than written as zero. + /// / are + /// already rescaled by (a double, not the raw + /// repository int counts) by the caller. /// private void AddSnapshot( List samples, @@ -133,8 +186,8 @@ public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource string? scopeKey, int buffered, int parked, - int failedLastInterval, - int deliveredLastInterval, + double failedLastInterval, + double deliveredLastInterval, TimeSpan? oldestPendingAge, int stuck) { diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/ZB.MOM.WW.ScadaBridge.SiteCallAudit.csproj b/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/ZB.MOM.WW.ScadaBridge.SiteCallAudit.csproj index a04c23ff..ff2cf80a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/ZB.MOM.WW.ScadaBridge.SiteCallAudit.csproj +++ b/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/ZB.MOM.WW.ScadaBridge.SiteCallAudit.csproj @@ -39,6 +39,12 @@ SiteCallAudit. Preferred over moving the interfaces into Commons (Commons has no Akka / Communication dependency and would have to carry a Communication-adjacent message). --> + + diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/KpiRateFoldIntervalCrossValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/KpiRateFoldIntervalCrossValidatorTests.cs new file mode 100644 index 00000000..45f5b047 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/KpiRateFoldIntervalCrossValidatorTests.cs @@ -0,0 +1,92 @@ +using Microsoft.Extensions.Options; +using ZB.MOM.WW.ScadaBridge.KpiHistory; +using ZB.MOM.WW.ScadaBridge.NotificationOutbox; +using ZB.MOM.WW.ScadaBridge.SiteCallAudit; + +namespace ZB.MOM.WW.ScadaBridge.Host.Tests; + +/// +/// P5-5 (arch-review Low): is the +/// boot-time guardrail for the "rate-fold interval triple" — +/// , +/// , and +/// . The sample sources now rescale +/// their Rate-classified samples onto SampleInterval, so the hourly rollup +/// fold's correctness no longer depends on this equality — but the +/// recorded/charted sample no longer equals the live dashboard tile unless it +/// holds, so this validator still fails the host at boot on any divergence, +/// naming all three configuration keys. +/// +public class KpiRateFoldIntervalCrossValidatorTests +{ + private static ValidateOptionsResult Validate( + KpiHistoryOptions kpiHistoryOptions, + NotificationOutboxOptions notificationOutboxOptions, + SiteCallAuditOptions siteCallAuditOptions) + { + var validator = new KpiRateFoldIntervalCrossValidator( + Options.Create(notificationOutboxOptions), + Options.Create(siteCallAuditOptions)); + return validator.Validate(Options.DefaultName, kpiHistoryOptions); + } + + [Fact] + public void AllThreeEqual_AtDocumentedDefaults_IsValid() + { + var result = Validate( + new KpiHistoryOptions(), new NotificationOutboxOptions(), new SiteCallAuditOptions()); + + Assert.True(result.Succeeded, result.FailureMessage); + } + + [Fact] + public void AllThreeEqual_AtANonDefaultCommonValue_IsValid() + { + var common = TimeSpan.FromMinutes(5); + var result = Validate( + new KpiHistoryOptions { SampleInterval = common }, + new NotificationOutboxOptions { DeliveredKpiWindow = common }, + new SiteCallAuditOptions { KpiInterval = common }); + + Assert.True(result.Succeeded, result.FailureMessage); + } + + [Fact] + public void DeliveredKpiWindowDiverges_FailsAndNamesAllThreeKeys() + { + var result = Validate( + new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) }, + new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromMinutes(15) }, + new SiteCallAuditOptions { KpiInterval = TimeSpan.FromMinutes(1) }); + + Assert.True(result.Failed); + Assert.Contains("ScadaBridge:KpiHistory:SampleInterval", result.FailureMessage); + Assert.Contains("ScadaBridge:NotificationOutbox:DeliveredKpiWindow", result.FailureMessage); + Assert.Contains("ScadaBridge:SiteCallAudit:KpiInterval", result.FailureMessage); + } + + [Fact] + public void KpiIntervalDiverges_FailsAndNamesAllThreeKeys() + { + var result = Validate( + new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) }, + new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromMinutes(1) }, + new SiteCallAuditOptions { KpiInterval = TimeSpan.FromMinutes(20) }); + + Assert.True(result.Failed); + Assert.Contains("ScadaBridge:KpiHistory:SampleInterval", result.FailureMessage); + Assert.Contains("ScadaBridge:NotificationOutbox:DeliveredKpiWindow", result.FailureMessage); + Assert.Contains("ScadaBridge:SiteCallAudit:KpiInterval", result.FailureMessage); + } + + [Fact] + public void BothDeliveredKpiWindowAndKpiIntervalDiverge_Fails() + { + var result = Validate( + new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) }, + new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromMinutes(15) }, + new SiteCallAuditOptions { KpiInterval = TimeSpan.FromMinutes(20) }); + + Assert.True(result.Failed); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Kpi/NotificationOutboxKpiSampleSourceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Kpi/NotificationOutboxKpiSampleSourceTests.cs index 0084c436..361b8377 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Kpi/NotificationOutboxKpiSampleSourceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Kpi/NotificationOutboxKpiSampleSourceTests.cs @@ -3,6 +3,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Kpi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Types.Kpi; using ZB.MOM.WW.ScadaBridge.Commons.Types.Notifications; +using ZB.MOM.WW.ScadaBridge.KpiHistory; using ZB.MOM.WW.ScadaBridge.NotificationOutbox.Kpi; namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests.Kpi; @@ -22,8 +23,28 @@ public class NotificationOutboxKpiSampleSourceTests DeliveredKpiWindow = TimeSpan.FromMinutes(1), }; + // Matches Options.DeliveredKpiWindow (1 minute) so the default-config tests below see + // an identity rescale factor (1.0) and their pre-existing raw-count assertions hold. + private static readonly KpiHistoryOptions DefaultKpiHistoryOptions = new() + { + SampleInterval = TimeSpan.FromMinutes(1), + }; + private static NotificationOutboxKpiSampleSource CreateSource(INotificationOutboxRepository repository) => - new(repository, Microsoft.Extensions.Options.Options.Create(Options)); + CreateSource(repository, DefaultKpiHistoryOptions); + + private static NotificationOutboxKpiSampleSource CreateSource( + INotificationOutboxRepository repository, KpiHistoryOptions kpiHistoryOptions) => + CreateSource(repository, Options, kpiHistoryOptions); + + private static NotificationOutboxKpiSampleSource CreateSource( + INotificationOutboxRepository repository, + NotificationOutboxOptions options, + KpiHistoryOptions kpiHistoryOptions) => + new( + repository, + Microsoft.Extensions.Options.Options.Create(options), + Microsoft.Extensions.Options.Options.Create(kpiHistoryOptions)); [Fact] public void Source_IsNotificationOutbox() @@ -131,6 +152,45 @@ public class NotificationOutboxKpiSampleSourceTests AssertSample(samples, "oldestPendingAgeSeconds", KpiScopes.Node, "node-a", 60); } + // P5-5 (arch-review Low): the KPI-history rollup fold sums every deliveredLastInterval + // sample recorded in an hour, which over-counts unless each sample is a non-overlapping + // delta covering exactly one recorder tick (KpiHistoryOptions.SampleInterval). Before the + // fix, this class recorded the RAW DeliveredKpiWindow-windowed count verbatim — with a + // 15-minute DeliveredKpiWindow sampled every 1-minute SampleInterval, 60 overlapping + // 15-minute counts summed to ~15x the true hourly total. Proven against the unfixed + // computation: temporarily short-circuiting DeliveredIntervalScaleFactor to always return + // 1.0 (i.e. "record the raw count verbatim", the pre-P5-5 behavior) makes this test fail + // (30 != 2); restoring the real scale factor passes it. + [Fact] + public async Task CollectAsync_RescalesDeliveredLastInterval_OntoKpiHistorySampleInterval() + { + var repository = Substitute.For(); + repository.ComputeKpisAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new NotificationKpiSnapshot( + QueueDepth: 0, StuckCount: 0, ParkedCount: 0, + DeliveredLastInterval: 30, OldestPendingAge: null)); + repository.ComputePerSiteKpisAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Array.Empty()); + repository.ComputePerNodeKpisAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Array.Empty()); + + // DeliveredKpiWindow (15 min) is 15x KpiHistoryOptions.SampleInterval (1 min) — a + // non-default SampleInterval configuration deliberately chosen to prove the rescale + // math, not just the identity (factor == 1) default-config case the other tests use. + var options = new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromMinutes(15) }; + var kpiHistoryOptions = new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) }; + var source = CreateSource(repository, options, kpiHistoryOptions); + + var samples = await source.CollectAsync(CapturedAt); + + var delivered = Assert.Single(samples, s => s.Metric == "deliveredLastInterval"); + // 30 counted over a 15-minute window, rescaled onto a 1-minute recorder tick: 30 / 15 = 2. + // Summing 60 such 1-minute-tick samples across an hour therefore reconstructs the true + // hourly total instead of the ~15x over-count a raw (unscaled) 15-minute count would + // produce if simply summed 60 times. + Assert.Equal(2.0, delivered.Value, precision: 6); + } + [Fact] public async Task CollectAsync_OmitsOldestPendingAge_WhenNull() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxOptionsValidatorTests.cs new file mode 100644 index 00000000..3563447b --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxOptionsValidatorTests.cs @@ -0,0 +1,92 @@ +using Microsoft.Extensions.Options; + +namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests; + +/// +/// P5-5 (arch-review Low): had no +/// registered validator before this — the dispatch/purge/KPI cadences and the +/// delivered-throughput KPI window () +/// could be zero/negative with no boot-time signal, which would spin an Akka +/// scheduler or — because NotificationOutboxKpiSampleSource divides by +/// DeliveredKpiWindow to rescale onto KpiHistoryOptions.SampleInterval +/// — divide by zero. +/// is validated on its BASE property, not the clamped +/// , so a +/// misconfigured non-positive value fails fast instead of silently clamping to 1. +/// +public class NotificationOutboxOptionsValidatorTests +{ + private static ValidateOptionsResult Validate(NotificationOutboxOptions options) => + new NotificationOutboxOptionsValidator().Validate(Options.DefaultName, options); + + [Fact] + public void DefaultOptions_AreValid() + { + var result = Validate(new NotificationOutboxOptions()); + Assert.True(result.Succeeded, result.FailureMessage); + } + + [Fact] + public void ZeroDispatchInterval_IsRejected() + { + var result = Validate(new NotificationOutboxOptions { DispatchInterval = TimeSpan.Zero }); + Assert.True(result.Failed); + Assert.Contains("DispatchInterval", result.FailureMessage); + } + + [Fact] + public void ZeroDispatchBatchSize_IsRejected() + { + var result = Validate(new NotificationOutboxOptions { DispatchBatchSize = 0 }); + Assert.True(result.Failed); + Assert.Contains("DispatchBatchSize", result.FailureMessage); + } + + [Fact] + public void ZeroStuckAgeThreshold_IsRejected() + { + var result = Validate(new NotificationOutboxOptions { StuckAgeThreshold = TimeSpan.Zero }); + Assert.True(result.Failed); + Assert.Contains("StuckAgeThreshold", result.FailureMessage); + } + + [Fact] + public void ZeroTerminalRetention_IsRejected() + { + var result = Validate(new NotificationOutboxOptions { TerminalRetention = TimeSpan.Zero }); + Assert.True(result.Failed); + Assert.Contains("TerminalRetention", result.FailureMessage); + } + + [Fact] + public void ZeroPurgeInterval_IsRejected() + { + var result = Validate(new NotificationOutboxOptions { PurgeInterval = TimeSpan.Zero }); + Assert.True(result.Failed); + Assert.Contains("PurgeInterval", result.FailureMessage); + } + + [Fact] + public void ZeroDeliveredKpiWindow_IsRejected() + { + var result = Validate(new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.Zero }); + Assert.True(result.Failed); + Assert.Contains("DeliveredKpiWindow", result.FailureMessage); + } + + [Fact] + public void NegativeDeliveredKpiWindow_IsRejected() + { + var result = Validate(new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromSeconds(-1) }); + Assert.True(result.Failed); + Assert.Contains("DeliveredKpiWindow", result.FailureMessage); + } + + [Fact] + public void ZeroMaxParallelDeliveries_IsRejected() + { + var result = Validate(new NotificationOutboxOptions { MaxParallelDeliveries = 0 }); + Assert.True(result.Failed); + Assert.Contains("MaxParallelDeliveries", result.FailureMessage); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/Kpi/SiteCallAuditKpiSampleSourceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/Kpi/SiteCallAuditKpiSampleSourceTests.cs index 7c23897c..2554b915 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/Kpi/SiteCallAuditKpiSampleSourceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/Kpi/SiteCallAuditKpiSampleSourceTests.cs @@ -5,6 +5,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Types; using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; using ZB.MOM.WW.ScadaBridge.Commons.Types.Kpi; +using ZB.MOM.WW.ScadaBridge.KpiHistory; using ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi; namespace ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests.Kpi; @@ -30,9 +31,21 @@ public class SiteCallAuditKpiSampleSourceTests KpiInterval = kpiInterval ?? TimeSpan.FromMinutes(1), }; + // Matches Options().KpiInterval (1 minute) so the default-config tests below see an + // identity rescale factor (1.0) and their pre-existing raw-count assertions hold. + private static readonly KpiHistoryOptions DefaultKpiHistoryOptions = new() + { + SampleInterval = TimeSpan.FromMinutes(1), + }; + private static SiteCallAuditKpiSampleSource CreateSource( - ISiteCallAuditRepository repo, SiteCallAuditOptions? options = null) => - new(repo, OptionsWrap(options ?? Options())); + ISiteCallAuditRepository repo, + SiteCallAuditOptions? options = null, + KpiHistoryOptions? kpiHistoryOptions = null) => + new( + repo, + OptionsWrap(options ?? Options()), + Microsoft.Extensions.Options.Options.Create(kpiHistoryOptions ?? DefaultKpiHistoryOptions)); private static IOptions OptionsWrap(SiteCallAuditOptions o) => Microsoft.Extensions.Options.Options.Create(o); @@ -205,6 +218,48 @@ public class SiteCallAuditKpiSampleSourceTests Assert.Equal(3, repo.Calls.Count); } + // --------------------------------------------------------------------- + // P5-5 (arch-review Low): failedLastInterval/deliveredLastInterval are rescaled onto + // KpiHistoryOptions.SampleInterval before recording, so the central hourly rollup fold's + // per-hour Sum stays correct regardless of how KpiInterval is tuned. Before the fix, this + // class recorded the RAW KpiInterval-windowed counts verbatim; with a 20-minute + // KpiInterval sampled every 1-minute SampleInterval, summing 60 overlapping 20-minute + // counts across an hour would over-count ~20x. Proven against the unfixed computation: + // temporarily short-circuiting IntervalScaleFactor to always return 1.0 (i.e. "record the + // raw count verbatim", the pre-P5-5 behavior) makes this test fail (40/1 != 2/0.05); + // restoring the real scale factor passes it. + // --------------------------------------------------------------------- + + [Fact] + public async Task CollectAsync_RescalesFailedAndDeliveredLastInterval_OntoKpiHistorySampleInterval() + { + var repo = new StubRepo + { + Global = new SiteCallKpiSnapshot( + BufferedCount: 0, + ParkedCount: 0, + FailedLastInterval: 1, + DeliveredLastInterval: 40, + OldestPendingAge: null, + StuckCount: 0), + }; + + // KpiInterval (20 min) is 20x KpiHistoryOptions.SampleInterval (1 min) — a + // non-default SampleInterval configuration deliberately chosen to prove the rescale + // math, not just the identity (factor == 1) default-config case the other tests use. + var options = Options(kpiInterval: TimeSpan.FromMinutes(20)); + var kpiHistoryOptions = new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) }; + + var samples = await CreateSource(repo, options, kpiHistoryOptions).CollectAsync(CapturedAt); + + // 1 failure / 40 deliveries counted over a 20-minute window, rescaled onto a + // 1-minute recorder tick: divide by 20. Summing 60 such 1-minute-tick samples across + // an hour therefore reconstructs the true hourly totals instead of the ~20x + // over-count a raw (unscaled) 20-minute count would produce if simply summed 60 times. + AssertHas(samples, "failedLastInterval", KpiScopes.Global, null, 0.05); + AssertHas(samples, "deliveredLastInterval", KpiScopes.Global, null, 2.0); + } + // --------------------------------------------------------------------- // 6. Multiple sites/nodes are all emitted with distinct ScopeKeys. // ---------------------------------------------------------------------