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

서버 로깅 구현 #349

Merged
merged 9 commits into from
Jul 17, 2024
Merged

서버 로깅 구현 #349

merged 9 commits into from
Jul 17, 2024

Conversation

jyoo0515
Copy link
Member

@jyoo0515 jyoo0515 commented Jul 15, 2024

  • 슬랙 쓰레드
  • 현재의 니즈) 앱에서 회원가입하면서 뉴스레터를 구독하는 유저들이 얼마나 있는지 확인하고 싶다
  • 이를 조금 더 범용성 있게 server_log 라는 엔티티를 만들어서 풀어봤습니다
    • server_event 는 domain event 와 이름이 겹쳐서 피했습니다
    • 범용성을 위해 column 은 최소한으로 두고 나머지 데이터는 그때 그때 달라질 수 있도록 했습니다
  • 빅쿼리에 쌓으면 좋을 것 같지만 일단 관련 기능이 없으므로 db 에 저장합니다
  • boundedContext 중 하나로 놓고 domainEvent 로 풀어볼까 했지만 이게 boundedContext 는 아닌 것 같아서 crossCuttingConcern 에 두고 필요한 곳에서 가져다 쓰도록 했습니다
  • jpa repository 를 사용하게 된다면 infra 모듈에 있는 server_log 는 없앨 수 있을 것 같습니다

Checklist

  • 충분한 양의 자동화 테스트를 작성했는가?
    • 계단정복지도 서비스는 사이드 프로젝트로 진행되는 만큼 충분한 QA 없이 배포되는 경우가 많습니다. 따라서 자동화 테스트를 꼼꼼하게 작성하는 것이 서비스 품질을 유지하는 데 매우 중요합니다.

@jyoo0515 jyoo0515 requested a review from a team as a code owner July 15, 2024 15:45
Copy link

🔥🔥🔥 Backend CI Failed. github action link 🔥🔥🔥

Copy link

🔥🔥🔥 Backend CI Failed. github action link 🔥🔥🔥

Copy link
Contributor

@Zeniuus Zeniuus left a comment

Choose a reason for hiding this comment

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

굿굿

Comment on lines 3 to 5
interface ServerLogPayload {
val type: ServerLogType
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ServerLogType이 이미 ServerLog에 있는데 굳이 여기도 들어갈 필요가 있으려나요?
여기도 넣으실 거면 ServerLog class의 init에 type == payload.type을 check 하면 좋겠습니당

Copy link
Member Author

Choose a reason for hiding this comment

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

사실상 type 별로 payload 가 생길거라고 생각했고 그래서 payload 만 알면 type 을 바로 알 수 있도록 interface 의 val 로 넣었습니다. ServerLog (ServerEvent) 의 필드로 넣은 것은 db 에 인덱스 걸고 저장하고 싶어서였는데, db 스키마와 애플리케이션 모델이 꼭 같은 필요는 없으니 다른 더 좋은 표현 방법이 있나 고민해보겠습니다
없으면 그냥 init 블록 둘 것 같네요 ㅎㅎ

Copy link
Member Author

@jyoo0515 jyoo0515 Jul 17, 2024

Choose a reason for hiding this comment

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

지금도 중복 저장되고 있겠군요.. 음

Copy link
Member Author

@jyoo0515 jyoo0515 Jul 17, 2024

Choose a reason for hiding this comment

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

4291255
db 에 중복저장 안되도록 함수로 빼고 init 블럭 안에 체크로직 넣었습니다
가독성을 위해서 type 은 text 로 함께 묶어서 들어가지 않고 따로 칼럼으로 두는게 좋을 것 같아서요

Copy link
Member Author

Choose a reason for hiding this comment

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

어차피 constructor 에 없는 필드라 저장은 안되겠네요
따로 converter 에서 paylod 와 타입을 매핑해주는 것보다 type 이 payload 에 종속적인게 관리하기 좋을 것 같아서 이렇게 두고,
만에 하나 runtime 에 에러가 날 수 있는 여지가 있기 때문에 try catch 로 감쌌습니다
6a9b16c


import java.time.Instant

data class ServerLog(
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 domain event랑 약간 겹치더라도 log 보다는 event라는 이름이 더 좋을 것 같은데요, log랑 event라는 워딩에서 느껴지는 차이가 꽤 분명한 것 같기 때문입니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

0857cc9
log -> event 로 이름 변경했습니다

Copy link

🔥🔥🔥 Backend CI Failed. github action link 🔥🔥🔥

@jyoo0515 jyoo0515 merged commit 5af4c47 into main Jul 17, 2024
1 check passed
@jyoo0515 jyoo0515 deleted the jason/20240715-server-event-log branch July 17, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants