はじめに
こんにちは、GTL Media ML チームの上村です。新卒ブログ第2弾は私が本番環境でやらかしちゃった話を紹介します。 誰かの責任が...みたいな重い話ではなく、こういうパターンもあるから気をつけようね、というお話なので教訓程度に捉えてください🙆♂️
第1弾の記事、内田くんの「新卒研修編」はこちらです。こちらも合わせて読んでみてください。
GTL Media ML チームでは、 アプリのエンハンスを行うべく、ニュース配信の推薦ロジックの改善を行っています。 ロジック改善では、ユーザログを分析して得た仮説に対して、ABテストを行い、検証することで開発を進めています。
ABテストの設計と運用についてはこちらを参考にしてください。
入社して4ヶ月程度経った頃、ロジック改善を行うべく、機能実装を行っていた時のお話です。 実装の仕方を何も知らなかった私が、優しい先輩方に手取り足取り教えていただいたことで、ようやく慣れ始めたと感じていた頃、それは起こりました。
どんなことをしたのか
少し前に、グノシーに配信する記事を選定するAPIに実装した、ABテストの処理にバグが発見されたため、その修正を行うべく新たな実装を行いました。
バグの部分については、実装前に先輩と打ち合わせしており、この部分が影響与えているんだろうねという箇所を特定していました。実装自体は単純なもので、特定の数行の処理を、まるごとABテストの分岐で、処理を行わなくさせるような実装を行いました。結果を言うと、ここで間違いが起きており、エラーの原因となる様な実装をしてしまっていました。しかしながら、誰もそれに気付くことがなく、Reviewが通り、ステージング環境での動作も問題が発生しなかったため、その実装は本番環境にマージされ、そのAPIで記事の配信が行われはじめました。
すると一部のユーザからアプリがクラッシュすると報告が上がり、急遽リバートが行われることになりました。先輩が直ぐに気付き、対応していただけたため、幸い大事には至らなかったのが助かりました。
事前に気付けなかったか
Review はなかったの？
単純な実装だったが故に、既存の処理の内容までは注視されなかったのが問題でした。特に問題だったのが、APIの実装が複雑すぎたといった問題がありました。普段から実装に携わっているベテランの先輩でも、その一部分だけの変更でアプリのエラーまで出るものだと直ぐに気付くことができなかったというものです。絶対に行わなければならない処理とそうでない処理が一つの関数の中に入り乱れていたため、パッと見では気づけないような実装になっていました。新しいロジックが実装されるに連れて実装が複雑になっていくのは仕方ないことではありますが、複雑すぎる実装が落とし穴となってしまいました。テストコードはなかったの？
普段からチェックしているテストは通っていました。しかし、特に今回のわずかな修正の分岐に関するテストは追加しておらず、これが大きな問題となってしまったのは言うまでもありません。ただ今回のバグは、ある変数のデフォルト値が問題になっていたこともあり、テストの段階で気付くのは難しかったのかなとも感じています。そのため、後日この問題に関するクライアント側での対応も実装されることになりました。実機確認したの？
もちろん、実機確認はしていました。ただ、今回は特殊な条件でのみ起こるエラーであったため、それに引っかからず問題なしと判断して本番環境まで持っていくことになりました。他にも悪かったところは？
さらによくなかったのが、新卒2人分のプルリクエストがまとめて本番にマージされたということでした。新卒2人ではバグ調査にお手上げだったため、先輩にやっていただいたのですが、どちらの実装にせよ、せっかく実装された正しい実装と一緒にリバートしなくてはいけない状況となってしまっていたため、互いに迷惑をかける事態になってしまっていたのには変わりはありませんでした。ただ、実際に不具合があったのは私の実装分でしたが...。
どうすれば良いか
今回の問題に立ち合い、やはり一番大事だと実感したのは、些細なコードの変更であっても、変更した内容に関連する可能な限り多くの影響を把握した上でコードを変更するのが大事だと言うことです。今回のように特定の条件で処理を行わなくするような実装は、特に注意しなければならないと思います。その他に、今回不十分だったテストコードの実装も大きな問題の一つだと感じています。
また問題の大きな原因の一つとして、やはりAPIの実装が複雑すぎたという点が挙げられます。今までの歴史が積み重なって複雑な実装になっている、という事ではありますが、定期的にリファクタリングがされていれば、問題にはならなかったのかなとも思います。そのため、現在チームとして、このAPIのリファクタリングを進め、同じようなことが起きないように対策を進めています。私自身も新卒として、 Clean Code を読み、実装の経験を積むことで、適切なリファクタリングを学んでいます。
また、まだまだ新米であり実装の全てが分かるわけではありません。実装面で少しでも分からないところがあれば、可能な限り自分で実装を追っていたり、それでも難しいところは、先輩に聞くことが大事になります。聞いてばかりでは良くないのかなと感じることもありますが、それでも不明瞭なところは潰していくに越したことはないと思います。他にも、特に実装に慣れるまでは、実装が本番にいく前に正しい挙動であることを確認できる術は全て確認した方が良いです。時間こそかかるかもしれませんが、大きな問題が発生してからでは遅いという事を実感しました。
気をつけたいこと
なるべく致命的な障害を生み出さないようにするために、デプロイする時にあらためて以下の事を徹底するようにしました。当然の事ではありますが、最低限これらの事項をチェックしておく事で、ほとんどの問題は事前に解決できるようになると思います。
- 条件分岐追加したときはif else両方テストする
- 該当コードをテストできていることを確認する（キャッシュも考慮）
- STG, PRD 環境それぞれで実機確認 & ログ確認
- デプロイは自分のだけ、他のデプロイと共存しない（慣れるまで）
- 不具合あったらすぐrevert or rolloutする
振り返って
色々な課題があったとはいえ、問題が発生してしまった事には変わりがないので、今後は同じミスを繰り返さないようにしていきたいと思います。様々な事を教示＆対応いただいた先輩には感謝しかないです。これからも、多くの事を学び、活かしていければなと思っています！
今回の記事では、できる限り人手でチェックできるところは事前にチェックしようと話でしたが、こういった流れをシステム化することがより良い開発に繋がるはずなので、そちらも検討していきたいと考えています。
この記事によって、今後誰かの同じ様なミスが未然に防がれる事を願っています。ここまで読んでいただき、ありがとうございました。
おわりに
弊社では2022年度エンジニアの採用を行っていますので、興味のある方は是非ご応募ください。
