From c2bdb7d5dec3f7f7a59f0de277ae33c1b91f7c82 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 16:57:38 -0400 Subject: [PATCH] feat(hosts): driver diagnostics channel on /hosts, S7 negotiated PDU size first (#640) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 5 +- docs/Telemetry.md | 17 ++ docs/drivers/S7.md | 6 +- .../Messages/Drivers/DriverHealthChanged.cs | 9 +- .../Protos/telemetry.proto | 6 + .../IDriverDiagnostics.cs | 58 ++++++ .../IDriverHealthPublisher.cs | 10 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 15 +- .../Components/Pages/Hosts.razor | 11 ++ .../Hosts/HostsDriverView.cs | 15 +- .../Telemetry/TelemetryProtoMapCentral.cs | 3 +- .../Grpc/TelemetryProtoMapNode.cs | 8 + .../Drivers/AkkaDriverHealthPublisher.cs | 6 +- .../Drivers/DriverInstanceActor.cs | 36 +++- .../S7TsapAndPduTests.cs | 3 + .../Hosts/HostsDriverViewTests.cs | 18 ++ .../TelemetryProtoMapCentralTests.cs | 37 ++++ .../Grpc/TelemetryStreamGrpcServiceTests.cs | 27 +++ .../DriverInstanceActorDiagnosticsTests.cs | 187 ++++++++++++++++++ .../DriverInstanceActorHostStatusTests.cs | 3 +- ...iverInstanceActorRediscoverySignalTests.cs | 3 +- .../DriverInstanceActorSkippedTagTests.cs | 3 +- ...InstanceActorSubscriptionReconcileTests.cs | 3 +- 23 files changed, 469 insertions(+), 20 deletions(-) create mode 100644 src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverDiagnostics.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorDiagnosticsTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index 1c68f757..6f371cd4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1113,8 +1113,9 @@ host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT fa - **S7 (#632/#638):** STRING/WSTRING/CHAR/WCHAR and DTL/DATE/TOD/TIME/S5TIME codecs (`S7ValueCodecs`; TOD/TIME/S5TIME surface as Int32 ms); an undecodable buffer or unrepresentable write is `BadOutOfRange` with no health impact. Optional `localTsap`/`remoteTsap`/`connectionType` override the slot TSAP (`remoteTsap` ⊕ `connectionType`). Test - Connect reads 2 bytes of `MB0` so a PUT/GET-disabled CPU fails the probe. The negotiated PDU size is logged and on - `S7Driver.GetDiagnostics()` only — there is no driver-diagnostics surface on `/hosts`. + Connect reads 2 bytes of `MB0` so a PUT/GET-disabled CPU fails the probe. The negotiated PDU size is logged, on + `S7Driver.GetDiagnostics()`, and (#640) on `/hosts` via the optional `IDriverDiagnostics` bag (string map on + `DriverHealthChanged.Diagnostics` / proto `has_diagnostics`+`diagnostics`; its digest is in the publish fingerprint). - **TwinCAT (#637):** TIME/TOD publish Float64 ms, DATE/DT a UTC DateTime; a tag authored `UDInt` keeps the raw counter (and keeps historizing — DateTime is not historized). - **AbLegacy (#636):** `AbLegacyAddress` (in `AbLegacy.Contracts`) is family-aware: PLC-5 `I:`/`O:` indices are OCTAL diff --git a/docs/Telemetry.md b/docs/Telemetry.md index 255c8588..49dd3be5 100644 --- a/docs/Telemetry.md +++ b/docs/Telemetry.md @@ -112,6 +112,23 @@ it — but it does mean the panel cannot answer "which node saw this?". Adding t widening the wire record, not changing the page. The one sub-instance breakdown the channel *does* carry is per-host connectivity, below. +### Driver diagnostics ride `driver-health` (Gitea #640) + +A driver may implement the optional `IDriverDiagnostics` (Core.Abstractions): a pure in-memory +`IReadOnlyDictionary GetDiagnostics()` of operator-facing facts that are not driver +*state* (S7's negotiated PDU size is the first; others opt in later). `DriverInstanceActor` pulls it on +every health publish (try/caught, unguarded by the invoker like `GetHostStatuses`), bounds it +(`DriverDiagnostics.Bound`: 16 entries, text truncated to 200 chars, ordinal key order), and carries it as +`DriverHealthChanged.Diagnostics` -> proto `DriverHealth.has_diagnostics = 13` / `map +diagnostics = 14` (additive; old nodes never set them, old central ignores them) -> `HostsDriverRow.DiagnosticItems`, +rendered as a small key/value list under the driver name on `/hosts` (nothing when empty). + +- **null ≠ empty**: null = driver lacks the capability; the presence flag preserves an empty bag. +- **The diagnostics digest is part of the publish fingerprint, and must stay there** - a diagnostic fact + (e.g. PDU size changing on a reconnect) moves no other component, so without it the dedup swallows the + publish. Pinned by `DriverInstanceActorDiagnosticsTests.A_diagnostics_change_republishes_despite_otherwise_unchanged_health` + (stable-health stub - the shared `StubDriver` returns `UtcNow` and would pass vacuously). + ### Per-host connectivity rides `driver-health` (Gitea #521) `DriverHealthChanged.HostStatuses` carries each driver's `IHostConnectivityProbe.GetHostStatuses()` diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 23ce7de8..30711365 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -223,8 +223,10 @@ The PDU size the CPU negotiates during the connection handshake (typically 240 b extended PDU) is snapshotted at every (re)connect and exposed as `S7Driver.NegotiatedPduSize` / `S7Driver.GetDiagnostics()["S7.NegotiatedPduSize"]` (0 before the first connect and after shutdown). It is also written to the `S7Driver connected` / `reconnected` log lines and to the AdminUI Test Connect -message (`S7 connected (CPU S71500, PDU 240 bytes)`). `DriverHealth` has no diagnostics bag in this -codebase, so the value is not yet on the `/hosts` health snapshot — adding one is a Core change. +message (`S7 connected (CPU S71500, PDU 240 bytes)`). S7 also implements the optional +`IDriverDiagnostics` capability (explicitly, string-valued: `S7.NegotiatedPduSize` = `"480"`, omitted while 0 = +not negotiated yet), so the value rides the driver-health snapshot and shows under the driver name on the +AdminUI `/hosts` page (Gitea #640). See `docs/Telemetry.md` ("Driver diagnostics ride `driver-health`"). ## Capability surface diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Drivers/DriverHealthChanged.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Drivers/DriverHealthChanged.cs index bf239d44..7aaea711 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Drivers/DriverHealthChanged.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Drivers/DriverHealthChanged.cs @@ -59,6 +59,12 @@ namespace ZB.MOM.WW.OtOpcUa.Commons.Messages.Drivers; /// Value-equality caveat — the same one carries: the /// record's generated Equals compares the sample list by REFERENCE. /// +/// +/// Operator-facing driver diagnostics from IDriverDiagnostics.GetDiagnostics() (e.g. S7's +/// negotiated PDU size), bounded and in ordinal key order; null when the driver does not implement the +/// capability (Gitea #640). Additive, never a state change. Same by-reference value-equality caveat as +/// . +/// public sealed record DriverHealthChanged( string ClusterId, string DriverInstanceId, @@ -70,7 +76,8 @@ public sealed record DriverHealthChanged( DateTime? RediscoveryNeededUtc = null, string? RediscoveryReason = null, IReadOnlyList? HostStatuses = null, - SkippedTagReport? SkippedTags = null) + SkippedTagReport? SkippedTags = null, + IReadOnlyDictionary? Diagnostics = null) { /// /// DPS topic name. Both the runtime AkkaDriverHealthPublisher and the AdminUI diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Protos/telemetry.proto b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Protos/telemetry.proto index a5529c2f..e2fdd725 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Protos/telemetry.proto +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Protos/telemetry.proto @@ -82,6 +82,12 @@ message DriverHealth { // proto3, which a repeated field does not. So no companion has_ flag (and one would collide: protoc // already generates a HasSkippedTags property for this field). SkippedTags skipped_tags = 12; + // Driver diagnostics bag (Gitea #640), e.g. S7's negotiated PDU size. Additive: an old node never sets + // these and an old central ignores them. A map cannot distinguish absent from empty, and the two differ + // ("driver has no IDriverDiagnostics" vs "it has one and currently has nothing to say"), so the presence + // flag carries that bit - the same shape as has_host_statuses. + bool has_diagnostics = 13; + map diagnostics = 14; } // Mirrors ZB.MOM.WW.OtOpcUa.Core.Abstractions.SkippedTagReport. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverDiagnostics.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverDiagnostics.cs new file mode 100644 index 00000000..549f89de --- /dev/null +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverDiagnostics.cs @@ -0,0 +1,58 @@ +namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +/// +/// Optional driver capability: expose a small bag of operator-facing diagnostic key/value pairs +/// (e.g. S7's negotiated PDU size) that ride the driver-health snapshot to the AdminUI /hosts +/// page (Gitea #640). +/// +/// +/// +/// Why a bag rather than fields on . The facts worth showing are +/// driver-specific and do not change the driver's state; a typed field per driver would +/// widen the core record and the telemetry proto for every driver. Same additive-never-a-state-change +/// rule as . +/// +/// +/// Values are strings so the telemetry proto stays a plain map<string,string>. Keep the +/// bag small ( entries, text truncated to +/// ): it is published on every state change. +/// +/// +public interface IDriverDiagnostics +{ + /// + /// Snapshot of the driver's diagnostics. Must be a pure in-memory read with no I/O — it is called on + /// the health-publish path, deliberately NOT through the resilience pipeline. + /// + /// The key/value pairs; empty when there is nothing to show yet. + IReadOnlyDictionary GetDiagnostics(); +} + +/// Bounds and helpers shared by every consumer of . +public static class DriverDiagnostics +{ + /// Maximum number of entries carried on a health snapshot; extras are dropped (ordinal key order). + public const int MaxEntries = 16; + + /// Maximum length of a key or value carried on a health snapshot; longer text is truncated. + public const int MaxValueLength = 200; + + /// + /// Normalises a driver's bag for publishing: ordinal key order, capped entry count, truncated text. + /// Deterministic so the same facts always produce the same fingerprint digest. + /// + /// The bag the driver returned. + /// A bounded, ordered copy. + public static IReadOnlyDictionary Bound(IReadOnlyDictionary raw) + { + var result = new SortedDictionary(StringComparer.Ordinal); + foreach (var kv in raw.OrderBy(k => k.Key, StringComparer.Ordinal)) + { + if (result.Count >= MaxEntries) break; + result[Truncate(kv.Key ?? "")] = Truncate(kv.Value ?? ""); + } + return result; + } + + private static string Truncate(string s) => s.Length <= MaxValueLength ? s : s[..MaxValueLength]; +} diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverHealthPublisher.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverHealthPublisher.cs index c1edf53a..fa478777 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverHealthPublisher.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriverHealthPublisher.cs @@ -34,6 +34,10 @@ public interface IDriverHealthPublisher /// . Contradicts the "Healthy, 0 errors, reads advancing" reading /// of a driver that is in fact ignoring part or all of its configuration (Gitea #541). /// + /// + /// Bounded driver diagnostics from ; null when the + /// driver is not an (Gitea #640). + /// void Publish( string clusterId, string driverInstanceId, @@ -42,7 +46,8 @@ public interface IDriverHealthPublisher DateTime? rediscoveryNeededUtc = null, string? rediscoveryReason = null, IReadOnlyList? hostStatuses = null, - SkippedTagReport? skippedTags = null); + SkippedTagReport? skippedTags = null, + IReadOnlyDictionary? diagnostics = null); } /// @@ -65,6 +70,7 @@ public sealed class NullDriverHealthPublisher : IDriverHealthPublisher DateTime? rediscoveryNeededUtc = null, string? rediscoveryReason = null, IReadOnlyList? hostStatuses = null, - SkippedTagReport? skippedTags = null) + SkippedTagReport? skippedTags = null, + IReadOnlyDictionary? diagnostics = null) { /* no-op */ } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 43a5dc08..8344ec0f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -32,7 +32,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.S7; /// /// public sealed class S7Driver - : IDriver, ITagDiscovery, IReadable, IWritable, IBatchSubscribable, IHostConnectivityProbe, ISkippedTagReporter, IDisposable, IAsyncDisposable + : IDriver, ITagDiscovery, IReadable, IWritable, IBatchSubscribable, IHostConnectivityProbe, ISkippedTagReporter, IDriverDiagnostics, IDisposable, IAsyncDisposable { /// Authored tags this driver could not build into its poll table (Gitea #541). private readonly SkippedTagLedger _skippedTags = new(); @@ -463,6 +463,19 @@ public sealed class S7Driver ["S7.NegotiatedPduSize"] = NegotiatedPduSize, }; + /// + /// view of the same facts, as strings, for the /hosts page + /// (Gitea #640). Empty before the first connect (PDU size 0 is "not negotiated yet", not a size). + /// + /// The diagnostics bag. + IReadOnlyDictionary IDriverDiagnostics.GetDiagnostics() + { + var pdu = NegotiatedPduSize; + return pdu > 0 + ? new Dictionary { ["S7.NegotiatedPduSize"] = pdu.ToString(System.Globalization.CultureInfo.InvariantCulture) } + : new Dictionary(); + } + private static readonly HashSet ByteAnchoredDataTypes = new() { S7DataType.Int64, diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Hosts.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Hosts.razor index ecaca6c3..4c7849fd 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Hosts.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Hosts.razor @@ -189,6 +189,17 @@ else @* Own line — inline after the name the two identifiers ran together. *@
@d.DriverInstanceId
} + @if (d.DiagnosticItems.Count > 0) + { + @* Driver-reported diagnostics (Gitea #640), e.g. S7 negotiated PDU size. + Renders nothing for a driver with none. *@ +
+ @foreach (var kv in d.DiagnosticItems) + { +
@kv.Key: @kv.Value
+ } +
+ } @(d.DriverType ?? "—") @d.State diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Hosts/HostsDriverView.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Hosts/HostsDriverView.cs index 70c0f38b..fb23b6e9 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Hosts/HostsDriverView.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Hosts/HostsDriverView.cs @@ -52,6 +52,8 @@ public sealed record HostsDriverInstanceInfo( /// is no answer — the driver cannot report skips, or it faulted before building a tag table. Lets /// "ignoring 20 of 22 tags" show even while every other column on the row — state, last read, error /// count, last error — reads perfectly healthy (Gitea #541). +/// Driver-reported diagnostics (e.g. S7 negotiated PDU size); null or empty when +/// the driver has none to show (Gitea #640). /// Whether the matching configured instance is authored enabled; /// when no instance matched. Fed to the row's expandable DriverStatusPanel (Gitea #546). An /// unmatched row defaults to enabled on purpose: the row only exists because a live snapshot arrived, @@ -62,8 +64,16 @@ public sealed record HostsDriverRow( DateTime? RediscoveryNeededUtc = null, string? RediscoveryReason = null, IReadOnlyList? HostStatuses = null, SkippedTagReport? SkippedTags = null, - bool Enabled = true) + bool Enabled = true, + IReadOnlyDictionary? Diagnostics = null) { + /// Diagnostics as ordered key/value pairs for rendering; empty when there are none, in which + /// case the page renders nothing. + public IReadOnlyList> DiagnosticItems { get; } = + (Diagnostics ?? new Dictionary()) + .OrderBy(k => k.Key, StringComparer.Ordinal) + .ToList(); + /// Whether this driver is ignoring at least one authored tag — the condition the whole /// column exists to make visible. public bool HasSkippedTags => SkippedTags is { SkippedCount: > 0 }; @@ -177,7 +187,8 @@ public static class HostsDriverView s.RediscoveryReason, s.HostStatuses, s.SkippedTags, - instance?.Enabled ?? true); + instance?.Enabled ?? true, + s.Diagnostics); }) .OrderBy(d => d.Name ?? d.DriverInstanceId, StringComparer.OrdinalIgnoreCase) .ThenBy(d => d.DriverInstanceId, StringComparer.OrdinalIgnoreCase) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Telemetry/TelemetryProtoMapCentral.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Telemetry/TelemetryProtoMapCentral.cs index b070cc53..c975196c 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Telemetry/TelemetryProtoMapCentral.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Telemetry/TelemetryProtoMapCentral.cs @@ -130,7 +130,8 @@ public static class TelemetryProtoMapCentral RediscoveryNeededUtc: msg.RediscoveryNeededUtc?.ToDateTime(), RediscoveryReason: msg.HasRediscoveryReason ? msg.RediscoveryReason : null, HostStatuses: ToHostStatuses(msg), - SkippedTags: ToSkippedTags(msg)); + SkippedTags: ToSkippedTags(msg), + Diagnostics: msg.HasDiagnostics ? new Dictionary(msg.Diagnostics) : null); } /// diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Grpc/TelemetryProtoMapNode.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Grpc/TelemetryProtoMapNode.cs index 904d84a7..01ffdaac 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Grpc/TelemetryProtoMapNode.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Grpc/TelemetryProtoMapNode.cs @@ -164,6 +164,14 @@ public static class TelemetryProtoMapNode msg.SkippedTags = payload; } + // Gitea #640. Presence flag first - an empty map cannot say whether the driver has the capability. + if (e.Diagnostics is not null) + { + msg.HasDiagnostics = true; + foreach (var kv in e.Diagnostics) + msg.Diagnostics[kv.Key ?? ""] = kv.Value ?? ""; + } + return msg; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/AkkaDriverHealthPublisher.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/AkkaDriverHealthPublisher.cs index fb2955a5..bef91b67 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/AkkaDriverHealthPublisher.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/AkkaDriverHealthPublisher.cs @@ -38,7 +38,8 @@ public sealed class AkkaDriverHealthPublisher : IDriverHealthPublisher DateTime? rediscoveryNeededUtc = null, string? rediscoveryReason = null, IReadOnlyList? hostStatuses = null, - SkippedTagReport? skippedTags = null) + SkippedTagReport? skippedTags = null, + IReadOnlyDictionary? diagnostics = null) { var msg = new DriverHealthChanged( clusterId, @@ -51,7 +52,8 @@ public sealed class AkkaDriverHealthPublisher : IDriverHealthPublisher rediscoveryNeededUtc, rediscoveryReason, hostStatuses, - skippedTags); + skippedTags, + diagnostics); DistributedPubSub.Get(_system).Mediator.Tell(new Publish(TopicName, msg)); // Phase 5: fan the same snapshot into the node-local live-telemetry hub (no-op until a gRPC // client subscribes). The DPS publish above is unchanged — the hub is a strictly additive tap. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs index 062fe6e2..ceeaab39 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs @@ -1153,6 +1153,28 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers } } + /// Bounded driver diagnostics, or null when the driver is not an + /// (Gitea #640). Pulled fresh and called UNGUARDED for the same reasons as + /// ; a throwing implementation loses only the diagnostics. + private IReadOnlyDictionary? CurrentDiagnostics() + { + if (_driver is not IDriverDiagnostics diag) return null; + try + { + return DriverDiagnostics.Bound(diag.GetDiagnostics()); + } + catch (Exception ex) + { + _log.Warning(ex, "DriverInstance {Id}: GetDiagnostics threw during health publish; omitting diagnostics", _driverInstanceId); + return null; + } + } + + /// Value digest of the diagnostics bag for the publish fingerprint. Null in, null out. The bag is + /// already in ordinal key order (), so equal facts digest equally. + private static string? DiagnosticsDigest(IReadOnlyDictionary? d) => + d is null ? null : string.Join('|', d.Select(kv => $"{kv.Key}={kv.Value}")); + /// Records the driver's rediscovery raise and re-publishes health so the signal reaches the /// AdminUI promptly rather than waiting for the next 30 s heartbeat. /// Advisory only. The served address space is deliberately NOT rebuilt: v3 authors raw tags @@ -1357,6 +1379,7 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers var errorCount = ErrorCount5Min(); var hostStatuses = CurrentHostStatuses(); var skippedTags = CurrentSkippedTags(); + var diagnostics = CurrentDiagnostics(); // _rediscoveryNeededUtc is PART OF THE FINGERPRINT on purpose. A rediscovery raise on an // otherwise-unchanged Healthy driver leaves (state, lastSuccess, lastError, errorCount) // identical, so without it the dedup below would swallow the very publish that carries the @@ -1375,14 +1398,20 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers // is identical to a clean driver's and the publish that carries the signal is exactly the one // dedup would swallow. Flattened to a STRING for the same reason as the host digest — the // report holds an IReadOnlyList, which a tuple compares by reference. + // + // The diagnostics digest (Gitea #640) is the fourth instance: a diagnostic fact (S7's PDU size + // changing across a reconnect) moves none of the other components, so without it the publish + // that carries it is deduped away. Flattened to a STRING - the bag is a dictionary, which a + // tuple compares by reference. var fingerprint = (health.State, health.LastSuccessfulRead, health.LastError, errorCount, - _rediscoveryNeededUtc, HostStatusDigest(hostStatuses), SkippedTagDigest(skippedTags)); + _rediscoveryNeededUtc, HostStatusDigest(hostStatuses), SkippedTagDigest(skippedTags), + DiagnosticsDigest(diagnostics)); if (_lastPublishedFingerprint is { } prev && prev.Equals(fingerprint)) return observed; _lastPublishedFingerprint = fingerprint; _healthPublisher.Publish( _clusterId, _driverInstanceId, health, errorCount, _rediscoveryNeededUtc, _rediscoveryReason, - hostStatuses, skippedTags); + hostStatuses, skippedTags, diagnostics); } catch (Exception ex) { @@ -1418,7 +1447,8 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers /// Fingerprint of the last call; null until first publish. private (DriverState State, DateTime? LastSuccess, string? LastError, int ErrorCount, - DateTime? RediscoveryNeededUtc, string? HostStatusDigest, string? SkippedTagDigest)? _lastPublishedFingerprint; + DateTime? RediscoveryNeededUtc, string? HostStatusDigest, string? SkippedTagDigest, + string? DiagnosticsDigest)? _lastPublishedFingerprint; /// protected override void PostStop() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs index f1dd5dde..4f9bd37f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs @@ -140,13 +140,16 @@ public sealed class S7TsapAndPduTests using var drv = new S7Driver(opts, "s7-pdu", factory); drv.NegotiatedPduSize.ShouldBe(0); drv.GetDiagnostics()["S7.NegotiatedPduSize"].ShouldBe(0); + ((IDriverDiagnostics)drv).GetDiagnostics().ShouldBeEmpty("0 is 'not negotiated yet', not a size (#640)"); await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); drv.NegotiatedPduSize.ShouldBe(480); drv.GetDiagnostics()["S7.NegotiatedPduSize"].ShouldBe(480); + ((IDriverDiagnostics)drv).GetDiagnostics()["S7.NegotiatedPduSize"].ShouldBe("480"); await drv.ShutdownAsync(TestContext.Current.CancellationToken); drv.NegotiatedPduSize.ShouldBe(0); + ((IDriverDiagnostics)drv).GetDiagnostics().ShouldBeEmpty(); } [Fact] diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Hosts/HostsDriverViewTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Hosts/HostsDriverViewTests.cs index 208eb998..9a37c04b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Hosts/HostsDriverViewTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Hosts/HostsDriverViewTests.cs @@ -272,6 +272,24 @@ public sealed class HostsDriverViewTests row.SkippedTags!.SkippedCount.ShouldBe(20); } + /// #640: diagnostics ride the snapshot onto the row, ordered by key; none renders nothing. + [Fact] + public void Diagnostics_reach_the_row_ordered_and_absent_diagnostics_yield_no_items() + { + var with = Snap("MAIN", "s7-a") with + { + Diagnostics = new Dictionary { ["Z"] = "2", ["S7.NegotiatedPduSize"] = "480" }, + }; + var without = Snap("MAIN", "s7-b"); + + var rows = HostsDriverView.Build([with, without], nodes: null, instances: null).Single().Drivers; + + var a = rows.Single(r => r.DriverInstanceId == "s7-a"); + a.DiagnosticItems.Select(k => k.Key).ShouldBe(["S7.NegotiatedPduSize", "Z"]); + a.DiagnosticItems[0].Value.ShouldBe("480"); + rows.Single(r => r.DriverInstanceId == "s7-b").DiagnosticItems.ShouldBeEmpty(); + } + /// /// A truncated sample must SAY it is truncated. Without the note a capped list reads as the whole /// list, and an operator who fixes the twenty named tags believes the job is done. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Telemetry/TelemetryProtoMapCentralTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Telemetry/TelemetryProtoMapCentralTests.cs index a5a5468b..e851e398 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Telemetry/TelemetryProtoMapCentralTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Telemetry/TelemetryProtoMapCentralTests.cs @@ -276,6 +276,43 @@ public sealed class TelemetryProtoMapCentralTests TelemetryProtoMapCentral.ToHealth(absentSkips).SkippedTags.ShouldBeNull(); } + /// + /// #640: the diagnostics bag round-trips with the null vs empty distinction intact, and a pre-#640 + /// node (field never set) decodes to null. + /// + [Fact] + public void ToHealth_diagnostics_round_trip_with_the_null_vs_empty_distinction_intact() + { + var populated = new DriverHealth + { + ClusterId = "c1", DriverInstanceId = "d1", State = "Healthy", + PublishedUtc = Timestamp.FromDateTime(SampleUtc), + HasDiagnostics = true, + Diagnostics = { ["S7.NegotiatedPduSize"] = "480", ["Other"] = "x" }, + }; + var mapped = TelemetryProtoMapCentral.ToHealth(populated).Diagnostics; + mapped.ShouldNotBeNull(); + mapped!.Count.ShouldBe(2); + mapped["S7.NegotiatedPduSize"].ShouldBe("480"); + + var empty = new DriverHealth + { + ClusterId = "c1", DriverInstanceId = "d1", State = "Healthy", + PublishedUtc = Timestamp.FromDateTime(SampleUtc), + HasDiagnostics = true, + }; + var mappedEmpty = TelemetryProtoMapCentral.ToHealth(empty).Diagnostics; + mappedEmpty.ShouldNotBeNull(); + mappedEmpty!.ShouldBeEmpty(); + + var absent = new DriverHealth + { + ClusterId = "c1", DriverInstanceId = "d1", State = "Healthy", + PublishedUtc = Timestamp.FromDateTime(SampleUtc), + }; + TelemetryProtoMapCentral.ToHealth(absent).Diagnostics.ShouldBeNull(); + } + /// /// An unparseable host state degrades to rather than throwing. A node /// running a newer build that added an enum member must not be able to kill central's telemetry diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Grpc/TelemetryStreamGrpcServiceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Grpc/TelemetryStreamGrpcServiceTests.cs index 1ebd3cb3..97bbfe6b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Grpc/TelemetryStreamGrpcServiceTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Grpc/TelemetryStreamGrpcServiceTests.cs @@ -248,6 +248,33 @@ public sealed class TelemetryStreamGrpcServiceTests .DriverHealth.HasHostStatuses.ShouldBeFalse(); } + /// + /// Node side of the #640 diagnostics carry: presence flag + entries written; a capability-less driver + /// (null) leaves the flag clear, and an empty bag still sets it. + /// + [Fact] + public void ToProto_health_writes_the_diagnostics_presence_flag_and_entries() + { + var withDiag = new DriverHealthChanged( + "cluster-a", "drv-1", "Healthy", null, null, 0, DateTime.UtcNow, + Diagnostics: new Dictionary { ["S7.NegotiatedPduSize"] = "480" }); + var evt = TelemetryProtoMapNode.ToProto(new TelemetryItem.Health(withDiag), "c"); + evt.DriverHealth.HasDiagnostics.ShouldBeTrue(); + evt.DriverHealth.Diagnostics["S7.NegotiatedPduSize"].ShouldBe("480"); + + var empty = new DriverHealthChanged( + "cluster-a", "drv-1", "Healthy", null, null, 0, DateTime.UtcNow, + Diagnostics: new Dictionary()); + var emptyEvt = TelemetryProtoMapNode.ToProto(new TelemetryItem.Health(empty), "c"); + emptyEvt.DriverHealth.HasDiagnostics.ShouldBeTrue(); + emptyEvt.DriverHealth.Diagnostics.ShouldBeEmpty(); + + var none = new DriverHealthChanged("cluster-a", "drv-1", "Healthy", null, null, 0, DateTime.UtcNow); + var noneEvt = TelemetryProtoMapNode.ToProto(new TelemetryItem.Health(none), "c"); + noneEvt.DriverHealth.HasDiagnostics.ShouldBeFalse(); + noneEvt.DriverHealth.Diagnostics.ShouldBeEmpty(); + } + [Fact] public async Task Client_disconnect_mid_stream_ends_cleanly_without_leaking_a_slot() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorDiagnosticsTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorDiagnosticsTests.cs new file mode 100644 index 00000000..19b0ee39 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorDiagnosticsTests.cs @@ -0,0 +1,187 @@ +using Akka.Actor; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Runtime.Drivers; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; + +/// +/// Covers the consumer (Gitea #640). A diagnostic fact (S7's negotiated +/// PDU size) moves no other fingerprint component, so unless its digest is in +/// PublishHealthSnapshot's fingerprint the dedup swallows the very publish that carries it. +/// The stub's health is deliberately STABLE: the shared StubDriver.GetHealth() returns +/// UtcNow, so the dedup never engages and a test on it passes vacuously. +/// +[Trait("Category", "Unit")] +public sealed class DriverInstanceActorDiagnosticsTests : RuntimeActorTestBase +{ + /// Base case: the bag reaches the publisher. + [Fact] + public void Diagnostics_reach_the_health_publisher() + { + var driver = new DiagStubDriver(); + driver.Set(("S7.NegotiatedPduSize", "480")); + var publisher = new RecordingHealthPublisher(); + var actor = SpawnDriverActor(driver, publisher); + + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + + AwaitAssert(() => + { + var latest = publisher.Published.LastOrDefault(); + latest.ShouldNotBeNull(); + latest!["S7.NegotiatedPduSize"].ShouldBe("480"); + }, TimeSpan.FromSeconds(3)); + } + + /// + /// The load-bearing case. Falsifiability: publish count must STRICTLY increase across the change; + /// drop DiagnosticsDigest from the fingerprint and this goes red. + /// + [Fact] + public void A_diagnostics_change_republishes_despite_otherwise_unchanged_health() + { + var driver = new DiagStubDriver(); + driver.Set(("S7.NegotiatedPduSize", "240")); + var publisher = new RecordingHealthPublisher(); + var actor = SpawnDriverActor(driver, publisher); + + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(0), TimeSpan.FromSeconds(3)); + ExpectNoMsg(TimeSpan.FromMilliseconds(200)); + var before = publisher.Published.Count; + + driver.Set(("S7.NegotiatedPduSize", "480")); + + AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(before), TimeSpan.FromSeconds(3)); + publisher.Published[^1]!["S7.NegotiatedPduSize"].ShouldBe("480"); + } + + /// Equal-but-distinct bags must be deduped: the digest compares by VALUE, not reference. + [Fact] + public void An_unchanged_diagnostics_bag_does_not_defeat_the_dedup() + { + var driver = new DiagStubDriver(); + driver.Set(("S7.NegotiatedPduSize", "480")); + var publisher = new RecordingHealthPublisher(); + var actor = SpawnDriverActor(driver, publisher); + + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(0), TimeSpan.FromSeconds(3)); + ExpectNoMsg(TimeSpan.FromMilliseconds(200)); + var before = publisher.Published.Count; + + driver.Set(("S7.NegotiatedPduSize", "480")); + + ExpectNoMsg(TimeSpan.FromMilliseconds(600)); + publisher.Published.Count.ShouldBe(before); + } + + /// A driver without the capability publishes null, not an empty bag. + [Fact] + public void A_driver_without_the_capability_publishes_null() + { + var publisher = new RecordingHealthPublisher(); + var actor = SpawnDriverActor(new StubDriver(), publisher); + + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(0), TimeSpan.FromSeconds(3)); + + publisher.Published.ShouldAllBe(p => p == null); + } + + /// A throwing implementation loses only the diagnostics, not the health publish. + [Fact] + public void A_throwing_diagnostics_provider_degrades_to_null_without_killing_the_health_publish() + { + var driver = new DiagStubDriver { Throw = true }; + var publisher = new RecordingHealthPublisher(); + var actor = SpawnDriverActor(driver, publisher); + + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + + AwaitAssert(() => publisher.Published.Count.ShouldBeGreaterThan(0), TimeSpan.FromSeconds(3)); + publisher.Published[^1].ShouldBeNull(); + } + + /// The bag is capped and truncated before it is published. + [Fact] + public void Oversized_bags_are_bounded() + { + var driver = new DiagStubDriver(); + driver.Set([.. Enumerable.Range(0, 40).Select(i => ($"k{i:D2}", new string('x', 500)))]); + var publisher = new RecordingHealthPublisher(); + var actor = SpawnDriverActor(driver, publisher); + + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + + AwaitAssert(() => + { + var latest = publisher.Published.LastOrDefault(); + latest.ShouldNotBeNull(); + latest!.Count.ShouldBe(DriverDiagnostics.MaxEntries); + latest.Values.ShouldAllBe(v => v.Length == DriverDiagnostics.MaxValueLength); + }, TimeSpan.FromSeconds(3)); + } + + private IActorRef SpawnDriverActor(IDriver driver, IDriverHealthPublisher publisher) + { + var parent = CreateTestProbe(); + parent.IgnoreMessages(_ => true); + return parent.ChildActorOf(DriverInstanceActor.Props( + driver, healthPublisher: publisher, healthPollInterval: TimeSpan.FromMilliseconds(50))); + } + + private sealed class RecordingHealthPublisher : IDriverHealthPublisher + { + private readonly List?> _published = []; + + public IReadOnlyList?> Published + { + get { lock (_published) return _published.ToArray(); } + } + + public void Publish( + string clusterId, + string driverInstanceId, + DriverHealth health, + int errorCount5Min, + DateTime? rediscoveryNeededUtc = null, + string? rediscoveryReason = null, + IReadOnlyList? hostStatuses = null, + SkippedTagReport? skippedTags = null, + IReadOnlyDictionary? diagnostics = null) + { + lock (_published) _published.Add(diagnostics); + } + } + + /// Stable-health driver exposing . + private sealed class DiagStubDriver : IDriver, IDriverDiagnostics + { + private static readonly DateTime FixedLastRead = new(2026, 10, 8, 12, 0, 0, DateTimeKind.Utc); + private volatile IReadOnlyDictionary _bag = new Dictionary(); + + public bool Throw { get; init; } + + public string DriverInstanceId => "diag-stub-1"; + + public string DriverType => "Stub"; + + public void Set(params (string Key, string Value)[] items) => + _bag = items.ToDictionary(i => i.Key, i => i.Value); + + public IReadOnlyDictionary GetDiagnostics() => + Throw ? throw new InvalidOperationException("broken") : _bag; + + public Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) => Task.CompletedTask; + + public Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken) => Task.CompletedTask; + + public Task ShutdownAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + public DriverHealth GetHealth() => new(DriverState.Healthy, FixedLastRead, null); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorHostStatusTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorHostStatusTests.cs index f32d7cca..6c0d7e5f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorHostStatusTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorHostStatusTests.cs @@ -211,7 +211,8 @@ public sealed class DriverInstanceActorHostStatusTests : RuntimeActorTestBase DateTime? rediscoveryNeededUtc = null, string? rediscoveryReason = null, IReadOnlyList? hostStatuses = null, - SkippedTagReport? skippedTags = null) + SkippedTagReport? skippedTags = null, + IReadOnlyDictionary? diagnostics = null) { lock (_published) _published.Add(new HealthPublish(health, hostStatuses)); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorRediscoverySignalTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorRediscoverySignalTests.cs index cf39eb2b..00292ba5 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorRediscoverySignalTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorRediscoverySignalTests.cs @@ -187,7 +187,8 @@ public sealed class DriverInstanceActorRediscoverySignalTests : RuntimeActorTest DateTime? rediscoveryNeededUtc = null, string? rediscoveryReason = null, IReadOnlyList? hostStatuses = null, - SkippedTagReport? skippedTags = null) + SkippedTagReport? skippedTags = null, + IReadOnlyDictionary? diagnostics = null) { lock (_published) { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorSkippedTagTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorSkippedTagTests.cs index 090aa352..48729fa0 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorSkippedTagTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorSkippedTagTests.cs @@ -209,7 +209,8 @@ public sealed class DriverInstanceActorSkippedTagTests : RuntimeActorTestBase DateTime? rediscoveryNeededUtc = null, string? rediscoveryReason = null, IReadOnlyList? hostStatuses = null, - SkippedTagReport? skippedTags = null) + SkippedTagReport? skippedTags = null, + IReadOnlyDictionary? diagnostics = null) { lock (_published) _published.Add(new HealthPublish(health, skippedTags)); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorSubscriptionReconcileTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorSubscriptionReconcileTests.cs index f63e8765..126adeb1 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorSubscriptionReconcileTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorSubscriptionReconcileTests.cs @@ -124,7 +124,8 @@ public sealed class DriverInstanceActorSubscriptionReconcileTests : RuntimeActor DateTime? rediscoveryNeededUtc = null, string? rediscoveryReason = null, IReadOnlyList? hostStatuses = null, - SkippedTagReport? skippedTags = null) + SkippedTagReport? skippedTags = null, + IReadOnlyDictionary? diagnostics = null) => Interlocked.Increment(ref _count); } }