Skip to content
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

QYOG-61 slack 메신저 생성 #146

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

QYOG-61 slack 메신저 생성 #146

wants to merge 4 commits into from

Conversation

ohchanghoon
Copy link
Collaborator

@ohchanghoon ohchanghoon commented Feb 23, 2024

JIRA

QYOG-61

Description

500번 에러 슬랙 알림 전송 구현

To Reviewer

스크린샷 2024-02-23 오후 6 11 27
로 인해서 node_env로 prefix


샘플
스크린샷 2024-02-23 오후 6 12 00

@ohchanghoon ohchanghoon added the Auto 자동화 작업 label Feb 23, 2024
@ohchanghoon ohchanghoon self-assigned this Feb 23, 2024
}

async sendNotification({ statusCode, exceptionError, processError }: { statusCode: number, exceptionError?: HttpInternalServerErrorException, processError?: HttpProcessErrorException }): Promise<void> {
console.log('this.webhookUrl: ', this.webhookUrl);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 필요한 console.log인가요?

@@ -30,6 +34,7 @@ export class HttpInternalServerErrorExceptionFilter
statusCode,
exceptionError,
);
this.slackService.sendNotification({ statusCode, exceptionError });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await 없어도 상관 없나요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

있어야합니다.

Copy link
Member

@rrgks6221 rrgks6221 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment 외로 PR전에 prettier를 적용되면 좋을거같습니다.
npm run format

@@ -30,6 +34,7 @@ export class HttpInternalServerErrorExceptionFilter
statusCode,
exceptionError,
);
this.slackService.sendNotification({ statusCode, exceptionError });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

있어야합니다.

@@ -45,6 +45,7 @@
"@nestjs/platform-express": "^10.0.0",
"@nestjs/swagger": "^7.1.16",
"@nestjs/typeorm": "^10.0.0",
"@slack/client": "^5.0.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 패키지 사용하지 않는거같은데 맞나요??

@@ -56,6 +57,7 @@
"mariadb": "^3.2.2",
"moment": "^2.30.1",
"mysql2": "^3.6.3",
"nestjs-slack-webhook": "^10.3.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 패키지 사용하지 않는거같은데 맞나요??

this.webhookUrl = this.appConfigService.get<string>(ENV_KEY.SERVER_ERROR_WEB_HOOK_URL);
}

async sendNotification({ statusCode, exceptionError, processError }: { statusCode: number, exceptionError?: HttpInternalServerErrorException, processError?: HttpProcessErrorException }): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매개변수를 봤을 때 일반적인 notification을 보내는게 아닌 에러시에만 노티를 보내는애같은데 input interface를 변경하거나 메서드명이 변경되면 좋을듯합니다.

await axios.post(this.webhookUrl, { text: message });
} catch (error) {
console.error('Error sending Slack notification:', error);
throw new Error('Failed to send Slack notification');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter영역에서 호출하는 경우 여기에 걸렸을 때 경우에따라 서버가 죽을수도있어서 console만 찍는게 나을수도있을거같습니다.

}

try {
await axios.post(this.webhookUrl, { text: message });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

가능하면 webhook client가 사용되면 좋을거같습니다.
이유는 slack에서 제공하는 interface를 더 활용할 수 있기 때문입니다.
https://www.npmjs.com/package/@slack/webhook

constructor(
private readonly appConfigService: AppConfigService
) {
this.webhookUrl = this.appConfigService.get<string>(ENV_KEY.SERVER_ERROR_WEB_HOOK_URL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slackGlobalService인데 url이 serverError로 고정돼는거보다는 변경될 수 있어야할거같아요.
dynamicModule로 만들거나, 메서드 매개변수로 url을 받는 방법 등이 있을거같은데 dynamicModule이 활용될 수 있으면 좋을거같긴하네요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto 자동화 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants