DM-55604: Bridge pino warn/error records to Sentry Logs - #609
DM-55604: Bridge pino warn/error records to Sentry Logs#609jonathansick wants to merge 2 commits into
Conversation
Ship server-side pino warn/error records to Sentry Structured Logs via Sentry.pinoIntegration() in sentry.server.config.js, so those records become searchable and trace-linked without creating Sentry issues or firing Slack alerts. The explicit reportError channel stays the sole alerting path. Key decisions: - Configure pinoIntegration with an empty error.levels list (via src/lib/sentry/pinoLogsConfig.ts) so the bridge stays strictly on the Logs channel and never double-captures with reportError. This invariant is unit-pinned. - Set enableLogs: true at the Sentry.init top level (stable API in @sentry/nextjs v10) to turn on Structured Logs, which the pinoIntegration log channel requires. - Verify the transport with a SentryTestButtons-style hook: a POST-only /admin/sentry/emit-log route handler emits a warn+error record, driven by a new "Emit server log" button on the /admin/sentry page. Next-iteration notes: - None. prodromos terraform for the Sentry project + Slack alerting is a separate PRD #598 task in the prodromos repo, not this branch. Closes #605
🦋 Changeset detectedLatest commit: 1cc7396 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
jonathansick
left a comment
There was a problem hiding this comment.
Stoker review of PR #609 (stacked diff vs tickets/DM-55604, commit 7adda7c).
Verdict: blocking. The bridge is correctly issue-free (empty error.levels, verified against @sentry/nextjs 10.58.0's actual pinoIntegration API; the diagnostics-channel mechanism and instrumentation.ts init ordering are sound) and the smoke route adds no meaningful risk beyond the existing /admin/sentry test surface. But log.levels ships ALL six pino levels to Sentry Logs while the changeset, PR body, code comments, and issue #605 scope all say warn/error only — production info records would flood Sentry Logs contrary to the documented design.
{
"stoker_review_version": 1,
"pr_number": 609,
"blocking": true,
"summary": "The pino→Sentry Logs bridge is correctly issue-free (empty error.levels, verified against @sentry/nextjs 10.58.0's actual pinoIntegration API) and the smoke route is fine, but log.levels ships ALL six pino levels to Sentry Logs while the changeset, PR body, code comments, and issue #605 scope all say warn/error only — production info records would flood Sentry Logs contrary to the documented design.",
"findings": [
{
"id": "f1",
"severity": "error",
"category": "correctness",
"file": "apps/squareone/src/lib/sentry/pinoLogsConfig.ts",
"line_start": 25,
"line_end": 25,
"summary": "log.levels is set to ['trace','debug','info','warn','error','fatal'], but the issue #605 scope, the changeset, the PR body, and this file's own docstring (\"warn and error are included in log.levels so those records reach Sentry Logs\") all describe a warn/error-only bridge. As written, every production info-level record (route logs, the 'Squareone starting' startup line, etc.) is shipped to Sentry Logs, contradicting the documented design and inflating Sentry Logs volume.",
"suggested_fix": "Change to `log: { levels: ['warn', 'error'] }` so the bridge matches the documented warn/error-only scope (or, if shipping all levels is actually intended, update the changeset, PR body, and docstrings to say so — but the PRD scope says warn/error)."
},
{
"id": "f2",
"severity": "warning",
"category": "tests",
"file": "apps/squareone/src/lib/sentry/pinoLogsConfig.test.ts",
"line_start": 6,
"line_end": 12,
"summary": "The log-channel test uses toContain('warn')/toContain('error'), which passes even when log.levels is over-broad — it failed to catch f1. The test pins containment, not the actual invariant.",
"suggested_fix": "Assert the exact expected list, e.g. `expect(pinoLogsIntegrationOptions.log?.levels).toEqual(['warn', 'error'])`, mirroring how the error-channel test pins `toEqual([])`."
},
{
"id": "f3",
"severity": "info",
"category": "clarity",
"file": "apps/squareone/src/components/SentryTestButtons/SentryTestButtons.tsx",
"line_start": 44,
"line_end": 51,
"summary": "The \"Emit server log\" button fires `void fetch(...)` with no success/failure feedback, so an operator smoke-testing on /admin/sentry cannot tell whether the POST succeeded without opening devtools. Acceptable for a test surface; noting for completeness.",
"suggested_fix": "Optionally surface a minimal confirmation (e.g. transient text or console.log of the response status) so the smoke test is observable from the page."
}
]
}- f1: scope pino→Sentry Logs bridge to warn/error only (log.levels) - f2: pin log.levels to exact list with toEqual - f3: add emit-log status feedback on the /admin/sentry button
Summary
warn/errorrecords to Sentry Structured Logs viaSentry.pinoIntegration()insentry.server.config.js, so they become searchable and trace-linked in Sentry.error.levelsinsrc/lib/sentry/pinoLogsConfig.ts): it never creates Sentry issues or fires Slack alerts, so the explicitreportErrorpath stays the sole alerting channel and there is no double-capture./admin/sentry/emit-logroute handler plus an "Emit server log" button on/admin/sentry— to verify the transport in a real server build.Validation steps
SENTRY_DSN, open/admin/sentry, click "Emit server log", and confirm the warn/error records (markersentry-logs-smoke-test) appear in Sentry Logs.References