feat: add structured logging with request correlation#115
Conversation
|
On it! I'm reviewing your PR adding structured logging with pino — I'll check it for correctness and rule-compliance and come back with a consolidated review. |
There was a problem hiding this comment.
Reviewed the structured-logging change against Dobby's rule set and the diff itself.
Rule compliance: clean. Conventional-commit title (feat:), opened ready-for-review (not draft), pino is genuinely imported by server code (not a dead bump), vi.hoisted() mock added correctly, and the retargeted dns-verification test preserves the original assertions' intent. No rule violations.
Verification (from review): built with adapter-node (pino externalized, resolves at runtime), 83 vitest tests pass, svelte-check clean, and a run of the built server confirmed structured JSON logging, X-Request-Id echo + sanitisation (abc/../123 evil -> abc123evil), /health excluded from logs, and locals.log.warn firing on /api/csp-report.
One minor nit inline — non-blocking.
|
|
||
| // Skip the noisy liveness/readiness probes. | ||
| const path = event.url.pathname; | ||
| if (path !== '/health' && path !== '/readyz') { |
There was a problem hiding this comment.
nit: the request-log skip excludes both /health and /readyz, but there's no /readyz route in the repo (only /health). Harmless dead condition — either add the readiness probe or drop the && path !== '/readyz' clause.
|
Reviewed! Everything looks clean — conventional commit title, pino is genuinely used, and the |
|
Thanks @dobby-coder — on the |
Closes #96.
Replaces ad-hoc
console.*calls with a structured logger and per-request correlation.Changes
src/lib/server/logger.ts— apinologger writing structured JSON to stdout, level fromLOG_LEVEL(defaultinfo). No transport, so it stays robust under the bundled adapter-node server.src/hooks.server.tsX-Request-Id, else mints one), attaches a child logger toevent.locals.log, and echoesX-Request-Idon the response.method,path,status,durationMs) — skipping the/healthand/readyzprobes to avoid noise.handleErrorhook that logs unhandled server errors with the request id + stack.app.d.ts—Localsnow carriesrequestIdandlog.console.*sites:services/dns-verification.ts(→ baselogger) andapi/csp-report/+server.ts(→locals.log).Notes
pinois a runtime dependency (imported by server code); the production image already shipsnode_modules, so it resolves at runtime.dns-verificationunit test asserted onconsole.error; it's been retargeted to the logger mock (same intent — errors are logged withorgId/domain/code). Themessagefield is nowerr.pino-prettylocally if desired.