fix(healthz): log level を error に変更し rescue 範囲の WHY コメントを追加#28
Merged
Conversation
503 を返すヘルスチェック失敗は「人間の調査が必要」な事象であり、 warn ではなく error レベルで出すのが運用上の慣例 (CloudWatch アラート設計の根拠)。 rescue StandardError は意図的に広く捕捉している: - Healthz は「アプリ全体の健全性」を 200/503 で表現するエンドポイント - 想定外例外 (NoMethodError 等の実装バグ) も 503 で ALB から退避すべき - この意図が普通のコントローラの常識と異なるため WHY コメントで明示 ログメッセージを "Healthz DB check failed" → "Healthz check failed" に変更: - DB に限らない捕捉になっているため、文言を整合させる - 関連テストも追随 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HealthzControllerの修正。シコードレビュー (M1) の議論結果を反映:warn→errorに変更: 503 を返すヘルスチェック失敗は「人間の調査が必要」な事象 (RDS フェイルオーバー / コネクションプール枯渇 / SG 変更 等)。CloudWatch のアラート設計でerrorレベルを拾うのが運用上の慣例。rescue StandardErrorの意図を WHY コメントで明示: Healthz は「アプリ全体の健全性」を 200/503 で表現する場所。普通のコントローラと違い、想定外例外 (NoMethodError 等の実装バグ) も 503 として ALB に伝えることが正しい。この意図がコードからは読み取れないため、WHY を残す。議論経緯
レビュー初稿では rescue 範囲を
ActiveRecord::ActiveRecordError, PG::Errorに絞ることも提案したが、議論の結果、Healthz の特殊性 (失敗の種類を区別する場所ではなく、失敗を 503 に変換する場所) を考慮してStandardError全捕捉が正解 と再評価。log level の指摘のみ採用。Test plan
bundle exec rspec spec/requests/healthz_spec.rb: 3 examples, 0 failuresbundle exec rubocop: no offensesRails.logger.warn期待) をerror期待に追随関連
healthy_threshold/unhealthy_thresholdを複数回失敗で初めて発火するため、errorレベル単発でアラート疲労になることはない (CloudWatch Alarm 側で N 分間 M 回のしきい値設定で吸収可能)🤖 Generated with Claude Code