Skip to content

feat: switch to Pino logger with per-request context#64

Merged
neekolas merged 3 commits intoxmtplabs:mainfrom
xmtp-coder-agent:fix/issue-63
Mar 25, 2026
Merged

feat: switch to Pino logger with per-request context#64
neekolas merged 3 commits intoxmtplabs:mainfrom
xmtp-coder-agent:fix/issue-63

Conversation

@xmtp-coder-agent
Copy link
Copy Markdown
Collaborator

@xmtp-coder-agent xmtp-coder-agent commented Mar 25, 2026

Resolves #63

Summary

  • Replace custom ConsoleLogger with PinocreateLogger() factory returns a Pino-backed logger that adapts Pino's (fields, msg) API to the codebase's (msg, fields?) convention
  • LOG_FORMAT=json env var — JSON output in production, pino-pretty for local dev (default)
  • Per-request child loggers — Each webhook request gets a child logger with requestId, deliveryId, eventName bound as context, propagated through the dispatcher to all handlers
  • Interface updatewarning()warn() (Pino convention), child(bindings) method added to Logger interface
  • TestLogger preserved — Updated with child() support; child loggers share the parent's messages array for easy assertions

Files changed

File Change
src/logger.ts Pino-backed createLogger(), updated Logger interface, updated TestLogger
src/config.ts Added logFormat field (from LOG_FORMAT env var)
src/main.ts Use createLogger(), pass per-request logger through callback
src/server.ts Create per-request child logger with requestId/deliveryId/eventName, pass to handleWebhook
src/handler-dispatcher.ts Accept optional per-request logger in dispatch()
src/handlers/close-task.ts warning()warn()
src/handlers/pr-comment.ts warning()warn()
src/github-client.ts warning()warn()
src/task-utils.ts warning()warn()
package.json Added pino and pino-pretty dependencies

Test plan

  • All 181 existing tests pass
  • New test: per-request child logger includes requestId, deliveryId, eventName
  • New tests: TestLogger.child() shares messages array, merges bindings
  • New tests: createLogger() returns logger with all methods including child()
  • bun run check passes (typecheck + lint + format + test)

Note

Switch logger to Pino with per-request child loggers bound to delivery ID and event name

  • Replaces ConsoleLogger with a Pino-backed createLogger factory in src/logger.ts; supports JSON or pretty-print output via the LOG_FORMAT env var.
  • Adds a child() method to the Logger interface; src/server.ts creates a per-request child logger pre-bound with deliveryId and eventName for structured log correlation.
  • Passes the per-request logger through HandlerDispatcher.dispatch and all downstream handler constructors so every log line carries request context.
  • Renames warning() to warn() across all handlers and test helpers to align with the Pino API.
  • Behavioral Change: pino and pino-pretty are excluded from the bundled output and must be present as runtime dependencies; LOG_FORMAT=json selects structured output, otherwise pretty-print is used.

Macroscope summarized c6d3aa6.

Replace custom ConsoleLogger with Pino for structured logging.
- LOG_FORMAT=json env var for production JSON output, pino-pretty for local dev
- Per-request child loggers with requestId, deliveryId, eventName context
- Child logger propagated through dispatcher to all handlers
- Rename warning() to warn() to match Pino convention
- TestLogger updated with child() support for test assertions

Resolves xmtplabs#63

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@neekolas
Copy link
Copy Markdown
Contributor

Uh oh. CI is failing

dist/server.js Outdated
@@ -1,5 +1,35 @@
// @bun
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This file should be gitignored and removed from the repo

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why didn't you respond to this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You're right — removed dist/server.js from the repo, added dist/ to .gitignore, and removed the "verify dist is up to date" CI step. The Dockerfile already builds dist at container build time so it was never needed in the repo.

src/server.ts Outdated
}

// Create per-request child logger with request context
const requestId = crypto.randomUUID();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should be able to get this from the webhook somewhere

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good call — the X-GitHub-Delivery header already uniquely identifies each webhook request. Replaced the crypto.randomUUID() with deliveryId from the header. The child logger now binds deliveryId and eventName as context.

xmtp-coder-agent and others added 2 commits March 25, 2026 05:38
The Bun bundler inlines __dirname from pino's thread-stream module,
embedding absolute paths into dist/server.js. This makes the build
non-reproducible across environments (local vs CI). Marking pino and
pino-pretty as --external keeps them as runtime imports instead.

Also switches from pino transport (worker threads) to direct
pino-pretty stream for dev mode, which is simpler and avoids
thread-stream entirely at runtime.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address PR review feedback:
- Remove dist/server.js from repo and add dist/ to .gitignore
- Remove "verify dist is up to date" CI step (dist is built at
  deploy time via Dockerfile)
- Update Dockerfile build command with --external pino flags
- Use X-GitHub-Delivery header as request identifier instead of
  generating a random UUID — the delivery ID already uniquely
  identifies each webhook request

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@neekolas neekolas merged commit dbbd661 into xmtplabs:main Mar 25, 2026
4 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.

Switch to Pino logger

2 participants