feat: Implement structured logging with NestJS Pino and a global exce…#559
Open
PaulicStudios wants to merge 2 commits intodevfrom
Open
feat: Implement structured logging with NestJS Pino and a global exce…#559PaulicStudios wants to merge 2 commits intodevfrom
PaulicStudios wants to merge 2 commits intodevfrom
Conversation
📝 WalkthroughWalkthroughAdds structured logging (nestjs-pino/pino) and a global AllExceptionsFilter; integrates logger instances across multiple controllers and updates main/app module to use the new logger and filters. No public API signatures were removed; a few controller method signatures were extended (team queue methods). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant NestApp as NestJS App
participant Controller as Controller (Auth/Event/Team/Match/User)
participant Service as Service Layer
participant DB as Database
participant Logger as Pino Logger
participant Filter as AllExceptionsFilter
Client->>NestApp: HTTP request
NestApp->>Controller: route handler
Controller->>Logger: log attempt_{action} (userId, metadata)
Controller->>Service: perform action
Service->>DB: query/update
DB-->>Service: result
Service-->>Controller: result
Controller-->>Client: response
alt exception thrown
Controller->>Filter: exception propagated
Filter->>Logger: log 4xx/5xx with context
Filter-->>Client: uniform error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/team/team.controller.ts (1)
272-286:⚠️ Potential issue | 🟡 Minor
joinQueueandleaveQueuelog entries are missinguserId, making them unattributable.Every other mutating endpoint in this controller (and across the PR) logs which user performed the action. Queue operations are important audit events that should include the actor.
♻️ Proposed fix
`@UseGuards`(JwtAuthGuard, MyTeamGuards) `@Put`(`event/:${EVENT_ID_PARAM}/queue/join`) - async joinQueue(`@Team`() team: TeamEntity) { + async joinQueue(`@Team`() team: TeamEntity, `@UserId`() userId: string) { if (team.inQueue) throw new BadRequestException("You are already in the queue."); if (!(await this.eventService.hasEventStartedForTeam(team.id))) throw new BadRequestException("The event has not started yet."); this.logger.log({ action: "join_queue", teamId: team.id, + userId, }); return this.teamService.joinQueue(team.id); } `@UseGuards`(JwtAuthGuard, MyTeamGuards) `@Put`(`event/:${EVENT_ID_PARAM}/queue/leave`) - async leaveQueue(`@Team`() team: TeamEntity) { + async leaveQueue(`@Team`() team: TeamEntity, `@UserId`() userId: string) { if (!team.inQueue) throw new BadRequestException("You are not in the queue."); this.logger.log({ action: "leave_queue", teamId: team.id, + userId, }); return this.teamService.leaveQueue(team.id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/team/team.controller.ts` around lines 272 - 286, The joinQueue (and the corresponding leaveQueue) log is missing the actor; update joinQueue(`@Team`() team: TeamEntity) and leaveQueue to include the authenticated user's id in the logger payload by accepting the user from the auth context (e.g. add a parameter to the handler to get request.user or a `@User`() user with user.id provided by JwtAuthGuard) and add userId: user.id to the this.logger.log calls before calling teamService.joinQueue/teamService.leaveQueue so the audit entry includes the actor.
🧹 Nitpick comments (4)
api/src/match/match.controller.ts (1)
27-27: Inconsistent logger declaration compared to other controllers.Other controllers in this PR (e.g.,
TeamController,EventController,UserController) declare the logger as:private readonly logger = new Logger(ControllerName.name);This file uses a string literal and omits
readonly:private logger = new Logger("MatchController");♻️ Align with the rest of the codebase
- private logger = new Logger("MatchController"); + private readonly logger = new Logger(MatchController.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/match/match.controller.ts` at line 27, Update the logger declaration in MatchController to match other controllers: change the property declaration from a non-readonly string-based logger to a readonly logger using the class name (i.e., replace the existing private logger = new Logger("MatchController") with a private readonly logger = new Logger(MatchController.name)) so it aligns with TeamController/EventController/UserController patterns and prevents accidental reassignment.api/src/common/TypeormExceptionFilter.ts (1)
25-30: Consider whetherexception.messagein the response leaks internal details.The
error: exception.messagefield may expose TypeORM internals (e.g., entity/table names, query fragments) to API consumers. TheAllExceptionsFilteruses a generic message for non-HTTP exceptions. You could align this by removing or generalizing theerrorfield.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/common/TypeormExceptionFilter.ts` around lines 25 - 30, The response currently returns potentially sensitive internals via error: exception.message in the TypeormExceptionFilter when sending HttpStatus.NOT_FOUND; change the JSON payload to avoid leaking internals by removing the error field or replacing it with a generic value (e.g., "Not Found" or "Resource not found") so it matches the AllExceptionsFilter behavior, and log the full exception.message privately using the filter's logger (e.g., inside the TypeormExceptionFilter.catch handler) for debugging instead of returning it to the client.api/src/common/AllExceptionsFilter.ts (1)
42-51: Logging stack traces for 4xx errors is likely too noisy.Every
BadRequestException,UnauthorizedException,NotFoundException, etc. will dump a full stack trace as a warning. In production, these are frequent and expected — the stack trace adds little diagnostic value for client errors while significantly increasing log volume.Consider logging only the message for 4xx and reserving stack traces for 5xx:
♻️ Reduce 4xx log verbosity
} else { // 4xx errors - if (exception instanceof Error) { - this.logger.warn( - `HTTP Exception: ${exception.message}`, - exception.stack, - ); - } else { - this.logger.warn(`HTTP Exception: ${exception}`); - } + this.logger.warn( + `HTTP Exception: ${exception instanceof Error ? exception.message : exception}`, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/common/AllExceptionsFilter.ts` around lines 42 - 51, The current 4xx branch in AllExceptionsFilter logs full stack traces for client errors; change the logic to detect HTTP status (use the HttpException/getStatus() or existing status extraction used earlier in the filter) and for statuses < 500 log only the exception message via this.logger.warn(`HTTP Exception: ${message}`) without the stack, reserving stack logging (this.logger.warn/this.logger.error with exception.stack) only for status >= 500 or non-HTTP errors; update the block that checks "exception instanceof Error" to consult the HTTP status and conditionally include the stack trace accordingly.api/src/event/event.controller.ts (1)
97-103: Logs fire before the service call — they record intent, not confirmed outcome.If
eventService.createEvent(...)(or any of the other service calls in this controller) throws, the log entry has already been emitted indicating the action succeeded. This pattern is repeated across every mutating endpoint (join_event,lock_event,unlock_event, etc.) and inteam.controller.tsas well.Either move the log after the
await(for async calls) or name the actionattempt_*to make the intent semantics explicit.♻️ Example fix for one call site
- this.logger.log({ - action: "create_event", - userId, - eventName: createEventDto.name, - }); - - return this.eventService.createEvent( + const result = await this.eventService.createEvent( userId, createEventDto.name, // ... ); + + this.logger.log({ + action: "create_event", + userId, + eventName: createEventDto.name, + }); + + return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/event/event.controller.ts` around lines 97 - 103, Logs are being emitted before the service call so they record intent not outcome; for each mutating controller method (e.g., createEvent in event.controller.ts, joinEvent, lockEvent, unlockEvent, and corresponding methods in team.controller.ts) move the this.logger.log call to after the await this.eventService.* (or await this.teamService.*) so it runs only on successful completion, or alternatively change the action name to "attempt_*" (e.g., "attempt_create_event") to indicate intent; update all occurrences consistently and ensure any early-return/error paths do not produce a success-style log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/auth/auth.controller.ts`:
- Around line 120-122: The catch block currently calls logger.error("Error in
FortyTwo callback:", e) which misuses NestJS Logger.error signature and risks
leaking sensitive data; update the catch to (1) detect BadRequestException vs
other errors (check instanceof BadRequestException) and extract a safe, minimal
message for logs, (2) pass a string stack trace (e.stack || String(e)) as the
second argument to logger.error rather than the raw Error object, and (3) avoid
serializing the whole Error (do not pass e directly) so sensitive OAuth response
bodies/tokens are not logged; reference the catch(e) block and the logger.error
call in auth.controller.ts and preserve BadRequestException message handling
while ensuring only safe strings are logged.
In `@api/src/common/AllExceptionsFilter.ts`:
- Around line 17-19: AllExceptionsFilter's catch currently assumes an HTTP
context; add a guard that checks the incoming ArgumentsHost type and returns
early for non-HTTP contexts so HTTP-specific calls aren't invoked. In the catch
method (AllExceptionsFilter.catch), before calling host.switchToHttp() or using
this.httpAdapter.getRequestUrl()/reply(), check host.getType && host.getType()
=== 'http' (or equivalent isHttp check) and skip the HTTP response
construction/this.httpAdapter.reply() when false; leave logging/exception
handling for non-HTTP contexts unchanged.
---
Outside diff comments:
In `@api/src/team/team.controller.ts`:
- Around line 272-286: The joinQueue (and the corresponding leaveQueue) log is
missing the actor; update joinQueue(`@Team`() team: TeamEntity) and leaveQueue to
include the authenticated user's id in the logger payload by accepting the user
from the auth context (e.g. add a parameter to the handler to get request.user
or a `@User`() user with user.id provided by JwtAuthGuard) and add userId: user.id
to the this.logger.log calls before calling
teamService.joinQueue/teamService.leaveQueue so the audit entry includes the
actor.
---
Nitpick comments:
In `@api/src/common/AllExceptionsFilter.ts`:
- Around line 42-51: The current 4xx branch in AllExceptionsFilter logs full
stack traces for client errors; change the logic to detect HTTP status (use the
HttpException/getStatus() or existing status extraction used earlier in the
filter) and for statuses < 500 log only the exception message via
this.logger.warn(`HTTP Exception: ${message}`) without the stack, reserving
stack logging (this.logger.warn/this.logger.error with exception.stack) only for
status >= 500 or non-HTTP errors; update the block that checks "exception
instanceof Error" to consult the HTTP status and conditionally include the stack
trace accordingly.
In `@api/src/common/TypeormExceptionFilter.ts`:
- Around line 25-30: The response currently returns potentially sensitive
internals via error: exception.message in the TypeormExceptionFilter when
sending HttpStatus.NOT_FOUND; change the JSON payload to avoid leaking internals
by removing the error field or replacing it with a generic value (e.g., "Not
Found" or "Resource not found") so it matches the AllExceptionsFilter behavior,
and log the full exception.message privately using the filter's logger (e.g.,
inside the TypeormExceptionFilter.catch handler) for debugging instead of
returning it to the client.
In `@api/src/event/event.controller.ts`:
- Around line 97-103: Logs are being emitted before the service call so they
record intent not outcome; for each mutating controller method (e.g.,
createEvent in event.controller.ts, joinEvent, lockEvent, unlockEvent, and
corresponding methods in team.controller.ts) move the this.logger.log call to
after the await this.eventService.* (or await this.teamService.*) so it runs
only on successful completion, or alternatively change the action name to
"attempt_*" (e.g., "attempt_create_event") to indicate intent; update all
occurrences consistently and ensure any early-return/error paths do not produce
a success-style log.
In `@api/src/match/match.controller.ts`:
- Line 27: Update the logger declaration in MatchController to match other
controllers: change the property declaration from a non-readonly string-based
logger to a readonly logger using the class name (i.e., replace the existing
private logger = new Logger("MatchController") with a private readonly logger =
new Logger(MatchController.name)) so it aligns with
TeamController/EventController/UserController patterns and prevents accidental
reassignment.
…lers and filters, including prefixing action logs with "attempt_".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/auth/auth.controller.ts (1)
120-141:⚠️ Potential issue | 🟠 MajorRemove redundant logging in catch blocks or adjust
AllExceptionsFilterto skip logging for 4xx errors.The controller logs
BadRequestExceptionat ERROR level and then throws it, butAllExceptionsFiltercatches it and logs again at WARN level. This results in duplicate log entries for the same error event. Either remove the logging from these catch blocks and rely on the filter, or configure the filter to skip logging for client errors (4xx) and only log server errors (5xx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/auth/auth.controller.ts` around lines 120 - 141, The controller currently logs BadRequestException and other errors inside the FortyTwo callback catch block (see the catch handling that checks instanceof BadRequestException, constructs errorMessage/errorStack, calls this.logger.error, then throws), which duplicates logging done by AllExceptionsFilter; remove the this.logger.error calls in this catch block and simply rethrow the appropriate exceptions (preserve the existing throw e and throw new BadRequestException(...) behavior), or alternatively update AllExceptionsFilter to skip logging for 4xx errors by checking exception.getStatus() < 500 before logging; choose one approach and apply consistently so client (4xx) errors are logged only once.
🧹 Nitpick comments (1)
api/src/match/match.controller.ts (1)
22-27: Moveloggerdeclaration before the constructor for consistency.The field is declared after the constructor body, which is syntactically valid but inconsistent with
auth.controller.ts(whereloggerprecedes the constructor) and the general TypeScript class-field convention.♻️ Suggested reorder
`@Controller`("match") export class MatchController { + private readonly logger = new Logger(MatchController.name); + constructor( private readonly matchService: MatchService, private readonly eventService: EventService, ) {} - - private readonly logger = new Logger(MatchController.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/match/match.controller.ts` around lines 22 - 27, Move the private readonly logger field declaration to appear before the constructor in the MatchController class to match the class-field ordering used elsewhere (e.g., auth.controller.ts); locate the logger defined as "private readonly logger = new Logger(MatchController.name)" and place it above the constructor that injects MatchService and EventService so fields are declared before dependency-injecting constructor parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/src/auth/auth.controller.ts`:
- Around line 120-141: The controller currently logs BadRequestException and
other errors inside the FortyTwo callback catch block (see the catch handling
that checks instanceof BadRequestException, constructs errorMessage/errorStack,
calls this.logger.error, then throws), which duplicates logging done by
AllExceptionsFilter; remove the this.logger.error calls in this catch block and
simply rethrow the appropriate exceptions (preserve the existing throw e and
throw new BadRequestException(...) behavior), or alternatively update
AllExceptionsFilter to skip logging for 4xx errors by checking
exception.getStatus() < 500 before logging; choose one approach and apply
consistently so client (4xx) errors are logged only once.
---
Nitpick comments:
In `@api/src/match/match.controller.ts`:
- Around line 22-27: Move the private readonly logger field declaration to
appear before the constructor in the MatchController class to match the
class-field ordering used elsewhere (e.g., auth.controller.ts); locate the
logger defined as "private readonly logger = new Logger(MatchController.name)"
and place it above the constructor that injects MatchService and EventService so
fields are declared before dependency-injecting constructor parameters.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ption filter.
Summary by CodeRabbit
Bug Fixes
Chores
Changes