🧹 Code health: Replace console.log with structured logging - #67
Conversation
Co-authored-by: Foshati <140284749+Foshati@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe API adds a shared Pino logger with environment-based configuration and replaces console logging in request completion, error handling, and server startup paths. ChangesAPI logging
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Express
participant API Server
participant Logger
participant Pino
API Server->>Logger: initialize configured logger
Logger->>Pino: set level and optional pretty transport
Express->>API Server: complete request
API Server->>Logger: log request metadata
Express->>API Server: report error
API Server->>Logger: log error
API Server->>Logger: log startup port
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
demo/apps/api/src/server.ts (2)
41-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using
pino-httpfor Express request logging.Instead of manually hooking into request completion to log metrics, consider using the standard
pino-httpmiddleware. It automatically handles request/response serialization, calculates precise response times, and integrates seamlessly with Express.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@demo/apps/api/src/server.ts` around lines 41 - 46, Replace the manual request-completion logging around the logger.info call with pino-http middleware integrated into the Express server setup. Configure it to provide request/response serialization and response duration metrics, and remove the redundant custom completion hook while preserving the existing logger integration.
81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider replacing remaining
console.logstatements.The PR objective states that
console.logis replaced withpinostructured logging, yet severalconsole.logstatements remain for the startup banner immediately following this line. For consistency, consider replacing them withlogger.info, or conditionally output the banner only in development.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@demo/apps/api/src/server.ts` at line 81, Replace the remaining startup-banner console.log statements immediately after the Express server startup log with logger.info calls, preserving their output and using the existing logger for consistent structured logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@demo/apps/api/src/logger.ts`:
- Around line 3-14: Update the environment check used by the exported logger so
the pino-pretty transport is enabled only when NODE_ENV is explicitly
development or unset, while production, staging, and test use the default logger
configuration. Adjust isProduction or replace it with a positive development
predicate, preserving the existing log level behavior.
In `@demo/apps/api/src/server.ts`:
- Around line 41-46: Update the request completion log in the server request
handler to use req.path instead of req.url, while preserving the existing
method, statusCode, and durationMs fields.
---
Nitpick comments:
In `@demo/apps/api/src/server.ts`:
- Around line 41-46: Replace the manual request-completion logging around the
logger.info call with pino-http middleware integrated into the Express server
setup. Configure it to provide request/response serialization and response
duration metrics, and remove the redundant custom completion hook while
preserving the existing logger integration.
- Line 81: Replace the remaining startup-banner console.log statements
immediately after the Express server startup log with logger.info calls,
preserving their output and using the existing logger for consistent structured
logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34108e01-887f-42af-8e59-97fd56708d8a
⛔ Files ignored due to path filters (1)
demo/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
demo/apps/api/package.jsondemo/apps/api/src/logger.tsdemo/apps/api/src/server.ts
| const isProduction = process.env.NODE_ENV === 'production'; | ||
|
|
||
| export const logger = pino({ | ||
| level: process.env.LOG_LEVEL || 'info', | ||
| ...(isProduction ? {} : { | ||
| transport: { | ||
| target: 'pino-pretty', | ||
| options: { | ||
| colorize: true, | ||
| }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a positive check for the development environment to avoid crashes.
Enabling pino-pretty for all non-production environments (e.g., staging, test) can cause the application to crash on startup if devDependencies are pruned in those environments. It is safer to explicitly check for development (or an unset environment).
🛡️ Proposed fix
-const isProduction = process.env.NODE_ENV === 'production';
+const isDevelopment = process.env.NODE_ENV === 'development' || !process.env.NODE_ENV;
export const logger = pino({
level: process.env.LOG_LEVEL || 'info',
- ...(isProduction ? {} : {
+ ...(isDevelopment ? {
transport: {
target: 'pino-pretty',
options: {
colorize: true,
},
},
- }),
+ } : {}),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isProduction = process.env.NODE_ENV === 'production'; | |
| export const logger = pino({ | |
| level: process.env.LOG_LEVEL || 'info', | |
| ...(isProduction ? {} : { | |
| transport: { | |
| target: 'pino-pretty', | |
| options: { | |
| colorize: true, | |
| }, | |
| }, | |
| }), | |
| const isDevelopment = process.env.NODE_ENV === 'development' || !process.env.NODE_ENV; | |
| export const logger = pino({ | |
| level: process.env.LOG_LEVEL || 'info', | |
| ...(isDevelopment ? { | |
| transport: { | |
| target: 'pino-pretty', | |
| options: { | |
| colorize: true, | |
| }, | |
| }, | |
| } : {}), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@demo/apps/api/src/logger.ts` around lines 3 - 14, Update the environment
check used by the exported logger so the pino-pretty transport is enabled only
when NODE_ENV is explicitly development or unset, while production, staging, and
test use the default logger configuration. Adjust isProduction or replace it
with a positive development predicate, preserving the existing log level
behavior.
| logger.info({ | ||
| method: req.method, | ||
| url: req.url, | ||
| statusCode: res.statusCode, | ||
| durationMs: duration | ||
| }, 'Request completed'); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid logging potentially sensitive query parameters.
Logging the full req.url can inadvertently expose sensitive data (e.g., tokens or PII) passed in query strings. Consider logging req.path instead to safely omit query parameters, or configure a redaction mechanism.
🛡️ Proposed fix
logger.info({
method: req.method,
- url: req.url,
+ url: req.path,
statusCode: res.statusCode,
durationMs: duration
}, 'Request completed');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info({ | |
| method: req.method, | |
| url: req.url, | |
| statusCode: res.statusCode, | |
| durationMs: duration | |
| }, 'Request completed'); | |
| logger.info({ | |
| method: req.method, | |
| url: req.path, | |
| statusCode: res.statusCode, | |
| durationMs: duration | |
| }, 'Request completed'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@demo/apps/api/src/server.ts` around lines 41 - 46, Update the request
completion log in the server request handler to use req.path instead of req.url,
while preserving the existing method, statusCode, and durationMs fields.
1 participant
🎯 What: Replaced
console.logwith structured logging usingpinoindemo/apps/api/src/server.ts. Createdlogger.tsfor central configuration, usingpino-prettyfor development readability and updating the error middleware to uselogger.error.💡 Why: Structured logging improves observability and debugging in production environments, making logs easily parseable by log management tools. This improves maintainability by centralizing logging configurations and preventing raw console outputs.
✅ Verification: Verified by starting the dev server and confirming logs output properly using
pino-pretty. Confirmed the linter and tests passed.✨ Result: A more robust logging system for the API, resolving the code health issue without changing external behavior.
PR created automatically by Jules for task 16409319671503239675 started by @Foshati
Summary by CodeRabbit
LOG_LEVELenvironment setting.