Conversation
malessani
force-pushed
the
feat/add-pino-logger
branch
from
8caef00 to
8b8e562
Compare
commit: |
There was a problem hiding this comment.
Pull request overview
This PR adds a new @commercelayer/hono-pino-logger package that provides Pino logger middleware for Hono applications with built-in request/response logging support. The package is designed to work with @hono/node-server and offers features like pretty printing in development, configurable log levels, sensitive data redaction, and minimal output mode.
Changes:
- New package
@commercelayer/hono-pino-loggerwith logger creation utilities and Hono middleware - Test suite with unit tests for logger and middleware functionality
- Comprehensive documentation including README with examples and usage patterns
- Example applications demonstrating basic and advanced usage scenarios
- GitHub workflow update to include the new package in pkg-pr-new publishing
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| packages/hono-pino-logger/* | Core package implementation with logger, middleware, types, and tests |
| examples/hono-pino-logger/* | Example applications demonstrating basic and advanced usage |
| .github/workflows/pkg-pr-new.yaml | Updated to include new package in CI/CD pipeline |
| pnpm-lock.yaml | Dependency updates for new package and its dependencies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(middleware).toBeDefined() | ||
| expect(typeof middleware).toBe('function') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The minimalOutput configuration option is not covered by tests. This is a key feature mentioned in the PR description and README. Consider adding test cases to verify that when minimalOutput: true, the serializers and customProps are correctly set to undefined/return undefined, and the output excludes request_id, user_agent, ip, and detailed request/response objects.
| const { | ||
| logger = defaultLogger, | ||
| logRequestBody = false, | ||
| minimalOutput = false, |
There was a problem hiding this comment.
The logResponseBody configuration option is defined in the HttpLoggerConfig interface (types.ts line 48) and documented in the README, but it's never used in the middleware implementation. The config is destructured on line 16 but the value is not applied anywhere. Either implement this feature or remove it from the interface and documentation.
| // Map express-style middleware to Hono | ||
| await new Promise<void>((resolve) => httpLogger(req, res, () => resolve())) | ||
|
|
||
| // Attach logger to context for use in route handlers | ||
| c.set('logger', req.log) |
There was a problem hiding this comment.
The middleware wraps the pinoHttp call in a Promise but doesn't handle potential errors. If pinoHttp throws an error or the Promise rejects, it could crash the application. Consider adding error handling around line 135 to ensure the middleware fails gracefully and logs errors appropriately.
| // Map express-style middleware to Hono | |
| await new Promise<void>((resolve) => httpLogger(req, res, () => resolve())) | |
| // Attach logger to context for use in route handlers | |
| c.set('logger', req.log) | |
| // Map express-style middleware to Hono with error handling | |
| try { | |
| await new Promise<void>((resolve, reject) => { | |
| try { | |
| ;(httpLogger as any)(req, res, (err?: unknown) => { | |
| if (err) { | |
| reject(err) | |
| return | |
| } | |
| resolve() | |
| }) | |
| } catch (err) { | |
| reject(err) | |
| } | |
| }) | |
| } catch (err) { | |
| const logger = (c.get && (c.get('logger') as any)) || defaultLogger | |
| if (logger && typeof logger.error === 'function') { | |
| logger.error({ err }, 'Error in honoHttpLogger middleware') | |
| } | |
| // Continue to next middleware/handler even if logging failed | |
| } | |
| // Attach logger to context for use in route handlers, if available | |
| if ((req as any).log) { | |
| c.set('logger', (req as any).log) | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : { | ||
| req: (req) => { | ||
| return { | ||
| method: req.method, | ||
| url: req.url?.split('?')[0], | ||
| query: req.query, | ||
| body: logRequestBody ? req.body : undefined, | ||
| remote_address: req.remoteAddress, | ||
| remote_port: req.remotePort, | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The serializers object when minimalOutput is false only defines a serializer for 'req', but doesn't include serializers for 'res' and 'err'. This means pino-http will use its default serializers for response and error objects. If you want to customize these as well (or if you want to ensure they follow the same pattern as the request serializer), consider adding explicit serializers for 'res' and 'err'. Alternatively, if the default serializers are acceptable, this is fine as-is.
4 participants
Closes #60
What I did
This PR will add a new package to use a shared logger configuration across projects.
There is also a
minimalOutputconfiguration to reduce the log output.How to test
Checklist