내가 2,000개의 Pull Request 후에 만든 코드 리뷰 체크리스트

나는 18개월 동안 남긴 2,147개의 PR 코멘트를 분류했습니다. 34%는 오류 처리에 관한 것이었고, 22%는 명명에 관한 것이었으며, 8%만이 성능에 대한 것이었습니다. 리뷰를 추적하기 시작했을 때 내가 예상했던 것과는 다릅니다. 나는 아키텍처 문제와 복잡한 버그를 발견할 것이라고 생각했습니다. 대신, 나는 반복적으로 동일한 기본적인 문제를 지적하고 있었습니다: 누락된 null 체크, 모호한 변수 이름, 그리고 사용자에게 유용한 정보를 제공하지 않는 오류 메시지. 동일한 문제가 운영 사고에서 발생하는 것을 보고, 이것들이 사소한 문제들이 아니라는 것을 깨달았습니다—그들은 시스템이 우아하게 실패하는 것과 3시 AM에 당신을 깨우는 시스템의 차이였습니다.

대부분의 코드 리뷰 체크리스트가 요점을 놓치는 이유

모두가 코드 스타일, 테스트 커버리지, 문서를 확인하라고 말합니다. 그건 좋지만, 운영이 중단되는 부분은 아닙니다. 내가 엔지니어링 블로그에서 공유되는 체크리스트를 보면, 실제로 중요한 것이 아니라 측정하기 쉬운 것에 집중하고 있습니다. 그들은 함수가 50줄 이하인지 확인하라고 말하지만, 오류 처리가 실제로 누군가를 2 AM에 디버깅하는 데 도움이 되는지 확인하라고는 말하지 않습니다.

최고의 코드 리뷰는 단순히 버그를 방지하는 것이 아니라—사고로 이어지는 부류의 버그를 방지합니다. 누락된 null 체크는 단순한 잠재적 충돌이 아니라, 그 충돌이 트랜잭션 중간에 발생할 때 잠재적 데이터 손상 사건입니다.

나는 내 PR 코멘트를 추적하기 시작했습니다. 내가 코드를 리뷰하고 승인한 후, 다음 PR에서 동일한 개발자가 동일한 실수를 하는 것을 보았기 때문입니다. 피드백이 지속되지 않았습니다. 그래서 나는 스프레드시트를 만들었습니다. 내가 남긴 모든 코멘트는 오류 처리, 명명, 테스트, 보안, 성능, 아키텍처 또는 기타로 분류되었습니다. 6개월 후, 나는 패턴을 볼 충분한 데이터를 모았습니다. 18개월 후, 그 패턴은 실제로 작동하는 체크리스트로 굳어졌습니다.

놀라운 부분은 목록에서 가장 높은 항목뿐만 아니라 그렇지 않은 항목이었습니다. 성능 최적화 코멘트는 내 리뷰 중 8%를 차지했습니다. 보안 문제는 6%였습니다. 이것들은 우리가 아키텍처 논의에서 집착하는 것들이지만, 일상적인 PR에서는 드물게 발생합니다. 무엇이 일반적일까요? 개발자들이 행복한 경로를 가정하고, 부실한 명명, 그리고 자정에 디버깅할 사람을 위한 것이 아니라 자신을 위한 오류 메시지를 작성하는 것입니다.

운영을 계속 중단시키는 오류 처리 패턴

여기 내가 실제 사고를 유발하는 빈도에 따라 순위 매긴 오류 처리 문제 목록입니다:

1. 백그라운드 작업에서의 조용한 실패. 코드는 예외를 포착하고, 로그를 기록한 다음 계속 진행합니다. 바로 다음에 결제 확인 이메일을 보내야 할 작업이라는 것을 깨닫기 전까지는 괜찮아 보입니다. 이제 고객은 청구되지 않았다고 생각하지만, 실제로는 청구되었습니다. 나는 이 패턴을 백그라운드 작업 PR의 40%에서 봅니다. 해결책은 간단합니다: 작업이 중요하면 예외를 포착하지 마십시오—그것이 위로 떠오르고 알림을 트리거하도록 하십시오. 중요하지 않다면, 조용한 실패가 괜찮은 이유를 문서화하십시오.
2. 맥락을 숨기는 일반적인 오류 메시지. "오류가 발생했습니다"는 나에게 아무것도 말해주지 않습니다. "결제 처리에 실패했습니다"는 더 낫지만 여전히 쓸모가 없습니다. "4242로 끝나는 카드 청구에 실패했습니다: 잔액 부족 (오류 코드: 카드 거부됨)"은 실제로 도움이 됩니다. 나는 PR의 약 30%에서 이것을 flag합니다. 내가 사용하는 테스트: 만약 당신이 3 AM에 운영 로그에서 이 오류를 보았다면, 추가 로그를 추가하고 재배포하지 않고 진단할 수 있을까요?
3. 특정 예외 대신 Exception을 포착하기. 이것은 논란의 여지가 있습니다, 왜냐하면 일부 스타일 가이드는 이를 권장하지만, 나는 이로 인해 버그가 숨겨지는 것을 너무 많이 보았습니다. Exception을 포착하면 NullPointerException, IllegalStateException 및 프로그래머 오류가 아닌 런타임 조건을 나타내는 모든 다른 예외도 포착됩니다. 당신이 처리할 것으로 예상하는 것을 포착하세요. 프로그래머 오류를 충돌시켜야—그것이 당신이 그것을 찾는 방법입니다.
4. 경계에서 외부 데이터를 검증하지 않기. API 응답, 사용자 입력, 파일 내용—프로세스 외부에서 온 것이라면 즉시 검증하십시오. 나는 개발자들이 비즈니스 논리 레이어에서 검증하는 것을 보는데, 이는 유효하지 않은 데이터가 이미 여러 함수에서 전파되었음을 의미합니다. 그 때쯤이면, 왜 null 값이 데이터베이스에 들어갔는지 디버깅하고 있습니다. 경계에서 검증하고, 빠르게 실패하고, 명확한 오류를 반환하십시오.
5. 지수 백오프 없이 재시도 로직. 서비스가 다운되어 즉시 재시도합니다. 여전히 다운되어 있으므로 즉시 다시 재시도합니다. 축하합니다, 재시도로 인해 서비스 저하를 완전한 중단으로 만들어 버렸습니다. 나는 재시도 로직을 추가하는 PR의 15%에서 이것을 봅니다. 항상 지수 백오프와 지터를 사용하십시오. 항상 최대 재시도 횟수를 설정하십시오. 항상 재시도하는 것이 이 작업에 맞는지 고려하십시오.
6. 배치 작업에서 부분 실패를 처리하지 않기. 당신은 1,000개의 레코드를 처리하고 있습니다. 레코드 500이 실패합니다. 레코드 501에서 1000은 어떻게 됩니까? 내가 리뷰하는 대부분의 코드에서 그들은 결코 처리되지 않습니다. 배치가 실패하고, 재시도되고, 레코드 500에서 다시 실패하고, 당신은 갇히게 됩니다. 부분 실패를 명시적으로 처리하십시오: 성공한 것, 실패한 것, 그리고 그 이유를 추적하십시오. 당신의 배치 작업을 다시 시작 가능하도록 만드십시오.
7. 데이터베이스 트랜잭션이 항상 성공할 것이라고 가정하기. 당신은 트랜잭션을 시작하고, 작업을 수행한 후, 커밋합니다. 하지만 커밋이 실패하면 어떻게 하죠? 트랜잭션 중간에 데이터베이스 연결을 잃으면 어떻게 하죠? 나는 데이터베이스 코드를 다루는 PR의 25%에서 이것을 처리하지 않는 코드를 봅니다. 결과: 당신의 애플리케이션 상태와 데이터베이스 상태가 달라지고, 데이터가 로그에 나타나는 결과와 일치하지 않는 이유를 찾아서 몇 시간을 소모해야 합니다.

변수 이름이 $50,000 사건을 일으킨 날

화요일 아침, 나는 Slack 메시지를 받았습니다: "우리는 잘못된 고객에게 $50,000를 환불했습니다." 나는 사고 채널을 열고 읽기 시작했습니다. 한 개발자가 우리 환불 처리 시스템의 버그를 수정하기 위해 전날 밤 수정사항을 푸시했습니다. 수정사항은 한 줄이었습니다. PR은 두 명의 수석 엔지니어에 의해 승인되었습니다. 테스트는 통과했습니다. 모든 것이 괜찮아 보였습니다.

버그는 변수 이름에 있었습니다. 원래 코드에는 센트로 환불할 금액을 나타내는 `refundAmount`라는 변수가 있었습니다. 개발자는 달러로 금액을 나타내는 `refundAmount`라는 새 변수를 추가했습니다. 그들은 원래 변수를 이름 변경하는 것을 잊었습니다. 코드는 잘 컴파일되었습니다—둘 다 정수였습니다. 테스트는 데이터가 우연히 센트와 달러가 거의 같아서 검증을 통과했기 때문에 통과했습니다.

운영 중, 우리는 그 아침에 200개의 환불을 처리했습니다. 그 중 절반은 잘못된 금액에 대한 것이었습니다. $10.00 환불이 $1,000 환불이 되었고, $5.00 환불이 $500 환불이 되었습니다. 누군가 이를 알아챌 때쯤이면, 우리는 $50,000을 초과 지급한 상태였습니다. 우리는 모든 환불을 수동으로 검토하고, 고객에게 연락하고, 어떤 경우에는 돈을 돌려달라고 요청해야 했습니다. 정리하는 데 삼일이 걸렸습니다.

근본 원인은 개발자의 실수가 아니었습니다—모두 실수를 할 수 있습니다. 그것은 동일한 범위에서 서로 다른 단위를 나타내는 두 개의 변수가 있었다는 점이었습니다. 코드 리뷰에서 그것을 발견하지 못했습니다. 리뷰어들이 로직에 집중하고 명명에 집중하지 않았기 때문입니다. 그 사고 이후로, 나는 체크리스트에 규칙을 추가했습니다: 만약 변수가 단위가 있는 양을 나타낸다면, 그 단위는 이름에 포함되어야 합니다. `amount`가 아닌 `amountCents` 또는 `amountDollars`. `duration`이 아닌 `durationSeconds` 또는 `durationMilliseconds`.

이것은 세세하게 보일 수 있습니다.