feat(@yourssu/logging-system): fix 10th lost-log #68

owl1753 · 2024-11-06T07:20:26Z

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

resolved bug: lost 10th log #53

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

없습니다.

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

기존에는 log 전송에 성공하면 localStorage를 비웠으나, 지금은 localStorage를 미리 비우고, 전송에 실패하면 다시 복원합니다. 이렇게 하면 같은 로그에 대한 중복 호출을 방지할 수 있습니다.
10번 째 로그가 날아간 이유는 기존에 10번 쨰 로그가 들어오면 해당 로그를 포함하지 않은 remainList를 전송하기 때문입니다. 지금은 10번 째 로그가 들어오자마자 이를 포함해 전송합니다.

버그 픽스

10번 째 로그가 손실되는 문제를 해결했습니다.
같은 로그를 반복적으로 전송할 수 있는 문제를 해결했습니다.

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

기존 제안되었던 timestamp 대신 더 간단한 방식을 사용했습니다. 다만 이렇게 하면 로그를 재전송 했을 때 로그 수가 10 <= 이 될 가능성이 있습니다

3️⃣ 추후 작업

사용자 이탈 시 로그 전송 작업을 마무리 후 새로운 버전을 배포합니다.

4️⃣ 체크리스트 (Checklist)

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

EATSTEAK

좀 빡세게 리뷰했습니다. 현재 구조에서 접근법은 괜찮은데, 좀 더 근본적인 리팩토링이 동반되었으면 좋겠습니다.

EATSTEAK · 2024-11-07T04:55:30Z

packages/logging-system/src/SetLocalStorage.ts

@@ -1,36 +1,32 @@
 import { LogRequestList, LogResponse, LogType } from './types/LogType';

 export const SetLocalStorageClear = () => {
-  const list: any[] = [];
+  const list: LogType[] = [];
  localStorage.setItem('yls-web', JSON.stringify(list));
 };

 export const SetLocalStorage = async (


PR의 내용과는 관계없습니다만, 이거 왜 SetLocalStorage인가요? 실제 하는 일은 로그를 전송하는 건데, 관심사 분리를 확실히 하고 로컬스토리지 저장과 로그 전송을 순차 실행하는 새로운 함수를 만드셔야 할 것 같습니다. 제가 YLS 코드에 대한 배경 지식이 없었는데, 지금 리뷰하면서 살펴보니 고쳐야 할 부분이 많아 보이네요.

예시) SetLocalStorage 함수를 storeLog, sendStoredLogs 함수로 분리 후 상위 호출 함수에서 두 함수를 순서대로 호출하도록 리팩토링

일단 이 PR과 관계없으므로 resolve 되지 않아도 Approve는 가능합니다.

EATSTEAK · 2024-11-07T05:03:32Z

packages/logging-system/src/SetLocalStorage.ts

-      const req: LogRequestList = {
-        logRequestList: remainList,
-      };
+  const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];


Suggested change

const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];

const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) ?? [];

|| 말고 nullish coalescing 연산자(??) 활용하시면 의도에 맞으실 것 같습니다.

EATSTEAK · 2024-11-07T05:05:32Z

packages/logging-system/src/SetLocalStorage.ts

@@ -1,36 +1,32 @@
 import { LogRequestList, LogResponse, LogType } from './types/LogType';

 export const SetLocalStorageClear = () => {
-  const list: any[] = [];
+  const list: LogType[] = [];
  localStorage.setItem('yls-web', JSON.stringify(list));
 };

 export const SetLocalStorage = async (
  logger: LogType,


이 PR만의 문제가 아닌 것 같긴 한데, 얘 이름이 logger인 이유를 모르겠네요. log 데이터 그 자체이므로 이름 수정이 필요합니다.

EATSTEAK · 2024-11-07T05:06:28Z

packages/logging-system/src/SetLocalStorage.ts

-      };
+  const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];
+  list.push(logger);
+  localStorage.setItem('yls-web', JSON.stringify(list));


Suggested change

localStorage.setItem('yls-web', JSON.stringify(list));

localStorage.setItem('yls-web.logs', JSON.stringify(list));

yls-web.logs 처럼 데이터의 정확한 명칭을 지정해서 의도를 명확히 하고, 앞으로 다른 데이터가 추가되어도 네이밍 문제가 생기지 않도록 하면 좋겠습니다.

EATSTEAK · 2024-11-07T05:07:24Z

packages/logging-system/src/SetLocalStorage.ts

@@ -1,36 +1,32 @@
 import { LogRequestList, LogResponse, LogType } from './types/LogType';

 export const SetLocalStorageClear = () => {
-  const list: any[] = [];
+  const list: LogType[] = [];
  localStorage.setItem('yls-web', JSON.stringify(list));


Suggested change

localStorage.setItem('yls-web', JSON.stringify(list));

localStorage.setItem('yls-web', "[]");

이거 그냥 이렇게 해도 되지 않나요?

EATSTEAK · 2024-11-07T05:07:43Z

packages/logging-system/src/SetLocalStorage.ts

@@ -1,36 +1,32 @@
 import { LogRequestList, LogResponse, LogType } from './types/LogType';

 export const SetLocalStorageClear = () => {


Suggested change

export const SetLocalStorageClear = () => {

export const ClearLocalStorage = () => {

이것도 네이밍이 미묘하네요.

EATSTEAK · 2024-11-07T05:08:19Z

packages/logging-system/src/SetLocalStorage.ts

+    try {
+      await putLog(req);
+    } catch (e) {
+      console.error('Failed to post log');


이 오류는 프로덕션에는 안나면 좋겠네요. 어짜피 Network 탭에서 요청 상태를 확인 가능하니까요.

혹시나 에러 로깅을 하고 싶으시다면 e도 포함해서 로깅해주시면 좋겠습니다.

Suggested change

console.error('Failed to post log');

console.error('Failed to post log', e);

EATSTEAK · 2024-11-07T05:09:40Z

packages/logging-system/src/SetLocalStorage.ts

+      await putLog(req);
+    } catch (e) {
+      console.error('Failed to post log');
+      const remainList: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];


remainList라는게 아직 못 보낸 로그처럼 느껴져서 logsAfterRequest 같은 느낌으로 수정했으면 좋겠습니다.

EATSTEAK · 2024-11-07T05:10:39Z

packages/logging-system/src/SetLocalStorage.ts

+      remainList.unshift(...list);
+      localStorage.setItem('yls-web', JSON.stringify(remainList));


Suggested change

remainList.unshift(...list);

localStorage.setItem('yls-web', JSON.stringify(remainList));

list.push(...remainList);

localStorage.setItem('yls-web', JSON.stringify(list));

이편이 깔끔하지 않을까요?

EATSTEAK · 2024-11-07T05:36:48Z

첨언) 타임스탬프 구현이 아닌 이 구현도 문제가 존재합니다. 반례 드릴게요.

로그를 전송하고, 로컬스토리지에서 로그가 제거됨
사용자가 로그 전송 요청이 응답하기 전에 페이지 이탈
이 상황에서 로그 전송 요청이 실패할 경우 로그 유실

개인적으로는 중복 전송보다 로그 유실이 더 큰 문제로 느껴져서 타임스탬프 구현을 선호합니다.

만약 로그 중복이 걱정되신다면 로그 스펙에 고유의 임의 ID를 추가한다면 완벽히 해결할 수 있겠네요.

owl1753 · 2024-11-07T11:58:18Z

첨언) 타임스탬프 구현이 아닌 이 구현도 문제가 존재합니다. 반례 드릴게요.

로그를 전송하고, 로컬스토리지에서 로그가 제거됨

사용자가 로그 전송 요청이 응답하기 전에 페이지 이탈

이 상황에서 로그 전송 요청이 실패할 경우 로그 유실

개인적으로는 중복 전송보다 로그 유실이 더 큰 문제로 느껴져서 타임스탬프 구현을 선호합니다.

만약 로그 중복이 걱정되신다면 로그 스펙에 고유의 임의 ID를 추가한다면 완벽히 해결할 수 있겠네요.

아하 이런 반례가 있군요... 저는 해당 상황을 생각하지 못했는데 만약 타임 스탬프로 구현한다고 해도 해당 상황에서 로그 중복 문제가 발생할 것 같네요. 제 생각에는 로그 전송이 실패하는 경우보다는 로그 중복 전송이 일어나는 경우가 더 많을 것으로 예상되어서요. (로그 전송 후 로컬스토리지에서 로그를 제거하기 전에 사용자가 이탈). 물론 로그 손실 문제 자체가 더 큰 문제인 것은 동의합니다. 로그 손실 + 중복 문제를 동시에 해결하기 위해 타임 스탬프 + 임의 ID를 사용하는 방식을 적용해서 해결해보겠습니다.

네이밍의 경우에는 일단 기존에 개발된 내용을 그대로 적용한 거라서 딱히 수정하진 않았는데, 수정할 필요성이 있어 보입니다. 근본적인 리팩토링이 필요하다는 의견도 동의하구요. 일단 이 PR과 관련 있는 부분들만 수정해서 머지한 후 이슈를 새로 파서 대대적인 리팩토링을 진행하겠습니다.

EATSTEAK · 2024-11-07T12:07:10Z

네네 그래도 리뷰 요청 주셨으니 빡세게 리뷰 드렸는데... 어떻게 느끼셨을지 모르겠네요. 지금 PR과 관계없는 내용이 많았는데, 관계 있는 내용만 수정하셔서 리뷰 요청 주시면 어푸룹 드리겠습니다. 화이팅!

나머지 구조적인 부분은 천천히 작업하셔도 충분하지 않나 싶습니다.

owl1753 added 4 commits September 25, 2024 17:17

fix: 10번째 로그 쌓일 시 즉시 전송

524ac65

fix: 전송에 실패한 로그 스토리지에 쌓기

e1225c4

fix: 보내지지 않은 로그는 다시 로컬 스토리지에 쌓이도록 수정

1e1a3ae

fix: 로그가 10개 쌓이면 전송

5d44077

owl1753 added fix YLS @yourssu/logging-system-react labels Nov 6, 2024

owl1753 self-assigned this Nov 6, 2024

owl1753 requested review from Hanna922, intersoom and nijuy as code owners November 6, 2024 07:20

owl1753 requested review from EATSTEAK and minai621 November 6, 2024 07:23

Merge branch 'main' into fix/lost-log

7f21067

EATSTEAK requested changes Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(@yourssu/logging-system): fix 10th lost-log #68

feat(@yourssu/logging-system): fix 10th lost-log #68

owl1753 commented Nov 6, 2024 •

edited

Loading

EATSTEAK left a comment

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK Nov 7, 2024

EATSTEAK commented Nov 7, 2024 •

edited

Loading

owl1753 commented Nov 7, 2024

EATSTEAK commented Nov 7, 2024 •

edited

Loading

	const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) \|\| [];
	const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) ?? [];

	localStorage.setItem('yls-web', JSON.stringify(list));
	localStorage.setItem('yls-web.logs', JSON.stringify(list));

	localStorage.setItem('yls-web', JSON.stringify(list));
	localStorage.setItem('yls-web', "[]");

		@@ -1,36 +1,32 @@
		import { LogRequestList, LogResponse, LogType } from './types/LogType';

		export const SetLocalStorageClear = () => {

	export const SetLocalStorageClear = () => {
	export const ClearLocalStorage = () => {

	console.error('Failed to post log');
	console.error('Failed to post log', e);

		remainList.unshift(...list);
		localStorage.setItem('yls-web', JSON.stringify(remainList));

feat(@yourssu/logging-system): fix 10th lost-log #68

Are you sure you want to change the base?

feat(@yourssu/logging-system): fix 10th lost-log #68

Conversation

owl1753 commented Nov 6, 2024 • edited Loading

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

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

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

버그 픽스

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

3️⃣ 추후 작업

4️⃣ 체크리스트 (Checklist)

EATSTEAK 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

EATSTEAK commented Nov 7, 2024 • edited Loading

owl1753 commented Nov 7, 2024

EATSTEAK commented Nov 7, 2024 • edited Loading

owl1753 commented Nov 6, 2024 •

edited

Loading

EATSTEAK commented Nov 7, 2024 •

edited

Loading

EATSTEAK commented Nov 7, 2024 •

edited

Loading