不要なコードや機能を安全に削除していく仕組みを作って18万行削除した話

ギフトモールでEngeering Managerをやっている @nori0620 です。ギフトモールのアプリケーションは7年以上運用しておりコードの肥大化、認知負荷の増大などの課題が出てきています。対応として機能ごとのサービス分割などの対応も進めていますが、この記事ではあえて「モノリシックな部分の不要なコードを減らしていく取り組み」にフォーカスした話をしてみます！

この記事の概要の3行まとめ

不要なコードが増えると開発効率/開発体験にネガティブな影響が出る
ギフトモールのコードベースは非常に大きく成長しており、その分不要そうなコードや機能が本当に使われていないことを確認するコストも高いことが課題だと感じた
不要そうなコードが実際に使われていないか検知する仕組みを作って、安全に18万行のコードを消せた

不要なコード・機能の存在は、なにが問題なのか

不要なコードはアプリケーションの挙動に対して悪い影響を及ぼしませんが、良い影響を与えてくれるものでもありません。開発チームの開発効率を下げたり、動作確認のコストを上げてしまったり、うっかり事故を誘発してしまったりすることがあるため、基本的には不要なコードは削除していくべきです。

チームのコード把握に対する認知負荷が高くなり開発効率が低下する

プロジェクトに不要なコードが多いと、その分だけ全体像はもちろん各所を把握するための認知負荷は高くなります。特に新しく入った人ほど、どれが使われていないコード/機能か把握しづらいためキャッチアップにコストがかかってしまい、活躍するまでのリードタイムが長くなってしまいます。

「リファクタリング」や「ライブラリ、フレームワーク、言語のアップデート」の際の対応コスト増に繋がる

例えば、あるメソッドのシグニチャを変更しようとした際、そのメソッドが利用されていない機能に依存されていた場合、余計に対応コストや調査コストがかかってしまいます。(そういうコードほど対応時の動作確認も難しい)

さらに上のような小さな改善に追加コストがかかることが多いと、ソフトウェアを改善しつづけるというモチベーション低下にもつながり悪循環に繋がる可能性があります。

またライブラリやフレームワーク、言語のアップデートといった対応をするとき、古く破壊的変更の影響を受けやすいコードほど改修が必要になりますが、そのようなコードの中には「実は利用されていない」コードも多く存在します。使っていないコードに対する改修作業はそのまま工数の増大につながってしまいます。

今動かすと問題があるコードを、うっかり動かして事故に繋がる可能性

例えば、次のようなケースでは思わぬ事故につながる可能性もあります - 動かさなくなったバッチコマンドを削除せずに残しておいて、何らかのミスで実行されてしまった場合 - Feature FlagやABテストなどで止めていたコードを、時間を経て意図せず動かしてしまった場合

このように挙動以外の部分にネガティブな影響があるため、本来削除されるべき不要なコードや機能が増えていけば増えていくほど、開発効率にじわじわとボディーブローのようなダメージを与えていきます。

ギフトモールの抱えていた(いる)課題

ギフトモールの開発チームは不要なコードを減らしていく取り組みを積極的に実施していますが、そもそも不要なコードが多数生まれることになったギフトモール特有の要因も存在します。

ギフトモールのサービス立ち上げ当初はまだ現在のようなエンジニア組織がなく、開発スピードを優先し「外部パッケージを利用する」ことを選択しました。この外部パッケージにはギフトモールでは利用しない機能も含まれるモノリシックな構成でした。またこれらの利用しない機能へのアクセスを容易に作り込めてしまう状態でもありました。

結果として不要なコードがあちこちに存在することとなり、また本当に不要であることを確かめる手間もかかる状態になってしまうことで、不要なコードを消すことが億劫になってしまっていました。

それゆえに「不要そうだが、本当に使われていないかわからない。その確認コストがかかる。」というコードが多く存在するという課題に対応する必要がありました。

大規模なコード削除を安全に行うために用意した仕組み

不要なコード利用を検知する仕組み

不要だと思われる機能の中にはサービス外部からアクセス可能な状態のものもあり、コード解析による機械的な判定では削除可能か判断するのが難しい状況でした。

そこで、機能が利用されているかどうかを判定するための簡易的な仕組みとして、リファラなどを記録する ActiveCodeChecker という仕組みを導入しました。この仕組みを使って一定期間記録がなければコードを消し、記録がある場合もコードの利用が不適切と思われるものは適宜書き換える作業をして極力コードを消せるようにしていくこととしました。

例えば MyLegacyClass というクラスの myLegacyMethod というメソッドが、おそらく利用されていないことを確認するには以下のように ActiveCodeChecker を差し込むイメージです。

<?php
namespace MyApp\Foo;
...
class MyLegacyClass
{
   ...   
    public function myLegacyMethod()
    {
        // おそらく使われていないが、確証が持てない部分に以下のように ActiveCodeChecker を仕込む
        // ( 第一引数は検知を通知する際に、わかりやすく識別するための値(グループ名)なので、任意 ))
        ActiveCodeChecker::notifyIfCalled('refactor_my_app');
        ...
    }
}

上記のように仕込んでおいて、残念ながら MyLegacyClass#myLegacyMethod が使われている箇所があったとすると、Slackに以下のような通知がリアルタイムに専用channelに来る仕組みになっています。

{
  "message": "[ActiveCodeChecker::notifyIfCalled] コードの実行が検知されました.",
  "group": "refactor_my_app",
  "called": {
    "file": "/var/www/sites/app/src/MyApp/Foo/MyLegacyClass.php",
    "line": 8
  },
  "caller": {
    "file": "/var/www/sites/app/src/MyApp/Bar.php",
    "line": 254
  },
  "url": "https://example.com/foo",
  "referer": "https://example.com/bar",
  "user_agent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.4896.75 Safari/537.36 Edg/100.0.1185.36",
  "_level": "info"
}

calledには呼び出されたコード位置の情報(=ActiveCodeChecker)を仕込んだ位置が入っています
callerは問題のコードを呼び出したコードの位置が入っています
url, referer, user_agentはHTTP経由でアクセスされていた場合は、その状況に関する情報が入っています

簡単に実装の雰囲気を紹介

<?php
namespace Giftmall\RefactoringTool;
...
class ActiveCodeChecker
{
    public static function notifyIfCalled(string $group)
    {
        // 呼び出し位置の情報をbacktrace経由で取得
        $backTrace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT, 2);
        $traceCalled = array_key_exists(0, $backTrace) ? $backTrace[0] : null;
        $traceCaller = array_key_exists(1, $backTrace) ? $backTrace[1] : null;

        try {
            $request =  ... ; // 利用状況も合わせて記録するためHTTP Requestを表現するオブジェクトを取得
            $logger =  ... ; // Loggerの取得
            // Loggerで情報を送信( Slackへの通知については、logを種別ごとにRoutingしてSlackの任意chに送る設定をできるような仕組みが社内あるので、それを利用しています )
            $logger->info('[ActiveCodeChecker::notifyIfCalled] コードの実行が検知されました.', 
                'group' => $group,
                'called' => $traceCalled !== null ? [
                    'file' => $traceCalled['file'],
                    'line' => $traceCalled['line'],
                ] : null,
                'caller' => $traceCaller !== null ? [
                    'file' => $traceCaller['file'],
                    'line' => $traceCaller['line'],
                ] : null,
                'url' => $request->getUri(),
                'referer' => $request->headers->get('referer'),
                'user_agent' => $request->headers->get('user-agent'),
            ]);
        } catch (\Throwable $e) {
            // 万が一、検知した内容を通知する部分でエラーが起きた場合、ここ起因で全体を落とさないようにするため E_USER_WARNINGでエラーを出して処理を継続
            trigger_error(
                sprintf(
                    "[ActiveCodeChecker] Failed to notify! [group=%s][called=%s::%s][error=%s]",
                    $group,
                    $traceCalled['file'],
                    $traceCalled['line'],
                    $e->getMessage()
                ),
                E_USER_WARNING
            );
        }
    }
}

Slackで通知している理由

上で紹介したようにコード自体はInfo Logを送信しているだけになっています。そのため、logをどう扱うかに関しては自由度はあったのですが、まずはSlack通知が既存の仕組みでもありお手軽なので、まずはそれで試してみて困ること(特に大量に通知がきたり)があるようなら何かしらの仕組みを考えようと思っていました。

実際、Slackで運用してみると以下のように実運用上問題がなく、むしろちょっと便利だったりしています。

基本、使われてないであろう箇所に仕込むので通知が来るケースは少なく、あったとしても大量に呼び出されているようなケースはほぼなかった
引数に指定できるgroup(検知用識別子)に、Slackの通知設定で設定しているマイキーワードを含めておけば自分に関心のある通知だけ受け取れる

仕組みを導入して、すぐにでた成果 / 18万行のコード削除

不要なコードが多数存在する中でも最も強い課題意識を持っていた点として、ごく限られた機能のためだけに大きなフレームワークに依存した外部パッケージを削除することによる影響を読みきれなかったことが挙げられます。

そこで ActiveCodeChecker を使い以下のような流れで、そのフレームワークの削除を進めました。

大きなフレームワークに依存している各クラスに ActiveCodeChecker を仕込む
ActiveCodeChecker によるコード利用検知の記録が見つかった箇所をレビューし、想定外の依存が判明した箇所について依存を切り離す開発を実施
一ヶ月後 ActiveCodeChecker の通知がなくなったことを確認し、切り離された箇所を削除( -2,282行 )
加えて、大きなフレームワークに対する依存が完全になくなったことを確認するためにフレームワークのboot部分に ActiveCodeChecker を仕込む
2週間後、新たに仕込んだ ActiveCodeChecker の記録がないことから大きなフレームワークへの依存もないと判断して当該フレームワークをすべて削除( -189,412行 )