💡 Key Takeaways
- The First 30 Seconds: What the PR Description Tells Me
- The Logic Layer: Does This Actually Solve the Problem?
- The Data Layer: Following the Information Flow
- The Security Lens: Thinking Like an Attacker
3년 전, 나는 주말 동안 내 회사에 $47,000의 손실을 초래한 풀 리퀘스트를 승인했다. 코드는 괜찮아 보였고, 테스트도 통과했다. 논리도 타당했다. 하지만 나는 유럽 고객을 위한 시간대 변환 처리에서 미묘한 부분을 놓쳤고, 일광 절약 시간이 시작되면서 우리의 결제 처리 시스템은 사용자 기반의 18%에 대해 조용히 실패하기 시작했다.
💡 주요 포인트
- 첫 30초: PR 설명이 나에게 주는 의미
- 논리 레이어: 실제로 문제를 해결하고 있는가?
- 데이터 레이어: 정보 흐름을 따라가기
- 보안 관점: 공격자처럼 생각하기
그 사건은 내가 코드 리뷰를 하는 방식을 영원히 바꿨다. 나는 사라 첸이며, 지난 10년 동안 3개의 다른 SaaS 회사에서 수석 엔지니어링 매니저로 일하며 주당 평균 15-20개의 풀 리퀘스트를 리뷰해왔다. 그것은 내 경력에서 약 8,000개의 PR에 해당하며, 단일 행의 핫픽스에서 50개 이상의 파일에 걸친 대규모 아키텍처 리팩터링에 이르기까지 다양하다. 나는 버그를 포함한 훌륭한 코드와 수년간 오류 없이 프로덕션에서 실행되는 엉망인 코드를 보았다.
내가 배운 것은 코드 리뷰가 실제로는 버그를 찾는 것이 아니라는 것이다—비록 그것이 중요하긴 하다. 그것은 현실과의 접촉에서 살아남는 시스템을 구축하는 것이다. 그것은 다음 개발자(대개는 6개월 후의 나 자신)가 두려움 없이 이해하고 수정할 수 있는 코드를 만드는 것이다. 그리고 그것은 애플리케이션이 확장될 때, 엣지 케이스가 나타날 때, 혹은 호주에 있는 한 고객이 전혀 예상하지 못한 일을 할 때만 드러나는 보이지 않는 문제를 잡는 것이다.
이것이 나의 체크리스트이다. 교과서에서의 이론적인 것이 아니라, 수많은 프로덕션 사건, 야간 디버깅 세션, 내가 승인한 버그가 실제 사용자에게 영향을 미친다는 것을 깨달았을 때의 그 기분이 포함된, 전투를 통해 검증된 것이다.
첫 30초: PR 설명이 나에게 주는 의미
코드의 한 줄도 보지 않기 전에, 나는 PR 설명을 읽는 데 30초를 보낸다. 이것은 사소해 보일 수 있지만, 잘못 작성된 설명은 종종 코드 자체에 문제가 있을 수 있다는 첫 번째 신호이다. 내 경험상, 자신의 코드가 무엇을 하는지 명확하게 설명할 수 없는 개발자는 구현을 충분히 생각하지 않았던 경우가 많았다.
나는 세 가지 특정한 것들을 찾고 있다: 해결하려는 문제, 취해진 접근 방식, 그리고 어떤 타협이 이루어졌는지. 좋은 PR 설명은 미니 디자인 문서처럼 읽혀야 한다. "사용자 인증에서 버그 수정"은 나에게 아무것도 말해주지 않는다. "높은 트래픽 기간 동안 0.3%의 사용자가 예기치 않게 로그아웃되는 상황을 초래한 JWT 토큰 갱신의 경쟁 조건을 수정하고 Redis를 사용하여 분산 잠금을 구현했습니다."라는 설명은 개발자가 문제를 깊이 이해하고 있다는 것을 말해준다.
나는 또한 PR의 크기가 적절한지 확인한다. 내 경험상의 규칙은: PR이 400줄 이상의 코드를 포함한다면(테스트 및 생성된 파일은 제외하고), 효과적으로 리뷰하기에는 너무 클 가능성이 높다. 구글의 엔지니어링 관행에 대한 연구에 따르면 리뷰의 효과는 200-400줄 이상의 경우 급격히 떨어진다고 한다. 나는 이 규칙이 정확하다는 것을 발견했고—300줄 정도를 리뷰한 후에는 내 주의가 흐트러지고 미세한 문제를 놓치기 시작한다.
거대한 PR을 만날 때, 나는 개발자에게 그것을 더 작고 논리적인 청크로 나누도록 요청한다. 네, 이것은 초기에는 더 많은 시간을 요구하지만, 내가 overwhelmed되고 그저 지나치고 싶어하는 상황에서 2,000줄의 PR에 스탬프를 찍는 상황을 방지한다. 나는 내 경력 초기에 이러한 방식으로 문제가 있는 PR을 너무 많이 승인했다.
설명은 또한 관련된 맥락과 연결되어야 한다: 다루어지고 있는 티켓이나 문제, 디자인 문서, 관련 PR들. 내가 이 변경이 왜 이루어지는지를 이해하기 위해 Jira나 Slack을 샅샅이 수색해야 한다면, 이는 전체 리뷰 프로세스를 지연시키는 마찰이 발생하는 것이다. 내 현재 회사에서는 적절한 맥락 없이 작성된 PR은 설명이 업데이트될 때까지 리뷰가 이루어지지 않는 정책을 수립했다. 이 간단한 규칙으로 우리의 리뷰 품질이 측정 가능하게 개선되었다.
논리 레이어: 실제로 문제를 해결하고 있는가?
PR이 무엇을 하려는 것인지 이해한 후, 나는 실제로 그것이 그렇게 되어 있는지 검증한다. 이건 자명한 듯 보이지만, 코드가 테스트를 통과했음에도 불구하고 기본적인 문제를 완전히 해결하지 않는 경우는 놀라울 정도로 많다. 증상을 수정하는 데만 머물며 근본 원인은 그대로 두는 경우도 여러 번 보았다.
"코드 리뷰는 실제로 버그를 찾는 것이 아니라—you는 현실과 접촉하는 시스템을 구축하는 것이고, 다음 개발자가 두려움 없이 이해할 수 있는 코드를 만드는 것이다."
나는 먼저 정신적으로 행복한 경로를 따라 생각해본다. 만약 이것이 환불을 처리하기 위한 새로운 기능이라면, 나는 고객이 환불을 요청하고 API 엔드포인트에서 비즈니스 로직을 통해 데이터베이스 업데이트까지의 코드 경로를 추적한다. 각 단계가 이치에 맞는가? 데이터를 올바르게 처리하고 있는가? 필요한 모든 레코드를 업데이트하고 있는가?
하지만 행복한 경로는 쉽다. 좋은 코드와 프로덕션 준비가 된 코드를 구분 짓는 것은 불행한 경로를 처리하는 방식이다. 결제 게이트웨이가 작동하지 않을 경우에는 어떻게 되는가? 환불 금액이 음수일 경우에는? 사용자가 이미 환불된 주문에 대해 환불을 요청할 경우에는? 나는 엣지 케이스에 대해 거의 편집증처럼 생각하게 되었고, 프로덕션 환경은 내가 고려하지 않은 모든 엣지 케이스를 찾아내는 혼돈의 엔진임을 배우게 되었다.
나는 방어적인 프로그래밍 패턴을 찾는다: 널 체크, 입력 검증, 경계 조건 처리. 그러나 나는 과잉 설계를 찾기도 한다. 모든 함수가 모든 가능한 엣지 케이스를 처리할 필요는 없다. 검증된 입력으로 한 장소에서만 호출되는 비공식 메서드의 경우, 불필요한 검증을 추가하는 것은 단지 잡음이다. 핵심은 각 함수의 계약을 이해하고 그것이 경계에서 유지되도록 하는 것이다.
내가 문제를 반복적으로 일으키는 패턴 중 하나는 "낙관적 업데이트"이다. 개발자는 운영이 성공할 것이라고 가정하고 성공을 확인하기 전에 UI 또는 데이터베이스 상태를 업데이트한다. 이것은 99%의 경우에 작동하지만, 그 1%는 시스템 상태가 일관성을 잃는 매우 혼란스러운 버그를 만들어낸다. 나는 항상 이러한 패턴을 알려주고 다음과 같이 물어본다: 이 운영이 중간에 실패하면 어떻게 되는가?
데이터 레이어: 정보 흐름을 따라가기
데이터 버그는 최악의 유형의 버그이다. 그 이유는 종종 조용하고 누적되기 때문이다. 논리 오류는 어플리케이션을 즉시 충돌시킬 수 있지만, 데이터 오류는 몇 주에 걸쳐 데이터베이스를 서서히 손상시킬 수 있으며, 당신이 그것을 알아차릴 때쯤이면 수천 개의 나쁜 레코드가 생기게 되고 이를 수정할 방법이 없다.
| 리뷰 초점 | 주니어 리뷰어 | 시니어 리뷰어 | 프로덕션에 대한 영향 |
|---|---|---|---|
| 구문 & 스타일 | 주요 초점 | 자동화/최소한의 주의 | 낮음 |
| 엣지 케이스 | 명백한 시나리오만 | 시간대, 지역, 스케일 문제 | 높음 |
| 테스트 커버리지 | 테스트가 존재하고 통과함 | 테스트가 실패 모드를 커버함 | 중요함 |
| 성능 | 코드가 올바르게 작동함 | 부하/스케일 하에서의 동작 | 높음 |
| 문서화 | 주석이 존재함 | 결정이 내려진 이유 | 중간 |
데이터에 영향을 미치는 코드를 리뷰할 때, 나는 그 데이터의 전체 생애주기를 추적한다. 그것은 어디에서 오는가? 어떻게 검증되는가? 어떻게 변환되는가? 어디에 저장되는가? 누가 접근할 수 있는가? 이것은 사용자 생성 콘텐츠나 금융 데이터와 같이 오류가 실제 결과를 초래할 수 있는 경우에 특히 중요하다.
특히 데이터 유형 변환에 주의한다. 나는 한 번 가격을 계산을 위해 부동 소수점에서 정수로 변환한 후 다시 부동 소수점으로 변환하는 코드를 승인한 적이 있다. 무해해 보였다. 그러나 부동 소수점 정밀도 문제로 인해 $10.99와 같은 가격이 종종 $10.98 또는 $11.00으로 바뀌었다. 우리는 고객들이 잘못된 요금에 대해 불만을 제기하기 시작할 때까지 그것을 발견하지 못했다. 이제 나는 특히 돈이나 측정과 관련된 모든 유형 변환을 면밀히 조사한다.
데이터베이스 마이그레이션은 내가 더욱 조심스럽게 접근하는 영역이다. 나는 몇 가지를 찾는다: 마이그레이션이 되돌릴 수 있는가? 중간에 실패하면 어떻게 되는가? 우리의 프로덕션 데이터베이스에서 얼마나 걸릴 것인가? 나는 1,000개의 레코드가 있는 개발 데이터베이스에서 잘 작동하지만 5천만 개의 레코드가 있는 프로덕션 데이터베이스에서 테이블을 20분 동안 잠글 수 있는 마이그레이션을 보았다. 이렇게 되면 전체 애플리케이션이 다운된다.
모든 기존 데이터를 수정하는 코드에 대해서는 다음과 같이 묻는다: 백업 계획이 있는가? 뭔가 잘못되면 이 변경을 롤백할 수 있는가? 내 이전 회사에서는 10,000개 이상의 레코드에 영향을 미치는 모든 데이터 마이그레이션은 프로덕션 스냅샷에서 드라이런을 수행하고 PR에 명시적인 롤백 절차를 문서화해야 한다는 정책을 세웠다. 이것은 여러 번 우리를 구했다.
나는 또한 데이터 접근 패턴을 살펴본다. 이 코드가 N+1 쿼리를 하고 있는가? 필요한 몇 개의 필드만 로드하고 있을 때 전체 컬렉션을 메모리에 불러오는가? 이러한 성능 문제는 개발 중에는 명확하지 않을 수 있지만, 확장될 때 중요해진다. 나는 한 번 괜찮아 보이는 코드를 검토했지만, 프로덕션에 배포되었더라면 데이터베이스 CPU가 100%로 치솟았을 코드를 보았다. 단지 그것이 너무...