refactor(be): request logging #63
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughA new SQL migration has been introduced to create a Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
apps/backend/prisma/migrations/20241205005919_request_logs/migration.sql (1)
2-13: Add indexes to improve query performance on the RequestLog tableTo optimize database queries on the
RequestLogtable, consider adding indexes on columns that are frequently filtered or sorted upon, such ascreatedAt,method,path, orstatus.Example of adding indexes:
CREATE INDEX idx_requestlog_createdat ON "RequestLog" ("createdAt"); CREATE INDEX idx_requestlog_method ON "RequestLog" ("method"); CREATE INDEX idx_requestlog_path ON "RequestLog" ("path");apps/backend/src/modules/logs-cleanup/logs-cleanup.controller.ts (1)
6-6: Consider replacing@Controllerwith@Injectablefor non-HTTP functionalitiesSince
LogsCleanupControllerdoes not handle any HTTP requests and is used for initializing and scheduling tasks, using@Controllermay be misleading. Replacing it with@Injectableclarifies its purpose.Apply this change:
-@Controller("logs-cleanup") +@Injectable()apps/backend/src/modules/logs-cleanup/logs-cleanup.service.ts (2)
7-7: Consider making MAX_COUNT configurableThe hardcoded limit of 500,000 records should be configurable through environment variables to allow for different environments and requirements.
-const MAX_COUNT = 500_000; +const MAX_COUNT = Number(process.env.MAX_REQUEST_LOGS) || 500_000;
24-31: Consider implementing batch deletionThe current implementation might cause performance issues when deleting a large number of records at once. Consider implementing batch deletion to reduce database load.
Example approach:
const BATCH_SIZE = 1000; let deleted = 0; while (true) { const batch = await this.prisma.requestLog.findMany({ where: { createdAt: { lte: last.createdAt } }, take: BATCH_SIZE, }); if (batch.length === 0) break; await this.prisma.requestLog.deleteMany({ where: { id: { in: batch.map(log => log.id) } } }); deleted += batch.length; }apps/backend/src/app.module.ts (1)
45-47: Consider excluding health check and GraphQL playground routesThe current implementation logs all routes including health checks and GraphQL playground, which might generate unnecessary logs.
-consumer.apply(RequestLoggerMiddleware).forRoutes("*"); +consumer + .apply(RequestLoggerMiddleware) + .exclude( + "health", + { path: GRAPHQL_PATH, method: RequestMethod.GET } // GraphQL playground + ) + .forRoutes("*");apps/backend/prisma/schema.prisma (1)
82-92: Add documentation comments to the RequestLog modelConsider adding documentation comments to describe the purpose and fields of the model.
+/// Stores HTTP request logs for monitoring and debugging purposes model RequestLog { id BigInt @id @default(autoincrement()) + /// HTTP method (GET, POST, etc.) method String + /// Request path path String + /// HTTP status code status Int + /// Request duration in milliseconds duration Int + /// Response body (if captured) response String? + /// Client's user agent string userAgent String? createdAt DateTime @default(now()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
apps/backend/prisma/migrations/20241205005919_request_logs/migration.sql(1 hunks)apps/backend/prisma/schema.prisma(1 hunks)apps/backend/src/app.module.ts(3 hunks)apps/backend/src/enums/log.enum.ts(1 hunks)apps/backend/src/middleware/request-logger-middleware.ts(1 hunks)apps/backend/src/modules/departure/departure.controller.ts(1 hunks)apps/backend/src/modules/logger/log.interceptor.ts(0 hunks)apps/backend/src/modules/logs-cleanup/logs-cleanup.controller.ts(1 hunks)apps/backend/src/modules/logs-cleanup/logs-cleanup.module.ts(1 hunks)apps/backend/src/modules/logs-cleanup/logs-cleanup.service.ts(1 hunks)apps/backend/src/modules/platform/platform.controller.ts(1 hunks)apps/backend/src/modules/stop/stop.controller.ts(1 hunks)apps/web/src/app/not-found.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- apps/backend/src/modules/logger/log.interceptor.ts
- apps/web/src/app/not-found.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/backend/src/modules/logs-cleanup/logs-cleanup.module.ts
🧰 Additional context used
📓 Learnings (1)
apps/backend/prisma/schema.prisma (1)
Learnt from: krystxf
PR: krystxf/metro-now#28
File: backend-nest/prisma/schema.prisma:73-83
Timestamp: 2024-11-12T12:31:10.218Z
Learning: In the file `backend-nest/prisma/schema.prisma`, for the `Log` model, adding `userId` or `requestId` fields is unnecessary because the application currently doesn't have user registration or API keys.
🔇 Additional comments (3)
apps/backend/src/enums/log.enum.ts (1)
13-13: LGTM!
Adding REQUEST_LOGS_CLEANUP to the LogMessage enum enhances the clarity and consistency of log messages.
apps/backend/src/modules/stop/stop.controller.ts (1)
20-20: LGTM!
Removing LogInterceptor is appropriate as logging is now managed by RequestLoggerMiddleware. Continuing to use CacheInterceptor ensures caching behavior remains intact.
apps/backend/src/modules/platform/platform.controller.ts (1)
36-36: Verify logging coverage for platform endpoints
The removal of LogInterceptor aligns with the PR's objective to refactor request logging. However, we should verify that the new RequestLoggerMiddleware adequately captures all necessary information for these platform endpoints, especially for error cases and large response payloads.
✅ Verification successful
Let me gather more information about the middleware configuration and logging setup.
Let me check the implementation of the RequestLoggerMiddleware to verify its logging coverage.
Let me get the complete implementation of the middleware to verify error handling and logging details.
Logging coverage is properly implemented for platform endpoints
The RequestLoggerMiddleware is well-configured to handle all platform endpoints with comprehensive logging:
- Applied globally to all routes via
forRoutes("*") - Captures essential request data: method, path, user-agent, status code, and duration
- Properly handles response bodies with string/JSON serialization
- Includes error handling for logging failures
- Has specific handling for platform endpoints via the
ignoreResponsecheck
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new RequestLoggerMiddleware is properly configured to handle platform endpoints
# Check if RequestLoggerMiddleware is properly imported and configured in the app module
rg -l "RequestLoggerMiddleware" "apps/backend/src/app.module.ts"
# Check if the middleware is configured to handle all routes
ast-grep --pattern 'configure(consumer: MiddlewareConsumer) {
$$$
consumer
.apply(RequestLoggerMiddleware)
.forRoutes($$$)
}'
Length of output: 233
Script:
#!/bin/bash
# Check the actual configuration in app.module.ts
rg -A 10 "configure.*MiddlewareConsumer" apps/backend/src/app.module.ts
# Check the RequestLoggerMiddleware implementation
rg -l "class RequestLoggerMiddleware" apps/backend/src/
Length of output: 306
Script:
#!/bin/bash
# Check the implementation of RequestLoggerMiddleware
rg -A 20 "class RequestLoggerMiddleware" apps/backend/src/middleware/request-logger-middleware.ts
# Check if it handles response data and errors
rg "response\." apps/backend/src/middleware/request-logger-middleware.ts
Length of output: 956
Script:
#!/bin/bash
# Get the complete middleware implementation using cat
cat apps/backend/src/middleware/request-logger-middleware.ts
# Also check for any error handling patterns
rg -A 5 "catch|error" apps/backend/src/middleware/request-logger-middleware.ts
Length of output: 2386
| res.on("finish", async () => { | ||
| const duration = Date.now() - start; | ||
| const { | ||
| method, | ||
| url: path, | ||
| headers: { "user-agent": userAgent = null }, | ||
| } = req; | ||
| const { statusCode } = res; | ||
| const responseString = | ||
| typeof responseBody === "string" | ||
| ? responseBody | ||
| : JSON.stringify(responseBody); | ||
|
|
||
| const ignoreResponse = ["/v1/stop/all", "/v1/platform/"].some( | ||
| (item) => path.startsWith(item), | ||
| ); | ||
|
|
||
| if (path.startsWith("/status")) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Log the request details to the database | ||
| await this.prisma.requestLog.create({ | ||
| data: { | ||
| method, | ||
| path, | ||
| status: statusCode, | ||
| duration, | ||
| userAgent, | ||
| response: ignoreResponse ? null : responseString, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to log request:", error); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Potential PII leakage from logging full response bodies
Storing the complete response body in the database may inadvertently capture sensitive or personally identifiable information (PII), leading to security and compliance risks. It's important to ensure that sensitive data is not logged.
Recommendations:
- Sanitize or redact sensitive information from the response before logging.
- Limit logging to metadata or summaries rather than full response bodies.
- Implement conditional logging to only log responses from non-sensitive endpoints.
| model RequestLog { | ||
| id BigInt @id @default(autoincrement()) | ||
| method String | ||
| path String | ||
| status Int | ||
| duration Int | ||
| response String? | ||
| userAgent String? | ||
|
|
||
| createdAt DateTime @default(now()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add index and field constraints to RequestLog model
Consider the following improvements to optimize queries and ensure data integrity:
model RequestLog {
id BigInt @id @default(autoincrement())
- method String
+ method String @db.VarChar(10)
path String
- status Int
+ status Int @db.SmallInt
duration Int
response String?
userAgent String?
createdAt DateTime @default(now())
+
+ @@index([createdAt(sort: Desc)])
}The changes:
- Add index on createdAt for faster cleanup queries
- Limit method length as HTTP methods are short
- Use SmallInt for status codes as they're small numbers
📝 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.
| model RequestLog { | |
| id BigInt @id @default(autoincrement()) | |
| method String | |
| path String | |
| status Int | |
| duration Int | |
| response String? | |
| userAgent String? | |
| createdAt DateTime @default(now()) | |
| } | |
| model RequestLog { | |
| id BigInt @id @default(autoincrement()) | |
| method String @db.VarChar(10) | |
| path String | |
| status Int @db.SmallInt | |
| duration Int | |
| response String? | |
| userAgent String? | |
| createdAt DateTime @default(now()) | |
| @@index([createdAt(sort: Desc)]) | |
| } |
| @UseInterceptors(CacheInterceptor) | ||
| @CacheTTL(4 * 1000) |
There was a problem hiding this comment.
💡 Codebase verification
Logging implementation for departure endpoints needs enhancement
The RequestLoggerMiddleware implementation has some limitations that should be addressed for departure endpoints:
- Query parameters are not being logged, which is crucial for debugging complex departure queries
- No response size limits or truncation logic is implemented, risking large payloads
- No specific handling for cached responses is present
Required changes:
- Add
req.queryto the logged data for better debugging - Implement response size limits to prevent storage issues
- Add cache hit/miss indicators in the logs
🔗 Analysis chain
Verify logging coverage for departure endpoints with complex queries
The removal of LogInterceptor is consistent with the logging refactor. However, these departure endpoints handle complex query parameters and potentially large response payloads. We should ensure that the new RequestLoggerMiddleware:
- Properly captures all query parameters for debugging
- Handles large response payloads efficiently
- Respects the cache TTL when logging cached responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify logging configuration for departure endpoints
# Check if the middleware implements response size limits
rg -A 5 "class RequestLoggerMiddleware" | rg "response.*body|maxSize|truncate"
# Check if query parameter logging is implemented
ast-grep --pattern 'class RequestLoggerMiddleware {
$$$
log($$$) {
$$$
query: $$$
$$$
}
}'
# Check for cache-aware logging configuration
rg "CacheInterceptor.*RequestLoggerMiddleware|RequestLoggerMiddleware.*CacheInterceptor" -A 5
Length of output: 287
Script:
#!/bin/bash
# Let's try a different approach to find the middleware implementation
# Find the middleware file first
fd "request.*logger.*middleware" --type f
# Search for logging-related patterns in the codebase
rg -g "!{test,spec}*" "implements.*NestMiddleware|extends.*NestMiddleware" -A 5
# Look for request/response logging patterns
rg -g "!{test,spec}*" "class.*Logger|class.*Log" -A 10
# Check for query parameter handling in the codebase
rg -g "!{test,spec}*" "req\.query|request\.query" -A 3
Length of output: 5062
Script:
#!/bin/bash
# Let's examine the request logger middleware implementation in detail
# Check the complete implementation of request logger middleware
cat apps/backend/src/middleware/request-logger-middleware.ts
# Look for request log schema to understand what's being logged
fd "prisma.schema" --type f --exec cat {}
# Check for any configuration related to response size limits
fd "config|constants" --type f | rg -i "log|size|limit" --files-with-matches | xargs cat
Length of output: 2297
krystxf
force-pushed
the
refactor/be-logging
branch
from
effb0e7 to
636898a
Compare
krystxf
force-pushed
the
refactor/be-logging
branch
from
636898a to
3749a52
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/backend/src/app.module.ts (1)
31-31: Consider grouping related modules togetherThe
LogsCleanupModuleandLoggerModuleare related to logging functionality but are separated in the imports array. Consider grouping them together for better code organization.PrismaModule, LoggerModule, + LogsCleanupModule, StatusModule, GtfsModule, RouteModule, - LogsCleanupModule, ConfigModule.forRoot(configModuleConfig),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
apps/backend/prisma/migrations/20241205005919_request_logs/migration.sql(1 hunks)apps/backend/prisma/schema.prisma(1 hunks)apps/backend/src/app.module.ts(3 hunks)apps/backend/src/enums/log.enum.ts(1 hunks)apps/backend/src/middleware/request-logger-middleware.ts(1 hunks)apps/backend/src/modules/departure/departure.controller.ts(1 hunks)apps/backend/src/modules/logger/log.interceptor.ts(0 hunks)apps/backend/src/modules/logs-cleanup/logs-cleanup.controller.ts(1 hunks)apps/backend/src/modules/logs-cleanup/logs-cleanup.module.ts(1 hunks)apps/backend/src/modules/logs-cleanup/logs-cleanup.service.ts(1 hunks)apps/backend/src/modules/platform/platform.controller.ts(1 hunks)apps/backend/src/modules/stop/stop.controller.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/backend/src/modules/logger/log.interceptor.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/backend/src/enums/log.enum.ts
- apps/backend/src/modules/logs-cleanup/logs-cleanup.module.ts
- apps/backend/prisma/schema.prisma
- apps/backend/src/modules/stop/stop.controller.ts
- apps/backend/src/middleware/request-logger-middleware.ts
- apps/backend/src/modules/platform/platform.controller.ts
- apps/backend/src/modules/departure/departure.controller.ts
- apps/backend/src/modules/logs-cleanup/logs-cleanup.controller.ts
- apps/backend/src/modules/logs-cleanup/logs-cleanup.service.ts
- apps/backend/prisma/migrations/20241205005919_request_logs/migration.sql
🔇 Additional comments (2)
apps/backend/src/app.module.ts (2)
3-3: LGTM: Import statements are well-organized
The new imports are properly structured and align with the middleware implementation requirements.
Also applies to: 11-11, 16-16
48-52: Consider middleware application strategy
The middleware is currently applied to all routes (*). This might have some implications:
- It will log all requests, including GraphQL operations at
GRAPHQL_PATH - Health checks and metrics endpoints might get logged unnecessarily
- High-traffic applications might generate excessive logs
Consider adding exclusion patterns for specific routes where logging isn't critical.
Changes
Summary by CodeRabbit
Release Notes
New Features
RequestLogtable for enhanced logging of HTTP requests.LogsCleanupModulefor automatic cleanup of old request logs.RequestLoggerMiddlewareto log request details to the database.Improvements
Bug Fixes
These updates enhance the application's logging capabilities and maintain the integrity of existing functionalities.