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

[FEAT] LogScreen, Log 로컬스토리지 저장 #10

Merged
merged 8 commits into from
Jan 31, 2024
Merged

Conversation

JjungminLee
Copy link
Collaborator

@JjungminLee JjungminLee commented Jan 31, 2024

1️⃣ 어떤 작업을 했나요? (Summary)

image
  • 로컬스토리지에 담긴 로그
image image

기존 코드에 영향을 미치는 변경사항

  • 많이 수정해둬서 한번 보시고 충돌날것 같으면 말씀해주세용

2️⃣ 알아두시면 좋아요!

[demo]
react router App.tsx에 한번 씌워서 Home.tsx에 Hanna님 코드도 넣어놨어요! 그 useLocation()에서 path받아오는걸 하고 싶었는데 그러려면 무조건 Router환경에서 해야해서 이렇게 바꿔뒀습니당!

[LogPayloadParams]
LogPayloadParams 에서 name을 eventName과 screenName으로 나눴습니당
createRandomId()의 경우 나중에 로그 자체 식별id만들때를 대비하여 헷갈리지 않게 하기 위해 이름을 createUserId()로 바꿨습니다!

[로컬스토리지 저장]
우선 Key값은 yls-web으로 하고 저장했는데요.지금 Logger이랑 useYLSLogger()가 뭔가 따로 노는것 같거든요.. 근데 로컬스토리지에 넣으려면 백엔드에 보내야하는 데이터 포멧 그대로 스토리지에 들어가야하고, 그러려면 Logger에 있는 포멧을 따라야하는데 이게 뭔가 좀 뒤죽박죽이라 어렵네여 ㅜ

[userId관련]
Logscreen에서도 굳이 userId가 필요할지 -> contextAPI쓰는거 까지한다면 굳이 안필요할거 같아용

3️⃣ 추후 작업

  • useYLSLogger()와 Logger가 따로 노는걸 어떻게 해야할지 고민

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?

@JjungminLee JjungminLee self-assigned this Jan 31, 2024
name: string;
export interface LogPayloadParams {
userId: number;
screenName: string | '';
Copy link
Member

Choose a reason for hiding this comment

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

엇 네이밍이 일치하지 않는 것 같네용 로그 모델과 네이밍이 통일되면 좋을 것 같아요!

{
  "userId": "${random-value}", // 유저를 특정할 수 없는 값, 로깅 시스템 내에서만 구별할 수 있도록
  "timestamp": "2023-12-04T10:30:00Z",
  "event": {
    "platform": "Web",
    "serviceName": "drawer",
    "name": "SearchButtonClicked",
    "message": "Clicked search button",
    "tags": ["search", "button", "click"],
  }
}

screenName -> serviceName
eventName -> name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

screenName은 /drawer/1 eventName은 버튼 클릭 이런걸 의도 했던건데 serviceName자체가 너무 포괄적인것 같은데.. 어떻게 하면 좋을까요?

Copy link
Member

@Hanna922 Hanna922 Jan 31, 2024

Choose a reason for hiding this comment

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

serviceName은 drawer, search, home 3가지구 name은 버튼 클릭 같은 내용이에요! 요건 다른 팀들이랑 공통으로 가져가는 거라 통일하면 좋을 것 같슴다..!!! 말씀해주신 drawer 하위 페이지들은 path에 넣으면 될 것 같아용!

yarn.lock Outdated
@@ -2635,6 +2635,11 @@ uri-js@^4.2.2:
dependencies:
punycode "^2.1.0"

uuid@^9.0.1:
Copy link
Member

Choose a reason for hiding this comment

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

혹시 어떤 package 추가하신 걸까요? 삭제가 안 된건가 해서요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 이거 삭제해 둘게용!

src/Logger.ts Outdated
},
events: events,
Copy link
Member

Choose a reason for hiding this comment

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

혹쉬 event와 별개로 events는 어떤 역할인가용?

src/Logger.ts Outdated

const click = ({ name }: LogPayloadParams) => {
console.log(`Logging click information for button: ${name}`);
if (window.localStorage.getItem('yls-web') == undefined) {
Copy link
Member

@Hanna922 Hanna922 Jan 31, 2024

Choose a reason for hiding this comment

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

사실 localStorage 저장까지 같이 작업하실 줄 몰랐어서..!! localStorage 저장 로직은 따로 함수로 분리하는 게 좋을 것 같은데 어떻게 생각하시나용?

앗 그리구 useEffect로 인해서 localStorage에 로그가 중복 생성되는 것 같아요!

Copy link
Member

Choose a reason for hiding this comment

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

남겨주신 'LogScreen에서도 userId가 필요할지'는 yes인 것 같습니다! 아무래도 LogClick보다 LogScreen이 먼저 일어날 수 밖에 없는데 사용처에서 처음에 userId를 넣어줘야 이후에 저희가 생성한 해시 값으로 사용할 수 있을 것 같네용

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 이거 함수로 따로 빼봐도 좋을것 같네요! 이 작업은 제가 담주까지 해둘게용! 사실 로컬스토리지 작업은 어쩌다보니 그냥 하게 된거라..! 테스트 용도로 봐주시면 될듯합니당! 로그 중복저장도 수정해둘게용!

@Hanna922 Hanna922 merged commit dabd1ed into develop Jan 31, 2024
@Hanna922 Hanna922 deleted the feat/#6 branch January 31, 2024 12:57
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.

[FEAT] Logscreen 컴포넌트 개발
2 participants