slp-modular-cms/aidlc-docs/features/gitea-deployment-workflow/construction/u4-observability/nfr-design/nfr-design-patterns.md

NFR Design Patterns — U4 Observability Integration

Unit: U4 Observability Integration NFRs addressed: NFR-01 (no server configuration), NFR-07 (no secrets in the repository), NFR-08 (graceful degradation), SECURITY-03, SECURITY-11, SECURITY-14, SECURITY-15

Two things dominate this design. First, every observability service must be optional — the same binary has to run with no DSN, no Umami and no network egress at all. Second, the mechanisms that protect credentials and enable alerting are the ones most likely to look correct while doing nothing, so each is designed to be independently testable rather than asserted by a comment.

---

Pattern 1 — OPEN-01 Resolved: the Correlation ID Is the W3C Trace ID

OPEN-01 asked whether the correlation identifier required by SECURITY-03 should be HttpContext.TraceIdentifier or the W3C traceparent trace ID. Decision: the W3C trace ID, taken from the ambient Activity, with TraceIdentifier as the fallback when no Activity exists.

builder.Logging.Configure(options =>
    options.ActivityTrackingOptions =
        ActivityTrackingOptions.TraceId |
        ActivityTrackingOptions.SpanId |
        ActivityTrackingOptions.ParentId);

That single call puts TraceId into the logging scope of every entry from every category, including framework categories, without touching a single call site. BR-U4-03 ("every log entry carries a correlation identifier") is satisfied by configuration rather than by discipline — which matters, because the alternative is remembering to pass an ID into thousands of existing log calls.

Why the W3C trace ID rather than TraceIdentifier

Property	HttpContext.TraceIdentifier	W3C trace ID
Crosses the master/slave HTTP boundary	No — host-local, meaningless elsewhere	Yes — HttpClient injects traceparent automatically
Matches what the client already sees	No	Yes — see below
Understood by Sentry natively	No	Yes — becomes the event's trace
Available outside a request (startup, background service)	No — there is no HttpContext	Yes, when an Activity is started
Cost	None	One line of configuration

The decisive argument is the master/slave boundary. This codebase's hardest diagnostic question is "the master says the slave rejected its API key — what did the slave actually see?". TraceIdentifier cannot answer it: the master's ID and the slave's ID are unrelated strings. The W3C trace ID is propagated by HttpClient on the outbound call and adopted by the slave's hosting layer, so both sides' log entries and both sides' Sentry events carry the same value. That is the entire reason a correlation ID exists here.

The second argument is alignment with what already exists. ASP.NET Core's built-in ProblemDetails writes traceId as Activity.Current?.Id ?? HttpContext.TraceIdentifier. Choosing the W3C trace ID means the value in the log, the value in the Sentry event, the value propagated to the slave and the value the browser was shown in the error response are one value — so an operator can be handed a traceId from a screenshot and find the log entry. Choosing TraceIdentifier would produce two competing identifiers for the same request, which is worse than having one imperfect one.

The ?? TraceIdentifier fallback is kept for exactly the framework's reason: no Activity means no trace ID, and a null correlation ID is worse than a host-local one.

The trap that makes this silently not work

Scopes are only rendered if the console provider is told to render them:

options.IncludeScopes = true;   // without this, TraceId is collected and then discarded

ActivityTrackingOptions populates the scope; IncludeScopes is what writes it out. Configure the first and forget the second and every log entry looks completely normal, with no correlation ID and no error anywhere. A test asserts a TraceId field is present in rendered console output, not merely that the option was set.

---

Pattern 2 — Registration Order: Logging Before Sentry

Rule: BR-U4-04.

// 1. Logging first — so a Sentry problem is itself logged.
builder.Logging.ClearProviders();
builder.Logging.Configure(o => o.ActivityTrackingOptions = ...);
if (builder.Environment.IsDevelopment())
{
    builder.Logging.AddSimpleConsole(o => { o.IncludeScopes = true; o.SingleLine = true; });
}
else
{
    builder.Logging.AddJsonConsole(o => { o.IncludeScopes = true; });
}

// 2. Sentry second, and only when a DSN is present.
builder.WebHost.UseCmsSentry(builder.Configuration);

Why the two console formats differ by environment: in test and production the process is supervised (systemd on the Pi) and its stdout lands in the journal, where JSON is greppable and TraceId is a field rather than a substring. Locally a developer reads it with their eyes, and JSON is hostile to that. Both set IncludeScopes, so the correlation ID is present either way — the formatting differs, the information does not.

Why ClearProviders(): the default host already added a console provider without IncludeScopes. Adding a second one produces every line twice, once with the correlation ID and once without — which reads as a logging bug and wastes real time.

---

Pattern 3 — Threshold Split Maps Onto Two Existing Options

Rules: BR-U4-02, BR-U4-06, BR-U4-07. This is the reconciliation of Q3 = C with Q16 = C, and it needs no custom code:

options.MinimumBreadcrumbLevel = LogLevel.Information;   // BR-U4-07
options.MinimumEventLevel      = LogLevel.Warning;       // BR-U4-06

Destination	Threshold	Mechanism
Console	Information, framework included	Logging:LogLevel:Default raised from Warning to Information
Sentry breadcrumbs	Information	MinimumBreadcrumbLevel
Sentry events	Warning	MinimumEventLevel

appsettings.json must change too. Today it says "Default": "Warning" and "Microsoft.AspNetCore": "Warning". Leaving those in place would cap the console and the breadcrumbs at Warning, and every Sentry event would arrive with an empty breadcrumb trail — the feature present, configured, and useless. Both move to Information; Microsoft.EntityFrameworkCore.Database.Command stays at Warning, because Information there logs every SQL statement including parameter values.

That last exclusion is not a volume concern, it is BR-U4-05: EF's command logging at Information prints parameter values, and the login path passes a normalised email through it.

---

Pattern 4 — Scrubbing as an Injectable, Testable Filter

Rules: BR-U4-13, BR-U4-14. Defence in depth, in two layers.

Layer 1 — never capture the body at all

options.MaxRequestBodySize = RequestSize.None;   // the SDK default; set explicitly

Set explicitly with a comment, because the safest version of "the request body is never sent to Sentry" is that it is never read into an event in the first place. Relying on the default means a future RequestSize.Small added by someone chasing a bug quietly starts shipping login payloads.

Layer 2 — remove credential headers before send

public interface ISentryEventScrubber
{
    SentryEvent Scrub(SentryEvent @event);
}

internal sealed class SentryEventScrubber : ISentryEventScrubber
{
    internal static readonly string[] RemovedHeaders =
    [
        "Cookie",              // carries refreshToken
        "Set-Cookie",
        "Authorization",       // bearer token
        "X-Master-Api-Key"     // master/slave shared secret
    ];

    public SentryEvent Scrub(SentryEvent @event)
    {
        foreach (var header in RemovedHeaders)
        {
            @event.Request.Headers.Remove(header);
        }

        @event.Request.Data = null;
        return @event;
    }
}

Wired in through both send hooks:

options.SetBeforeSend((e, _) => scrubber.Scrub(e));
options.SetBeforeSendTransaction((t, _) => scrubber.ScrubTransaction(t));

Why SetBeforeSendTransaction as well as SetBeforeSend: performance transactions carry request data too. Scrubbing only events leaves a second, less obvious channel open — and it is less obvious precisely because nobody thinks of a transaction as containing headers. Enabling tracing later without touching this file would otherwise start leaking.

Why an interface rather than an inline lambda: the scrub list is the single most security-critical piece of code in this unit, and a lambda inside a UseSentry callback cannot be unit-tested without initialising the SDK. As an injected class it is tested directly: construct a SentryEvent with all four headers plus a body, scrub, assert each is gone and that the retained fields (method, path, query, user agent, IP, username) survive.

Set-Cookie is in the list although the functional design did not name it. Response headers can be attached to an event, and Set-Cookie on the login and refresh responses contains the refreshToken being issued. Removing the request cookie while sending the response cookie would protect nothing.

Not configurable, by construction (functional design, domain-entities.md): RemovedHeaders is a static readonly array in code, not an options property. A configurable scrub list is a supported way to switch the protection off by omission.

---

Pattern 5 — Flush Before Exit, or the Event That Matters Most Is the One You Lose

The Sentry SDK batches and sends in the background. A process that throws during startup and exits kills that background sender before it transmits — so the migration failure, the one event in the whole system that requires immediate human attention (BR-U4 security event table, level Critical), is exactly the event most likely never to arrive.

MigrateCoreDatabase (U2) therefore gains a flush on its failure path:

catch (Exception ex)
{
    logger.LogCritical(ex, "...");          // existing behaviour
    await SentrySdk.FlushAsync(TimeSpan.FromSeconds(5));
    throw;                                   // still fails fast — unchanged
}

Five seconds, bounded: a host that cannot reach its database is already down, and five seconds of additional downtime buys the alert that tells someone why. FlushAsync is a no-op when the SDK was never initialised, so the no-DSN path is unaffected.

This is a change to a U2 file made by U4 and is called out in the commit message, since U2 is already committed.

---

Pattern 6 — Alertable Security Events: Source-Generated Templates + a Sentry Tag

Rules: the six events in domain-entities.md; FR-19 alert rules; SECURITY-14. This is the pattern that decides whether FR-19 is implementable at all.

The problem

Sentry groups log-derived events by their message. Emitted with interpolation —

_logger.LogWarning($"Login failed for {email} on {endpoint}");   // WRONG

— every distinct email produces a separate Sentry issue. An alert rule of the form "more than 20 failed logins in 5 minutes" can then never fire, because no single issue ever reaches 20. The feature appears to work: events arrive, they are visible, they are tagged. Only the alerting silently cannot exist.

The pattern

One source-generated LoggerMessage per event type, with a fixed template and a stable EventId:

internal static partial class SecurityEvents
{
    public const int FailedLoginEventId          = 5001;
    public const int AuthorizationDeniedEventId  = 5002;
    public const int MasterApiKeyRejectedEventId = 5003;
    public const int AdminBypassRejectedEventId  = 5004;
    public const int RateLimitTriggeredEventId   = 5005;
    public const int MigrationFailureEventId     = 5006;

    [LoggerMessage(
        EventId = FailedLoginEventId,
        Level = LogLevel.Warning,
        Message = "Security event: login failed on {Endpoint} (account exists: {AccountExists})")]
    public static partial void FailedLogin(ILogger logger, string endpoint, bool accountExists);

    [LoggerMessage(
        EventId = AdminBypassRejectedEventId,
        Level = LogLevel.Warning,
        Message = "Security event: admin bypass rejected on {Path} (reason: {Reason})")]
    public static partial void AdminBypassRejected(ILogger logger, string path, BypassRejectionReason reason);

    // …4 more
}

The template is a compile-time constant, so all failed logins group into one Sentry issue with the variable parts as structured fields. The rate-based alert rule becomes possible.

Tagging, so alert rules filter on a tag rather than a substring

internal sealed class SecurityEventProcessor : ISentryEventProcessor
{
    private static readonly FrozenDictionary<int, string> Tags = /* 5001 → "failed_login", … */;

    public SentryEvent Process(SentryEvent @event)
    {
        if (@event.Extra.TryGetValue("EventId", out var id) &&
            id is int eventId &&
            Tags.TryGetValue(eventId, out var tag))
        {
            @event.SetTag("security_event", tag);
        }
        return @event;
    }
}

Alert rules then read security_event:failed_login, which is stable across message-wording changes. Matching on message text would break the day someone improves the wording — silently, because a rule that matches nothing looks identical to a rule with nothing to match.

BypassRejectionReason

An enum — InvalidSignature, Expired, WrongIssuer, WrongAudience, NotAdmin, Malformed — not a string, and never the token. AdminTokenValidator (U1) currently returns bool; it gains an overload returning the reason so this event can carry it. The distinction is operationally real: InvalidSignature means someone is forging tokens, Expired means an administrator left a tab open.

Where each event is emitted

Event	Assembly	Emission site
Failed login	Modules.Identity	Auth service login path
Authorization denied	Core	Authorization failure handler
Master API key rejected	Modules.Master	API key authentication handler
Admin bypass rejected	Modules.Availability	AvailabilityMiddleware.IsAdminBypass
Rate limit triggered	Core	AddCmsRateLimiting → OnRejected
Migration failure	Core	MigrateCoreDatabase catch (Pattern 5)

SecurityEvents therefore lives in SlpModularCms.Core, which all four assemblies already reference.

OnRejected does not exist yet on the rate limiter — AddCmsRateLimiting sets only RejectionStatusCode. It is added here. Without it, a brute-force attempt against /api/v1/Auth/login returns 429 and leaves no trace anywhere, which makes the one rate limiter this application has unobservable.

---

Pattern 7 — The Sentry Tunnel Endpoint

Rules: BR-U4-15…21. An anonymous endpoint that performs an outbound request on demand needs every one of these constraints.

Destination computed once, at startup

public sealed class SentryTunnelTarget
{
    public Uri EnvelopeEndpoint { get; }        // https://{host}/api/{projectId}/envelope/
    public bool IsConfigured { get; }

    // Parsed from the DSN: https://{publicKey}@{host}/{projectId}
}

Registered as a singleton and parsed at startup. No part of the destination ever comes from the request (BR-U4-16). This is the single rule that separates a tunnel from a server-side request forgery primitive, and computing it once makes it structurally impossible to accidentally read from the caller.

An unparseable DSN fails at startup rather than per request — consistent with U3 Pattern 1 and with SECURITY-15.

Size limit enforced twice

if (request.ContentLength > maxBytes)
{
    return Results.StatusCode(StatusCodes.Status413PayloadTooLarge);
}

var buffer = await request.Body.ReadAtMostAsync(maxBytes + 1, cancellationToken);
if (buffer.Length > maxBytes)
{
    return Results.StatusCode(StatusCodes.Status413PayloadTooLarge);
}

Both checks are needed. Content-Length is absent under chunked transfer encoding and is attacker-controlled in any case, so the pre-check is an optimisation, not the control. The bounded read is the control. Trusting ContentLength alone gives an anonymous caller an unbounded memory allocation on a Raspberry Pi.

Path, and why it is /sentry-tunnel

Same path as the reference project, where nginx serves it. Here the application does — NFR-01 forbids depending on the reverse proxy — but keeping the path identical means the frontend tunnel option, the vite dev proxy and any operator's muscle memory carry over unchanged between the two workspaces.

Deliberately not under /api/v1: it is not a versioned CMS API and must not appear in the OpenAPI document, and /api/v1 maps to the strict CSP path list in U3 for reasons that have nothing to do with this endpoint.

Rate limiting — added beyond what the functional design asked for

The tunnel gets its own fixed-window limiter ("sentry-tunnel", in the existing RateLimiting configuration section). An anonymous endpoint that triggers an outbound HTTPS request per call is a free amplifier and a way to burn the Sentry plan's quota from outside. The three existing controls — fixed destination, size cap, no DSN means no forwarding — bound what each call can do but not how many calls there can be. This closes that.

Behaviour on failure

Condition	Response	Rationale
No DSN configured	404	BR-U4-18. 404 rather than 503, because with no DSN the endpoint genuinely does not exist
Payload too large	413	BR-U4-17
Rate limit exceeded	429	Existing limiter behaviour
Forwarding to Sentry fails	202, logged server-side at Warning	The browser must not retry or console-error over a failed error report. Failing to report an error must not itself become an error
Instance availability-disabled	503 from the gate	BR-U4-20, accepted loss

BR-U4-20 needs no code. The tunnel is a mapped endpoint, and AvailabilityMiddleware runs before endpoint execution, so it is gated by default. Not being on _bypassPrefixes is the whole implementation — worth stating so nobody "fixes" it later.

HttpClient

A named client via IHttpClientFactory with a short timeout (5s) and no retry. A dropped error report is acceptable; a request thread held open by an anonymous caller is not.

---

Pattern 8 — Graceful Absence Everywhere

Rules: BR-U4-08, BR-U4-09, NFR-08.

public static IWebHostBuilder UseCmsSentry(this IWebHostBuilder b, IConfiguration config)
{
    var dsn = config["Observability:SentryDsn"];
    if (string.IsNullOrWhiteSpace(dsn))
    {
        return b;      // BR-U4-08 — a supported state, not an error, and not a warning
    }
    return b.UseSentry(o => { /* … */ });
}

No warning is logged when the DSN is absent. Local development is the common case, and a startup warning that always appears is noise that trains people to ignore startup warnings — including the ones from U3 Pattern 9 that matter.

Beyond that, the SDK is already fire-and-forget: an unreachable Sentry queues, then drops. Nothing wraps a request in a try/catch for Sentry's benefit, because there is nothing to catch. The only place the design adds a deliberate barrier is the tunnel's 202-on-failure above.

---

Pattern 9 — Frontend: Sentry, Umami, and Same-Origin Configuration

Packages

Package	Version	Note
@sentry/react	^10.68.0	Same major as the reference project, so patterns transfer
—		No Umami package; a script tag is the whole integration

lib/sentry.ts

export function initSentry(): void {
  const { sentryDsn, appEnv } = getAppConfig();
  if (!sentryDsn) return;                       // BR-U4-26

  Sentry.init({
    dsn: sentryDsn,
    environment: appEnv,
    release: __APP_VERSION__,
    tracesSampleRate: 0.1,
    sendDefaultPii: false,
    tunnel: '/sentry-tunnel',                   // BR-U4-15 — never the ingest URL
  });
}

__APP_VERSION__ requires define in vite.config.ts and a declaration in vite-env.d.ts, both following the reference project exactly.

tanstackRouterBrowserTracingIntegration is not adopted. The reference project uses it, but it needs the router instance, and importing @/router from sentry.ts — which main.tsx calls before anything else — inverts the initialisation order and drags the whole route tree into the startup path. Route-level tracing is not worth that; it can be added later inside main.tsx where the router is already imported.

The dev tunnel proxy points at our own API

server: {
  proxy: {
    '/sentry-tunnel': { target: 'https://localhost:7221', changeOrigin: true, secure: false },
  },
},

Different from the reference, and the difference is the point: the reference proxies straight to Sentry's ingest host, which means the DSN's project ID is hard-coded in vite.config.ts and has to stay in sync by hand (the reference file carries a LET OP comment saying exactly that). Here the proxy targets the local API, which derives the destination from its own DSN — so there is nothing to keep in sync and no project ID in a committed file.

config.ts — relaxed by exactly one case

const configSchema = z.object({
  apiBaseUrl: z.union([z.literal(''), z.string().url()]),   // BR-U4-24
  appTitle: z.string(),
  sentryDsn: z.string().optional(),
  appEnv: z.string().optional(),
  umamiScriptUrl: z.string().optional(),
  umamiWebsiteId: z.string().optional(),
});

apiBaseUrl resolves via import.meta.env.VITE_API_BASE_URL ?? ''. z.union with z.literal('') permits empty and still rejects htp://localhost:7221; z.string().url().optional() would not, and z.string() alone would accept anything.

api-client.ts needs one check. With apiBaseUrl === '', request URLs must be built as `${base}${path}` where path already starts with /. Any code doing `${base}/${path}` or new URL(path, base) breaks on an empty base — new URL throws on an empty base. This is verified by test rather than by reading, because the failure only appears in the production configuration, which is the one nobody runs locally.

UmamiAnalytics

Adopted essentially verbatim from the reference project: useEffect, guard on import.meta.env.DEV || !scriptUrl || !websiteId, guard on an existing element by id, defer, data-website-id.

One deliberate change: no cleanup that removes the script. The reference returns a cleanup removing the element, which under React 18 StrictMode double-invocation means inject → remove → inject. Umami's script registers listeners and sends the initial page view on load; removing the element does not unregister them, so the double-invocation can produce a duplicated first page view. The component is mounted for the application's lifetime and has nothing to clean up. The duplicate guard (getElementById) handles re-invocation on its own.

This is why the functional design's UmamiAnalytics test list includes "not injected twice on re-render" — that assertion is what catches the regression if the cleanup is ever reinstated.

---

Pattern 10 — Configuration Shape

"Observability": {
  "SentryDsn": "",                    // from an environment variable in test/production
  "Environment": "",                  // falls back to ASPNETCORE_ENVIRONMENT
  "TracesSampleRate": 0.1,
  "TunnelMaxPayloadBytes": 204800     // 200 KB
}

Logging changes alongside it:

"Logging": {
  "LogLevel": {
    "Default": "Information",
    "Microsoft.AspNetCore": "Information",
    "Microsoft.EntityFrameworkCore.Database.Command": "Warning"
  }
}

The DSN is committed empty and supplied per environment as Observability__SentryDsn. Consistent with D-16 and NFR-07. A DSN is not a standing credential — the frontend's copy is in the page source — but it identifies a project, it belongs to an account, and there is no reason for the repository to be where it is written down.

---

Decisions Taken Here Without Asking

Three values were chosen rather than escalated, because a sensible default exists and the work would otherwise have blocked. Each is one line to change:

Decision	Chosen	Reasoning	Change it if
Console format	JSON outside Development, human-readable locally	Supervised process in test/production; journald and jq both handle JSON, and TraceId becomes a field	You read production logs by eye more often than you query them
TracesSampleRate	0.1, backend and frontend	Sentry's free plan counts transactions against the same quota as errors, and this design's value is in errors, not performance traces. The reference project uses 1.0 — for a low-traffic marketing site with no API	Traffic stays low and you want full traces
Tunnel max payload	200 KB	Sentry envelopes with a stack trace and breadcrumbs run tens of KB; 200 KB is generous without being an allocation risk	Envelopes are rejected in practice