Как ускорить код-ревью

Представим, что перед вами стоит задача — ускорить процессы код-ревью в компании: сейчас код-ревью занимает сутки, а в плохих случаях и вовсе длится неделями, из-за чего фичи в прод едут медленнее, чем хотелось бы.

Что можно с этим сделать? Одним подходом может быть «установить чёткие дедлайны для ревьюеров и бить их палками, если что-то идёт не так». Это не системный подход, поэтому давайте его сразу пропустим и перейдём к более интересному.

Чтобы повлиять на систему, надо в ней разобраться, определить основные циклы обратной связи, найти парето-точки, где изменения в процессе сильно повлияют на результат.

Что такое код-ревью; какие у него цели?

Код-ревью это процесс, который превращает Непроверенный Код в Одобренный Код. Это нужно, чтобы сократить количество ошибок, которые изначально не заметил разработчик, изменивший код.

Одним из способов радикально сократить время на код-ревью будет отменить его вообще. Давайте представим, к чему это может привести.

Разработчик Олег пушит свои изменения напрямую в мастер, откуда эти изменения выезжают в продакшен. Если Олег сделал всё правильно и ничего не забыл, то нам повезло и мы ускорили пайплайн в этом месте. Но при этом есть разработчица Александра, которой придётся пересматривать свои ещё не смёрженные изменения с учётом только что вмёрженных изменений Олега. Если вам кажется, что это просто — ну, да, запустить rebase origin/master действительно просто, но что если изменения Олега меняют предположения о системе, из которых исходила Александра? В таком случае автоматический ребейз может либо выдать мёржконфликт (и это будет хорошо! Это может заострить внимание на нарушенных предположениях), либо, что страшнее, молча сделать вид, что всё нормально, хотя изменения Олега и Александры работают по отдельности и не работают вместе.

Здесь важно, что Александра и Олег меняют одну и ту же систему — если их изменения затрагивают независимые системы, или же системы, которые как-то зависят друг от друга, но отгорожены публичными интерфейсами, то всё становится сильно проще.

Ключевой момент здесь: изменения меняют предположения о системе; под словом «предположения» я имею в виду изменения, которые обладают потенциалом «каскада», где потребуется менять какую-то ещё часть кода системы. Такие изменения можно грубо охарактеризовать, разбив их на три уровня:

  • уровень интерфейсов между горизонтальными (равноправными) частями системы. Например, «чтобы показать попап, надо нарисовать в своём компоненте <Popup>{popupContent}</Popup>».
  • уровень фич. Например, «мы хотим показывать попап в такой ситуации».
  • уровень платформы. Например, «наше приложение запускается в контейнере с Node.js такой-то версии».

Помимо обеспечения стабильности, код-ревью целенаправленно создаёт боттлнек, который помогает распространить знания о грядущих изменениях. Мы замедляем часть системы в надежде, что это повысит throughput системы в целом за счёт меньшего количества переработок в будущем и починки багов в продакшене прямо сейчас.

Когда мы говорим о скорости системы (в данном случае нас интересует конкретный процесс — превращение фичи из идеи в работающий в продакшене код), следует помнить о том, что latency и throughput —это две разные метрики. Например, если мы посадим половину разработчиков дежурить и проводить код-ревью как только появляется пулреквест от второй половины, то мы снизим latency (это хорошо), но также снизим и throughput (это, как правило, плохо).

В этом контексте часто говорят о time to market определенной фичи, но это не очень подходящая метрика: если мы релизим одну фичу в неделю, но зато её провели через конвейер за полдня, то у нас будет отличный ttm (зарелизили за полдня!), но отвратительный throughput (в неделю только одну фичу!).

Код

На самом деле мы хотим уменьшить latency и увеличить throughput одновременно. Это возможно за счёт снижения времени на код-ревью без снижения качества.

Важный момент: это сработает, если бутылочным горлышком системы действительно является код-ревью. Если горлышко в другом месте, то усилия по улучшению процесса ревью будут бесполезны в контексте улучшения latency/throughput (но могут, например, повысить уверенность разработчиков, что тоже бывает полезным).

Чтобы понять, что является горлышком, можно соорудить подобие канбан-доски и посмотреть, где же фичи застревают на самом деле. Как простой пример, если мы настаиваем, что все изменения должны пройти ещё и через ручное тестирование, а тестировщик у нас один и он не успевает справиться с потоком входящих задач, то ускорение код-ревью не исправит ситуации, но только ухудшит метрики (задачи ещё дольше висят в статусе «надо тестировать», ухудшается атмосфера, тестировщик уходит в запой от непрекращающейся тревоги и пайплайн встаёт намертво). Попытки ускорить систему в любом месте кроме бутылочного горлышка бесполезны и могут быть даже вредны.

Но предположим, что мы измерили, и проблема действительно во времени, которое уходит на код-ревью, поэтому давайте снижать это время.

В код-ревью в целом можно выделить четыре стадии:

  1. Время от создания пулреквеста до начала ревью
  2. Время ревью, где ревьюер должен осознать суть изменений, на какие предположения опирается пулреквест, не нарушены ли они. (В плохих случаях здесь также идёт анализ кода на соответствие код-стайлу, в большинстве случаев это можно вынести на шаг 1 через автоматизацию)
  3. Время правок, где автор пулреквеста исправляет замечания с шага 2; после чего мы снова переходим на шаг 1, если только на шаге 2 не было никаких замечаний.
  4. Мёрж изменений, тестирование, релиз, потенциально — отлавливание проблем в продакшене.

Даже если мы не в состоянии изменить структуру, мы можем повлиять на количество итераций в цикле 1-3. Один из главных рисков — во время цикла система изменяется таким образом, что придётся ещё раз вносить изменения относительно предположений, или же отлавливать баги после; и то, и другое — переработка, необходимость которой возникла из-за долгих процессов ревью.

Чтобы снизить количество такой переработки надо снижать количество потенциально изменчивых предположений внутри пулреквеста. Как самый простой способ: делать пулреквесты небольшими, потенциально даже меньше, чем фича (таким образом, для реализации и деплоя фичи в мастер должны попасть несколько пулреквестов). Это снижает вероятность переработки тем образом, что когда Олег приходит менять, скажем, Popup, он учитывает потребителей, которые уже в мастере, и может автоматически отрефакторить все вызовы попапа на новый синтаксис.

Другой способ снизить количество изменчивых предположений — сделать куски системы более независимыми, ограничить их взаимодействие более стабильными интерфейсами и протоколами; тогда нам может помешать только два момента: те изменения, которые ломают внешний API, и те ситуации, где мы зачем-то зависим от видимых проявлений, не являющихся частью формального внешнего API (закон Хайрума гласит, что, при достаточно большом количестве потребителей, на каждое наблюдаемое поведение системы найдётся потребитель, который зависит от этого поведения).

Ещё один подход, близкий к предыдущему — избегать общего «стейта» в широком смысле: так, например, если тестировать изменения скриншотами, то мы создаём общий стейт в виде скриншота всей страницы. Если перед тем, как мы успеем смёржить нашу фичу, в мастер попал коммит, меняющий другую часть страницы, то придётся снова переснимать скриншоты — но если у нас есть уверенность, что изменения затронут только определённую часть страницы и мы ограничим скриншот до этой части, то вероятность того, что скриншот изменится по нерелевантным причинам, станет ощутимо меньше.

Снижение количества предположений одновременно снижает и нагрузку на ревьюера — меньше факторов, которые надо рассматривать, чтобы понять, что изменения не сломают систему в целом.

Но мы же не можем релизить половину фичи! — скажете вы, и будете отчасти правы, мы не хотим, чтобы пользователь увидел наполовину готовое. Для этого можно использовать фича-флаги, эксплицируя время разработки внутри кода. Можно автоматизировать удаление ненужного кода через несколько дней после финального пулреквеста.

Наибольший риск — гигантский пулреквест, дизайн которого настолько тщательно раскритикован во время код-ревью, что его практически придётся переделывать с нуля. В этих случаях хорошо заранее согласовать общий дизайн решения, как правило, митинг на полчаса-час обойдётся дешевле двух недель переработки.

Люди

До этого текст делал неявное предположение, что мы работаем с идеальными ревьюерами, которые ответственно относятся к проведению ревью и чьи комментарии действительно стоит учитывать, а не игнорируют до последнего и придираются к запятым вместо вдумчивого анализа изменений. К сожалению, в реальном мире так далеко не всегда. С учётом того, что работать нужно с теми людьми, которые есть, надо упрощать проведение хорошего, внимательного ревью, и не поощрять поверхностное.

Например, жёсткие короткие дедлайны поощряют второе — потому что смотрят только на время, проведённое за ревью. Автоматический анализ зависимостей от кода, затронутого изменениями, помогает первому, если сделано достаточно надёжно, чтобы ревьюер мог полагаться на этот анализ. Маленький размер пулреквеста снижает необходимую глубину погружения ревьюера, а большой размер провоцирует на поверхностный взгляд (нет времени анализировать три тысячи строк, так что гляну так, на всякий случай).

Часто говорят о роли код-ревью в ещё трёх процессах: снижение code ownership, распространение знаний о коде, обучение best practices и программированию в целом. Это всё полезные цели, но если мы достигли точки, где код-ревью само по себе стало заметной проблемой, то стоит вынести, хотя бы временно, их из критического пути в асинхронные методы коммуникации (например, рассылки и чейнджлоги).

Проблема code ownership заключается в том, что у человека возникает ощущение, что какой-то кусок кода принадлежит ему и враждебно относится к попыткам изменить этот кусок. Это решается предотвращением: если обозначить владельцами команды, то кусок кода не мой, но наш, моей команды. Это работает, если над системой работают несколько независимых команд — тогда включается механизм мы-и-они.

Если же команда одна, то скорее всего проект не настолько большой и важный, чтобы это было серьёзной проблемой; но если всё-таки хочется что-то сделать, то код-ревью как раз выступит инструментом разделения ответственности за код, в основном, через обязательное прохождение ревью кода, написанного потенциальным «кодовладельцем».

У людей помимо всего прочего есть свои biases, но у каждого человека их набор и их проявления более-менее стабильны, поэтому со временем можно составлять «портрет» ревьюера. Например, Игнат всегда находит, что откомментировать, в любом пулреквесте, поэтому назначая ревью на него, стоит предположить, что случится пинг-понг.

Большая картина

Теперь немного посмотрим на картину в целом: как определить, что процесс код-ревью выстроен хорошо?

  1. Ошибки в продакшене — редкость, и обычно связаны с непредсказуемыми факторами: код-ревью выполняет свою главную роль повышения стабильности.
  2. Код-ревью редко превращается в пинг-понг: ревьюеры правильно и полноценно коммуницируют замечания.
  3. Время прохождения код-ревью обладает небольшой вариативностью: не бывает гигантских или чрезвычайно сложных пулреквестов.
  4. В системе нет мест, которые страшно менять: у разработчиков есть вера в то, что код-ревью поймает возможные ошибки.
  5. Через ревью проходят как мелкие фичи, так и большие инфраструктурные изменения.

Каждый из пунктов при желании можно измерять и отслеживать — но стоит сравнивать проект только с этим же проектом, для каждой системы целевые значения метрик свои: требования к надёжности в NASA и стартапе разные.

Слишком длинно, давай вкратце

  1. Пулреквесты должны быть маленькими: меньше строк кода — меньше предположений — быстрее ревью,
  2. Пулреквесты могут быть на половинку фичи, используйте фича-флаги: ранняя фиксация предположений в мастере снижает вероятность их нарушения,
  3. Чем меньше ревью требует ручного анализа, тем лучше: разделяйте систему на независимые и отграниченные интерфейсами части, чтобы снизить количество необходимых предположений.

Опубликовано 4 февраля

Обновлено 21 сентября

Если понравился этот пост, то подпишись на мой Патреон. поддержать