feat(web): add operation outcome metrics - #331
Conversation
230425e to
d8c85ac
Compare
berntpopp
left a comment
There was a problem hiding this comment.
Review — approve
Rebased onto main (330d736a) and reviewed in depth. Additive, web-only, no schema change, no new dependency. Two substantive checks below, one of which is a test-strength finding worth a follow-up.
Label cardinality is genuinely bounded
This is the load-bearing claim for any new Prometheus counter, so I traced every value that can reach a label:
operation— a closed TS union of 5 values.result—success|error.failure_class— every call site passes a string literal except one,err.codefromPasswordPolicyError, which is itself a bounded union ('too-short' | 'same-as-old'). Thereasonvalue in the login path is likewise'locked' | 'invalid-credentials'.
The dispatcher path is the one that could have leaked unbounded values, and it doesn't: failureClassForResult only returns a mapped value from the fixed mapKnownFailureClass allowlist, otherwise falling back to status-code-derived constants (validation, unauthenticated, forbidden, not-found, server-error, unknown). An arbitrary body.error string can never become a label. normalizeFailureClass then sanitizes to [a-z0-9_-] and truncates at 64 chars as defence in depth.
Worst-case series count is small and fixed. No cardinality risk.
Request IDs — behaviour is correct, but the test doesn't prove it
The shipped behaviour is right. I verified against the installed Fastify 5.10.0 that requestIdHeader defaults to false (lib/config-validator.js sets data.requestIdHeader = false), so no client header is trusted, and genReqId: () => randomUUID() replaces Fastify's sequential counter with an unguessable value.
However, tests/web-gate/integration/request-id.test.ts spoofs x-request-id, and that is not the header Fastify would ever trust. When requestIdHeader: true is set, it maps to request-id. I probed both configurations directly:
DEFAULT (what this PR ships):
spoof via 'x-request-id' -> 10a74eb3-… (ignored)
spoof via 'request-id' -> 274bddac-… (ignored)
IF requestIdHeader:true had been set:
spoof via 'x-request-id' -> 63ef607a-… (ignored)
spoof via 'request-id' -> SPOOFED-VALUE <-- trusted
So the test passes identically whether or not the protection is in place — it cannot fail for the reason its name describes. Not a vulnerability today, just false confidence in the guard.
Two cheap follow-ups (either is fine, non-blocking):
- Spoof
request-idin the test as well asx-request-id. - Set
requestIdHeader: falseexplicitly in theFastify({...})options, pinning the behaviour against a future Fastify default change rather than relying on it.
Everything else
recordOperationEventis invoked via optional chaining throughout, so a server built without metrics is unaffected.- Metric registration follows the existing
# HELP/# TYPE/pushSeriespattern inmetrics.ts;varlens_operation_events_totalis correctly typedcounter. - No secrets, usernames, or PHI reach a label — failure classes only.
- The migration-expectation realignment mentioned in the description is already reconciled, since
0015is onmain.
Verification
make typecheck— cleanVARLENS_WEB=1 make test— 4786 passed (mainbaseline 4785)- Postgres integration gate against a fresh DB — 16 passed / 1 skipped
- No
console.*, no Electron/fuse/IPC invariants touched, gitleaks clean
Merge-order note
Conflicts with #332 in tests/main/storage/postgres-migrations-idempotent.test.ts (both adjust migration expectations). Clean against #340. Whichever of this and #332 lands second will need a rebase.
Summary
varlens_operation_events_totaloutcomes for login/password changes, upload staging, and import/batch-import operationsWhy
The existing request and IPC metrics show traffic and latency but not whether important web workflows succeed or fail. These bounded outcome labels support operational triage without exposing user data or introducing unbounded values.
Compatibility
Validation
make cimake agent-check