refactor(kpi): K2/K6/K7 review fixups — empty-batch guard + sealed repo + uniform TryAddEnumerable + KPI-age doc fidelity + coverage

This commit is contained in:
Joseph Doherty
2026-06-17 20:00:43 -04:00
parent 456e61dff3
commit e6c15250ce
7 changed files with 46 additions and 8 deletions
@@ -45,6 +45,8 @@ public class KpiSampleEntityTypeConfiguration : IEntityTypeConfiguration<KpiSamp
// Series index — backs the bucketed query path (filter one series, scan in // Series index — backs the bucketed query path (filter one series, scan in
// capture order). Names locked for migration discoverability. // capture order). Names locked for migration discoverability.
// Global-scope rows (ScopeKey IS NULL) are included in the index and are
// seeked via the IS NULL predicate, so the index covers all scope levels.
builder.HasIndex(s => new { s.Source, s.Metric, s.Scope, s.ScopeKey, s.CapturedAtUtc }) builder.HasIndex(s => new { s.Source, s.Metric, s.Scope, s.ScopeKey, s.CapturedAtUtc })
.HasDatabaseName("IX_KpiSample_Series"); .HasDatabaseName("IX_KpiSample_Series");
@@ -10,7 +10,7 @@ namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories;
/// <c>KpiSample</c> table (M6 "KPI History &amp; Trends"). See the interface for the /// <c>KpiSample</c> table (M6 "KPI History &amp; Trends"). See the interface for the
/// contract; this class adds notes on the data-access strategy per method. /// contract; this class adds notes on the data-access strategy per method.
/// </summary> /// </summary>
public class KpiHistoryRepository : IKpiHistoryRepository public sealed class KpiHistoryRepository : IKpiHistoryRepository
{ {
private readonly ScadaBridgeDbContext _context; private readonly ScadaBridgeDbContext _context;
@@ -29,6 +29,12 @@ public class KpiHistoryRepository : IKpiHistoryRepository
{ {
ArgumentNullException.ThrowIfNull(samples); ArgumentNullException.ThrowIfNull(samples);
// Avoid a no-op SaveChanges round-trip on quiet sampling ticks.
if (samples.Count == 0)
{
return;
}
// Bulk-insert one sampling pass. AddRange + a single SaveChanges keeps the // Bulk-insert one sampling pass. AddRange + a single SaveChanges keeps the
// whole batch in one round-trip; the store assigns each row's identity. // whole batch in one round-trip; the store assigns each row's identity.
_context.KpiSamples.AddRange(samples); _context.KpiSamples.AddRange(samples);
@@ -20,7 +20,9 @@ namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox.Kpi;
/// <c>capturedAtUtc</c> rather than wall-clock <c>now</c>: the stuck cutoff is /// <c>capturedAtUtc</c> rather than wall-clock <c>now</c>: the stuck cutoff is
/// <c>capturedAtUtc - <see cref="NotificationOutboxOptions.StuckAgeThreshold"/></c> and the /// <c>capturedAtUtc - <see cref="NotificationOutboxOptions.StuckAgeThreshold"/></c> and the
/// delivered window is <c>capturedAtUtc - <see cref="NotificationOutboxOptions.DeliveredKpiWindow"/></c>. /// delivered window is <c>capturedAtUtc - <see cref="NotificationOutboxOptions.DeliveredKpiWindow"/></c>.
/// So a sample captured at the same instant equals the live tile. /// The COUNT metrics (queueDepth, stuckCount, parkedCount, deliveredLastInterval) equal the
/// live tile at the same instant; the <c>oldestPendingAgeSeconds</c> metric is computed against
/// the repository's internal clock and may differ from the live tile by the query-execution latency.
/// </para> /// </para>
/// <para> /// <para>
/// Emits Global (<c>ScopeKey == null</c>), per-Site (<c>ScopeKey == SourceSiteId</c>), and /// Emits Global (<c>ScopeKey == null</c>), per-Site (<c>ScopeKey == SourceSiteId</c>), and
@@ -1,4 +1,5 @@
using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Kpi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Kpi;
using ZB.MOM.WW.ScadaBridge.NotificationOutbox.Delivery; using ZB.MOM.WW.ScadaBridge.NotificationOutbox.Delivery;
using ZB.MOM.WW.ScadaBridge.NotificationOutbox.Kpi; using ZB.MOM.WW.ScadaBridge.NotificationOutbox.Kpi;
@@ -50,7 +51,9 @@ public static class ServiceCollectionExtensions
// KPI history (M6): the recorder singleton enumerates every IKpiSampleSource each // KPI history (M6): the recorder singleton enumerates every IKpiSampleSource each
// sampling pass to snapshot the outbox delivery KPIs into the central history store. // sampling pass to snapshot the outbox delivery KPIs into the central history store.
services.AddScoped<IKpiSampleSource, NotificationOutboxKpiSampleSource>(); // TryAddEnumerable is idempotent — no double-registration if AddNotificationOutbox
// is ever called more than once, matching the AuditLog (K8) idiom.
services.TryAddEnumerable(ServiceDescriptor.Scoped<IKpiSampleSource, NotificationOutboxKpiSampleSource>());
return services; return services;
} }
@@ -20,9 +20,12 @@ namespace ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi;
/// The cutoffs are derived from <see cref="SiteCallAuditOptions"/> exactly as the /// The cutoffs are derived from <see cref="SiteCallAuditOptions"/> exactly as the
/// live <c>SiteCallAuditActor</c> KPI handlers derive them /// live <c>SiteCallAuditActor</c> KPI handlers derive them
/// (<c>stuckCutoff = capturedAtUtc - StuckAgeThreshold</c>, /// (<c>stuckCutoff = capturedAtUtc - StuckAgeThreshold</c>,
/// <c>intervalSince = capturedAtUtc - KpiInterval</c>) so a sampled value equals /// <c>intervalSince = capturedAtUtc - KpiInterval</c>). The COUNT metrics
/// the live tile computed at the same instant. The recorder's /// (buffered, parked, failedLastInterval, deliveredLastInterval, stuck) equal the
/// <c>capturedAtUtc</c> is the single anchor for both cutoffs. /// live tile at the same instant; the <c>oldestPendingAgeSeconds</c> metric is
/// computed against the repository's internal clock and may differ from the live
/// tile by the query-execution latency. The recorder's <c>capturedAtUtc</c> is
/// the single anchor for both cutoffs.
/// </para> /// </para>
/// <para> /// <para>
/// Registered DI-scoped (next to the rest of the Site Call Audit composition) so /// Registered DI-scoped (next to the rest of the Site Call Audit composition) so
@@ -33,12 +36,13 @@ namespace ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi;
public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource public sealed class SiteCallAuditKpiSampleSource : IKpiSampleSource
{ {
// ── Metric catalog (the M6-agreed metric-name strings for this source) ── // ── Metric catalog (the M6-agreed metric-name strings for this source) ──
// Declaration order matches the emission order in AddSnapshot.
private const string MetricBuffered = "buffered"; private const string MetricBuffered = "buffered";
private const string MetricParked = "parked"; private const string MetricParked = "parked";
private const string MetricFailedLastInterval = "failedLastInterval"; private const string MetricFailedLastInterval = "failedLastInterval";
private const string MetricDeliveredLastInterval = "deliveredLastInterval"; private const string MetricDeliveredLastInterval = "deliveredLastInterval";
private const string MetricOldestPendingAgeSeconds = "oldestPendingAgeSeconds";
private const string MetricStuck = "stuck"; private const string MetricStuck = "stuck";
private const string MetricOldestPendingAgeSeconds = "oldestPendingAgeSeconds";
private readonly ISiteCallAuditRepository _repository; private readonly ISiteCallAuditRepository _repository;
private readonly SiteCallAuditOptions _options; private readonly SiteCallAuditOptions _options;
@@ -1,4 +1,5 @@
using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Kpi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Kpi;
using ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi; using ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi;
@@ -44,7 +45,9 @@ public static class ServiceCollectionExtensions
// KPIs (global + per-site + per-node) into the KpiSample history store. // KPIs (global + per-site + per-node) into the KpiSample history store.
// Scoped so each pass resolves a fresh ISiteCallAuditRepository scope, // Scoped so each pass resolves a fresh ISiteCallAuditRepository scope,
// mirroring the actor's scope-per-message repository access. // mirroring the actor's scope-per-message repository access.
services.AddScoped<IKpiSampleSource, SiteCallAuditKpiSampleSource>(); // TryAddEnumerable is idempotent — no double-registration if AddSiteCallAudit
// is ever called more than once, matching the AuditLog (K8) idiom.
services.TryAddEnumerable(ServiceDescriptor.Scoped<IKpiSampleSource, SiteCallAuditKpiSampleSource>());
return services; return services;
} }
@@ -106,6 +106,24 @@ public class KpiHistoryRepositoryTests
Assert.Equal(new[] { 1d, 2d }, series.Select(p => p.Value).ToArray()); Assert.Equal(new[] { 1d, 2d }, series.Select(p => p.Value).ToArray());
} }
[Fact]
public async Task RecordSamplesAsync_EmptyBatch_IsNoOp_AndGetRawSeriesAsync_EmptyTable_ReturnsEmpty()
{
await using var ctx = NewContext();
var repo = new KpiHistoryRepository(ctx);
// Empty batch must not throw (early-return guard, no SaveChanges round-trip).
await repo.RecordSamplesAsync(Array.Empty<KpiSample>());
// With nothing written, GetRawSeriesAsync must return a non-null empty list.
var series = await repo.GetRawSeriesAsync(
"NotificationOutbox", "queueDepth", "Global", scopeKey: null,
fromUtc: Base, toUtc: Base.AddMinutes(60));
Assert.NotNull(series);
Assert.Empty(series);
}
[Fact] [Fact]
public async Task PurgeOlderThanAsync_DeletesOnlyRowsOlderThanCutoff_AndReturnsCount() public async Task PurgeOlderThanAsync_DeletesOnlyRowsOlderThanCutoff_AndReturnsCount()
{ {