feat(server): migrate to pino for asynchronous structured logging (fi…#881
Conversation
github-actions
Bot
added
enhancement
|
Hi @ManoharKonala, thanks for contributing to InternHack! 🎉 I have automatically:
Our workflows will now analyze your changes to classify:
Tip Ensure your PR description references the issue it resolves (e.g. Happy coding! 🚀 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughBackend logging is migrated to pino with a root logger and child loggers; server index and one service are updated to use the structured logger. Frontend: local clipboard handlers and copy icon imports are removed in favor of a shared CopyButton, merge-conflict markers resolved, and an ESLint hook suppression added. ChangesLogging Infrastructure Migration
Frontend clipboard consolidation and small fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🤖 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 `@server/src/utils/logger.ts`:
- Around line 35-42: The logger.error method is flattening Error instances to {
message, stack } which strips other fields (e.g., Prisma's code/cause); update
the error handling in the error(msg: string, error?: unknown, meta?: unknown)
function so that when error is an Error it is passed as the original object
under the key err (i.e., { err: error }) instead of { err: { message, stack } },
while keeping the existing payload merge with meta and calling
logger.error(payload, msg); this preserves Pino's error serializer and
downstream checks like err?.code in milestone.service.ts.
🪄 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: d2fb7cb3-57d4-4566-a88d-684fb1aed52f
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
server/package.jsonserver/src/index.tsserver/src/utils/logger.ts
| error(msg: string, error?: unknown, meta?: unknown) { | ||
| const errInfo = error instanceof Error | ||
| ? { message: error.message, stack: error.stack } | ||
| : error; | ||
| console.error(fmt("ERROR", module, msg, meta ?? errInfo)); | ||
| const errInfo = | ||
| error instanceof Error | ||
| ? { err: { message: error.message, stack: error.stack } } | ||
| : { err: error }; | ||
|
|
||
| const payload = meta ? { ...errInfo, ...(meta as object) } : errInfo; | ||
| logger.error(payload, msg); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In pino v10, when calling logger.error({ err: someError }, "msg"), does Pino preserve the Error's type/name, stack, and custom enumerable fields compared with manually flattening it to { message, stack }?
💡 Result:
Yes. In Pino, when you call logger.error({ err: someError }, "msg") (where the error is placed under the special top-level key "err"), Pino applies its standard error serializer and serializes the Error into a structured object that includes the Error’s constructor name as type, the message, and the stack, and it also includes any additional enumerable properties attached to the Error. The std error serializer returns an object of the form: - type: constructor name (Error’s name) - message: the supplied error message - stack: the stack when the error was generated - raw: the original Error kept non-enumerable for subsequent serializers - any additional enumerable properties that the original Error had [1] So compared with manually flattening to { message, stack }: - Pino preserves Error’s type/name via type (constructor name) [1] - Pino preserves stack via stack [1] - Pino preserves custom enumerable fields attached to the Error by copying them into the serialized output [1] Note: Pino’s built-in behavior is tied to using the error property name "err" (or the configured errorKey). The core docs state the standard error serializer is used when logging errors under that key (and also when passing an Error instance directly as the first argument) [2].
Citations:
- 1: https://github.com/pinojs/pino-std-serializers
- 2: https://github.com/pinojs/pino/blob/c77d8ec5ce/docs/API.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section of logger.ts
file="server/src/utils/logger.ts"
if [ -f "$file" ]; then
echo "== $file =="
nl -ba "$file" | sed -n '1,200p' | sed -n '25,90p'
else
echo "Missing file: $file"
fi
# Find call sites of logger.error(payload, msg) or of logger.error wrapper
# Search for ".error(" usages in server/src that use this logger util.
rg -n --hidden --glob 'server/src/**/*.{ts,js}' "logger\.error\(" server/src/utils/logger.ts server/src | head -n 50 || true
# Search for the wrapper signature error(msg, error?, meta?)
rg -n --hidden --glob 'server/src/**/*.{ts,js}' "error\\(msg: string, error\\?: unknown, meta\\?: unknown\\)" -S server/src || true
# Search for milestone.service.ts lines around 131-171 to see example errors
target="server/src/module/milestone/milestone.service.ts"
if [ -f "$target" ]; then
echo "== $target =="
nl -ba "$target" | sed -n '110,210p'
else
echo "Missing file: $target"
fiRepository: Sachinchaurasiya360/InternHack
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="server/src/utils/logger.ts"
if [ -f "$file" ]; then
echo "== $file =="
nl -ba "$file" | sed -n '25,90p'
else
echo "Missing file: $file"
fi
rg -n --hidden --glob 'server/src/**/*.{ts,js}' "logger\.error\(" server/src/utils/logger.ts server/src | head -n 80 || true
rg -n --hidden --glob 'server/src/**/*.{ts,js}' "error\\(msg: string, error\\?: unknown, meta\\?: unknown\\)" -S server/src || true
target="server/src/module/milestone/milestone.service.ts"
if [ -f "$target" ]; then
echo "== $target =="
nl -ba "$target" | sed -n '120,200p'
else
echo "Missing file: $target"
fiRepository: Sachinchaurasiya360/InternHack
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="server/src/utils/logger.ts"
if [ -f "$file" ]; then
echo "== $file =="
nl -ba "$file" | sed -n '25,90p'
else
echo "Missing file: $file"
fi
rg -n --hidden --glob 'server/src/**/*.{ts,js}' "logger\.error\(" server/src/utils/logger.ts server/src | head -n 80 || true
rg -n --hidden --glob 'server/src/**/*.{ts,js}' "error\\(msg: string, error\\?: unknown, meta\\?: unknown\\)" -S server/src || true
target="server/src/module/milestone/milestone.service.ts"
if [ -f "$target" ]; then
echo "== $target =="
nl -ba "$target" | sed -n '120,200p'
else
echo "Missing file: $target"
fiRepository: Sachinchaurasiya360/InternHack
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="server/src/utils/logger.ts"
echo "== file exists? =="
ls -l "$file" || true
echo "== wc -l =="
wc -l "$file" || true
echo "== snippet (lines 1-120) =="
awk '{printf "%d\t%s\n", NR, $0}' "$file" | sed -n '1,120p'
echo "== snippet (lines 25-70) =="
awk '{printf "%d\t%s\n", NR, $0}' "$file" | sed -n '25,70p'
echo "== find wrapper signature error(msg: string, error?, meta?) =="
rg -n "error\\(msg: string, error\\?: unknown, meta\\?: unknown\\)" -S "$file" || true
echo "== search for calls to milestone.service.ts around 120-200 =="
target="server/src/module/milestone/milestone.service.ts"
ls -l "$target" || true
wc -l "$target" || true
awk '{printf "%d\t%s\n", NR, $0}' "$target" | sed -n '110,200p'
echo "== find usages of logger.error(...) in server/src =="
rg -n --hidden --glob 'server/src/**/*.{ts,js}' "logger\\.error\\(" server/src | head -n 80 || trueRepository: Sachinchaurasiya360/InternHack
Length of output: 13455
Preserve full Error objects when logging (don’t flatten to { message, stack }).
server/src/utils/logger.ts currently converts Error instances into { err: { message, stack } }, so Pino’s built-in err error serializer can’t retain fields like Prisma’s code/cause (e.g., server/src/module/milestone/milestone.service.ts checks err?.code). Pass the original error under err instead.
Suggested fix
error(msg: string, error?: unknown, meta?: unknown) {
- const errInfo =
- error instanceof Error
- ? { err: { message: error.message, stack: error.stack } }
- : { err: error };
-
- const payload = meta ? { ...errInfo, ...(meta as object) } : errInfo;
- logger.error(payload, msg);
+ const errInfo = error === undefined ? {} : { err: error };
+ const payload = meta
+ ? { ...errInfo, ...(meta as Record<string, unknown>) }
+ : errInfo;
+
+ return Object.keys(payload).length > 0
+ ? logger.error(payload, msg)
+ : logger.error(msg);
},🤖 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 `@server/src/utils/logger.ts` around lines 35 - 42, The logger.error method is
flattening Error instances to { message, stack } which strips other fields
(e.g., Prisma's code/cause); update the error handling in the error(msg: string,
error?: unknown, meta?: unknown) function so that when error is an Error it is
passed as the original object under the key err (i.e., { err: error }) instead
of { err: { message, stack } }, while keeping the existing payload merge with
meta and calling logger.error(payload, msg); this preserves Pino's error
serializer and downstream checks like err?.code in milestone.service.ts.
Sachinchaurasiya360
left a comment
Sachinchaurasiya360
left a comment
There was a problem hiding this comment.
Review
Verdict: Approve with minor notes
Issues
**1. must be a **
It is listed under dependencies but is only used when NODE_ENV !== 'production'. Move it to devDependencies — it drags 15+ transitive packages into the production bundle for no reason.
2. logger.error signature change is a silent breaking change
The old signature was error(msg, error?, meta?). The new implementation wraps the error under { err: { message, stack } } before passing to pino. Any existing call site passing (msg, error) will now serialize differently. Please grep all createLogger consumers and verify no call site is affected before merging.
3. Module prefix lost from log messages
The old logger embedded [module] in the message string. It is now a structured module field on the JSON object (which is better for structured logging), but anyone grepping raw logs for [Redis] or [AI] will silently miss results. Worth a note in the PR description.
4. LOG_LEVEL env var is undocumented
It is consumed in logger.ts but not present in .env.example or any README. Add it.
5. Remaining console.* calls in services
The PR description says scope is intentionally limited to index.ts and logger.ts — that is fine, but please link a follow-up issue so the remaining raw console calls across services are tracked as debt.
Sachinchaurasiya360
left a comment
Sachinchaurasiya360
left a comment
There was a problem hiding this comment.
Review
Verdict: Approve with minor notes
Issues
1. pino-pretty must be a devDependency
It is listed under dependencies but is only used when NODE_ENV !== 'production'. Move it to devDependencies — it drags 15+ transitive packages into the production bundle for no reason.
2. logger.error signature change is a silent breaking change
The old signature was error(msg, error?, meta?). The new implementation wraps the error under { err: { message, stack } } before passing to pino. Any existing call site passing (msg, error) will now serialize differently. Grep all createLogger consumers and verify no call site is affected before merging.
3. Module prefix lost from log messages
The old logger embedded [module] in the message string. It is now a structured module field on the JSON object (better for structured logging), but anyone grepping raw logs for [Redis] or [AI] will silently miss results. Worth noting in the PR description.
4. LOG_LEVEL env var is undocumented
Consumed in logger.ts but absent from .env.example and any README. Add it.
5. Remaining console.* calls in services
Scope intentionally limited to index.ts and logger.ts — that is fine, but please link a follow-up issue so the remaining raw console calls across services are tracked as debt.
|
fix the lint |
- Move pino-pretty to devDependencies - Fix logger.error call site passing meta as error - Document LOG_LEVEL in .env.example files
github-actions
Bot
added
the
type:security
|
@Sachinchaurasiya360 |
Sachinchaurasiya360
merged commit a6611c3
into
Sachinchaurasiya360:main
2 participants
Resolves #880
This PR migrates the backend's centralized logging utility from a synchronous console.log wrapper to Pino, a high-performance, asynchronous JSON logger.
Why
Currently, the backend relies heavily on console.log, console.warn, and console.error. In a high-traffic Node.js environment, writing to process.stdout synchronously blocks the event loop, causing severe latency spikes during heavy I/O operations (like bulk scraping, cron processing, or massive API traffic). Furthermore, unstructured text logs are extremely difficult to index, parse, and filter reliably in production log aggregators (e.g., Datadog, AWS CloudWatch, ELK).
What Changed
Pino Engine: Refactored server/src/utils/logger.ts to instantiate a root Pino logger. This replaces the old fmt() wrapper while keeping the exact same createLogger(moduleName) interface so that no existing call sites across the codebase break.
Graceful Developer Experience: Integrated pino-pretty as a dev dependency. When NODE_ENV !== "production", logs are automatically piped through pino-pretty to provide human-readable, colorized terminal output with accurate timestamps.
Production Readiness: In production, the logger falls back to emitting raw, incredibly fast JSON lines ({"level":30,"time":...}), ensuring optimal performance and seamless integration with observability stacks.
Index Cleanup: Upgraded critical startup and shutdown logs in server/src/index.ts to use the new structured logger instead of direct console.* calls.
Testing / Verification
Verified compilation success (npx tsc --noEmit) with no TypeScript signature errors.
Passed all 106 backend unit tests successfully.
Confirmed startup, database connections, and cron initializations log gracefully in development mode.
Next Steps / Scope
This PR intentionally limits the refactor to index.ts and logger.ts to keep the diff reviewable. Future tech-debt tasks can iteratively replace any remaining raw console.log calls across other services.
Summary by CodeRabbit
New Features
Refactor
Chores