Skip to content

feat(web): add operation outcome metrics - #331

Merged
berntpopp merged 1 commit into
berntpopp:mainfrom
robspan:web-operation-outcome-metrics
Aug 7, 2026
Merged

feat(web): add operation outcome metrics#331
berntpopp merged 1 commit into
berntpopp:mainfrom
robspan:web-operation-outcome-metrics

Conversation

@robspan

@robspan robspan commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add bounded varlens_operation_events_total outcomes for login/password changes, upload staging, and import/batch-import operations
  • attach server-generated UUID request IDs to logs and responses without trusting client-supplied values
  • align the PostgreSQL migration integration expectation with the existing upstream 0015 migration

Why

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

  • web mode only; desktop behavior and default desktop CI paths are unchanged
  • no new dependency or database schema change

Validation

  • make ci
  • web build and static/integration web gates
  • PostgreSQL migration integration against a fresh database
  • make agent-check

@berntpopp
berntpopp force-pushed the web-operation-outcome-metrics branch from 230425e to d8c85ac Compare August 7, 2026 07:50

@berntpopp berntpopp left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
  • resultsuccess | error.
  • failure_class — every call site passes a string literal except one, err.code from PasswordPolicyError, which is itself a bounded union ('too-short' | 'same-as-old'). The reason value 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):

  1. Spoof request-id in the test as well as x-request-id.
  2. Set requestIdHeader: false explicitly in the Fastify({...}) options, pinning the behaviour against a future Fastify default change rather than relying on it.

Everything else

  • recordOperationEvent is invoked via optional chaining throughout, so a server built without metrics is unaffected.
  • Metric registration follows the existing # HELP / # TYPE / pushSeries pattern in metrics.ts; varlens_operation_events_total is correctly typed counter.
  • No secrets, usernames, or PHI reach a label — failure classes only.
  • The migration-expectation realignment mentioned in the description is already reconciled, since 0015 is on main.

Verification

  • make typecheck — clean
  • VARLENS_WEB=1 make test — 4786 passed (main baseline 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.

@berntpopp
berntpopp marked this pull request as ready for review August 7, 2026 08:18
@berntpopp
berntpopp merged commit 4b324ad into berntpopp:main Aug 7, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants