# 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.

```csharp
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:

```csharp
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.

```csharp
// 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:

```csharp
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

```csharp
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

```csharp
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:

```csharp
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:

```csharp
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 —

```csharp
_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`:

```csharp
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

```csharp
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

```csharp
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

```csharp
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.

```csharp
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`

```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

```ts
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

```ts
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

```jsonc
"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:

```jsonc
"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 |