feat - 슬랙 알람 연동 #191

jumining · 2024-08-19T09:41:30Z

Related issue 🚀

closed chore - server error 발생 시 slack 연동 #160

Work Description 💚

신규 회원가입 발생 시 알람 전송
서버 500 내부 에러 발생 시 알람 전송
서버 4XX 오류 발생 시 알람 전송 (local에서 저희가 개발 시에 발생하는 오류는 알림 전송되지 않도록 IP 설정했습니다)

PR 참고 사항

prod dev를 분리하고 싶었는데 잘 알지 못해 local에서 4XX 알람만 안가게 분리만 해두었습니다
메시지 유형은 슬랙 3번채널 테스트 보낸 메시지 확인해주세요! 총 3가지 유형이며 색상과 메시지 유형이 다르도록 했습니다!
예시로 하나만 첨부합니다.

kseysh

고생하셨습니다! 몇가지 리뷰 남겼으니 확인 부탁드려요~

kseysh · 2024-08-19T10:40:05Z

src/main/java/sopt/org/hmh/domain/slack/SlackSender.java

+    public void sendNotificationNewUser(String userName, String os) {
+       sendSlackNotification(newUserWebUrl, "🎉 신규 유저 회원 가입 발생", generateNewUserSlackAttachment(userName, os));
+    }
+
+    public void sendNotificationServerInternalError(Exception e, HttpServletRequest request) {
+        sendSlackNotification(serverErrorWebUrl, "🚨 서버 내부 에러 발생", generateInternalErrorSlackAttachment(e, request));
+    }
+
+    public void sendNotificationServerRuntimeException(ExceptionBase e, HttpServletRequest request) {
+        sendSlackNotification(serverErrorWebUrl, "😭 서버 오류 발생", generateRuntimeExceptionSlackAttachment(e, request));
+    }


P4: Slack 메시지를 보내는 메서드는 static으로 만들어 MessageSender를 사용하는 쪽에서 의존성을 최소한으로 가지도록 설계하는게 더 나을 것이라고 생각되는데 이렇게 설계하신 이유가 있으실까요?

또한, SlackSender 객체의 책임은 메시지를 보내는 것이라고 생각해요.
SlackSender가 다른 객체에서 필요한 메시지를 보내는 메서드를 하나씩 다 만들어주다보면 (ex) sendNotificationNewUser, sendNotificationServerInternalError, sendNotificationServerRuntimeException) MessageSender가 다른 객체가 어떤 메서드가 필요한지를 아는 상태가 되어서 객체의 책임 분리가 확실히 되지 않고 있다는 생각이 듭니다.

적어도 메시지 포매팅은 다른 객체에서 책임을 가지는 것이 맞다고 생각됩니다!

책임 분리 중심으로 코드 수정했는데 한 번 더 봐주시고 리뷰해주시면 좋을 것 같아요 :)
단순 계산 메소드라면 static으로 사용했겠지만 static 사용을 하지 않은 이유는 slack 메시지 전송은 slack 외부 api에 의존하고 있기 때문에 static 보다는 의존성 주입으로 사용하도록 했습니다.
책임 분리에 신경써서 다시 코드 리팩토링했는데 static은 객체 지향 설계 원칙 중 다형성에도 위배되는 것 같아서 객체 지향 중심적으로 코드를 짠다면 이렇게 설계하는게 좋을 것 같아 설계했습니다!

음... 제가 static 사용에 대해서 이야기 한 것은 SlackSender가 static이 아니기 때문에 갖는 장점이 현재로서 없다고 판단되었기 때문이에요.
예를 들어 SlackSender가 필드를 저장하지도 않고, 따라서 객체 생성이 필요 없어 static으로 생성한다면 메모리 측면에서 이득이고, Slack.getInstance 또한 싱글톤으로 구현되어 항상 같은 값을 반환하기도 합니다.

다형성에 어긋난다라는 말에는 동의합니다! 저희가 메시지를 보내는 방식을 Slack이 아닌 다른 방식으로 사용하고자 한다면 분명 OCP에 어긋나는 일일테니까요!
하지만, 현재 사용하는 방식이 다형성을 고려하고 있다기에는 어려운 면이 있을 것 같아요.
현재도 Slack이 아닌 다른 방식으로 메시지를 보내고자 한다면 SlackSender와 SlackSender를 사용하는 모든 부분은 변경해야 할테니까요. 그래서 만약 다형성을 위해 객체 지향 원칙을 지키고자 한다면 SlackSender를 추상화한 MessageSender를 만들고, 이를 상속한 SlackSender를 만들면 좋을 것 같아요.
이후에 메시지 보내는 방식을 변경하고자 할 때 MessageSender를 구현한 새로운 객체를 만들기만 하면 쉽게 메시지 보내는 방식을 변경할 수 있으니 OCP도 잘 만족하는 방식일 것 같아요.

+ 추가적으로 지금 현재는 슬랙 관련 상수 추가 부분만 푸쉬된 것 같은데 책임 분리한 부분 어느 부분인지 알 수 있을까요?

kseysh · 2024-08-19T10:45:43Z

src/main/java/sopt/org/hmh/domain/slack/SlackSender.java

+
+    private Attachment generateNewUserSlackAttachment(String userName, String os) {
+        return Attachment.builder()
+                .color("98ff98")  // 초록색


98ff98이라는 매직 넘버를 초록색이라는 상수 이름으로 사용한다면 불필요한 주석을 줄일 수 있을 것 같아요

kseysh · 2024-08-19T10:53:40Z

src/main/java/sopt/org/hmh/domain/slack/SlackSender.java

+                .fields(List.of(
+                        generateSlackField("User Info", "- ID : " + request.getRemoteUser()
+                                + " \n- IP : " + request.getRemoteAddr()
+                                + " \n- TOKEN: " + request.getHeader("Authorization")),


P1: Bearer 토큰이 Slack에 노출된다는 것이 괜찮은 방식일지 모르겠어요..!
토큰이 아닌 userId와 같은 다른 방식으로 메시지를 보내는 것이 더 안전한 방법일 것이라는 생각이 듭니다!

고민했는데 슬랙에서 클라분들이 오류날 때 메시지와 함께 계속 토큰도 같이 보내주시길래 넣어봤습니다.
토큰 넣는게 고려되는 사항이라면 userid만 넣어도 무방할 것 같아요~

kseysh · 2024-08-19T10:59:35Z

src/main/java/sopt/org/hmh/domain/slack/SlackSender.java

+                .title(requestTime + " 발생 오류 로그")
+                .fields(List.of(
+                        generateSlackField("User Info", "- ID : " + request.getRemoteUser()
+                                + " \n- IP : " + request.getRemoteAddr()


현재 IP는 0:0:0:0:0:0:0:1만 오는 것 같은데, 의도한 동작이 맞을까요?
또한, IP를 메시지 필드로 설정해둔 이유도 궁금해요!

또한, 현재 nginx를 통해 포트 포워딩을 하고 있어서 IP가 제대로 오는지 확인도 한 번 하면 좋을 것 같아요

0:0:0:0:0:0:0:1가 localhost인데 PR메시지에 있는 local일 때는 에러메시지를 보내지 않도록 설정하기 위해 설정해두었어요.
오류 발생 시에 따로 로그 확인안할 수 있도록 최대한 많은 요청 정보를 얻고자 필드에 넣었습니다. 많은 레퍼런스에서 ip주소를 넣기도 하구요

ip확인데 관해서는 현재 코드가 local에 있어서 local에서만 테스트가 가능해서 ip가 제대로 오는지 확인할 수 없는 상황입니다
사실 빼버리고 싶네요

사용하지 않는 정보라면 빼는 것이 맞을 것으로 생각됩니다!

kseysh · 2024-08-19T11:10:43Z

src/main/java/sopt/org/hmh/domain/slack/SlackSender.java

+        try {
+            slackClient.send(webhookUrl, WebhookPayloads.payload(p -> p
+                    .text(title)
+                    .attachments(List.of(attachment))
+            ));
+        } catch (IOException slackError) {
+            log.debug("Slack 통신 예외 발생: {}", slackError.getMessage());
+        }
+    }


외부 라이브러리 try-catch로 감싸주신 것 아주 좋은 것 같아요!

kseysh · 2024-08-19T11:22:53Z

src/main/java/sopt/org/hmh/global/common/exception/CommonControllerAdvice.java

+        if (!Objects.equals(request.getRemoteAddr(), "0:0:0:0:0:0:0:1")) // 로컬에서 오류 발생 시 전송 X
+            slackSender.sendNotificationServerRuntimeException(exception, request);


P3: if문 중괄호로 감싸주시면 좋을 것 같아요!

sonarLint에서 수정 요청하길래 바꾸었는데 이부분 규칙 무시하도록 설정해야겠네요

kseysh · 2024-08-20T14:38:08Z

src/main/java/sopt/org/hmh/domain/slack/constant/AttachmentColor.java

+package sopt.org.hmh.domain.slack.constant;
+
+import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+
+@Getter
+@RequiredArgsConstructor
+public enum AttachmentColor {
+
+    GREEN("98ff98"),
+    ORANGE("ffb700"),
+    RED("ff0000");
+
+    private final String color;
+}


Java에서 제공하는 Color.GREEN.getCode() 이런 방식으로도 사용할 수 있는데 제공하는 것으로 사용하는건 어떤가요?

hex문자로 또 변환을 해줘야하겠지만 변환함수는 만들어서 사용하면 괜찮을 것 같아요 Color사용 좋은 것 같습니당 이후에 색 추가되는 상황에서 사용하기에 편할 것 같구요!😀

… feat/#160-slack-alarm

kseysh

고생하셨습니다~

jumining added 5 commits August 19, 2024 18:28

add - #160 slack 의존성 추가

95f506f

feat - #160 slack으로 메세지 보내는 slackSender 생성

b3fb70b

feat - #160 서버 500에러 발생 시 슬랙 알림 전송

2aa57a1

feat - #160 신규 회원가입자 발생 시 메시지 전송

1f4797b

feat - #160 서버 4XX 에러 발생 시 슬랙 알림 전송

1321438

jumining added ✨ Feat 새로운 기능 추가 👩🏻‍💻 주민 주민이가 작성한 Label labels Aug 19, 2024

jumining requested a review from kseysh August 19, 2024 09:41

jumining self-assigned this Aug 19, 2024

kseysh reviewed Aug 19, 2024

View reviewed changes

jumining added 4 commits August 20, 2024 19:03

add - #160 슬랙 관련 상수 추가

b1d5ba5

feat - #160 slack api 사용과 메세지 구성하는 메소드 분리

9c416cb

feat - #160 분리된 슬랙 메소드 적용

8b66186

chore - #160 필요없는 import문 삭제

7746d21

kseysh reviewed Aug 20, 2024

View reviewed changes

jumining added 5 commits September 10, 2024 13:53

feat - #160 두 개의 메시지 빌더를 묶는 인터페이스 생성 후 리팩토링

e17abec

chore - #160 java.awt.Color 사용

d9f53e8

chore - #160 슬랙메시지빌더가 호출되는 곳에서 이름 수정

406dced

Merge branch 'develop' of https://github.com/Team-HMH/HMH-Server into…

5dd0ecc

… feat/#160-slack-alarm

fix - #160 바뀐 클래스명으로 수정

e79985b

kseysh approved these changes Oct 8, 2024

View reviewed changes

jumining merged commit f19afda into develop Oct 8, 2024
1 check passed

jumining changed the title ~~feat - #160 슬랙 알람 연동~~ feat - 슬랙 알람 연동 Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat - 슬랙 알람 연동 #191

feat - 슬랙 알람 연동 #191

jumining commented Aug 19, 2024

kseysh left a comment

kseysh Aug 19, 2024

kseysh Aug 19, 2024

kseysh Aug 19, 2024

jumining Aug 20, 2024

kseysh Aug 20, 2024

kseysh Aug 19, 2024

kseysh Aug 19, 2024

jumining Aug 19, 2024

kseysh Aug 19, 2024

kseysh Aug 19, 2024

jumining Aug 19, 2024

jumining Aug 19, 2024

kseysh Aug 20, 2024

kseysh Aug 19, 2024

kseysh Aug 19, 2024

jumining Aug 19, 2024

kseysh Aug 20, 2024

jumining Sep 10, 2024 •

edited

Loading

kseysh left a comment

		if (!Objects.equals(request.getRemoteAddr(), "0:0:0:0:0:0:0:1")) // 로컬에서 오류 발생 시 전송 X
		slackSender.sendNotificationServerRuntimeException(exception, request);

feat - 슬랙 알람 연동 #191

feat - 슬랙 알람 연동 #191

Conversation

jumining commented Aug 19, 2024

Related issue 🚀

Work Description 💚

PR 참고 사항

kseysh left a 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

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

Choose a reason for hiding this comment

jumining Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

kseysh left a comment

Choose a reason for hiding this comment

jumining Sep 10, 2024 •

edited

Loading