Building a scalable PR review process.

Motivation and Background

We are growing fast! As a growing organization / team. A common sentiment we’ve observed is that completing thorough code reviews in a reasonable timeframe while balancing other tasks can be difficult, but there are a lot of things that can be optimized in the code review process.

Moreover, we realized that PR(s) could be good knowledge based in terms of domain knowledge (underlying feature) and for coding standards. Our idea was PR should be self sufficient so that anybody (Assuming he has time) can do the code review.

Growing Pains

Code review is the most important part of the software development process. Rapidly growing dev teams who work on parallel feature developers often realize that their code review process is not as robust and dynamic as they would like it to be. Below were the limitations we wanted to fix:

Our PR(s) were huge that sometime lead to quality compromise

• Team was raising big big PR(s). The bigger PR, most likely it will suffer from so called LGTM syndrome (looks good to me).
• Big PR(s) slows down the velocity of the team. BIG PR(s) lead a long time to review , too much back & forth with the author, dragging many days/week while pressure of delivery piles up.

For example , consider the graph below. Here a single dot represents the PR merge event. X axis present lines of code and Y axis present reviewer engagement. We are getting more engagement , meaningful comments from the reviewer when LoC is less.

Participation in the PR(s) reviews were less and not well distributed!

When PR(s) are big, it affects the reviewers participation. Whenever our team used to launch new PR, they used to supply this PR to 1 of the senior engineers via 1:1 chat.This was limiting us in 2 ways

• Context of the PR was getting limited to 1 person.
• Meaningful domain comments, design suggestion, solid principle suggestion,TDD suggestion were not getting broadcasted to the whole team. Moreover few of the comments were generic to the team.

Our PR(s) were not having enough meta, we were not following any standard PR template!

As our team and business started growing , We felt that to actually scale PR review , We need solid and template based meta along with PR. Basically we wanted a system that can force reviewers to forcefully consider various dimensions of the code review before signing it off.

For example — Code formatting is the key thing in our PR review process. Assuming that some newly joined engineer is doing his PR(s) review, it’s fair & possible that he missed to raise comment on unformatted PR(s). To avoid/minimize similar checks, we wanted to have a solid meta and template for PR(s).

We didn’t have any dedicated channel for PR feed / Collaboration.

Frequently we need async collaboration on a few review comments left by reviewers. Getting into the calls and doing code review in sync is not always possible to senior reviewers.

Our PR feedback loops were slow

Effective code review processes have fast feedback loops.When the handoff between authors and their reviewers is slow or burdensome, both parties must put in more time and effort to get their changes approved and merged.

Guiding Principles

After realizing and experiencing our growing pains , We had a clear idea of what exactly we wanted from our code review process.To place the code review system nicely , We read and brainstormed with tech leadership and dev team on what could be guiding principles for us. Below are few principles which helped us really:

Ideal PR size!

• A study of a Cisco Systems programming team revealed that a review of 200–400 LOC over 60 to 90 minutes should yield 70–90% defect discovery.
• With this number in mind, a good pull request should not have more than 250 lines of code changed.

For example : We have seen below set up

When PR was small

Assume the author raises PR having 10 lines , We have seen securing multiple meaningful comments on this PR.

When PR was big

When the author raises PR having 1000 lines , We have seen LGTM syndrome happening with us.

Single Responsibility principle!

• Ideally our 1 PR should solve 1 problem. Before raising a pull request , try to apply SRP principle and if your PR is doing multiple things, try to break it down into multiple PR(s). Though this is subjective, we might have to take exceptions on a few instances.
• Pull requests that follow the SRP reduces the overhead caused by revising a code that attempts to solve several problems.

Meta of the PR matters!

As we stated earlier that good meta of the PR could be a game changer for scaling the async code review process. It does matter:

• How we are summarizing PR.
• How we are summarizing highlights of the modification/changes of the PR.
• How are summarizing the motivation/content of the PR.
• What kind of checklist author has followed and many more.

ETA of the PR review!

A good review process requires that pull requests get addressed as soon as possible in order to prevent the project from being impeded. Ideally, pull requests are reviewed within two hours of their submission.

Handoff should be as fast as possible!

Success of the code review process depends upon the fact that how fast does this enable the feedback loops and handoff (shortening the time for a reviewer to provide quality feedback and for a developer to receive that feedback) between author and reviewer.When teams provide fast, high-quality feedback, they preserve context throughout the code review process — avoiding long gaps between feedback where authors and reviewers lose context.

For example — Consider below mental effort model of the code review

Moving toward Solution

Reaching till here was satisfactory but still we had the entire process to be crafted.The key thing here was to conceptualize and place a system for the PR template.But before conceptualizing this , We wanted to clearly define how the effective and in-effective code review process look like.

What Does the effective code review process look like ?

1. A developer is assigned to work on a new feature in Jira. They clarify the requirements with the product manager before breaking up the task into smaller subtasks.
2. After they complete the first subtask and all unit tests pass locally, they open a pull request on bitbucket. We have registered a slack web-hook on a new event. This helps us PR to get alerted on the slack channel/ They read through their own code before requesting reviewers. They also raise over slack to collaborate quickly.
3. The reviewer promptly reviews the developer’s changes. Since the pull request is small and manageable, they only have a few minor points of feedback. All linting and basic security checks are automated.
4. The developer is notified about the requested changes. They address the feedback right away, make sure that all tests again pass locally, and push their changes. The reviewer approves the pull request.
5. The developer merges the pull request. They continue on to the next subtask.
6. After all subtasks are completed, the task is automatically transitioned to quality review. The product manager validates that the changes work in a staging environment and marks the task as complete.

What does effective PR look like ?

• Ideally PR should be self reviewed thoroughly. Developers and Engineers should be encouraged to review themselves before raising PR.
• Ideal PR should follow SRP and should solve 1 problem at a time. Though this might vary depending upon the feature / complexity etc.
• Ideal PR should bring a clear title and description. While raising PR , we should assume that the code reviewer has joined our team today and has no context why this change has been done.Always remember that any one in the organization could be reading your pull request.
• A good pull request tends to be reviewed quickly.Ideally our pull request should not be more than > 250 lines.
• Ideal PR should follow a kind of template which surfaces why , what and how part of underlying changes.
• Ideal PR should be well formatted.
• Double eyes are always better than single eyes. Always expect 2 approval on your PR(s).

Introducing PR template

PR templates offer us uniformness in our pull request. Moreover It offers PR authors and reviewers a set of checklists that both parties need to check upon.Our vision was to let PR templates bring some meta which helps any individual below why :

• Why has PR been raised ?
• What changes does this PR bring ?
• What has been assumed in this PR ?

Apart from this , We have realized that we should bring some more important meta for the PR.Based on above ideation , we were able to craft below PR template

## Summary
<! — — Provide the general summary of your changes in the title above >
- Added and Implemented Funnel metrics in Targeting flow

## Description
<! — — Describe your changes in details >

- Have added funnel metrics in RealTimeOfferConsumer and TargetingJourneyOrchestartor.
- Have added logging in TargetingCommitStep.

## Motivation & Context

<! — — Why are these changes required ? >

We are getting latency on rolling out offers to our users while running big campaigns. To debug this , we wanted to record a few tech metrics in the targeting flow.

## Related Links / JIRA Issue

<! — — If it fixes an open issue,link the issue here.>
// if it fixes an open issue ,please link to the issue here.

## Affected Service(s)

<! — — put an `x` in 1 or more of the following >
[] xx
[] yy
[] zz

## Type of Change(s)

<! — — What type of changes does your code introduce ? Put an `x` in all the boxes that apply>
[] fix bugs in test / code.
[] new feature
[] docs
[] refactor
[] Other: Please elaborate.

## How have I tested this ?

<! — — provide instructions on how you have tested this change.>
[] Manual Testing
[] Snapshot Testing
[] Unit Testing
[] API Testing
[] Other: Please elaborate

## Risks

<! — — put an `x` in of the following >
[] **low** : backward compatible,new feature or patches which do not break existing functionality.
[] **medium** : existing functionality in our domain got altered but this does not break contracts with other domains.
[] **high** : breaking contracts with other domains.

## Checklist

<! — — Go over all the following points and put an `x` in all the boxes that apply. If you are unsure , Pls connect us>
-[] Any technical debt is marked with `TODO: <debt details>`.
-[] My code follows the code style of this project.
-[] My changes required updating in docs and I have updated the docs already.
-[] I have checked new dependencies for security vulnerabilities.

## PR Dependency
Link of PR(s) that need to be merged before this PR.

— — — — — — — — — — — — — — — — — — — — — — — — — — — — — — — — — —
## Checklist for code reviewer
-[] Code is well formatted ?
-[] PR is under 250 lines ?
-[] Code is not duplicated anywhere ?
-[] Code does not bring any race condition as such?
-[] Code is backward compatible?
-[] Code is using SOLID principle / Designs?
-[] Code brings significant unit testing?
-[] Code has monitoring and alerting in place
-[] Does code change any API contract ?

Example

New PR workflow

We were ready to launch a new PR workflow. Our team was excited to get onboarded with a new PR workflow.

Prerequisites

Before rolling out this , we had few pre-requisite to follow

• Integrating the above PR template with all the team repositories.
• Integrating all team repositories with slack channel (code-review-feed) so that notification and collaboration can be leveraged on slack channel.
• Disabling direct push / commit on dev and master branches.

Step by step workflow

• X will change in the code.
• X will commit in the given branch.
• X will push the code and will raise an PR.
• X will get the PR template to pop up , He will fill everything there assuming the reviewer is new and does not have context.
• PR will get notified on slack channel.
• X will loop reviewers.
• The reviewer will approve the PR and add the reaction in a slack thread.

Next?

We are working on a few solutions to automating data captured by pull requests.

About the Author

This doc is the cumulative effort of the RummyCircle Engineering Team.