-
Notifications
You must be signed in to change notification settings - Fork 0
[25.10.01 / TASK-248] Refactor - 에러 로그 개선 및 액세스 로그 추가 #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Walkthrough요청-응답 접근 로그 미들웨어를 추가하고 로깅 유틸/타입을 도입했으며, 로거 JSON 포맷터와 에러/검증 미들웨어를 로깅 유틸로 통합해 에러 흐름을 중앙화했습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as App
participant AL as accessLogMiddleware
participant H as Handlers (Controllers / Other MW)
participant EH as errorHandling.middleware
participant L as Logging Util
C->>A: HTTP Request
A->>AL: accessLogMiddleware(req,res,next)
AL->>AL: recordRequestStart(req)
Note right of AL: res.finish 리스너 등록 (status < 400일 때 logAccess)
AL->>H: next()
alt 정상 흐름 (status < 400)
H-->>A: 응답 작성
A->>L: logAccess 실행 on finish (level = getLogLevel)
A-->>C: Response
else 오류 발생
H->>EH: next(error)
EH->>L: logError(req,res,error)
EH-->>C: Error Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewersPoem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/middlewares/accessLog.middleware.ts (1)
13-18
: res.once('finish') 사용을 고려해보세요.현재
res.on('finish')
를 사용하고 있는데, 이벤트는 한 번만 발생하므로res.once('finish')
를 사용하는 것이 더 명시적이고 효율적입니다.Express의 Response 객체는 요청당 생성되고 GC되므로 메모리 누수 우려는 없지만,
once()
를 사용하면 의도가 더 명확해집니다.- res.on('finish', () => { + res.once('finish', () => { if (res.statusCode < 400) { // 400 이상은 에러 로그로 처리 logAccess(req, res);src/types/express.d.ts (1)
11-12
: 타입 안정성 검토: 옵셔널 프로퍼티 사용을 고려하세요.
requestId
와startTime
이 필수 프로퍼티로 선언되어 있지만, 이들은accessLogMiddleware
에 의해 런타임에 설정됩니다.
src/utils/logging.util.ts
의recordRequestStart
함수를 보면req.requestId = req.requestId || randomUUID()
패턴을 사용하는데, 이는requestId
가 없을 수 있음을 가정합니다.미들웨어가 항상 최상단에 등록되어 있어 실제로는 문제가 없지만, TypeScript 타입 안정성을 위해 옵셔널로 선언하는 것을 고려해보세요:
- requestId: string; - startTime: number; + requestId?: string; + startTime?: number;이렇게 하면 로깅 유틸리티에서 이미 수행하고 있는 null-check가 타입 레벨에서도 반영됩니다.
src/utils/logging.util.ts (1)
94-120
:logAccess
함수의 응답 시간 기본값을 검토하세요.Line 99에서
req.startTime
이 없을 때responseTime
을0
으로 설정하는데, 이는 실제로 시간이 측정되지 않은 경우를 나타내기에는 오해의 소지가 있을 수 있습니다.undefined
로 설정하는 것이 더 명확할 수 있습니다.다음과 같이 수정을 고려해보세요:
- const responseTime = req.startTime ? Date.now() - req.startTime : 0; + const responseTime = req.startTime ? Date.now() - req.startTime : undefined;그리고
AccessLogData
인터페이스에서responseTime
을 선택적 필드로 변경:- responseTime: number; + responseTime?: number;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/app.ts
(2 hunks)src/configs/logger.config.ts
(1 hunks)src/middlewares/accessLog.middleware.ts
(1 hunks)src/middlewares/errorHandling.middleware.ts
(2 hunks)src/middlewares/validation.middleware.ts
(2 hunks)src/types/express.d.ts
(1 hunks)src/types/index.ts
(1 hunks)src/types/logging.ts
(1 hunks)src/utils/__test__/logging.util.test.ts
(1 hunks)src/utils/logging.util.ts
(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-04T14:05:58.537Z
Learnt from: HA0N1
PR: Check-Data-Out/velog-dashboard-v2-api#5
File: src/middlewares/error-handling.middleware.ts:1-2
Timestamp: 2024-12-04T14:05:58.537Z
Learning: `src/middlewares/error-handling.middleware.ts` 파일의 에러 핸들링 미들웨어에서 `NextFunction`을 사용하지 않으며, `err`은 커스텀 에러로 사용되므로 `NextFunction`과 `ErrorRequestHandler`를 임포트할 필요가 없습니다.
Applied to files:
src/middlewares/errorHandling.middleware.ts
📚 Learning: 2024-12-04T13:26:58.075Z
Learnt from: HA0N1
PR: Check-Data-Out/velog-dashboard-v2-api#5
File: src/middlewares/auth.middleware.ts:116-117
Timestamp: 2024-12-04T13:26:58.075Z
Learning: 'velog-dashboard-v2-api' 코드베이스에서는 `src/types/express.d.ts` 파일에서 Express의 `Request` 인터페이스를 확장하여 `user`와 `tokens` 속성을 추가하였습니다.
Applied to files:
src/types/express.d.ts
📚 Learning: 2024-12-06T14:29:50.385Z
Learnt from: HA0N1
PR: Check-Data-Out/velog-dashboard-v2-api#6
File: src/controllers/user.controller.ts:11-12
Timestamp: 2024-12-06T14:29:50.385Z
Learning: TypeScript Express 프로젝트에서, `Express.Request` 인터페이스는 전역으로 확장되어 `user`와 `tokens` 프로퍼티를 포함합니다. `user` 프로퍼티는 `VelogUserLoginResponse | VelogUserVerifyResponse` 타입이고, `tokens`는 `accessToken`과 `refreshToken`을 가진 객체입니다.
Applied to files:
src/types/express.d.ts
🧬 Code graph analysis (7)
src/app.ts (1)
src/middlewares/accessLog.middleware.ts (1)
accessLogMiddleware
(8-21)
src/types/logging.ts (1)
src/types/index.ts (3)
LogContext
(46-46)ErrorLogData
(46-46)AccessLogData
(46-46)
src/utils/logging.util.ts (1)
src/types/logging.ts (3)
LogContext
(4-11)ErrorLogData
(16-23)AccessLogData
(28-33)
src/middlewares/accessLog.middleware.ts (1)
src/utils/logging.util.ts (2)
recordRequestStart
(125-128)logAccess
(94-120)
src/middlewares/errorHandling.middleware.ts (1)
src/utils/logging.util.ts (1)
logError
(51-85)
src/utils/__test__/logging.util.test.ts (1)
src/utils/logging.util.ts (5)
getClientIp
(10-17)getLogLevel
(36-40)createLogContext
(22-31)logError
(51-85)logAccess
(94-120)
src/middlewares/validation.middleware.ts (2)
src/exception/index.ts (1)
BadRequestError
(11-11)src/exception/badRequest.exception.ts (1)
BadRequestError
(3-7)
🔇 Additional comments (22)
src/types/index.ts (1)
45-46
: LGTM! 로깅 타입 export가 잘 구성되어 있습니다.기존 패턴을 따르며 새로운 로깅 관련 타입들을 명확하게 export하고 있습니다.
src/app.ts (1)
28-28
: LGTM! 액세스 로그 미들웨어가 올바른 위치에 등록되었습니다.미들웨어 체인의 최상단에 배치하여 모든 요청을 확실하게 캡처할 수 있도록 했습니다.
res.on('finish')
이벤트를 활용하므로 다른 미들웨어보다 먼저 등록되어도 응답 완료 시점에 로그가 기록됩니다.src/middlewares/accessLog.middleware.ts (1)
8-10
: LGTM! 요청 시작 시점 기록이 적절합니다.
recordRequestStart
를 호출하여requestId
와startTime
을 초기화하는 것은 올바른 접근입니다.src/middlewares/errorHandling.middleware.ts (2)
14-18
: LGTM! CustomError 처리 로직이 올바르게 개선되었습니다.에러 응답을 보내기 전에
logError
를 호출하여 요청 컨텍스트와 함께 에러를 기록하는 흐름이 적절합니다. 응답 상태 코드를 먼저 설정한 후 로깅하므로logError
에서 정확한statusCode
를 참조할 수 있습니다.
24-35
: LGTM! Internal Server Error 처리가 올바르게 개선되었습니다.에러 로깅이 응답 전송 전에 수행되어 요청 컨텍스트와 함께 에러를 추적할 수 있습니다. CustomError 경로와 동일한 패턴을 따르고 있어 일관성이 좋습니다.
src/configs/logger.config.ts (1)
15-32
: 객체 메시지 평탄화 로직이 올바르게 구현되었습니다.새로운 로깅 유틸리티(
logError
,logAccess
)가 구조화된 객체를 전달할 때 평탄화하여 로그 출력을 일관되게 유지하는 방식이 적절합니다.typeof
체크와null
체크로 안전성을 확보했으며, 문자열 메시지에 대한 하위 호환성도 유지됩니다.src/middlewares/validation.middleware.ts (2)
4-4
: 중앙화된 에러 처리를 위한 임포트가 적절합니다.
BadRequestError
를 사용하여 검증 실패를 예외로 전파하는 방식이 새로운 중앙화된 에러 미들웨어 구조와 일치합니다.
13-27
: 검증 실패 시 예외 기반 흐름으로 올바르게 전환되었습니다.직접 HTTP 응답을 반환하던 기존 방식에서
BadRequestError
를 throw하는 방식으로 변경하여, 에러 미들웨어에서 통합 로깅 및 응답 처리가 가능해졌습니다. Express 5.x의 네이티브 프로미스 에러 핸들링과도 호환됩니다.src/utils/__test__/logging.util.test.ts (6)
14-19
: 로거 모킹이 적절하게 구성되었습니다.테스트 격리를 위해 winston 로거를 모킹하여 실제 로그 출력 없이 호출 검증이 가능합니다.
46-63
:getClientIp
테스트 커버리지가 충분합니다.
x-forwarded-for
,x-real-ip
, 폴백 시나리오를 모두 검증하여 다양한 프록시 환경에서의 동작을 보장합니다.
65-77
:getLogLevel
로직이 올바르게 테스트되었습니다.상태 코드별 로그 레벨 매핑(200→info, 404→warn, 500→error)이 정확히 검증됩니다.
79-90
:createLogContext
테스트가 요청 컨텍스트 추출을 정확히 검증합니다.requestId, userId, method, url, ip 등 모든 필드 추출을 확인하며, 선택적 필드(userAgent)의 undefined 처리도 검증합니다.
92-143
:logError
테스트가 다양한 에러 시나리오를 포괄합니다.일반 에러,
CustomError
의 에러 코드 포함, 500 에러의 스택 트레이스 포함 등 핵심 동작이 모두 검증됩니다.
146-161
:logAccess
테스트가 액세스 로그 구조를 정확히 검증합니다.응답 크기, 응답 시간, 상태 코드 등 액세스 로그의 필수 필드들이 올바르게 포함되는지 확인합니다.
src/types/logging.ts (3)
1-11
:LogContext
인터페이스가 명확하게 정의되었습니다.요청 컨텍스트의 공통 필드들을 잘 추상화했으며, 선택적 필드(
userId
,userAgent
,ip
)의 처리가 적절합니다.
13-23
:ErrorLogData
인터페이스가 에러 로깅 요구사항을 충족합니다.
LogContext
를 확장하여 코드 중복을 방지하고, 에러 관련 필드(errorCode
,stack
,responseTime
)를 선택적으로 포함할 수 있어 유연합니다.
25-33
:AccessLogData
인터페이스가 액세스 로깅에 적합합니다.응답 메트릭(
statusCode
,responseTime
,responseSize
)을 명확히 정의하며, 성능 모니터링에 필요한 정보를 포함합니다.src/utils/logging.util.ts (5)
7-17
:getClientIp
함수가 다양한 프록시 환경을 적절히 처리합니다.
x-forwarded-for
의 첫 번째 IP를 우선 사용하고,x-real-ip
,socket.remoteAddress
순으로 폴백하는 로직이 일반적인 프록시 구성과 일치합니다.
19-31
:createLogContext
함수가 요청 컨텍스트를 안전하게 추출합니다.옵셔널 체이닝(
req.user?.id
)과 폴백(req.requestId || randomUUID()
)을 통해 누락된 필드를 안전하게 처리합니다.
33-40
:getLogLevel
함수의 매핑 로직이 명확합니다.HTTP 상태 코드를 로그 레벨로 변환하는 규칙이 직관적이며, 404를 warn으로 분리한 것이 합리적입니다.
42-85
:logError
함수가 포괄적인 에러 로깅을 제공합니다.조건부 스택 트레이스 포함 로직(CustomError 4xx는 제외, 5xx 및 일반 에러는 포함)이 운영 환경에서 적절한 디버깅 정보를 제공하면서도 로그 크기를 관리합니다.
{ message: errorLogData }
래핑이 logger.config.ts의 평탄화 로직과 정확히 연동됩니다.
122-128
:recordRequestStart
함수가 멱등성을 보장합니다.기존
requestId
가 있으면 재사용하고, 없으면 새로 생성하는 방식으로 중복 호출에도 안전합니다.
if (res.statusCode < 400) { | ||
// 400 이상은 에러 로그로 처리 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조건 절이랑 주석이랑 다른데?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
400 미만은 액세스로그로, 400 이상은 에러 로그로 에러 핸들링 미들웨어에서 처리해서 이렇게 적었습니다. 헷갈리긴 하네요ㅎㅎ;;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 400 미만만 액세스 로그, 그 외 에러 로깅
으로 가시죠!
/** | ||
* 기본 로그 컨텍스트 정보 | ||
*/ | ||
export interface LogContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요건 어떤 object 를 보고 따라 만드신 것 일까요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰가 너무 늦었네요,,!
고생하셨습니다!! 코멘트 몇가지만 확인해주시면 좋을 것 같아요!!
좋았던 점
- 로그들을 logging.util.ts로 중앙화한 점이 좋았습니다.
- requestId로 분산된 로그를 하나의 요청으로 추적할 수 있게 된 점이 인상적이었습니다.
})), | ||
}); | ||
return; | ||
throw new BadRequestError(`API 입력 검증 실패, errors: ${errors}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exception 분리하신 부분이 좋은 것 같습니다!👏
*/ | ||
export const getLogLevel = (statusCode: number): 'info' | 'warn' | 'error' => { | ||
if (statusCode < 400) return 'info'; | ||
if (statusCode === 404) return 'warn'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
404는 Bad Request로 알고있는데, warn으로 하신 다은님만의 의도가 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예전에 서비스 운영환경에 액세스로그를 추가했었는데 스캐닝이 자꾸 들어와서 난감하더라고요.
/password.html
/.env
이런 식으로 몇백건씩 들어오는데 아무래도 404로 찍혀서, 나중에 에러 로그를 볼때 분간이 어려울 것 같다는 생각을 했었습니다. 이걸 어떻게 거를 수 있을지는 잘 몰라서 warn으로 내렸습니다ㅎㅎ...
src/types/logging.ts
Outdated
* 에러 로그 데이터 | ||
*/ | ||
export interface ErrorLogData extends LogContext { | ||
logger: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 부분은 error만 들어가는 것으로 이해했는데 맞을까요?
맞다면 string 대신 'error'로 명시해주는 것도 좋을 것 같습니다!
다은님은 어떻게 생각하시는지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로거 설정을 맨 나중에 해서 놓쳤던 부분이네요!! 감사합니다
src/types/logging.ts
Outdated
* 액세스 로그 데이터 | ||
*/ | ||
export interface AccessLogData extends LogContext { | ||
logger: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 동일한 의견입니다!
const jsonFormat = winston.format.printf((info) => { | ||
// info.message가 객체인 경우 | ||
if (typeof info.message === 'object' && info.message !== null) { | ||
return JSON.stringify({ | ||
timestamp: info.timestamp, | ||
level: info.level.toUpperCase(), | ||
logger: info.logger || 'default', | ||
...info.message, // 로그 데이터 평탄화 | ||
}); | ||
} | ||
|
||
return JSON.stringify({ | ||
timestamp: info.timestamp, | ||
level: info.level.toUpperCase(), | ||
logger: 'default', | ||
message: info.message, | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 key 도 같은데 평탄화만 추가되는 부분이라, 아래와 같은 것은 어떨까요?
const jsonFormat = winston.format.printf((info) => {
const isObject = typeof info.message === 'object' && info.message !== null;
return JSON.stringify({
timestamp: info.timestamp,
level: info.level.toUpperCase(),
logger: info.logger || 'default',
...(isObject ? info.message : { message: info.message }),
});
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다!! 감사합니다!
requestId: string; | ||
startTime: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이제 모든 request 가 이 값을 middleware 에서 Injection 받고 시작하는거죠?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 맞습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋았던 점
- request id 를 줘서, 한 사람의 request start to end, 요청 흐름을 모두 볼 수 있게 처리해주셔서 너무 좋았습니다 :). 코드도 대체로 깔끔한 것 같고, 테스트 코드도 좋습니다!
아쉬운 점
- 디테일한 부분에서 코멘트 한 번 참고 부탁드려요!
- 다만,
request id
를 줌으로써 생기는 오버헤드에 대해 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 리뷰가 너무 늦어졌네요;;
죄송합니다..
다른 분들이 잘 남겨주셔서 별도로 제가 볼 부분은 없고, 다른 분들 의견만 반영하고 머지하시면 될 것 같네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/configs/logger.config.ts
(1 hunks)src/middlewares/accessLog.middleware.ts
(1 hunks)src/types/logging.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/configs/logger.config.ts
- src/types/logging.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/middlewares/accessLog.middleware.ts (1)
src/utils/logging.util.ts (2)
recordRequestStart
(125-128)logAccess
(94-120)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test (22)
🔇 Additional comments (2)
src/middlewares/accessLog.middleware.ts (2)
1-2
: LGTM!필요한 타입과 유틸리티 함수를 올바르게 임포트했습니다.
20-20
: LGTM!
next()
호출 위치가 올바릅니다. 로깅은 비동기로 처리되고 요청 처리는 즉시 계속됩니다.
🔥 변경 사항
기존에 메세지 위주로만 남기던 에러 로그들을 개선하고, 액세스 로그를 추가하였습니다.
🏷 관련 이슈
📸 스크린샷 (UI 변경 시 필수)
잘못된 로그인
repo에서의 DB 에러시
쿼리 파라미터 타입 에러
not found (에러 중 유일하게 WARN)
성공적인 액세스 로그
📌 체크리스트
Summary by CodeRabbit