複雑さに潜り込む - 大規模PHPアプリケーションにおける例外・モニタリング・ロギング

みなさん、PHP書いてますか？ここ2ヶ月くらいPHPも書いていたのでその話を書きます。

この記事はVOYAGE GROUP techlog / Advent Calendar 2016の記事です。

例えば以下のような話に身に覚えはありませんでしょうか。

• 例外がどこかで握りつぶされており、例外的状況なのにエラー表示がまちまち。レスポンスステータスも一貫性がない。エラーログが適切に出ていない。
• エラーログ出力用コードがいろんなところで散乱している。エラー文字列整形のための適当なヘルパメソッドがクラスごとに実装されている。
• エラーごとにエラー表示のためのメッセージを設定するのが面倒になり、「システムエラーが起きました」とだけ表示されるようになってしまった。
• 例外ハンドリング周りのコードは考えるのが面倒なのでコピペだらけになっている。
• オブジェクトの依存関係がクラスのプロパティに大量に埋め込まれている。これを初期化するためのロジックがコンストラクタないし初期化のためのメソッドに組み込まれている。これが親クラスとなり、継承されたクラスには暗黙的に大量のプロパティが継承されている。またそのためテストコードから大量の初期化処理が必要になる。

そんな問題を解決すべく、ここ最近取り組んでいた話をします。

環境

• PHP 5.3.x / CodeIgniter 1.7.x
• 5,6年ほど運用されているPHPアプリケーション / だいたい10万行くらい
• 社内 / 社外のユーザが利用する管理用のアプリケーション
• php-cs-fixerでほとんどのコードの体裁は整っている
• Class LoaderはCodeIgniterとcomposerのものが併用されている / たまに require_once もある

兎にも角にもモニタリング

アプリケーションの課題は何か？をチームで考え始めたとき、以下の点が話し合われました。

• 何がどこで失敗しているかわかりづらい
• 既存コードを読み解くコストが高い / 不必要に複雑になっている
• よく使われているコードに費やす時間が少なく、新しい機能に時間が割かれがちになる
• 当時メンテナンスしていたエンジニアがいなくなっており、すぐに構造について聞ける人がいない

どれも大事ですが、個人的にはアプリケーションエンジニアの行動指針として特に3番目は大事だと思っています。新機能はもちろんビジネスの目玉になるかもしれませんし、戦略上重要な場合も多いでしょう。しかしながら、既存コードは価値を生み出し続けており、事業を支えているという場面は少なくありません。費やす時間が少なくなればコードの保守は疎かになります。エンジニアチームとして保守フェーズに入っている機能をどのように質を保ちつつメンテナンスしていくかというのは課題です。

そこでまず、よく使われているコードで問題が起こっている箇所を効率よく知る方法はなんだろうかと考えました。結論としてはNew Relicでアプリケーションモニタリングをはじめました。

Application Performance Management & Monitoring | New Relic

New Relicでは以下のことができます。

• 時系列でのPHPランタイム状況の把握
• 実行に時間のかかっているコードのプロファイリング
• 遅いクエリの抽出 / 1つのHTTP requestごとに発生したクエリの本数と所要時間などの把握
• エラー状況のモニタリング / HTTP Requestごとに発生したエラー内容の把握

いま着手しているアプリケーションはCodeIgniterの1.7系をベースとしつつ一部のAPIではSlim Frameworkが利用されています。そのため、特定のフレームワークによるプロファイリングツールではなく、PHPランタイムの情報からトレースしてくれるようなツールを探していました。New Relicの場合は今回のユースケースにあったため、利用を開始しました。DB側でもスロークエリログを出力していますが、New RelicからだとどのHTTP Requestのパターンにおいて何本クエリが発行されておりレスポンスタイムを長くする要因になっているか、ということを把握しやすくなっています。このあたりはOR Mapperを改良して例えばSQLコメントにHTTP requestごとのIDを埋め込み、traceabilityを確保する方法もあります。私達の実装ではOR Mapperが必ず使われているわけではなかったので、モニタリングツール側でトレースできることは役立ちました。

「よく使われるコードとはどこか」という観点はもちろんのこと「ユーザにとってストレスのかかっている画面 / URLはどこか」というのがアプリケーションパフォーマンスモニタリングによって開発者間で共有できるようになったのは大きな変化でした。これにより、パフォーマンスに問題のある箇所のissueを明確にできたのはいうまでもありません。

catch (Exception $e) の香り / アプリケーションベース例外の設計とハンドリング

ここで質問です。MVCにおけるコントローラクラスのあるメソッドで以下のコードがありました。このコードの好ましくない点はどこでしょうか。

<?php
try{
    // DBにつないで結果返したり
    // 権限チェックしたり
    // とにかく色々な処理
} catch (Exception $e) {
    error_log($e->getMessage());
    $this->tpl->display('error.html', array('message' => $e->getMessage()));
}

色々ありそうですね。単品でもよくないのですがこんなコードが300箇所弱ありました。私が最初これを見たときに思ったのは以下の点です。

• ルート例外で catch しているのはなぜ？
• スタックトレースはどこにも出力していないけどここで catch するまえにどこかで出しているのだろうか・・？
• $this->tpl がいろいろ握ってそうな気配がある
• エラー画面に例外のメッセージそのまま出してるけど、ログと画面表示に同じメッセージ出して後から原因追えるのだろうか？
• （ぱっと見わからないけど）これレスポンスステータス返していないのでは？
• catch 句の中身がだいたいコピペ

他にも細かい点は色々ありますが、ぱっと思ったのはそういうところです。結果的にきれい汚いだとかコードスタイルだとかを置いておいて以下の点がよくありませんでした。

• スタックトレースがないので原因究明がしづらくなっており、論理例外が起きても対応が放置されている。開発環境でのみスタックトレースを表示するという制御ができない。
• アプリケーション例外が throw Exception("処理に失敗しました") のようにルート例外にメッセージだけ設定して throw されている箇所が多々ある。
• エラーログの内容が少ないため、いつどこで何が要因で失敗しているのかがあとからトレースできない
• エラー表示のための tpl がクラスのプロパティ（実際には親コントローラのプロパティ）に依存しているのでコードの分離がしづらくなっている。そのためエラー表示のためのコードをコントローラに入れざるを得なくなっていた。
• エラーログ出力のためのコードが各 catch の中で書かれているため、たまに出力が忘れられており、エラーが起きていることすら把握できない状況になっている部分があった。

これらの課題にアプローチするためにはアプリケーション構造から着手するのが効率がよいと考えました。そこで、今定期的にコンサルに来ていただいている @t_wada さんとも相談しつつ、以下の方針を立てました。*1

• 大域の例外ハンドラを実装する。例外を throw すれば適切にエラー画面が表示されるようになること。また例外に応じて適切にHTTP response statusが設定されること。これをうけて、ユーザの各実装において try catch は必要最低限にすること。
• 独自定義の例外はすべてベース例外を継承すること。ベース例外には画面表示向けのエラーメッセージとシステム向けのエラーメッセージを分けて保持できること。
• 例外を catch して挙動を変更する場合にも、特別な場合*2以外には例外を投げ直すこと。例外を投げ直す場合にはスタックトレースを第三引数で引き継ぐこと。

ベース例外は以下のような設計になっています。

<?php
// 注: Throwableは独自実装になっていて、PHP 7のためのpolyfillです。
class BaseException extends \Exception implements Throwable
{
    protected $userMessage;

    /**
     * @param string $message
     * @param int $code
     * @param \Exception|NULL $previous
     * @param string $userMessage
     */
    public function __construct($message = "", $code = 0, \Exception $previous = null, $userMessage = "")
    {
        parent::__construct($message, $code, $previous);
        $this->userMessage = $userMessage;
    }
}

$userMessage （このネーミングももっといいものがあっただろうとは思いますがひとまずはこれで・・）にはユーザに見せるための文字列を設定します。コンストラクタは PHP: Exception - Manual のコンストラクタに第4引数を追加しており、ここにユーザ表示用文字列を設定できるようにしています。独自例外を定義する場合にはこのユーザ向けメッセージを各例外のデフォルト値として設定できますし、あるいは例外を throw する際に適宜生成して埋め込むこともできるという設計になっています。またあとでも出てきますが、ベース例外の $code についてはHTTP response status codeとして利用されるようになっています。これは「Webアプリケーションの例外コードはHTTP response status codeとするのが明確なのではないか」という考えからです。このあたりは @t_wada さんと議論していて気がついたことですが、PHPの例外コードは案外使われていないという話になり、それならHTTP response status codeとしてしまうのが見通しがよさそうですね、という話をしました。

PHPでは大域の例外ハンドラを設定するために PHP: set_exception_handler - Manual を利用できます。 set_exception_handler を利用すると、すべての throw されてきた例外についてのハンドラ関数を設定できます。*3これはフレームワーク側での仕事でなければなりません。私達がこのアプリケーションで利用しているCodeIgniter 1.7系では大域例外ハンドラはありません。そこでフレームワーク初期化時に set_exception_handler によって大域例外ハンドラを設定するように変更しました。*4

これでめでたく大域例外ハンドラが仕事をしてくれる・・と思ったわけですが、まだ工夫をする必要がありました。大域例外ハンドラからもエラー表示用のテンプレートエンジンを利用する必要がありました。このテンプレートエンジンの初期化処理はコントローラの親クラスのコンストラクタで初期化されており、コントローラからは利用できない、という状況になっていました。*5そこでDIコンテナを導入しました。

DIコンテナの導入

大域例外ハンドラでテンプレートエンジンを利用するためにまだ問題がありました。親コントローラのコンストラクタに大きく依存しているアプリケーション初期化処理がありました。

Webアプリケーションの入り口はどこか？というとHTTP Requestです。なのでフレームワークのユーザからするとコントローラの初期化時、すなわちコンストラクタに色々書きたくなってしまうのでしょう。SlimやSilexなどのマイクロフレームワークだとコントローラではなくRouterなので初期化処理はDIコンテナでやるというのは自然かもしれません。とはいえDIコンテナのインタフェースが用意されていない場合、コントローラ初期化時にアプリケーションで利用する様々なコンポーネントの初期化処理を書くという選択はありがちだと思います。例えばセッションを読み取りつつ権限をチェック、テンプレートエンジンの初期化しつつ便利メソッドを生やす、ヘッダー・フッターなど共通部分のコンポーネントをログイン情報をもとに初期化する、DBに接続しコネクションを保持する、などです。これはWebアプリケーションのtestability、あるいはコントローラのtestabilityを下げる要因でもあります。コントローラが状態を持ちすぎているためです。

そこで私達は段階的にこの依存関係の箱を移していこうと考えました。今の親コントローラに初期化処理を依存した状態ではコントローラ以外からセッションをつかったコードを書いたり、テンプレートエンジンを利用するコードを書くことができません。これはアプリケーションの自由度を増やしますが、ある意味で複雑度を増す選択です。この選択には慎重でしたが、DIコンテナは軽量かつdisposableなコンポーネントとして利用できると考え、進める判断をしました。*6

DIコンテナを作り、親コントローラのコンストラクタにおける初期化ロジックを移植しました。これはアプリケーションのどこからでも利用できるようになっています。具体的にはCodeIgniterはルートオブジェクト*7 ã‚’ &= get_instance() によってアプリケーションのどこからでも取得できます。この仕組みに乗っかり、DIコンテナもルートオブジェクト経由で取得できるようにしました。このあたりはチームメンバーががんばってくれました。

<?php
$ci =& get_instance();
$container = $ci->pimple->container;

DIコンテナを経由することで、大域例外ハンドラで利用可能な初期済みのテンプレートエンジン及びセッション情報が取得できるようになり、従来通りのエラーページが表示できるようになりました。ちなみにDIコンテナにはPimpleを使っています。

大域例外ハンドラの実装は概ね以下のようになりました。フレームワーク初期化時に post_controller_constructor Hookからこれを呼ぶことで大域例外ハンドラを設定しています。

<?php
class ExceptionHook
{
    public function setExceptionHandler()
    {
        set_exception_handler(array($this, "handler"));
    }

    /**
     * @param Exception $e
     */
    public function handler(Exception $e)
    {
        $ci =& get_instance();
        $container = $ci->pimple->container;
        $twig = $container['twig'];
        log_message("適当なエラーメッセージがいい感じにここに埋め込まれる秘密の仕組み", $e->getMessage());
        if ($e instanceof BaseException) {
            // 例外コードをHTTPレスポンスステータスコードとして埋める
            header('HTTP', true, $e->getCode());
            $twig->display('error.html', $e->getUserMessage);
            return;
        }
        header('HTTP', true, 500);
        $twig->display('error.html', $e->getMessage());
    }
}

これでようやく例外の足回りが整いました。 throw すればいい感じにHTTP responseが生成され、エラーログが出力される世界になりました。

とはいえ、例外に関するのコードはアプリケーションコード全体からみるとごく一部です。メンテナンス性を高めるにはやることがまだまだあります。

使われていないコードを消すことはメンテナンス性を高める

知らないコードを読むとき、どのように読みますか？私は知らないコードを読むとき、関数であればまずinputとoutputを意識して読み解きます。副作用がないならこれはさくっと終わります・・が、もし関数外部の状態が変更されていれば頭の中のスタックが溢れそうになってしまいそうになりながら、状態を片隅に置きつつ別のコードを読み進めます。その状態は初期状態がどこで定義されているのか、あるいは場合によって状態の設定というのは方法が変わってくるのか。ああこのプロパティは外部からもどうやら変更されているようだ、お手上げだ・・などと思いつつ読み進めます。まあようするにコードを読むというのは体力のいる作業ですし、できればさっさと結論を知りたいのです。このセンテンスのように。

いらないコードは消しましょう。使われていないコードは消しましょう。コードが短いことは、将来のチームメンバーの時間を節約します。

dead code、つまり呼ばれていないコードをまず消すことを考えるでしょう。しかしながらこれを将来に渡って習慣づけて続けていなかければまた雑草がぼうぼうに生えた花壇のようになってしまいます。そこでコーディングガイドラインに「コードの消し方」を書きました。sebastianbergmann/phpdcd をdead code検出のために試用しましたが、CodeIgniterの独自loader環境においては限定的なdead codeしか検出できなかったため、PhpStormのinspectionを使ってdead codeを探すというのをメインでやりました。

使われていない関数やクラスがある場合、私達が取りうる手段は2つあります。*8

• さっさと消す
• @deprecated にする

継続的に使われていない関数やクラスを消していくためにはどうしたらいいか？明らかに呼び元がない関数は消して良いでしょう。しかしPHPの場合、実行時にはじめて呼ばれるとわかる場合もあります。例えば PHP: call_user_func - Manual をつかって $callback を動的に指定されている場合、静的解析でどの関数がコールされるかを推定するのは難しいです。もし消すのが不安な場合には PHP: trigger_error - Manual をつかって E_USER_DEPRECATED エラーを生成しましょう。

<?php
/**
 * @deprecated
 */
function d()
{
    trigger_error("function d() is deprecated since 2016/12/14. use f() instead.", E_USER_DEPRECATED);
}

関数をdeprecatedにする場合には上記のようにすることをコーディングガイドラインにいれています。*9 私たちはこれをエラーハンドラからロギングすることにより、非推奨な関数が呼ばれているかどうかをモニタリングできます。またPhpStormなどのPHPDocがサポートされたエディタをつかっていれば呼び出し側のコードにwarningを出せるため、実装の手助けにもなります。

この方法は「今すぐには消せないけど将来的には置き換えたいコード」を示すためにも良い方法です。

次のステップ

という感じにざーっと進めてきたのがこの2ヶ月ほどの取り組みでした。ひとまずアプリケーションとしての足回りを整えてきました。とはいえこれが今までチームの中で見過ごされてきていたのが最大の課題だなと思っており、継続的にアプリケーションを改善できるようにするにはどうしたらいいものかというのが私の最近の関心ごとです。

解決できていない課題も残っています。しかしながらこれらは一気に解決できるものではなく、組織でアプローチすべき課題だと考えています。今回これとは別に以下を進めました。

• PHP 5.3系からPHP 5.6系へのバージョンアップのためのコードの書き換え (コンストラクタ書き換え / static callの対応 などなど)
• require_once を消してcomposerのautoloadに移植。あるいはCodeIgniterのLoaderからcomposerのautoloadへの段階的なライブラリの移植。
• 既存のSelenium baseのブラウザテストと別にさくさくとHTTP response statusをテストするための並列HTTPテストツール作成*10
• 使っていない use 句の削除 / 使ってないライブラリの削除・あるいは同機能のライブラリへの統一
• @property アノテーションを利用して簡易的にCodeIgniterのloader経由で呼ばれているクラス及びメソッドもinspectionを可能にする

課題は見えたところからコツコツ解消していかなければなりません。組織としてこうした課題に対してどのように対応するか、あるいはどのように時間を割いていくのか。個人的には複雑なアプリケーションが実装されるのはある程度しかたないないと考えています。そしてcomplicatedなアプリケーションはある程度技術的なアプローチによって改善可能であるし、事業を支えるプロダクトに改めて息を吹き込むというのはやりがいのあるテーマだと思っています。

もしそんなアプリケーション開発の話をしたいという方がいらっしゃれば是非 #ajiting しましょう！お気軽にお声がけくださいませ。 voyagegroup.com/recruit/

明日のアドベントカレンダーの記事もお楽しみに。

---

P.S. 某原稿の締め切りが伸びたので気合い入れて記事を書こうと21時ごろからスーパードライ片手に書き進めていたら一万字を超えてしまい反省しています

*1:t_wadaさんにはテストや例外設計のことであったり、開発チームの課題の掘り起こしであったり、ペアプロをチームメンバーとしていただいたりしています。

*2:たとえばretry可能な例外などの場合には例外の投げ直しをせずに通常の処理をretryするなど

*3:もちろん、握りつぶされている例外については大域の例外ハンドラで捕捉することはできません。

*4:ちなみにCodeIgniter 1.7系ではPHPエラーのための大域ハンドラがありますが、例外のための大域ハンドラはありません。

*5:ログインしているときとしていないときとでエラー表示を分けたい、という考えからそういう設計になっているようです。ユーザからの使い勝手を考えるとここは挙動を変えないほうがよいだろうと判断し、そのまま踏襲しました

*6:ちなみに私がよく書くGoの世界ではDIコンテナはあまり一般的ではありません。typeとinterfaceが大概の依存関係を明示してしまうからです。というのは横道にそれるので注釈で・・

*7:CodeIgniter用語でいうとベースオブジェクトになるかもしれません。CI_Baseのことです。

*8:放置するというのをいれると3つですね

*9:このアプリケーションに関して言えば、コーディングガイドラインがいままでなかったのです。この2ヶ月の取り組みの中であわせて、アプリケーションとチームにあったコーディングガイドラインを作っています。これはまだ作成途中ですが、レビューの指針にもなりますし、何よりチームのPHPに対する知見を集約しレベルを引き上げようという意図があります。

*10:これはGoの net/http を使い、ログイン後のセッションを持ち回しつつGo 1.7のsubtestをつかってparallelにさくさくとHTTP responseに対してテストをする、というツールです。