-
Notifications
You must be signed in to change notification settings - Fork 0
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] 디스코드 알림 로직 추가 #126
base: develop
Are you sure you want to change the base?
[FEAT] 디스코드 알림 로직 추가 #126
Conversation
Test Results115 files 115 suites 11s ⏱️ Results for commit 97e577e. ♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
/noti 멋있게 하루만에 완성하고 올리는건데, 오히려 이도저도 아니게되었네요.. |
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.
/noti
@coli-geonwoo 사소한 질문 1개랑 컨벤션 리뷰 1개 남기고 approve했습니다.
@EnableConfigurationProperties(DiscordProperties.class) | ||
public class DiscordNotifier { | ||
private static final String NOTIFICATION_PREFIX = ":rotating_light: [**Error 발생!**]\n```\n"; |
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.
✅ 반영완료
return Arrays.stream(throwable.getStackTrace()) | ||
.map(StackTraceElement::toString) | ||
.limit(STACK_TRACE_LENGTH) | ||
.collect(Collectors.joining(System.lineSeparator())); |
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.
10줄로 정한 이유가 있나요?
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.
- jda(java discord api) 발송 제한이 2000줄
- 10줄 넘어로 stack trace 많이 본 적이 없어서요.
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.
/noti @coli-geonwoo
- 개인적으로는 Discord Bot이 전반적으로 어떻게 돌아가는지 추가적으로 설명해 주었으면 좋겠어요.
- 현재 디스코드 알림은 500 에러 상황에서만 던지기로 했던 것 같은데, 확인 부탁드릴께요 (Aspect 안 쓰고 ExceptionHandler에서 DiscordNotifier 주입받아서 사용하면 될 것 같은데...)
- 이전 테스크 중에 로그 남기는 것이 있었는데, 현재 잘 작동하고 있나요?
@Getter | ||
@ConfigurationProperties(prefix = "discord") | ||
public class DiscordProperties { | ||
|
||
private final String token; | ||
private final String channelId; | ||
|
||
public DiscordProperties(String token, String channelId) { | ||
validate(token); | ||
validate(channelId); | ||
this.token = token; | ||
this.channelId = channelId; | ||
} |
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.
o? application.yml 추가해야 하는 거 아닌가요?
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.
dev와 prod에 업데이트 해놓았습니다.
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.
token 값 탈취당하면 저희 토큰 활용해서 디도스 보낼 수 있어서 당연히 secret 파일에 업데이트 해놓았슴다.
@AfterThrowing(pointcut = "restControllers()", throwing = "exception") | ||
public void sendDiscordNotification(JoinPoint joinPoint, Exception exception) { | ||
discordNotifier.sendErrorMessage(exception); | ||
} |
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 에러도 예외 던지잖아요! 500 에러 일때만 알림 줘야죠!
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 로 로깅되는 것들을 발송한다! 라고 착각하고 있었네요.
aspect 없애고, globalexceptionhandler에서 ServerException이나 Exception(핸들링 되지 않은 예외) 올 때만 발송하도록 하였습니다.
private JDA initializeJda(String token) { | ||
try { | ||
return JDABuilder.createDefault(token).build().awaitReady(); | ||
} catch (InterruptedException e) { | ||
throw new DTInitializationException(InitializationErrorCode.JDA_INITIALIZATION_FAIL); | ||
} | ||
} |
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.
JDA가 뭐죠?
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.
Java로 래핑한 Discord API 입니다.
특정 봇 토큰 발급 후, API를 발송한 봇 토큰을 넣어 초기화한 후, 발송할 channel ID를 대상으로 메시지 발송 및 수신을 도와주는 라이브러리에요. 자세한 작동원리는 저도 공부하는 중이라 문서화 해놓겠습니다.
public interface ChannelNotifier { | ||
|
||
void sendErrorMessage(Throwable throwable); | ||
} |
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.
[조언 부탁]
이거 interface화 한 이유
-
DiscordNotifier 생성자에서 JDA를 초기화하고 있음
-
여기서 JDA 초기화란 Discord bot의 토큰을 넣어 Discord API를 JAVA 로 래핑하는 라이브러리인 JDA를 생성함을 의미함.
-
그러나, JDA에 들어가는 토큰은 실제 유효한 토큰만 가능함(null 값이나 임의 String을 넣으면 터짐)
=> 여기서 문제가 발생함 -
test에서 Notifier는 모킹되어야 함(아니면 실제 채널에 폭탄 메시지감<- 이미 한번 갔던 것 처럼)
-
모킹은 특정 객체의 상속객체를 만듦으로서 생성됨 즉, 구현체인 DiscordNotifier가 먼저 생성되어야 함
-
그런데 앞서 말했듯 DiscordNotifier의 JDA는 무조건 유효한 token만으로 초기화됨, 따라서 properties에 Null이나 "testToken" 같은 값을 넣으면 Jda초기화가 안되어 모킹자체가 안됨
=> test context 자체가 bean creation 단계에서 터져서 테스트가 다 터짐
내가 할 수 있었던 선택
1안 : Profile로 "dev" ,"prod"에서만 사용하자
-> DiscordNotifier가 붙인 GlobalExceptionHandler도 연쇄적으로 Profile을 분리해주어야 함
-> Profile 명시적으로 선언하는 것은 테스트를 의식한 것 같아 싫음
2안: 인터페이스 분리해서 test에서 모킹가능하도록 하자
->그래, 이게 상대적으로 낫다...
마음에 들진 않으므로 대안 있으면 제발 주세요...이거만 1시간째 고민중..
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.
- 해당 이유라면 인터페이스를 도입하는 것이 좋을 것 같아요. 그리고 Local 환경에서도 알림 안가게 할 꺼면 그냥 2개 객체 만들고 사용해도 좋을 것 같네요. 최근 영한님 강의에서 본 코드 공유해드려요.
@Slf4j
@Configuration
public class PayConfig {
@Bean
@Profile({"local", "test"})
public LocalPayClient localPayClient() {
return new LocalPayClient();
}
@Bean
@Profile({"prod", "dev"})
public TossPayClient tossPayClient() {
return new TossPayClient();
}
}
위와 같이 적은 후에, Test 환경의 application.yml에서는 기본 값으로 test 프로파일로 실행하게 하면 될 듯!
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.
그리고 클래스 네이밍 자체가 좀 그래;;
ChannelNotifier 인터페이스와 DiscordNotifier 구현체라고 하면, "채널이 뭐지? 왜 알리지?", "그러면 디스코드 채널 말고 다른 알림이 있나?"라는 생각이 들 것 같음
그러면 차라리 ErrorNotifier 와 구현체로 DiscordNotifier, ConsoleNotifier를 구현체로 하고, 각각 프로파일 상황에 맞춰 사용하면 좋을 듯!
/noti 한번 개선해보았습니당. 천천히 확인해주세요! |
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.
/noti @coli-geonwoo
수정된 내용 바탕으로 리뷰 남겼습니다! 리뷰 반영해주세요!
- 이게 실제 알람갔을 때 보여지는 예시 한 번만 공유해주세요!
|
||
discord: | ||
channel-id: testChannelId | ||
token: testToken |
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.
이거 flyway랑 동일하게 추가할 것이면 차라리 ---
위로 올려 적는 편이 좋겠죠?
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.
아니다. 어짜피 ErrorNotifier
구현체가 2개로 생길꺼니까 그냥 제거 되겠네요.
public interface ChannelNotifier { | ||
|
||
void sendErrorMessage(Throwable throwable); | ||
} |
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.
- 해당 이유라면 인터페이스를 도입하는 것이 좋을 것 같아요. 그리고 Local 환경에서도 알림 안가게 할 꺼면 그냥 2개 객체 만들고 사용해도 좋을 것 같네요. 최근 영한님 강의에서 본 코드 공유해드려요.
@Slf4j
@Configuration
public class PayConfig {
@Bean
@Profile({"local", "test"})
public LocalPayClient localPayClient() {
return new LocalPayClient();
}
@Bean
@Profile({"prod", "dev"})
public TossPayClient tossPayClient() {
return new TossPayClient();
}
}
위와 같이 적은 후에, Test 환경의 application.yml에서는 기본 값으로 test 프로파일로 실행하게 하면 될 듯!
public interface ChannelNotifier { | ||
|
||
void sendErrorMessage(Throwable throwable); | ||
} |
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.
그리고 클래스 네이밍 자체가 좀 그래;;
ChannelNotifier 인터페이스와 DiscordNotifier 구현체라고 하면, "채널이 뭐지? 왜 알리지?", "그러면 디스코드 채널 말고 다른 알림이 있나?"라는 생각이 들 것 같음
그러면 차라리 ErrorNotifier 와 구현체로 DiscordNotifier, ConsoleNotifier를 구현체로 하고, 각각 프로파일 상황에 맞춰 사용하면 좋을 듯!
@ExceptionHandler(DTServerErrorException.class) | ||
public ResponseEntity<ErrorResponse> handleServerException(DTServerErrorException exception) { | ||
log.error("message: {}", exception.getMessage()); | ||
channelNotifier.sendErrorMessage(exception); | ||
return toResponse(exception.getHttpStatus(), exception.getMessage()); | ||
} |
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.
[선택] channelNotifier.sendErrorMessage(exception)
실행 중에 에러 나면 어떻게 해야 되려나...
일단 써보고 문제 생기면 바꿔도 상관 없을 듯?
@MockitoBean | ||
private ChannelNotifier channelNotifier; | ||
|
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.
아까전에 위에서 한 이야기 반영되면 빼도 될 것 같아요!
BaseControllerTest와 BaseDocumentTest, BaseServiceTest 포함해서요!
/noti |
🚩 연관 이슈
closed #121
🗣️ 리뷰 요구사항 (선택)
@unifolio0
이거 하나만 인정해..

콜리식 일처리
는 일정잡고 미루는게 아니란걸..