-
Notifications
You must be signed in to change notification settings - Fork 3
[Feat] MDC 로깅 구현 #121
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
[Feat] MDC 로깅 구현 #121
Conversation
JJUYAAA
left a 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.
수고하셨습니다! 궁금한 점이 있어 리뷰 남겼습니다!
| finally { | ||
| filterChain.doFilter(request, response); | ||
| MDC.clear(); | ||
| } |
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.
이부분에 MDC.clear()를 넣은 이유가 따로 있을까요? 기능적으로 문제는 없어 보이지만, MDCLoggingFilter에서 이미 finally 블록으로 MDC.clear()를 처리하고 있기 떄문에 JwtFilter의 clear() 호출은 중복되어 불필요하다는 생각이 듭니다!
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.
주연님 말씀이 맞는 것 같습니다 사실상 중복 코드인 것 같아요. 처음에는 필터단에서의 동작 방식 자체에 대해 약간 부족하게 알고있기도 했고, 혹시나하는 마음에 MDC 를 put 하는 위치마다 모두 clear 를 해주도록 했었는데 그럴 필요가 없을 것 같네용 좋은 지적 감사합니다!
다만 이 브랜치를 베이스 삼아 다음 PR을 열어두어었다보니 잘못하면 커밋 내역이 꼬여버릴 것 같아서 이 부분은 추후에 고쳐둘게요!
JangIkhwan
left a 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.
수고하셨어요. 로그 파일을 심각도 별로 나누어서 저장하는 점이 인상깊어요! 그리고 궁금한 점이 있는데요. MDCLoggingFilter에서 X-Request-ID 헤더값은 Nginx에서 넘어오는게 맞나요??
네 맞습니다. |
Related issue 🛠
Work Description 📝
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
로컬에서 확인하고싶으시면 logs 폴더는 미리 만들어두셔야합니다