[BE] 요청 로그에 사용자 정보를 추가 #298

ehBeak · 2024-08-22T08:26:08Z

작업 내용

요청 로그에 사용자 정보를 추가합니다.

사용자 인증이 필요한 API를 요청한 경우 : USERID:{userId}

사용자 인증이 필요없는 API를 요청한 경우 : USERID:ANONYMOUS

특이 사항

`LogFilter`가 `Filter` 구현이 아닌 `Interceptor`을 구현하도록 수정

기존의 Filter를 구현한 LogFilter 대신에 HandlerInterceptor를 구현한 LogInterceptor를 새로 구현했습니다.
로직은 기존의 LogFilter와 동일합니다.

수정 이유
로그에 남길 사용자 정보를 가져오기 위해서, '인증된' 사용자의 아이디를 가져와야 합니다.
때문에 요청으로 들어온 쿠키 값에서 id를 추출하고 아이디가 유효한지 검증하는 과정이 필요합니다.

만약 쿠키가 유효하지 않다면 예외가 발생합니다. Filter에서 예외가 발생하면 GlobalExceptionHanlder에서 잡지 못하기 때문에 클라이언트에게 500을 응답합니다. 이를 해결하기 위해 HandlerInterceptor을 구현하도록 수정했습니다.

리뷰 요구사항 (선택)

AOP 적용 예정
현재 로그를 남기는 과정에서 아래와 같은 문제가 발생합니다. 아래 문제를 AOP로 해결할 예정입니다.
(1) LogInterceptor에서 쿠키가 있는 경우 인증정보를 가져오기 위해 인증을 거칩니다. 때문에 이중 인증이 발생하는 API가 있습니다.
(2) 예외 발생시, (클라이언트에게는 4XX을 반환하지만)응답 로그가 남지 않습니다.
(Filter를 구현하면 이를 해결할 수 있지만 클라이언트에게 400을 응답하는 것이 더 우선이라 생각하여 Interceptor로 두었습니다.)

github-actions · 2024-08-22T08:27:20Z

Test Results

138 tests 138 ✅ 9s ⏱️
30 suites 0 💤
30 files 0 ❌

Results for commit 12be148.

♻️ This comment has been updated with latest results.

seokmyungham

배키 고생하셨어요 🔥
필터에서 발생한 예외를 ExceptionHandler로 처리하기 위해 인터셉터로 변경하셨네요
저는 꽤 합리적인 판단이라고 생각해요 😀

현재 로그를 남기는 과정에서 아래와 같은 문제가 발생합니다. 아래 문제를 AOP로 해결할 예정입니다.
(1) LogInterceptor에서 쿠키가 있는 경우 인증정보를 가져오기 위해 인증을 거칩니다. 때문에 이중 인증이 발생하는 API가 있습니다.

AOP를 어떤 방식으로 사용하실지는 잘 모르겠지만,
제 개인적인 생각에는 현재 로그 인터셉터에서 JWT 로직에 관한 부분만 새로운 인터셉터를 생성해서 책임 분리하는 방법도 좋아보여요.
인터셉터 체인을 사용해서 가장 앞단에 JWT 와 관련된 인터셉터를 두고, 클레임 정보만 다음 인터셉터, ArguementResolver로 넘기도록하면 될 것 같은데 어떻게 생각하시나요?

토큰을 다루느라 LogGenerator에 쿠키를 파싱하는 로직이 섞여있는데 이를 함께 분리하면 될 것 같아요

(2) 예외 발생시, (클라이언트에게는 4XX을 반환하지만)응답 로그가 남지 않습니다.

제가 테스트를 한 환경에서는 요청 응답 로그가 모두 정상적으로 보이는데 😅
혹시 어떤 부분을 말씀하시는 건지 자세히 알려주실 수 있을까요??

그리고 또 인터셉터에서 토큰 검증이 실패할 경우에
아무런 요청, 응답 로그도 보이지 않는데 의도하신 부분인지 궁금해요 😊

seokmyungham · 2024-08-22T14:01:23Z

backend/src/main/java/kr/momo/config/filter/LogGenerator.java

+        log.info("REQUEST [{}][USERID:{}][{} {}]", traceId, userInfo, httpMethod, requestURI);
+    }
+
+    private Optional<String> getCookieValue(Cookie[] cookies) {


이 로직은 LogGenerator 클래스 역할과 어울리지 않는 것 같아요
사용자 정보를 매개변수로 받는게 한 가지 일만 수행하는 관점에서 더 적절해보이는데 어떻게 생각하시나요?

네 맞습니다. 어색하다고 느꼈는데, 인터셉터를 분리하는 방향으로 해결할 수 있겠네요!

제 개인적인 생각에는 현재 로그 인터셉터에서 JWT 로직에 관한 부분만 새로운 인터셉터를 생성해서 책임 분리하는 방법도 좋아보여요.

위 내용을 반영해서 수정했습니다~

seokmyungham · 2024-08-22T14:02:11Z

backend/src/main/java/kr/momo/config/filter/LogGenerator.java

+            userInfo = String.valueOf(jwtManager.extract(userInfo));
+        }
+
+        log.info("REQUEST [{}][USERID:{}][{} {}]", traceId, userInfo, httpMethod, requestURI);


응답 로그에도 사용자 정보를 남기면 좋을 것 같은데, 요청 로그에만 명시한 이유가 궁금합니다!

요청에 사용자 정보를 남겨 두었으니, 응답 로그는 traceId로 해당 사용자가 요청하는 것인지 판단할 수 있다고 생각했습니다.

pr 본문을 예로 들면, userId가 2인 요청 로그의 traceId가 8032f46f 이므로 userId가 2인 사용자의 응답 로그를 보려면 traceId가 8032f46f인 응답 로그를 찾아보는 식입니다!

응답 로그만 확인했을 때도 정보가 명확했으면 좋겠어요 😊
사용자 정보를 응답 로그에 추가하는 건 별로일까요?

동의합니다. 방대한 양의 로그를 한 번 찾는 것도 수고로운데, 다른 로그를 다시 찾아봐야 한다면 오버헤드가 더 클 것 같아요

seokmyungham · 2024-08-22T14:35:15Z

backend/src/main/java/kr/momo/config/WebConfig.java

@@ -1,20 +1,28 @@
 package kr.momo.config;

 import java.util.List;
+import kr.momo.config.filter.LogInterceptor;


패키지 이름도 수정하면 좋을 것 같아요!!

동의합니다!

ikjo39

배키 금방 구현하셨네요!
재즈가 올려준 리뷰만 반영한다면 크게 수정할 부분은 보이지 않아보여 approve 하겠습니다. 😁

ikjo39 · 2024-08-22T16:30:40Z

backend/src/main/java/kr/momo/config/filter/LogInterceptor.java

+    public void afterCompletion(
+            HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex
+    ) {
+        long startTime = (Long) request.getAttribute("startTime");


하드 코딩된 startTime도 상수로 분리해보는건 어떨까요?

ikjo39 · 2024-08-22T16:46:10Z

backend/src/main/java/kr/momo/config/WebConfig.java

@@ -1,20 +1,28 @@
 package kr.momo.config;

 import java.util.List;
+import kr.momo.config.filter.LogInterceptor;


동의합니다!

ehBeak · 2024-08-22T18:52:50Z

리뷰 주신 내용 반영해서 다시 올립니다! 확인 부탁드려요~

인터셉터 체인을 사용해서 가장 앞단에 JWT 와 관련된 인터셉터를 두고, 클레임 정보만 다음 인터셉터, ArguementResolver로 넘기도록하면 될 것 같은데 어떻게 생각하시나요?

사용자 정보를 추출하는 Interceptor와 로그를 남기는 Interceptor를 따로 분리했습니다. ArguementResolver로 넘기는 부분이 잘 이해가 안 갑니다. 한번 더 설명해 주실 수 있을까요? 🥹

제가 테스트를 한 환경에서는 요청 응답 로그가 모두 정상적으로 보이는데 😅
혹시 어떤 부분을 말씀하시는 건지 자세히 알려주실 수 있을까요??

쿠키 값이 유효하지 않을 때 응답 로그가 뜨지 않습니다!
쿠키 값이 유효하지 않은 상태로 요청을 보내면 요청이 UserInfoInterceptor를 지날 때, 예외가 발생합니다. 해당 예외는 GlobalExceptionHanlder에서 잡고 예외 관련 로그가 남게 됩니다. 하지만 LoggingInterceptor를 거치지 않아서 요청과 응답로그가 뜨지 않습니다.

그리고 또 인터셉터에서 토큰 검증이 실패할 경우에
아무런 요청, 응답 로그도 보이지 않는데 의도하신 부분인지 궁금해요 😊

이 부분과 같은 맥락인 것 같습니다.

seokmyungham

배키 요청 사항 반영하느라 고생하셨어요 🔥🔥

사용자 정보를 추출하는 Interceptor와 로그를 남기는 Interceptor를 따로 분리했습니다. ArguementResolver로 넘기는 부분이 잘 이해가 안 갑니다. 한번 더 설명해 주실 수 있을까요?

현재 코드에서 사용자 정보를 HttpServletRequest에 저장했으니
ArgumentResolver에선 또 토큰 인증을 할 필요없이 가져다 꺼내쓰기만 하면 될 것 같아서 드린 의견입니다 😊

@Override
public Long resolveArgument(
        @NonNull MethodParameter parameter,
        ModelAndViewContainer mavContainer,
        @NonNull NativeWebRequest webRequest,
        WebDataBinderFactory binderFactory
) {
    HttpServletRequest request = (HttpServletRequest) webRequest.getNativeRequest();
    String userInfo = (String) request.getAttribute(USER_INFO);

      ... userInfo가 올바르지 않으면 적절히 예외 반환 ..

    return Long.parseLong(userInfo);
}

seokmyungham · 2024-08-22T19:21:24Z

backend/src/main/java/kr/momo/config/interceptor/UserInfoInterceptor.java

+
+@Component
+@RequiredArgsConstructor
+public class UserInfoInterceptor implements HandlerInterceptor {


토큰 인증에 대한 책임을 가지고 있으니 더 명확한 이름으로 JwtInterceptor 는 어떤가요?

네 좋습니다~ 제안해 주신 이름이 확실히 명확한 것 같아요!

seokmyungham · 2024-08-22T19:24:11Z

backend/src/main/java/kr/momo/config/WebConfig.java

+    public void addInterceptors(InterceptorRegistry registry) {
+        registry.addInterceptor(userInfoInterceptor).addPathPatterns(BASE_URL);
+        registry.addInterceptor(loggingInterceptor).addPathPatterns(BASE_URL);
+    }


명시적으로 인터셉터 호출 순서를 지정해주었으면 좋겠어요 😊

seokmyungham · 2024-08-22T19:31:29Z

backend/src/main/java/kr/momo/config/filter/LogGenerator.java

+            userInfo = String.valueOf(jwtManager.extract(userInfo));
+        }
+
+        log.info("REQUEST [{}][USERID:{}][{} {}]", traceId, userInfo, httpMethod, requestURI);


응답 로그만 확인했을 때도 정보가 명확했으면 좋겠어요 😊
사용자 정보를 응답 로그에 추가하는 건 별로일까요?

seokmyungham · 2024-08-22T19:38:41Z

backend/src/main/java/kr/momo/config/interceptor/LoggingInterceptor.java

+        String traceId = traceIdGenerator.generateShortUuid();
+        MDC.put(TRACE_ID, traceId);
+        logGenerator.logRequest(traceId, request);
+        request.setAttribute(START_TIME, System.currentTimeMillis());


현재 로깅에 필요한 정보를 MDC와 HttpServletRequest에 나누어 저장하고 있는데 정보 관리를 일관되게 하면 더 좋을 것 같다는 생각이 들어요.

제가 생각했을 땐 로그와 관련된 정보(TraceId, userInfo)는 MDC , 애플리케이션 로직에 필요한 정보(userInfo)를 각각 분리해서 저장하면될 것 같아요. 그럼 사용하는 입장에서 실수하지 않고 목적에 따라 일관되게 사용할 수 있겠죠?

MDC에 관리해야 할 정보가 늘어난다면 MDC.clear()로 모든 키-값 페어를 삭제할 수 있습니다.

오호 그렇네요! 반영하겠습니다 👍

hw0603

고생했어요 배키!

재즈가 추가로 남겨준 리뷰를 제외하면 더 수정할 부분은 없는 것 같아 저는 Approve 드립니다🙂 반영 후 머지해주세요

ehBeak · 2024-08-22T22:35:07Z

리뷰 반영 했습니다! 확인 부탁드려요!

응답 로그만 확인했을 때도 정보가 명확했으면 좋겠어요 😊
사용자 정보를 응답 로그에 추가하는 건 별로일까요?

필요성을 느끼지 못해서 추가하지 않았는데,
요청 정보를 꼭 확인 해야하는 불편함이 있는 것 같습니다. 요청과 통일성도 필요하고요! 반영하여 수정했습니다~

현재 코드에서 사용자 정보를 HttpServletRequest에 저장했으니
ArgumentResolver에선 또 토큰 인증을 할 필요없이 가져다 꺼내쓰기만 하면 될 것 같아서 드린 의견입니다 😊

아 그렇군요! 이해했습니다 👍
다만, 리팩토링한 코드는 MDC를 사용하고 있으니, 지금대로 유지하겠습니다 :)

ehBeak added 3 commits August 22, 2024 13:19

refactor: IP 주소 정보 삭제

0c203e0

feat: 요청에 사용자 정보 추가

4942a3b

fix: 예외 발생시 500응답 수정

900eb58

ehBeak requested review from ikjo39, hw0603, seokmyungham and seunghye218 as code owners August 22, 2024 08:26

ehBeak self-assigned this Aug 22, 2024

ehBeak added 🐈‍⬛ 백엔드 백엔드 관련 이슈에요 :) ♻️ 리팩터링 코드를 깎아요 :) 🚀 기능 기능을 개발해요 :) labels Aug 22, 2024

seokmyungham requested changes Aug 22, 2024

View reviewed changes

refactor: 패키지 이름 수정

5c2df39

ikjo39 approved these changes Aug 22, 2024

View reviewed changes

ehBeak added 2 commits August 23, 2024 03:17

refactor: 역할에 따라 인터셉터 분

a6702fd

refactor: userInfo 상수로 추출

98dea92

hw0603 added this to the 4차 데모데이 milestone Aug 22, 2024

seokmyungham requested changes Aug 22, 2024

View reviewed changes

hw0603 approved these changes Aug 22, 2024

View reviewed changes

ehBeak added 4 commits August 23, 2024 07:11

refactor: 인터셉터 등록 순서 명시적으로 지정

b6beff9

refactor: 인터셉터 이름 수정

ccb5489

refactor: 응답 로그에도 사용자 정보 추가

539d9c3

refactor: MDC를 통해서 유저 정보를 가져오도록 수정

3f23c48

seokmyungham approved these changes Aug 22, 2024

View reviewed changes

Merge branch 'develop' into refactor/288-refactor-info-log

12be148

hwinkr merged commit 820d310 into develop Aug 23, 2024
6 checks passed

hwinkr deleted the refactor/288-refactor-info-log branch August 23, 2024 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] 요청 로그에 사용자 정보를 추가 #298

[BE] 요청 로그에 사용자 정보를 추가 #298

ehBeak commented Aug 22, 2024

github-actions bot commented Aug 22, 2024 •

edited

Loading

seokmyungham left a comment •

edited

Loading

seokmyungham Aug 22, 2024

ehBeak Aug 22, 2024

seokmyungham Aug 22, 2024

ehBeak Aug 22, 2024

seokmyungham Aug 22, 2024

hw0603 Aug 22, 2024

seokmyungham Aug 22, 2024

ikjo39 Aug 22, 2024

ikjo39 left a comment •

edited

Loading

ikjo39 Aug 22, 2024

ikjo39 Aug 22, 2024

ehBeak commented Aug 22, 2024

seokmyungham left a comment •

edited

Loading

seokmyungham Aug 22, 2024

ehBeak Aug 22, 2024

seokmyungham Aug 22, 2024

seokmyungham Aug 22, 2024

seokmyungham Aug 22, 2024

ehBeak Aug 22, 2024

hw0603 left a comment

ehBeak commented Aug 22, 2024

[BE] 요청 로그에 사용자 정보를 추가 #298

[BE] 요청 로그에 사용자 정보를 추가 #298

Conversation

ehBeak commented Aug 22, 2024

관련 이슈

작업 내용

특이 사항

LogFilter가 Filter 구현이 아닌 Interceptor을 구현하도록 수정

리뷰 요구사항 (선택)

github-actions bot commented Aug 22, 2024 • edited Loading

Test Results

seokmyungham left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikjo39 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehBeak commented Aug 22, 2024

seokmyungham left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 left a comment

Choose a reason for hiding this comment

ehBeak commented Aug 22, 2024

`LogFilter`가 `Filter` 구현이 아닌 `Interceptor`을 구현하도록 수정

github-actions bot commented Aug 22, 2024 •

edited

Loading

seokmyungham left a comment •

edited

Loading

ikjo39 left a comment •

edited

Loading

seokmyungham left a comment •

edited

Loading