2024-08-23

SpringBoot3.4で構造化ログが標準でできるようになると聞いて

SpringBoot Gradle

SpringBootでのサーバーアプリケーションはもうコンテナ運用も当たり前で、コンテナ運用が当たり前ってことはログも生ログファイルを見るんじゃなくどこかに集められたものを見るのが当たり前で、複数のサーバーで運用されるログを集約してみる場合は何かしら横断的にログを検索する項目があるのも当たり前で、集約システムとか挟むんだからログもパースできるのが当たり前で。

いろんな事情に後押しされて、現代のサーバーアプリケーションは構造化ログを出すのが当たり前です。 SpringBootで構造化ログを出力する場合、現段階ではECS LoggingやLogstash Logback Encoderなどを使用するのが一般的でしょう。

ECS Loggingとかの話

「ECS Logging」と言うとコンテナ運用の話もあいまって「Amazon ECSのログのこと？」となり「うちAWSじゃないしなぁ」とか「Amazon ECS使ってるけどAWS依存させたくないなぁ」とかの反応もしばしばありますが、このECSはElasticCommonSchemaで、Elasticsearch文脈です。

github.com

ECS LoggingはElasticsearchを使用する場合にはもちろん使えますが、別にElasticsearchを使わなくても（たとえばAmazon CloudWatch LogsやDatadogなど）、JSONの構造化ログをパースできるものであれば使えます。

Logstash Logback Encoderも似たようなもん。名前の通りLogback用なので、Log4j2とかを使うならあったものを探したりする感じです。

github.com

で、これらはSpringBootの外のライブラリであり、独自に依存を追加する必要があります。また、SpringBootの依存管理対象外のため、バージョンも自身で面倒を見る必要があります。地味に面倒臭いですが、必要なので仕方なく面倒を見ていました。しばしば忘れられがちでもあります。

SpringBoot3.4から追加ライブラリなしで構造化ログを出力できるようになる

本題。冒頭のポストの通り、SpringBoot3.4.0でSpringBoot自身が構造化ログをサポートするようです。詳しくは以下のSpringのブログを見てください。

spring.io

ここでは logging.structured.format.console=ecs を書いていますが logging.structured.format.console=logstash もOKです。ブログの通りECSとLogstash、つまり ecs と logstash の二値がサポートされているようです。

マイルストーンリリースの使い方

GradleやMavenでSpringのマイルストーンリリースを使用する場合、（おそらく普段使用しているであろう）MavenCentralRepositoryにはないのでマイルストーンリポジトリの参照を追加してやる必要があります。

Springのマイルストーンリポジトリは https://repo.spring.io/milestone です。（Springの公式ドキュメントにも書いてるんだけど、探すの面倒だったので割愛）

Gradleではbuild.gradle.kts の repositories に追加すればOK

repositories {
    mavenCentral()
    maven {
        url = uri("https://repo.spring.io/milestone")
    }
}

どうでもいいけどIntelliJさんの補完がこれを候補にだしてくれて、やっぱみんな追加するのSpringなんだなぁってなりました。

で、これだけだと動かない。なぜならSpringBootプラグインを使ってるので、プラグインの使うリポジトリも追加してやる必要がある。こっちは build.gradle.kts よりも前に解決されなきゃなので settings.gradle に書きます。

pluginManagement {
    repositories {
        gradlePluginPortal()
        maven {
            url = uri("https://repo.spring.io/milestone")
        }
    }
}

SpringBootGradlePluginのバージョンを 3.4.0-M2 にする。

plugins {
    java
    id("org.springframework.boot") version "3.4.0-M2"
}

これでOK。ちゃんと意図したバージョン使われてるかは確認しとこーね。

あとは logging.structured.format.console プロパティいじったり、 gradle dependencies で依存ライブラリ見たり、実装コード追って見たり、ブログ参照しながらLogWriterカスタマイズしたり、するとよい。

SpringBoot3.4以降の対応

稼働中のアプリケーションの場合、現在使用している構造化ログがいきなりつかえなくなるわけでもないので、現在使用しているものがあるなら別段慌てる必要はありません。構造化ログの項目追加などカスタマイズしている場合は対応内容も変わるでしょう。ログ出力周りは性能などの兼ね合いもあります。動いているなら私なら特に慌てません。何か機会があればちょっと見てみるかーくらいの温度感で対応すると思います。

一方、新規であれば「ECS LoggingかLogstash Logback Encoderなどを検討する」の前に「SpringBoot標準機能のこれで十分だったりしないかな？」を検討すると思います。新規でなくてもログのカスタマイズなど手を入れていなくて単に構造化だけをやっている、まだ構造化ログにできていない、などの段階であれば、とりあえずプロパティ有効にしてみるとかもありだと思います。こだわりのない依存ライブラリ管理は一つでも減らせるに越したことはないです。

そんな感じ。

2024-06-20

ThreadContextやMDCはremoveせずにcloseしよう

Java

新くもなんともない話です。当たり前と思う人には昔から当たり前（昔度合いはおまけ３を参照）。でも知らないことは悪ではないし、そうなんだーってなればいいだけの話。

一連のログに同じ値を付与したい時、 MDC (SLF4J) や ThreadContext (Log4j2) を使うかと思います。中身は java.lang.ThreadLocal とかだったりするので、サーバーアプリケーションやマルチスレッドでは取り除いてあげないと事故の元です。

// 微妙なやり方
void method() {
    MDC.put("username", "hoge");

    // ... なんか処理

    MDC.remove("username");
}

こんなことしちゃうと例外が発生した場合に remove されなくて事故るので、伝統的な方法では finally でやるかと思います。

// 伝統的なやり方
void method() {
    try {
        MDC.put("username", "hoge");

        // ... なんか処理

    } finally {
        MDC.remove("username");
    }
}

ThreadContext もここまではほぼ同じ。

あと細かいけれど、上記の "username" が二箇所に出るとか、複数の項目を扱うときとか、あんま嬉しくないです。

本題: 後片付けはJavaに任せよう

try-with-resources を使えます。

SLF4Jだとこう。

void methodMDC() {
    try (var ignore = MDC.putCloseable("username", "hoge")) {

        // ... なんか処理

    }
}

Log4j2だと ThreadContext のかわりにCloseableThreadContext です。

void methodThreadContext() {
    try (var ignore = CloseableThreadContext.put("username", "hoge")) {

        // ... なんか処理

    }
}

try-with-resources で close することで remove 漏れもないし、キーを複数書かずによくなり、取り違えも無くなります。 put と remove を1メソッドに書けない（事前処理と事後処理を別のところで書かなきゃいけないなど）とかでなければ、常にこれらを使うことをお勧めします。

SFL4J は補完で putCloseable メソッドが出てくるので気づきやすいのですが、 Log4j2は org.apache.logging.log4j.CloseableThreadContext と別クラスになるため認知率はガクッと下がる印象です。Log4j2やSFL4J以外を使っていても、同様のものがあると思います。

var を使っているのは素直にやると MDC.MDCCloseable や CloseableThreadContext.Instance と長くなるにも関わらず、この変数の型に興味がないためです。こういうとこは var が輝く。

あと変数名を ignore とかにするのは、未使用変数でもこれは警告しないでくれってIDEAへの主張です。

try-with-resources を使用する場合にはよく使うと思います。ignoreでもignoredでもいいし、複数あるとignore1とか連番になったりでダサくなります。個人的には _ とかにしたいんだけども。 IDEの警告はゼロにしとくのがいいです。

脱線: 濫用？

MDC や ThreadContext はログライブラリのものなんですが、使えるからと言う理由でデータ受け渡しに便利に使用されていたりします。

そんな使い方をするんじゃねぇ、必要であればその情報受け渡しをきっちり設計しようよ……と思ったりもするんですが、引数で引き回す以外の方法となると、DBなどの永続技術を使うか ThreadLocal などを使用する必要が出てきます。単に動くだけならどんな方法でも簡単なんですが、適切に設計するのは難しく、下手なことするくらいならこっち使っておく方がマシか……と思ったりも。 ThreadLocal を使うと非同期処理を使う時とかに適切に受け渡す必要があったり（たとえば SpringBootでAsyncを使う時に知っておきたいExecutorのこと (2023-08-24) で書いているように）、複数このようなものが出てくると漏れたりしがちで、あと引きまわしたいスコープも似たり寄ったりになるので、まぁもういいかって思ったり、いややはりちゃんと、となったり。

Observation の Context もだし、今回の例で username と書いたけど認証コンテキストもそんな感じ。たぶん各々で適切に取り扱う何かを作って、必要なとこでアダプタを作るーとかすれば綺麗なんだろけど、コストかけるとこでもないかなぁ……とかとか。なやましい。 ~~動けばいいねん動けば~~

おまけ

これらがないときは AutoCloseableでなくてもtry-with-resourcesがしたい (2013-01-05) の方法で足掻いたりしかけて、わかりづらくなるかーってなって、普通に finally でやったりしてました。

// 今は不要
void method() throws Exception {
    MDC.put("username", "hoge");
    try (AutoCloseable ignore = () -> MDC.remove("username")) {

        // ... なんか処理

    }
}

putCloseable あるのにこれやってたら色々考え直した方がいいです。

おまけ２

はてなブログにいつのまにかAIでのタイトル生成機能がついてたので試してみた。

どれもコレジャナイ感。だけど、AIがつけるタイトルと自分がつけるタイトルが近い文章のほうがいい文章なのかもしれないなぁ、とか思ったりもした。

おまけ３

冒頭で「昔から」って書いたので、どれくらいかなーと調べてみた。

SLF4J は 1.7.8 で入っているので、 2014-12-14。
Log4j2 は 2.6 で入っているので、2016-05-25。

へー。日付は CentralRepository のタイムスタンプね。

2024-06-12

コードレビューのコメントすべてに対応する必要があるか

いろんな現場でちょいちょい話題になるので「前にツイートしたなぁ」と見てみたらすっごい長かった。

コメントあると「対応しなければならない」ってなるの。あれが諸悪の根源だと思ってる。
— irof (@irof) 2020年9月17日

諸悪の根源？

とりあえず全文転記。誤変換とかもそのまま。

コメントあると「対応しなければならない」ってなるの。あれが諸悪の根源だと思ってる。

目に見えてる問題を潰したくなるのはごく当たり前の感情だと思うけど、目に見えてるもの全て潰したからと言って完璧にはならないことは理解する必要がある。言えるのは「目に見えるものはない」というだけで……そしてコメント対応とかだと、「見た目」のメタファすら使えなくなる。

「コメント対応」のような問題の潰し方をすると必ず歪になる。これは自分の目で見つけたものなら曲がりなりにも「自分の目」の精度は高くないものの一定の見方ではあるのに対し、コメントは特定部分になるので、一貫性のない歪になるのが確約される。その人には見えないように、はできるかもだけど。

ともかく「コメントは全て対応しなければならない」と言うのは、そのほうがいいって刷り込みだったり、対応しなければならないって強迫観念だったりで、ゼロリスク進行にも通じるものがあると思う。こう言うのがあると「コメントしたら進まなくなるし今回はやめておこう」とか変な力が働いたりする。

issueが数十件ある某リポジトリに「こんないっぱい問題あるんですね」とか言われたのだけど、ゼロにするの目標にしたりしてないし、制御できてるならそれでいいと思うんだよね。数千とかなって見る気も対応する気も失せるのとは違うと思うのだけど、ゼロありきの思考だと同じに見えてしまう。

コメントはコメント、対応するかどうかは別の問題……と言っても、スルーだとこれはこれでコメントする気が失せるので、何かしらのリアクションと言うか、変化は必要。何も変化させない行為は無駄と感じてしなくなるものだと思う。実際無駄なわけだし。

と言うことで「コメントを対応しなければならない」と考えるのは、対応し切るまでそこで停滞するし、それを嫌うとコメントそのものをしづらくなるし、コメントされた側は凹んだりするし(面倒なのはわかる)、対応したところで歪にしかならない。ほんと悪いところしかない。まず対応の確約を捨てるべき。

少し語弊があるな。対応に「コメント見ました」で特に対象物を変更しない、と言うのも含めるなら対応するとしても良い。とにかくコメントに対して「何か変更を加えてコメントが一切出なくするのが理想」みたいなのを信仰してたりする。はっきり言って呪いだと思う。

どうやったらしなくて良いようにできるかで、「別課題で管理します」みたいなのがある。これはそれなりに有効なのだけど、その課題を作る手間もかかるし、同様にして出てきた他の課題との重複とかも確認しなきゃ、そうして作られた課題群はただのゴミの山になる。何もないよりマシかどうかは微妙。

レビューとかのコメントは有効なんだ。有効なんだけど、効くところが限定的。でもそれでしか拾えないものがあって、外せないものになる。外せないからここで賄えそうなものは全部寄せられる。結果、得意分野じゃないとこも合わせた定量評価をしようと言う動きが出てくる。どれも順当なのに結果は不幸。

人がコメントすることでしか賄えないものと、人が作るべきものってのは多分一致する。機械的に検出できるもの、そして機械的に対応できるものは作らなくて良い。開発の道具はそうして進歩してきたわけで、その先のいくつかの候補がたまにバズるアレとかソレなんだろう。その活動全部でプログラミング。

コメントは「ここから見たらこう見える」と言う、見えたことを語るものなんじゃないかなってのが最近思ってることで。そこに無理矢理複数の視点を混ぜるから、玉石混淆になってしまい、扱いに困った結果「全部対応」の引力に飲み込まれる。とは言え人がやる以上「ふと気づいた」は避けれなかったりも。

コメントの全対応を強いるのは、コメントと対応を定量評価してるからで、そうさせたのはコメントに様々な期待を相乗りせたこと。そもそもが人間の感覚に頼ったピーキーなものなのに、そこで見つけられたものに対処するのは一定の成果を挙げてきたから強化されてきたが、このモデルが軋みを上げている。

きっと計算機が高額で遅かった頃は合理的だったんだ。そして多分とっくにひっくり返ってる。なのでこの一連のスレッドのコンテキストを理解できない組織にいる人も居るんだろうけど、なんでもコメントして全部対応に寄りかかってるとこはまだまだ多い印象ある。

「全てのコメントを片付けなければならないと考えるのが諸悪の根源」として始めたのに、全然根源でない……と言うか、表出した事象に過ぎなかった。表出した事象は短絡的に潰してはいけない。掘り下げて出てきたものも、まだ表層かもしれない。根っこはどこだ……

機械的に対応できるような、人がしなくてもいいコメントが含まれてるのは一因。これは検証済みマージとかと同様に機械的なチェックでカバーして棲み分ければ減らせるかもしれない。この性質のコメントの割合が減れば、対応の難度が上がり、全対応の無理筋加減はわかりやすくなるかもしれない。

これはコメントのほとんどが機械的に対応できるものなので、コメントはそう言うものと認識されていると言う仮説。この手のコメントは誤りの指摘であり、多いのは事実。このことが「コメントされたら直すもの」「コメントされるとは誤っている」と考えてしまうことに繋がる。のかもしれない。

https://x.com/irof/status/1306446734176862208

いや長いな。ふむふむ、と読んでたんだけど最後のほうで

「全てのコメントを片付けなければならないと考えるのが諸悪の根源」として始めたのに、全然根源でない……と言うか、表出した事象に過ぎなかった。

とか言ってて「おいっ」ってなった。まぁいいや。とりあえずこれをChatGPTに要約してもらう。

コメントに全て対応しなければならないという考え方が問題だと述べています。目に見える問題を全て解決しても、完璧にはならないことを理解する必要があります。コメント対応に追われると、一貫性のない対応になりがちです。全てのコメントに対応することが理想と考えるのは誤りで、時には「見ました」と言うだけの対応でも十分です。コメント対応は合理的に管理し、必要ならば機械的に処理できる部分を分けて対応の負担を減らすことが重要です。コメントは多くの場合、人間の感覚に基づいているため、全対応を目指すのは非現実的です。

そんなこと書いたっけ……？あってるような、ちょっと違うような。

「挙げている問題点」を抜き出してもらうとこんな感じ。

一貫性のない対応:
- コメントに全対応すると、特定部分に偏った対応になり、一貫性のない歪な結果になる。
進行の停滞:
- コメントに全対応しようとするとプロジェクトの進行が停滞し、コメントする意欲も削がれる。
対応の過剰負担:
- 機械的に対応できるコメントが多く含まれており、これに全対応しようとするのは非現実的で負担が大きい。

へぇ。他には？とか聞いてるとどんどん出てくるけど、無理に挙げてる感じで重複もでてくる。

ゼロリスク進行の弊害:
- コメントを全て解決しようとするゼロリスク進行の考え方が、実際のリスク管理や問題解決を歪める。
心理的な負担:
- コメント対応が多いと、開発者は心理的に圧迫され、モチベーションが下がる可能性がある。
無駄な労力の増加:
- 必要のない対応に時間や労力を割くことで、重要な作業に集中できなくなる。
コメントの質の低下:
- 全対応を求める風潮があると、コメントが雑になり、質の低いフィードバックが増える可能性がある。
コメント対応の遅延:
- 全てのコメントに対応しようとすると、対応が遅くなり、重要なフィードバックが埋もれてしまう。

あ、「ゼロリスク進行」は「ゼロリスク信仰」の誤変換なんだけど、そのまま謎ワードで解釈されてるね。興味深。

こういうのはすぐ「コメント対応するなってこと？」みたいな極論に振るんで、それも聞いてみる。

要はバランス（キリッ

このスレッドは特に大きくアップデートもないかな。私はこんな感じに考えてる。

コードレビューのコメントに関するスレッドは他にもあって。

コメントしづらくなる問題とレビュアーのできる対応

プルリクエストにコメントすると停滞してしまうのを見ると、「プルリクエスト単体で完成度あげるよりマージして全体の中で整合とった方がいい」派の私は、プルリクエストのコメントを控えるようになってしまう。
— irof (@irof) 2023年9月28日

プルリクエストにコメントすると停滞してしまうのを見ると、「プルリクエスト単体で完成度あげるよりマージして全体の中で整合とった方がいい」派の私は、プルリクエストのコメントを控えるようになってしまう。

控えちゃいけない、自分の職責を果たせ。と思って、なんとかコメントを書き、かつ停滞を避けるために「このプルリクエストで対応する必要はありませんが」などの枕詞を置くが、FYI程度に読み流されたり、マージ後は喉元すぎた感じで忘れられたりすると、この形のコメントはダメなんだなぁと。

おそらくやるべきはPRにコメントじゃなく新規Issueをたてる、なんだろなぁとは思うが、現行プロセスと喧嘩しがち。

どこに書くか迷ってどこにも書かず内心に抱えるくらいならどこかにアウトプットすべき、ではあるんだが、プルリクエストのコメントがあってそうで、なんかあってないんだよなぁ。

むろんこのスレの2つ目のコメントのように「対応不要」と言いつつコメントしたり、approveは出しつつコメント書いたりとかはするんだけど、どうも「コメントは全て対応しなきゃいけない」みたいな謎フォースもある。こういうのとか

コメントはコメントであってコメントでしかないんだよってコメントは結構ある。コメントの重さ的なの示せる機能あったら使うけど、そうでないのは(参考)(無視可)(感想)とかつけたりして。でもなんかスルーになれると対応して欲しいのもスルーされるようになって、まぁなんというか。 https://x.com/irof/status/1458012694741618689

で冒頭の「プルリク単体での完成度より全体整合」を優先してチーム状況が許した場合に選択肢に挙がるのがこれ。触ってるプロダクトのコード全部を共同所有できてる場合には最適なんだけど、そうじゃなかったら弊害も多い。実際うまく行ってるとこといかなかったとこがある。

https://irof.hateblo.jp/entry/2019/10/25/113652

「とっととマージして全体の中で整合取ろうぜ」とか言うと、マージしたあとのブランチ（mainにマージするならmain）が「マージ直後は壊れうる」をヘッジする他の仕組みがいるんだよね。「壊れないを保証」なんて言い出したら歴史改変につながるから、可能な限り壊れないための施策は施した上で「もし壊れた時は行燈を引いて全員が手を止めて早急に復旧する」になるんだけど、なかなか道は険しくて。一足飛びに行こうとすると事故って戻したくなっちゃう系。

長いスレッドはこの2つくらいかな。他にもちょいちょいあって、いくつか拾ってみる。

他に似たようなので細かいの

「レビュー指摘対応」ってコミットコメントには、どんな指摘だったのかとか、なぜその指摘に従ったのかとか、その辺の情報が足りないのです。 https://x.com/irof/status/752659191299317761

「コメントに対応する」というお仕事になると、コミットコメントには「なぜこの変更をしたのか」の背景をかくべきなので、妥当と言えば妥当なんだよなぁと。

レビューコメントには意図を返して欲しい。……あ、「指摘対応」とかのコメントはそれはそれでそういうメッセージか…… https://x.com/irof/status/956680702296170497

6年前に通った道だった。螺旋かどうかは不明。きっと反復横跳びも大事。

コードレビューを何かのゲートや承認のようなものにしたくないんだよなー。特にPR運用とか、どうしても差分ばっか目立つし……差分以外へのコメントしづらいし……同チームでなにやってんの、って思ったりする…… https://x.com/irof/status/1171755778979422209

コードレビューのコメントは有用だと思うんだけど、ゲートにするかは別の話。PR運用はそれがゲートになりやすく（Approveとか後押しする仕組みもある）、個人的には嫌いで、それが「脱ブランチファースト」のブログにつながってる。タイトルはちょいミスったと思ってるけど、まいいや。

irof.hateblo.jp

先のポストが2019年9月11日でブログが10月25日なので、多分この時期のマイブームだったんだろう。

レビューでコメントしたら「必ず何か変更しなければならない」となってるのはあんまよくなくて、(レビューの位置付けにもよるけど)そう言うもんじゃ無いよーとかは重ねて言わなきゃなんない。

「レビューの位置付けにもよる」とか奥歯に物が挟まったようなのを入れてるのは、レビューがフェーズゲート的な承認プロセスになっている場合とかもあるので。てか何も言わなきゃそっちのが多い気もする。この位置付けのレビューでは全コメントの決着が必要になってきたり、こなかったり。 https://x.com/irof/status/1577809260859121664

ふむ。

コメントするかどうかと、その後どうするかは分けて欲しいんよね。コメントする時にその辺とか考えると言うの控えるようになる。もちろんレビューの目的に沿わないコメントは言わないのがよいだろけど。

指摘された「から」直す、じゃねーんですよ。と。 https://x.com/irof/status/1677624849529974785

この辺りは後で紹介する「間違いだらけの設計レビュー」で「問題検出」「問題指摘」としてわけられてるところ。フェーズを示すほうが振る舞いやすい気はしないでもない。

「静的解析ツールに指摘されて提案されたまま直す」の、序盤はやっていいけれど、ある程度理解したらやってはいけない系になるものも多いです。コミットコメントに「静的解析の指摘対応」とか出てくるのはそのスメル。 https://x.com/irof/status/1651145378291662848

コミットコメントでありがちでイヤーな感じのする筆頭がこの手の「ツールに指摘されて直せっていわれた通り直してます！」と言うもの。昔は「こんなコメントはダメだ！」みたいなこと言ってたんですが、ここでは「スメル」にトーンダウン。というのも「ダメ」って言うと誤魔化しコメントを書くようになるだけなんですよね。なので「指摘対応」としかできないなら「指摘対応」としてその事実を記録しとくでいいや、と言う感じ。表出した事象を潰してはいけない。

参考

なぜ重大な問題を見逃すのか？間違いだらけの設計レビュー第3版

作者:森崎修司
日経BP

Amazon

最近第３版が出て、アジャイルやAIの話が盛り込まれました。「設計レビュー」と題されていますが、ソフトウェア開発における大体のレビューに適用できます。

こちらの本、というかレビューの本はだいたい「問題」にフォーカスしているため、コードレビューを通じた技術知識の伝達などにはあまり触れられていません。どちらかというと本書で言う「二兎追い」や「知識のひけらかし」と言った間違い系として扱われます。PRレビューなどをゲートとして扱うのであれば合ってる。

レビュー技法も大まかに触れられていて、大分類に「マネジメントレビュー」「オーディット」「ピアレビュー」、このピアレビューの小分類で「ウォークスルー」「テクニカルレビュー」「インスペクション」が挙げられています。この辺りはレビューに向き合うときベースとして持っていると整理しやすい知識です。「レビュー」は特に人間系で行われる活動なので、不定形の化け物になっちゃいがちなので、枠があると考えやすい、はず。

インスペクションが厳格として挙げられていますが、本書の解説だとどれくらい厳格かは分かりづらいと思います。私が読んだことあるのはこの辺。

ソフトウェアインスペクション

作者:Gilb,Tom,Graham,Dorothy
構造計画研究所

Amazon

レビューで厳格さを求めるならこれくらいやらなきゃなんない、ってのがわかって、いい感じに折り合いをつけられるようになるかと。私は実務でインスペクションは試したくらいで、本格的に導入したことはないです。たまにカンフル剤的にやるのはいいかも。

あとこの文脈だとこの辺

みんなではじめるデザイン批評―目的達成のためのコラボレーション&コミュニケーション改善ガイド

作者:アーロン・イリザリー（Aaron Irizarry）,アダム・コナー（Adam Connor）
ビー・エヌ・エヌ新社

Amazon

デザインってついてますが、グラフィカルなデザインだけの話ではないです。どっちかってと批評、コミュニケーションの本。おまけのポスターは印刷して部屋の壁に貼ってる。

まーでもわかるんだよ

レビューイ、レビュアーの関係って簡単に上下関係ができちゃうんだよね。指摘される側と指摘する側。元々上下関係がある場合もあるし、レビューを通してできていっちゃう場合もある。そうしたくなくても瑕疵が指摘されると謎の上下関係ができる。何だそりゃって思うし嫌いなんだけど、でも自分も近い振る舞いをしている気はする。そんなつもりなくても。指摘内容によっては「正しい」「間違ってる」が明確なものもあるから、そういう関係性にするフォースはかなり強い。

レビューしていただく側（いただくとか言う表現をあえて使う）はご指摘を賜ったら意に沿って然るべき……書いててゾワゾワした。

ベテランの人のコメントは本人が軽い気持ちだったとしても、その意図を忖度しちゃうとかはありがちで。なのでコメントする側が気を配らないといけないとは思う。思うんだけど、その気を配るコストを常にかけるのはそれはそれで費用対効果を考えると微妙にも思える。気を配った結果コメントするコストが爆増して「（すべき）コメントしない」ってなるのも本末転倒だし。

コードはコード、人は人、コードへの指摘は人格攻撃ではない。なんてことを言っても、コードに自分の思いを載せれば載せるほど愛着は湧くわけで。コードの共同所有などをうまく機能するにはそういうの必要だと思うし、愛着があればあるほど文句を言われればイラっとするし、貶められたと感じて、防御反応は出る。防御しちゃったら突破されたときに屈服、服従になる。そうなると勝者敗者の関係になって、全て従うのが正しくなるだろう。そんなこと考えると、そもそもそんな争いをせずに済ませようってなり、「指摘対応」という最適化された行動や無難なコメントになるんだと思う。お互いが様子見してぶつからないように、という行動。タックマンモデルでいう形成期の様子見な振る舞いかしら。成長するならいいんだけど。

レビュアー、レビューイに上下関係はない。綺麗事でもなんでもなく、あっちゃいけないんだ。対等な関係でできるのがいいと思うものの、いきなり「対等でなければできないやり方」をしてもうまくなんて行くわけもない。レビューに閉じた話じゃなくなりはじめてるし、この辺にしとこ。

どうでもいいけど「レビューイ」って変換しづらいし言いづらいし普段も滅多に使わない。最初聞いた時「レビュアー」が「ア」だから「アイウエオ」で「イ」なのかな？って思ってた。「レビュウ」とかあるんかな、とか。

まとめ

向き合ってるプロダクトやプロジェクト、メンバーなどいろんな要因で変わるもんです。なので「こうすべき」みたいな強い主張はないです。私個人としてニュートラルならこんな考え方だという話。なにもないなら主張することもあるけど、なんらか形があるときに押し付けたりはしない、くらいのものです。

「コメントしたら全部対処すべき」が一般的だと認識はしてる。コードレビューをゲートとして置くプロセスも理解できるし、それが立て付けられてるなら従ってます。それならそれで「もっとちゃんとやってこーぜ」と後押ししたりもする。「誰かが確認しないとマージしない」って暗黙のルールならちゃんとGitHubの設定そうしとこーぜ、とかね。そういう設定を勧めときながら個人としては好きじゃない、みたいなのは誰しもあると思います。その系です。

あ、タイトルの「対応する必要があるか」に対しては「対応しようよ。対応は別にコードの修正とかだけじゃなく、リアクションボタンだけでもいいからさ。」みたいな感じです。無視は辛い。的外れとか場違いとか不要なコメントをしてるとかなら無視でいいかもだけど、それも教えたげるのがいいと思う。「コメントは変更指示ではない」と言うのだけは言える。