docs(code-review): add Driver.Historian.Gateway findings
Per-module review atb990adae(this branch's base): all 10 checklist categories completed, 3 findings recorded and resolved in the two prior commits on this branch (55b12dc2,a86a0976). Not run through regen-readme.py per the review task's instructions - the aggregated README.md is unchanged.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,239 @@
|
||||
# Code Review — Driver.Historian.Gateway
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway` |
|
||||
| Reviewer | Claude Code |
|
||||
| Review date | 2026-08-10 |
|
||||
| Commit reviewed | `b990adae` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Scope
|
||||
|
||||
The sole historian backend: a thin gRPC seam (`IHistorianGatewayClient` /
|
||||
`HistorianGatewayClientAdapter`) over the `ZB.MOM.WW.HistorianGateway.Client` package, plus
|
||||
`GatewayHistorianDataSource` (HistoryRead), `GatewayAlarmHistorianWriter` (alarm `SendEvent`),
|
||||
`GatewayTagProvisioner` (`EnsureTags` upsert), the `Mapping/` pure mappers, and the
|
||||
`Recorder/` continuous-historization write path (`FasterLogHistorizationOutbox`,
|
||||
`GatewayHistorianValueWriter`, `HistorizationOutboxEntrySerializer`). 17 source files, ~1,850 LOC
|
||||
(excluding generated/obj); test suite `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests`
|
||||
(130 tests: 124 offline unit + 6 env-gated `Category=LiveIntegration`, skip cleanly offline).
|
||||
|
||||
Read against `CLAUDE.md`'s "Historian / HistoryRead" section in full before reviewing — several
|
||||
things that look like defects on first read are deliberate, documented decisions (Boolean→Int2,
|
||||
the dead `Quality` field on `WriteLiveValues`, the per-path-own-channel design, the two `EnsureTags`
|
||||
gotchas, the #491/#537/#538 live-gate history). None of those were re-litigated here.
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
| # | Category | Result |
|
||||
|---|---|---|
|
||||
| 1 | Correctness & logic bugs | 1 finding (Driver.Historian.Gateway-001, High) — Resolved |
|
||||
| 2 | OtOpcUa conventions | No issues found |
|
||||
| 3 | Concurrency & thread safety | No issues found |
|
||||
| 4 | Error handling & resilience | No issues found |
|
||||
| 5 | Security | No issues found |
|
||||
| 6 | Performance & resource management | 1 finding (Driver.Historian.Gateway-002, Low) — Resolved |
|
||||
| 7 | Design-document adherence | No issues found |
|
||||
| 8 | Code organization & conventions | No issues found |
|
||||
| 9 | Testing coverage | 1 finding (Driver.Historian.Gateway-003, Medium) — Resolved |
|
||||
| 10 | Documentation & comments | No issues found |
|
||||
|
||||
### Category notes
|
||||
|
||||
**1. Correctness & logic bugs.** See Driver.Historian.Gateway-001 (Severity write/read mismatch).
|
||||
Everything else checked out: `SampleMapper`/`GatewayQualityMapper` quality-byte bucketing is exact
|
||||
and tested per-byte; `AggregateModeMapper`/`HistorianTypeMapper` are exhaustive-matrix-guarded
|
||||
(`Every_..._member_is_classified` tests fail on a future unmapped enum member rather than silently
|
||||
defaulting); `GatewayReadEventsTests` confirms the truncation/continuation-point boundary
|
||||
(`collected.Count == maxEvents` checked *before* `Add`, so an exact-cap read has no continuation but
|
||||
one more matching event sets it); `AlignAtTimeSnapshots` correctly keeps the first of a duplicate
|
||||
timestamp and Bad-fills gaps at the *requested* time, not the nearest returned one.
|
||||
|
||||
**2. OtOpcUa conventions.** Driver-level code contains no OPC UA SDK reference (`uint` status codes
|
||||
via the named `HistorianStatusCodes`, not the SDK's `Opc.Ua.StatusCodes` — matches the repo's stated
|
||||
pattern of keeping the driver layer SDK-free so `StatusCodeParityTests` elsewhere can reflect-guard
|
||||
it). No authorization decisions are made in this driver — HistoryRead access-level gating happens at
|
||||
materialization in the server layer per CLAUDE.md, and this module is a pure translation seam.
|
||||
Serilog-style `ILogger<T>` used throughout via constructor injection.
|
||||
|
||||
**3. Concurrency & thread safety.** `GatewayHistorianDataSource`'s health counters
|
||||
(`_totalQueries`/`_totalSuccesses`/`_totalFailures`/etc.) are all mutated and read under one
|
||||
`_healthLock`, preserving the `TotalQueries == TotalSuccesses + TotalFailures` invariant at every
|
||||
observed snapshot (per its own XML remarks and `GatewayHealthSnapshotTests`).
|
||||
`FasterLogHistorizationOutbox` documents an explicit single-writer assumption (the recorder actor
|
||||
processes messages sequentially) and its `_state` lock protects only the in-memory FIFO index, never
|
||||
held across an `await` — correct, since FasterLog itself tolerates concurrent enqueue/scan. No shared
|
||||
mutable state was found that lacked a lock or a documented ownership model.
|
||||
|
||||
**4. Error handling & resilience.** `GatewayAlarmHistorianWriter` and `GatewayHistorianValueWriter`
|
||||
both honor their "never throws" contracts (verified by dedicated exception-classification tests
|
||||
covering the typed client hierarchy, a defensive raw `RpcException` path, and an unclassifiable
|
||||
exception default). Cancellation is handled as a first-class, non-poisoning case in both the alarm
|
||||
writer (mid-drain cancellation yields `RetryPlease`, never `PermanentFail`) and the tag provisioner
|
||||
(a shutdown `OperationCanceledException` is logged quietly and counted `Failed`, never thrown). The
|
||||
fire-and-forget `EnsureTagsAsync` hook is genuinely non-blocking end to end — no `await` in its
|
||||
caller's synchronous path can escape as an exception (verified by tracing `AddressSpaceApplier`'s
|
||||
fire-and-forget dispatch, outside this module's scope but confirmed non-blocking here).
|
||||
|
||||
**5. Security.** No API key, tag value, hostname, or credential is ever passed to a logger call
|
||||
anywhere in the module (audited every `_logger.Log*` call site) — failures are logged by exception
|
||||
*type name* only. `HistorianGatewayClientAdapter.Create` echoes the `Endpoint` in its
|
||||
`InvalidOperationException` (deliberately, documented as non-secret) but never the `ApiKey`. TLS /
|
||||
certificate-validation options (`UseTls`, `AllowUntrustedServerCertificate`, `CaCertificatePath`) map
|
||||
onto the package client with the documented inverted `RequireCertificateValidation` semantics and are
|
||||
correctly *not* forwarded on a plaintext h2c connection (pinned by
|
||||
`Adapter_ignores_tls_only_settings_on_a_plaintext_endpoint`). No SQL or other injectable surface in
|
||||
this module.
|
||||
|
||||
**6. Performance & resource management.** See Driver.Historian.Gateway-002 (missing dispose on
|
||||
`GatewayTagProvisioner`). Everything else: `GatewayHistorianDataSource` and
|
||||
`GatewayAlarmHistorianWriter` both correctly dispose their owned `IHistorianGatewayClient` (verified
|
||||
by dedicated tests); the three `GatewayHistorian.Create*` factories deliberately open independent
|
||||
gRPC channels per path rather than sharing one, a documented and reasonable trade (each channel's
|
||||
lifetime stays simple; the gateway pools/amortizes historian sessions server-side). Channel
|
||||
construction is lazy (no network I/O at construction, confirmed by the adapter's own tests) so a
|
||||
misconfigured/disabled path never dials. Per-call deadlines exist via
|
||||
`ServerHistorianOptions.CallTimeout` → `HistorianGatewayClientOptions.DefaultCallTimeout`, and every
|
||||
seam method threads a `CancellationToken` through to the package client — no unbounded call was found.
|
||||
|
||||
**7. Design-document adherence.** Matches `CLAUDE.md`'s "Historian / HistoryRead" section point for
|
||||
point: gateway is the sole backend; Boolean→Int2 is implemented exactly as documented (with an
|
||||
explicit-presence pin test guarding the proto3-optional contract); the continuous-historization
|
||||
outbox/writer/serializer shape matches the FasterLog-outbox design; the `Quality` field is
|
||||
deliberately populated despite being a documented-dead input on the SQL write path, with the
|
||||
reasoning captured in the class's own remarks (proto3 default-0 would be a worse lie than the
|
||||
correct-but-ignored 192). No stale documentation was found in this module.
|
||||
|
||||
**8. Code organization & conventions.** Clean separation: `Mapping/` holds pure, stateless,
|
||||
internal-visibility mappers (each independently unit-tested); `Recorder/` holds the
|
||||
continuous-historization write path; the top-level files are the three public seams
|
||||
(`IHistorianGatewayClient`, the adapter, and the `GatewayHistorian` factory facade the Host consumes).
|
||||
`InternalsVisibleTo` is scoped to the test project only. The `ServerHistorianOptions` type living in
|
||||
`Runtime` rather than this driver (creating the documented Host→{Runtime, Gateway}, Gateway→Runtime
|
||||
diamond) is explained inline in the `.csproj` and is a reasonable ownership call, not a layering
|
||||
violation.
|
||||
|
||||
**9. Testing coverage.** See Driver.Historian.Gateway-003 (two offline-coverage gaps, now closed).
|
||||
Otherwise the suite is thorough: every mapper has a matrix-guard test for unmapped enum members,
|
||||
every writer's outcome-classification switch is covered by every named branch (including the
|
||||
FIFO-truncation semantics of the FasterLog outbox — append/peek/remove/restart-durability/capacity
|
||||
drop-oldest/periodic-commit-recovery all have dedicated tests), and the six env-gated
|
||||
`Category=LiveIntegration` tests (skip cleanly without `HISTGW_*` env vars, confirmed offline) cover
|
||||
the write/read/alarm/EnsureTags round trips against a real gateway per `docs/Historian.md`'s
|
||||
re-run recipe.
|
||||
|
||||
**10. Documentation & comments.** XML docs are extensive, accurate against the code they describe,
|
||||
and several carry load-bearing historical context (e.g. why each `GatewayHistorian.Create*` factory
|
||||
opens its own channel, why `Quality` is populated despite being server-ignored, why
|
||||
`HistorianEvent.Id` is deliberately left unset). No misleading or stale comment was found.
|
||||
|
||||
## Findings
|
||||
|
||||
### Driver.Historian.Gateway-001
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | High |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Mapping/AlarmEventMapper.cs:39` (write side) and `Mapping/EventMapper.cs:62` (read side) |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `AlarmEventMapper.ToHistorianEvent` wrote the alarm's `Severity` wire property as
|
||||
`alarm.Severity.ToString()` — the .NET enum's *name* (`"Low"`/`"Medium"`/`"High"`/`"Critical"`).
|
||||
`AlarmHistorianEvent.Severity`'s own XML doc states the contract: "Mapped to Historian's numeric
|
||||
priority on the sink side." The read side, `EventMapper.ParseSeverity`, `int.TryParse`s that exact
|
||||
property and falls back to the minimum severity (1) on any unparseable value — which an enum name
|
||||
always is. So round-tripping any alarm event's severity through this module's own write mapper then
|
||||
its own read mapper silently collapsed every severity to the minimum, regardless of the real
|
||||
severity: a `Critical` alarm and a `Low` alarm would read back identically as severity 1. This is a
|
||||
self-contained defect (both mappers live in this module) independent of whether the gateway's own
|
||||
storage happens to echo arbitrary custom event properties back on `ReadEvents` — the write and read
|
||||
mappers disagreed with each other and with their own documented contract, and the existing test
|
||||
(`AlarmEventMapperTests.Maps_source_time_type_and_rich_properties`) asserted the *wrong* value
|
||||
(`"High"`) as correct, so no test caught it.
|
||||
|
||||
**Recommendation:** Map `AlarmSeverity` to a numeric OPC UA Part 9 priority string before writing it
|
||||
to the wire property, using the same severity convention already established elsewhere in the
|
||||
codebase for this exact enum (`ZB.MOM.WW.OtOpcUa.Runtime.Drivers.NativeAlarmProjector.MapSeverity`:
|
||||
Low=200, Medium=500, High=700, Critical=900).
|
||||
|
||||
**Resolution:** `55b12dc2` (branch `review/historian-gateway`), 2026-08-10. Added a private
|
||||
`AlarmEventMapper.ToNumericSeverity` mirroring `NativeAlarmProjector.MapSeverity`'s convention and
|
||||
routed the `Severity` property through it instead of `alarm.Severity.ToString()`. TDD: added
|
||||
`AlarmEventMapperTests.Severity_round_trips_through_EventMapper_as_numeric_priority` (a
|
||||
`[Theory]` over all four `AlarmSeverity` values, round-tripping through both mappers) and corrected
|
||||
the existing assertion to `"700"`; both were RED against the pre-fix code (confirmed by running the
|
||||
suite before the source change) and are GREEN after it. All 130 module tests pass
|
||||
(124 run + 6 skipped `LiveIntegration`).
|
||||
|
||||
### Driver.Historian.Gateway-002
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | Performance & resource management |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayTagProvisioner.cs` |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Unlike its two siblings — `GatewayHistorianDataSource` (`IAsyncDisposable`,
|
||||
disposes its `IHistorianGatewayClient`) and `GatewayAlarmHistorianWriter` (`IDisposable`, same) —
|
||||
`GatewayTagProvisioner` implemented neither `IDisposable` nor `IAsyncDisposable`, so the gRPC channel
|
||||
`GatewayHistorian.CreateProvisioner` opens for it was never closed. `IHistorianProvisioning` (the
|
||||
interface it's registered as) does not declare disposal either, so nothing in the codebase had any
|
||||
way to close that channel. Impact is bounded — `AddHistorianProvisioning` registers it as a single
|
||||
process-lifetime DI singleton (built once at startup, not per-deploy), so this was not an
|
||||
accumulating leak — but it is a real gap in graceful-shutdown hygiene and an inconsistency with the
|
||||
established pattern in this same module.
|
||||
|
||||
**Recommendation:** Implement `IAsyncDisposable` on `GatewayTagProvisioner`, disposing the
|
||||
underlying `IHistorianGatewayClient` — the built-in `Microsoft.Extensions.DependencyInjection`
|
||||
container tracks and disposes any singleton instance it constructs that implements
|
||||
`IDisposable`/`IAsyncDisposable`, regardless of the declared service-interface type, so no other
|
||||
wiring change is required.
|
||||
|
||||
**Resolution:** `55b12dc2` (branch `review/historian-gateway`), 2026-08-10. Added
|
||||
`GatewayTagProvisioner : IHistorianProvisioning, IAsyncDisposable` with a `DisposeAsync()` delegating
|
||||
to `_client.DisposeAsync()`. TDD: added
|
||||
`GatewayTagProvisionerTests.DisposeAsync_disposes_underlying_client`, confirmed RED (compile error —
|
||||
`GatewayTagProvisioner` did not implement `IAsyncDisposable`) before the fix, GREEN after.
|
||||
|
||||
### Driver.Historian.Gateway-003
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Medium |
|
||||
| Category | Testing coverage |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Recorder/GatewayHistorianValueWriter.cs`, `Recorder/HistorizationOutboxEntrySerializer.cs` |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Two production-critical pieces of the continuous-historization write path had no
|
||||
offline unit coverage:
|
||||
|
||||
- `GatewayHistorianValueWriter` — the recorder's `WriteLiveValues` adapter, and per CLAUDE.md's
|
||||
"KNOWN LIMITATION 2" history the exact class where the #491 `ConfigureAwait(false)`-destroys-`Self`
|
||||
defect and the #537/#538 gate findings were discovered live — was exercised only by the env-gated
|
||||
`Category=LiveIntegration` suite (`tests/.../Live/GatewayLiveIntegrationTests.cs`), which skips
|
||||
cleanly offline. Its ack-to-bool mapping (`Success || Queued`), timestamp-presence handling (a null
|
||||
timestamp must leave the proto field unset, not send a default), and non-throwing exception
|
||||
contract had zero offline verification.
|
||||
- `HistorizationOutboxEntrySerializer` — a hand-rolled fixed-layout little-endian binary
|
||||
(de)serializer with manual span-offset arithmetic — was only exercised *indirectly* through
|
||||
`FasterLogHistorizationOutboxTests`, and only ever with single-character ASCII tag names ("A"/"B"/
|
||||
"C"), never testing the one part of the layout arithmetic (the UTF-8 *byte*-length prefix vs. the
|
||||
tag's *char* length) that a multi-byte tag name would exercise.
|
||||
|
||||
**Recommendation:** Add direct offline unit tests for `GatewayHistorianValueWriter` (reusing the
|
||||
existing `FakeHistorianGatewayClient` test double already used by every other seam consumer in this
|
||||
module) and for `HistorizationOutboxEntrySerializer` (direct Serialize/Deserialize round trips,
|
||||
including a multi-byte UTF-8 tag name, an empty tag, and boundary `ushort`/`double` values).
|
||||
|
||||
**Resolution:** `a86a0976` (branch `review/historian-gateway`), 2026-08-10. Added
|
||||
`tests/.../Recorder/GatewayHistorianValueWriterTests.cs` (10 tests: success/queued/soft-failure ack
|
||||
mapping, exception swallowing, empty-batch short-circuit, timestamp presence/absence, quality/value
|
||||
forwarding, null-argument guards, dispose delegation) and
|
||||
`tests/.../Recorder/HistorizationOutboxEntrySerializerTests.cs` (4 tests: full round trip incl.
|
||||
`DateTimeKind` preservation, a multi-byte UTF-8 tag `"Pumpe_Ω_日本語_Temp"`, an empty tag, and
|
||||
`double.MinValue`/`ushort.MaxValue` boundaries). All 14 new tests pass; full module suite is
|
||||
124 passed / 6 skipped (LiveIntegration) / 0 failed.
|
||||
Reference in New Issue
Block a user