Merge #809 live PercentGood/DurationGood count Good only (Part 13), matching HistoryRead

Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk
2026-11-11 12:05:51 -05:00
parent c3b77a2e4f ed05d50795
commit 70bc960111
11 changed files with 421 additions and 126 deletions
@@ -6,10 +6,16 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Compatibility.Aggregates;
 /// <summary>Adapts the versioned baseline algorithms to the SDK 2 calculator protocol without a second SDK runtime.</summary>
 internal static class CompatibilityAggregateFactory
 {
+    /// <remarks>
+    ///     PercentGood / DurationGood are served by the application-owned <see cref="GoodOnlyStatusAggregateCalculator"/>
+    ///     (#809: Uncertain time is not Good, as in Part 13 and HistoryRead); every other id is the unchanged baseline.
+    /// </remarks>
    internal static IAggregateCalculator Create(NodeId id, DateTimeUtc start, DateTimeUtc end, double interval,
        bool stepped, AggregateConfiguration configuration, ITelemetryContext telemetry) =>
-        new Adapter(Aggregators.CreateStandardCalculator(id, (DateTime)start, (DateTime)end, interval, stepped, configuration, telemetry)
-            ?? throw new ArgumentOutOfRangeException(nameof(id), "Unsupported baseline aggregate."));
+        new Adapter(GoodOnlyStatusAggregateCalculator.Overrides(id)
+            ? new GoodOnlyStatusAggregateCalculator(id, (DateTime)start, (DateTime)end, interval, stepped, configuration, telemetry)
+            : Aggregators.CreateStandardCalculator(id, (DateTime)start, (DateTime)end, interval, stepped, configuration, telemetry)
+              ?? throw new ArgumentOutOfRangeException(nameof(id), "Unsupported baseline aggregate."));

    /// <summary>Replaces exactly the existing standard factories; no aggregate IDs are added or removed.</summary>
    internal static async ValueTask RegisterAsync(AggregateManager manager, CancellationToken ct)
@@ -0,0 +1,83 @@
+using Opc.Ua;
+using DataValue = ZB.MOM.WW.OtOpcUa.OpcUaServer.Compatibility.Aggregates.LegacyDataValue;
+
+namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Compatibility.Aggregates;
+
+/// <summary>
+///     Application-owned override (#809) of the baseline <see cref="StatusAggregateCalculator"/> for
+///     <c>PercentGood</c> and <c>DurationGood</c> only: a region counts toward the Good time only when its status is
+///     Good. Uncertain time is in neither Good nor (unless <c>TreatUncertainAsBad</c>) Bad, per Part 13
+///     §5.4.3.31/§5.4.3.33 (Mantis 0006166) — the rule the HistoryRead evaluators (<c>TimeWeightedAggregator</c>,
+///     SqlServer + QuestDB) already follow.
+/// </summary>
+/// <remarks>
+///     The baseline 1.5.378.182 <c>ComputeDurationGoodBad</c> counts Uncertain regions as Good when
+///     <c>TreatUncertainAsBad</c> = false. This class leaves the copied file untouched and replaces only the Good
+///     branch: the slice values, simple bounds, stepped regions, empty-slice <c>BadNoData</c>, timestamp, value type
+///     and Calculated status bit are the baseline's. <c>DurationBad</c> / <c>PercentBad</c> / <c>WorstQuality*</c>
+///     fall through to the baseline unchanged.
+/// </remarks>
+internal sealed class GoodOnlyStatusAggregateCalculator : StatusAggregateCalculator
+{
+    /// <summary>True for the two aggregate ids this class overrides.</summary>
+    internal static bool Overrides(NodeId aggregateId) =>
+        aggregateId == ObjectIds.AggregateFunction_PercentGood || aggregateId == ObjectIds.AggregateFunction_DurationGood;
+
+    /// <summary>Initializes the calculator; same contract as the baseline constructor.</summary>
+    public GoodOnlyStatusAggregateCalculator(
+        NodeId aggregateId,
+        DateTime startTime,
+        DateTime endTime,
+        double processingInterval,
+        bool stepped,
+        AggregateConfiguration configuration,
+        ITelemetryContext? telemetry)
+        : base(aggregateId, startTime, endTime, processingInterval, stepped, configuration, telemetry)
+    {
+    }
+
+    /// <inheritdoc />
+    protected override DataValue ComputeValue(TimeSlice slice)
+    {
+        if (AggregateId == ObjectIds.AggregateFunction_DurationGood) return ComputeGoodOnly(slice, usePercent: false);
+        if (AggregateId == ObjectIds.AggregateFunction_PercentGood) return ComputeGoodOnly(slice, usePercent: true);
+        return base.ComputeValue(slice);
+    }
+
+    /// <summary>The baseline <c>ComputeDurationGoodBad(slice, isBad: false, usePercent)</c> with Good-only counting.</summary>
+    private DataValue ComputeGoodOnly(TimeSlice slice, bool usePercent)
+    {
+        List<DataValue>? values = GetValuesWithSimpleBounds(slice);
+        if (values == null || values.Count == 0)
+        {
+            return GetNoDataValue(slice);
+        }
+
+        List<SubRegion> regions = GetRegionsInValueSet(values, false, true);
+
+        double duration = 0;
+        double total = 0;
+        foreach (var region in regions)
+        {
+            total += region.Duration;
+            if (StatusCode.IsGood(region.StatusCode))
+            {
+                duration += region.Duration;
+            }
+        }
+
+        if (usePercent)
+        {
+            duration = duration / total * 100;
+        }
+
+        var value = new DataValue
+        {
+            WrappedValue = SdkValueBoundary.FromClr(duration),
+            SourceTimestamp = GetTimestamp(slice),
+            ServerTimestamp = GetTimestamp(slice)
+        };
+        value.StatusCode = value.StatusCode.WithAggregateBits(AggregateBits.Calculated);
+        return value;
+    }
+}
@@ -17,3 +17,14 @@ The isolated `scripts/validation/opcua-aggregate-baseline` process pins only SDK
 Run the generator with a **new** output path, then compare it with `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/Fixtures/opcua-1.5.378.182-aggregates.json`. It refuses to overwrite evidence. Never regenerate expectations from the compatibility implementation. Existing historian parity tests retain their application-specific documented divergences from the baseline SDK, while new compatibility tests check the complete old-calculator output.

 This preserves older behavior deliberately; it does not claim that all older SDK algorithms implement the newest OPC UA interpretation. A future semantic migration needs its own consumer-impact decision and versioned tests.
+
+## Recorded semantic decisions
+
+### #809 — PercentGood / DurationGood count Good time only (owner-approved 2026-11-11)
+
+The baseline `StatusAggregateCalculator.ComputeDurationGoodBad` counts Uncertain regions as **Good** when `TreatUncertainAsBad = false` (1.5.378.106 and SDK 2.0.1 count Good only). Part 13 §5.4.3.31/§5.4.3.33 (Mantis 0006166) and the HistoryRead evaluators (`TimeWeightedAggregator`, SqlServer + QuestDB) count Good regions only, so live AggregateFilter results disagreed with HistoryRead whenever Uncertain data was present. ScadaBridge uses neither AggregateFilter nor these aggregates.
+
+- The copied upstream files above are **unchanged** (their SHA-256s still match `baseline-sources.json`).
+- `GoodOnlyStatusAggregateCalculator.cs` (application code, not a baseline copy) subclasses the baseline `StatusAggregateCalculator` and replaces only the PercentGood / DurationGood computation: the same slice values, simple bounds, stepped regions, empty-slice `BadNoData`, timestamps, value type and Calculated bit, with a region counted as Good only when its status is Good — whatever `TreatUncertainAsBad` says (when true, Uncertain is Bad, so it was never Good). `CompatibilityAggregateFactory.Create` routes exactly those two IDs to it; the other 35 IDs, including DurationBad / PercentBad / WorstQuality, are the baseline unchanged.
+- Golden test: the 64 vectors this changes (38 PercentGood + 26 DurationGood, all with an effective `TreatUncertainAsBad = false` over data with Uncertain regions) are exempted **exactly** — the PercentGood / DurationGood vectors whose baseline output differs from the same case's baseline vector with `TreatUncertainAsBad = true`, where 1.5.378.182 already counted Good regions only — and each is pinned to that twin's output. The pin is therefore baseline evidence, never regenerated from the application's calculator; any further drift in any of the 6,144 cases fails. `Sdk2AggregateCompatibilityTests.Issue809_*` also pins a hand-computed case.
+- Live-vs-history parity: `SdkAggregateParityTests.Live_AggregateFilter_PercentGood_and_DurationGood_equal_HistoryRead_over_Uncertain_samples` compares the registered calculator with `TimeWeightedAggregator` over Good / Uncertain / Bad mixes with no masks, and checks that the data discriminates against the pre-#809 baseline.
@@ -10,7 +10,9 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer;
 ///     <para>
 ///         The aggregate evaluators (<c>AggregateEvaluator</c> / <c>TimeWeightedAggregator</c> in
 ///         <c>Driver.Historian.Sql.Common</c>) are written against fixed settings, not parameterised: Uncertain time counts as
-///         Good (<see cref="TreatUncertainAsBad"/> = false) in the time-weighted family; the per-sample Average / Minimum /
+///         Good (<see cref="TreatUncertainAsBad"/> = false) in TimeAverage2 / Total2, while PercentGood / DurationGood count
+///         Good time only and PercentBad / DurationBad Bad time only — Uncertain in neither (Part 13 §5.4.3.31-34, Mantis
+///         0006166); the per-sample Average / Minimum /
 ///         Maximum / Range / *ActualTime / Count are defined by Part 13 over Good values whatever this setting, as the
 ///         SDK computes them under it; an interval is Good only when all of
 ///         it is (<see cref="PercentDataGood"/> = 100) and Bad only when none of it is non-Bad
@@ -23,6 +25,16 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer;
 ///         the SDK manager so live AggregateFilter subscriptions agree with history reads; a test pins both sides.
 ///     </para>
 ///     <para>
+///         The live calculators are the application's frozen 1.5.378.182 copies (<c>Compatibility/Aggregates</c>), whose
+///         <c>ComputeDurationGoodBad</c> counts Uncertain time as Good under <see cref="TreatUncertainAsBad"/> = false. That
+///         made live PercentGood / DurationGood disagree with HistoryRead whenever Uncertain data was present. #809
+///         (owner-approved 2026-11-11) overrides exactly those two (<c>GoodOnlyStatusAggregateCalculator</c>), so live and
+///         HistoryRead now agree on them — pinned by
+///         <c>SdkAggregateParityTests.Live_AggregateFilter_PercentGood_and_DurationGood_equal_HistoryRead_over_Uncertain_samples</c>.
+///         Other known live-vs-HistoryRead differences (e.g. #821, an Uncertain first sample in a late-start interval) are
+///         documented in <c>docs/Historian.md</c>.
+///     </para>
+///     <para>
 ///         A request asking for <c>UseServerCapabilitiesDefaults</c>, or spelling out exactly these values, is served;
 ///         any other configuration is refused per node with <c>BadAggregateConfigurationRejected</c> rather than silently
 ///         answered under settings the client did not ask for (it used to be ignored).