scadaproj/code-reviews/Telemetry/findings.md

Code Review — Telemetry

Field	Value
Library	ZB.MOM.WW.Telemetry/
Packages	ZB.MOM.WW.Telemetry, ZB.MOM.WW.Telemetry.Serilog
Component spec	components/observability/spec/SPEC.md
Shared contract	components/observability/shared-contract/ZB.MOM.WW.Telemetry.md
Status	Reviewed
Last reviewed	2026-11-09 (re-review; baseline 2026-06-01 at 5f75cd4)
Reviewer	Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated)
Commit reviewed	91fb2b4
Open findings	0

Summary

2026-11-09 re-review (91fb2b4, scadaproj#15). Re-reviewed at 91fb2b4 (2026-11-09) against the 2026-06-01 baseline 5f75cd4. Source is at 0.2.0, which is built but not published; the feed still carries 0.1.0. Changes since the baseline:

• nested redaction (05cc62a): RedactionEnricher projects StructureValue/SequenceValue/ DictionaryValue into mutable views, honours removals, and writes back only what changed;
• one canonical Resource-attribute map (ZbResource.BuildAttributes) shared by the OTel SDK and the Serilog OTLP sink;
• an eager OTLP options validator;
• the 0.2.0 ZbSerilogOptions hooks (MinimumLevelGovernor, ConfigureLogger).

The baseline redaction fixes (Telemetry-001/002/003) hold: removal and nested masking work, and are tested. The Resource map is genuinely single-sourced. The DI extensions have container-building tests (AddZbTelemetryTests, MultiHostTests, MinimumLevelGovernanceTests). All 42 tests pass (Serilog 30, core 12).

There are three new Low findings, each verified with a scratchpad probe:

• Telemetry-009: enrichers added through the new ConfigureLogger hook run after RedactionEnricher, so properties they add are never redacted.
• Telemetry-010: the redaction projection keys logged dictionaries by ToString(). Distinct keys that stringify the same, such as DateTime keys differing only by milliseconds, collapse, and an entry is silently dropped from the log even when the redactor changes nothing.
• Telemetry-011: the Telemetry-006 fail-fast validator accepts a scheme-less host:port endpoint as an "absolute URI", and the pre-built-options AddZbTelemetry(IServiceCollection, ZbTelemetryOptions) overload skips validation entirely.

2026-06-01 baseline (5f75cd4). The library is small, focused, and well-structured: two packages with a clean Serilog/OTel boundary (the Serilog.* stack appears only in the .Serilog package; the core package is pure OTel + ASP.NET Core framework reference), correct argument validation, deliberate sealed types, thorough XML docs, and a deliberate no-process-global-state design for AddZbSerilog that is well covered by MultiHostTests. The identity triple, Resource omission rules, exporter wiring (Prometheus always-on, OTLP additive), and trace/log correlation all match the spec's intent and are exercised by the 19 tests.

The most material problems are in the redaction seam — the one component the review brief flags as security-critical. RedactionEnricher honours only replacement of scalar properties: it silently ignores the redactor removing a key (a documented capability of ILogRedactor), and it cannot see inside destructured/structured property values, so a secret logged as a field of {@Object} is never scrubbed. Both let secrets reach sinks despite a conforming redactor. Secondary themes: a spec drift around an undocumented service.instance.id Resource attribute, two hand-maintained Resource-attribute builders that can drift apart, and a stale doc-comment on MapZbMetrics. Tests are solid for the happy paths but have no coverage for redactor removal or structured-value redaction.

Checklist coverage

2026-11-09 re-review at 91fb2b4:

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	Nested projection/rebuild, removal reconciliation and change detection (ValueEquals) are correct, except that dictionary keys collide on ToString() → Telemetry-010. Governor ordering (after ReadFrom.Configuration) works as documented.
2	Public API surface & compatibility	☑	Additive: ZbResource.BuildAttributes (public), the ZbSerilogOptions overload, and InternalsVisibleTo for the Serilog package. The options-object AddZbTelemetry overload skips the new validation → Telemetry-011.
3	Concurrency & thread safety	☑	Enricher state is a Lazy with ExecutionAndPublication, and projection is per event. No issues found.
4	Error handling & resilience	☑	The validator gives named ArgumentExceptions, but its absolute-URI test is too permissive and is bypassed by one overload → Telemetry-011.
5	Security & secret handling	☑	Redaction reach is now top-level plus nested, with removal honoured. Properties added by ConfigureLogger enrichers bypass redaction → Telemetry-009. Exception text remains outside the redactor seam, by design and unchanged.
6	Performance & resource management	☑	The per-event projection allocates only when a redactor is registered and the event has properties. Untouched properties are not reallocated. No issues found.
7	Spec & shared-contract adherence	☑	Resource attributes (service.*, host.name, site.id, node.role) are identical across all three signals via BuildAttributes. No issues found.
8	Packaging, dependencies & project layout	☑	Version 0.2.0; repository URLs corrected to scadaproj; the Serilog stack is still confined to .Serilog. No issues found.
9	Testing coverage	☑	42 tests pass, including removal and nested redaction and governor precedence. Untested: enricher ordering against ConfigureLogger (Telemetry-009), colliding dictionary keys (Telemetry-010), a scheme-less OTLP endpoint (Telemetry-011).
10	Documentation & XML docs	☑	The ILogRedactor/README redaction-reach text is accurate. ConfigureLogger docs advertise "extra enrichers" without saying they run after redaction (Telemetry-009).

2026-06-01 baseline checklist (5f75cd4)

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	Redactor "remove" path is a no-op (Telemetry-001); structured values opaque to redactor (Telemetry-002).
2	Public API surface & compatibility	☑	Surface minimal, sealed, nullable-correct. ZbResource.InstanceId is an added public member not in the contract (Telemetry-004).
3	Concurrency & thread safety	☑	No issues found. Enrichers stateless; Lazy uses ExecutionAndPublication; Activity.Current is async-local.
4	Error handling & resilience	☑	Guard clauses present. new Uri(OtlpEndpoint) can throw late on malformed input (Telemetry-006).
5	Security & secret handling	☑	Redaction gaps (Telemetry-001/002) are security-relevant — secrets can survive a conforming redactor.
6	Performance & resource management	☑	Per-event dictionary snapshot when a redactor is registered (Telemetry-007); acceptable but noted.
7	Spec & shared-contract adherence	☑	Undocumented service.instance.id attribute (Telemetry-004); two Resource builders that can drift (Telemetry-005).
8	Packaging, dependencies & project layout	☑	No issues found. Serilog stack confined to .Serilog; central versions; correct net10.0; framework ref justified.
9	Testing coverage	☑	No tests for redactor removal or structured-value redaction (Telemetry-003).
10	Documentation & XML docs	☑	MapZbMetrics doc-comment is stale: claims "only valid when Exporter = Prometheus" (Telemetry-008).

Findings

Telemetry-001 — RedactionEnricher ignores property removal, leaving secrets in the event

Severity	High
Category	Security & secret handling
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs:49-67

Description

ILogRedactor.Redact is documented to let a project "remove or replace any sensitive values" (ILogRedactor.cs:13 and the XML doc on the interface method: "remove or replace"; the shared contract repeats "Remove or replace any sensitive values"). RedactionEnricher builds a snapshot dictionary, hands it to the redactor, then writes back only via AddOrUpdateProperty for entries that remain in the snapshot and HasChanged:

foreach (var entry in snapshot)
{
    if (HasChanged(logEvent, entry.Key, entry.Value))
        logEvent.AddOrUpdateProperty(propertyFactory.CreateProperty(entry.Key, entry.Value));
}

If a redactor removes a key from the dictionary (properties.Remove("apiKey")) — the most natural way to implement "must not leave the process" — that key simply no longer appears in the write-back loop, so the original property is never removed from logEvent. The secret reaches every sink unredacted, even though the redactor did exactly what its contract permits. This defeats the seam's stated operational guarantee ("secrets never leave the process in log events") for any removal-style redactor.

Recommendation

After calling the redactor, reconcile deletions: for each property key present on the original logEvent but absent from the returned snapshot, call logEvent.RemovePropertyIfPresent(key). (Capture the original key set before mutation, then diff.) Add a test asserting a removing redactor scrubs the property (see Telemetry-003).

Resolution

Resolved in 544a6dd, 2026-06-01 — RedactionEnricher now captures the original property key set and calls RemovePropertyIfPresent for any key the redactor dropped from the snapshot, so a removing redactor scrubs the property; covered by a new removing-redactor test.

Telemetry-002 — Redactor cannot inspect or scrub destructured/structured property values

Severity	Medium
Category	Security & secret handling
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs:49-55

Description

The snapshot only unwraps ScalarValue; every other LogEventPropertyValue (StructureValue from {@Object}, SequenceValue, DictionaryValue) is passed to the redactor as the raw Serilog wrapper object:

snapshot[property.Key] = property.Value is ScalarValue scalar ? scalar.Value : property.Value;

A project redactor written against the seam (IDictionary<string, object?> of "values") therefore sees an opaque StructureValue for a destructured payload — it cannot read or mask a secret field inside a logged object (e.g. logger.Information("{@Command}", cmd) where cmd.ApiKey is sensitive). MxGateway's reference redactor specifically guards "which command payloads must not leave the process" (per ILogRedactor XML doc and the contract), which is precisely the destructured-object case. The seam silently cannot meet that requirement; the redactor only works for top-level scalar properties.

Recommendation

Document the seam's actual reach (scalar top-level properties only) on ILogRedactor and in the shared contract, and/or recursively project StructureValue/SequenceValue/ DictionaryValue into the snapshot and rebuild them on write-back so nested fields are redactable. At minimum, make the limitation explicit so consumers do not assume nested payloads are scrubbed when they are not.

Resolution

Resolved in 544a6dd, 2026-06-01 (documented the scalar-only limitation), then superseded by 05cc62a, 2026-06-01 — nested redaction implemented. RedactionEnricher now projects each structured value into a mutable nested view the redactor descends into recursively (StructureValue → IDictionary<string,object?>, SequenceValue → IList<object?>, DictionaryValue → IDictionary<string,object?>), so a field nested inside a {@Object} can be masked or removed. The Project/Rebuild round-trip preserves StructureValue.TypeTag and original dictionary keys, and a structural ValueEquals skips write-back for properties the redactor left untouched (no reallocation; scalar fast path retained). The earlier documented-limitation wording on the ILogRedactor XML doc, shared contract, and README was replaced to document the recursive reach.

Telemetry-003 — No tests for redactor removal or structured-value redaction

Severity	Medium
Category	Testing coverage
Status	Resolved
Location	ZB.MOM.WW.Telemetry/tests/ZB.MOM.WW.Telemetry.Serilog.Tests/RedactionTests.cs:33-69

Description

RedactionTests covers exactly two redaction behaviours: a registered redactor replacing a scalar value, and a no-op when none is registered. The FakeRedactor only ever reassigns properties["apiKey"]. There is no test that a redactor which removes a key actually scrubs it (the Telemetry-001 defect would have been caught), and no test that a redactor can mask a field of a destructured/structured property (Telemetry-002). For a seam whose entire purpose is secret containment, the most security-relevant behaviours are untested.

Recommendation

Add tests: (a) a redactor calling properties.Remove(key) results in the property being absent from the emitted LogEvent; (b) a redactor attempting to mask a nested field of a {@Object} payload, asserting the documented behaviour (whichever resolution Telemetry-002 takes). These should fail today and pin the fixes.

Resolution

Resolved in 544a6dd, 2026-06-01, then extended in 05cc62a — added Removing_redactor_scrubs_the_property_from_the_event (red→green for Telemetry-001), a Resource-attribute parity test, and (for the Telemetry-002 implementation) a nested-reach suite: mask and remove a field inside a destructured {@Object}, mask a sequence element, mask a dictionary value, mask a field two levels deep, and an untouched-structure-survives check. The earlier Redactor_cannot_reach_a_field_inside_a_destructured_object limitation test was replaced.

Telemetry-004 — service.instance.id Resource attribute is undocumented in spec and contract

Severity	Low
Category	Spec & shared-contract adherence
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbResource.cs:19-45

Description

ZbResource adds a service.instance.id attribute (deterministic MachineName:ProcessId) to the Resource, and exposes it as a new public member ZbResource.InstanceId. The normalized Resource attribute set is enumerated exhaustively in two authoritative docs — SPEC.md §2 and METRIC-CONVENTIONS.md §4 — and neither lists service.instance.id; the shared contract (ZB.MOM.WW.Telemetry.md) likewise documents ZbResource.Build as populating only service.name/namespace/version/site.id/node.role/host.name and does not mention an InstanceId member. The attribute itself is a reasonable, standards-aligned improvement (and disabling the OTel SDK's random-GUID default is sensible for cross-signal correlation), but it is a silent divergence: the spec/contract are now stale relative to the code. Per REVIEW-PROCESS §2.7, both directions of drift must be flagged.

Recommendation

Add service.instance.id (with the MachineName:ProcessId rationale) to the Resource table in SPEC.md §2 and METRIC-CONVENTIONS.md §4, and document the public ZbResource.InstanceId member in the shared contract, so the normalized spec and the code agree.

Resolution

Resolved in 544a6dd, 2026-06-01 — kept the attribute (documented the MachineName:ProcessId rationale) and added service.instance.id to the Resource tables in SPEC.md §2 and METRIC-CONVENTIONS.md §4, plus the ZbResource.InstanceId member to the shared contract; spec and code now agree.

Telemetry-005 — Two hand-maintained Resource-attribute builders can silently drift

Severity	Low
Category	Spec & shared-contract adherence
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbResource.cs:38-64, ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogConfig.cs:125-151

Description

The Resource attached to metrics/traces is built by ZbResource.Configure (via the OTel AddService + AddAttributes API), while the Resource attached to the OTLP log sink is built independently by ZbSerilogConfig.BuildResourceAttributes (a hand-rolled Dictionary<string, object>). The two currently agree, but they enumerate the same six/seven attributes in two places with two different mechanisms, so a future change to one (a new attribute, a renamed key, a changed omission rule) will silently desynchronize logs from metrics/traces and break the cross-signal correlation the library's whole "unifying hinge" depends on. There is no test asserting parity between the two attribute sets.

Recommendation

Derive both from a single source of truth — e.g. have ZbResource expose the canonical attribute map (already mostly the shape BuildResourceAttributes returns) and have the Serilog sink consume it — or add a parity test that asserts the two attribute sets are key-for-key identical for a representative options object.

Resolution

Resolved in 544a6dd, 2026-06-01 — introduced ZbResource.BuildAttributes as the single source of truth; ZbResource.Configure (OTel SDK) and ZbSerilogConfig.BuildResourceAttributes (OTLP log sink) now both derive from it, and a parity test asserts the two sets are identical.

Telemetry-006 — Malformed OtlpEndpoint throws UriFormatException late, with no context

Severity	Low
Category	Error handling & resilience
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryExtensions.cs:127-135

Description

ConfigureOtlp does otlp.Endpoint = new Uri(options.OtlpEndpoint) with no validation. A malformed endpoint string (typo, missing scheme) throws a raw UriFormatException deep inside OTel exporter construction at host-build time, with no mention of which option was at fault. BuildOptions already fails fast and clearly for a missing ServiceName, but does not validate that OtlpEndpoint is a well-formed absolute URI when Exporter == Otlp (nor that it is non-empty — an Otlp exporter with a null endpoint is silently registered and points nowhere). The Serilog path (ZbSerilogConfig) has the same untyped string→endpoint handoff.

Recommendation

In BuildOptions, when Exporter == ZbExporter.Otlp, validate OtlpEndpoint with Uri.TryCreate(..., UriKind.Absolute, out _) and throw an ArgumentException naming the option (consistent with the existing ServiceName guard) rather than letting a bare UriFormatException escape later.

Resolution

Resolved in 544a6dd, 2026-06-01 — added ZbTelemetryOptionsValidator.Validate, called from both BuildOptions and AddZbSerilog: when Exporter == Otlp it requires a non-empty, well-formed absolute OtlpEndpoint and throws a named ArgumentException (no-op for Prometheus); covered by three new tests.

Telemetry-007 — Redaction snapshot allocates a dictionary on every log event

Severity	Low
Category	Performance & resource management
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs:49-67

Description

When an ILogRedactor is registered, Enrich allocates a new Dictionary<string, object?>(logEvent.Properties.Count), copies every property into it, and then iterates again to diff/write-back — on every single log event, across every logging thread. Enrichers are on the hottest path in the library (they run for each event the level filter admits). The early-return when no redactor is registered keeps the common case free, so the cost is borne only by redaction-enabled consumers (MxGateway), but for a high-volume gateway this is non-trivial steady-state allocation/GC pressure.

Recommendation

Consider redacting in place against logEvent.Properties without a full snapshot copy (e.g. only materialize replacements for keys the redactor touches), or short-circuit when the event has no properties. At minimum, document the per-event cost so consumers can weigh enabling redaction on very hot loggers. Acceptable as-is given redaction is opt-in and security-first.

Resolution

Resolved in 544a6dd, 2026-06-01 — Enrich now short-circuits before any snapshot allocation when the event has no properties (and still early-returns when no redactor is registered), so the per-event dictionary copy is only paid when there is actually something to redact.

Telemetry-008 — MapZbMetrics XML doc claims it is "only valid when Exporter = Prometheus" — stale

Severity	Low
Category	Documentation & XML docs
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbMetricsEndpointExtensions.cs:11-14

Description

The XML doc on MapZbMetrics states it is "Only valid when ZbTelemetryOptions.Exporter = ZbExporter.Prometheus." That contradicts the actual wiring: ApplyMetricsExporter (ZbTelemetryExtensions.cs:107-116) always calls AddPrometheusExporter() regardless of the Exporter setting — OTLP is purely additive. The library's own README ("Prometheus is always wired for metrics regardless of the Exporter setting") and the test AddZbTelemetry_OtlpExporter_StillServesPrometheusEndpoint both confirm /metrics works under Exporter = Otlp. The doc-comment therefore tells consumers the opposite of the real (and intended) behaviour and could lead them to wrongly believe MapZbMetrics is a no-op under OTLP. The same stale wording is mirrored in the shared contract (ZB.MOM.WW.Telemetry.md, MapZbMetrics summary).

Recommendation

Update the doc-comment to state that the Prometheus exporter is always registered and MapZbMetrics is valid under any Exporter value (Prometheus is always-on; OTLP is an overlay). Align the shared-contract summary for MapZbMetrics to match.

Resolution

Resolved in 544a6dd, 2026-06-01 — rewrote the MapZbMetrics XML doc to state it is valid under any Exporter value (Prometheus always-on; OTLP additive overlay) and aligned the matching shared-contract summary.

Telemetry-009 — Enrichers added via ZbSerilogOptions.ConfigureLogger run after RedactionEnricher, so their properties are never redacted

Severity	Low
Priority	P3
Category	Security & secret handling
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogExtensions.cs:118-120, ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogOptions.cs:41-46

Description

AddZbSerilogCore calls ZbSerilogConfig.Apply(…), which registers RedactionEnricher, and only then invokes serilogOptions.ConfigureLogger. Serilog runs enrichers in registration order, so any lc.Enrich.With(...) or Enrich.WithProperty(...) added in the hook stamps its properties after redaction has already run. They reach every sink unredacted. The ConfigureLogger XML doc and the README advertise this hook as the place for "extra enrichers" and say it runs "after … the shared enricher/redaction/export set". They never say this means its enrichers are outside the redaction guarantee that ILogRedactor documents ("Called on every log event before it reaches any sink"). Enrichers that come from ReadFrom.Configuration (for example FromLogContext) are unaffected, because they are registered before redaction.

Failure scenario

MxGateway, which registers an ILogRedactor that masks apiKey, moves to the 0.2.0 overload and adds s.ConfigureLogger = (lc, sp) => lc.Enrich.With(new RequestHeadersEnricher(sp)), which stamps a ClientApiKey or apiKey property. Every log event now carries the raw key to the file and OTLP sinks. A scratchpad probe reproduced it: a LoggerConfiguration with RedactionEnricher (redactor masks apiKey) followed by Enrich.WithProperty("apiKey","s3cret") emitted apiKey="s3cret". Today this is latent: no family app uses the ConfigureLogger hook yet.

Recommendation

Register RedactionEnricher as the final stage. One way is to invoke ConfigureLogger before ZbSerilogConfig.Apply's redaction step. Another is to apply redaction through a wrapping sink or the last enricher after the hook. A third is to keep the order but document plainly that hook-added enrichers bypass ILogRedactor. Add a test that enriches a sensitive property in ConfigureLogger and asserts it is redacted.

Resolution

Resolved 2026-11-09 in the commit tagged (scadaproj#48) (Telemetry 0.2.1, unpublished) — AddZbSerilogCore now applies the shared enrichers with no provider, invokes ConfigureLogger, then registers RedactionEnricher last (new internal ZbSerilogConfig.ApplyRedaction; Apply(…, sp) still includes redaction for direct callers). Option doc updated. Test Properties_stamped_by_ConfigureLogger_enrichers_are_redacted (failed without the fix: apiKey reached the sink raw).

Telemetry-010 — Redaction projection keys logged dictionaries by ToString(), silently dropping entries whose keys stringify identically

Severity	Low
Priority	P3
Category	Correctness & logic bugs
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs:134-143 (projection), :163-173 (rebuild)

Description

Project turns a DictionaryValue into a ProjectedDictionary keyed by pair.Key.Value?.ToString() ?? "". Two distinct keys with the same string form overwrite each other in both the projected values and OriginalKeys. Examples are DateTime/DateTimeOffset keys that differ below one second (the default ToString() has no fractional seconds), and a null key alongside "". Rebuild then emits one entry where there were two. ValueEquals sees the element count differ and writes the shrunken dictionary back onto the event. This happens for every event once any ILogRedactor is registered, even one that does nothing.

Failure scenario

A scratchpad probe registered a no-op ILogRedactor and logged log.Information("{Map}", new Dictionary<DateTime,string>{ [12:00:00.100]="first", [12:00:00.200]="second" }). With the enricher the result was [(11/9/2026 12:00:00 PM: "second")]. Without it the result was [(…12:00:00: "first"), (…12:00:00: "second")]. Diagnostic log data (for example per-timestamp samples) is silently lost in MxGateway, the redaction-enabled consumer. Nothing about it is secret.

Recommendation

Keep the original ScalarValue keys alongside the string view, for example by projecting to a list of key/value pairs or disambiguating colliding string keys. Alternatively, skip projection and pass a dictionary through untouched whenever its stringified keys are not unique. Add a regression test with colliding keys.

Resolution

Resolved 2026-11-09 in the commit tagged (scadaproj#48) (Telemetry 0.2.1, unpublished) — RedactionEnricher.Project keeps every dictionary entry: a key whose string form collides gets a #n suffix in the redactor's view and OriginalKeys restores the real key on rebuild. Test Dictionary_keys_that_print_identically_are_all_kept (two DateTime keys differing by milliseconds, no-op redactor; failed without the fix: one entry survived).

Telemetry-011 — OTLP fail-fast validation accepts scheme-less host:port and is skipped by the pre-built-options AddZbTelemetry overload

Severity	Low
Priority	P3
Category	Error handling & resilience
Status	Resolved
Location	ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryOptionsValidator.cs:36, ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryExtensions.cs:40-46

Description

The Telemetry-006 resolution added ZbTelemetryOptionsValidator.Validate, which checks Uri.TryCreate(OtlpEndpoint, UriKind.Absolute, out _). A scheme-less host:port, which is the typo the original finding named ("missing scheme"), parses as an absolute URI whose scheme is the host name, so it passes. The checks it ran:

• "localhost:4317" → True, scheme localhost;
• "collector:4317" → True, scheme collector;
• only a literal IP such as 10.0.0.5:4317 is rejected.

Separately, the validator is called only from BuildOptions (the two lambda overloads) and AddZbSerilog. The public AddZbTelemetry(IServiceCollection, ZbTelemetryOptions) overload validates neither the OTLP endpoint nor the required ServiceName.

Failure scenario

An operator sets Exporter=Otlp, OtlpEndpoint="otel-collector:4317", missing http://. Startup passes validation, the exporter gets a URI with scheme otel-collector, and OTLP traces, metrics and logs fail later inside the exporter rather than as the named startup error Telemetry-006 promised. Prometheus /metrics keeps working, so the loss is easy to miss. Any caller that builds ZbTelemetryOptions itself and uses the options overload gets no validation at all. No family app uses that overload today.

Recommendation

Require uri.Scheme to be http or https (the OTLP gRPC/HTTP schemes) and a non-empty uri.Host. Call BuildOptions-equivalent validation, including ServiceName, from the AddZbTelemetry(IServiceCollection, ZbTelemetryOptions) overload as well. Add a test for "localhost:4317".

Resolution

Resolved 2026-11-09 in the commit tagged (scadaproj#48) (Telemetry 0.2.1, unpublished) — ZbTelemetryOptionsValidator now requires an absolute http/https URI with a host (localhost:4317, otel-collector:4317, ftp://… rejected); AddZbTelemetry(IServiceCollection, ZbTelemetryOptions) now runs the same ServiceName + OTLP validation as the lambda overloads (param name options). Tests AddZbTelemetry_Throws_WhenOtlpEndpointIsSchemeLessOrNotHttp and AddZbTelemetry_PrebuiltOptionsOverload_ValidatesServiceNameAndOtlpEndpoint (failed without the fix). Behaviour change: configs with a scheme-less endpoint now fail at startup, as Telemetry-006 intended.