How to reduce code review from two weeks to several hours. The experience of the Yandex.Market team

    Man is the main value of any enterprise. The success of the whole thing depends on how people communicate with each other, how they achieve their goals together, and on teamwork. Constantly honing skills, processes and tools allows you to work more efficiently.


    We at the Market are working to deliver new features to our users as quickly as possible. To do this, we constantly study our processes and optimize all stages of the task. Today we will tell Habr’s readers about the optimization of one of them, namely the code review process.


    First you need to imagine what kind of flow in development we have adopted:



    The same thing, only point by point and in words:


    1. The developer takes the task.
    2. Makes her.
    3. Fills in on Github and opens PR.
    4. Passes a code review.
    5. Collects a demo stand and gives it to a tester.
    6. The tester checks the demo stand.
    7. Verified task is collected in release.
    8. Testing in release.
    9. Layout on prod.
    10. Final testing in battle.

    By areas of responsibility, the task is divided into the following steps:


    1-5 - responsibility on the developer;
    6-7 - responsibility on the tester;
    8-10 - responsibility on the release master.


    So, they began to analyze what takes us the most time. Fortunately, there are internal analysis tools. Everyone relied on what will take the longest, of course, the development process itself (the status is "In Work") ... and it turned out that way. But one moment surprised us the most. The average duration of a review is two weeks!


    Step 1. Parsing PR


    Starting to study pull requests, they immediately faced one very interesting fact. It turned out that we have a huge cemetery of unclosed pull requests. Most of the authors of these PRs no longer worked for the company, but their legacy was still with us. Who has never seen dinosaurs, here they are:



    These pull requests added noise and interfered with the correct code review time analysis. With a calm mind, we closed them. It only remained to recount the time. But it was still in the area of ​​2-3 days. Not good.


    Step 2: Disassembling with a Browner


    Reviewer is a system developed in Yandex that calls in PR two random people with expertise in a mutable code and taking into account vacations and absences. Analysis of weekly PRs revealed a group of people who constantly drag out the code review. After interviewing colleagues, we found another problem in our process. Colleagues complained that they received too many pull requests per day from the jealous, and they simply did not have enough time for their main work.


    This did not coincide with our indicators: one or two PR per day per person. The study led to Github, where we have a separate team of reviewers. This team has not been updated for several years. Since then, the number of employees has doubled, but none of the newcomers were included in the team of reviewers. In other words, half of the team did not participate in the code review at all! Corrected this annoying misunderstanding.


    Step 3. Extending Tools


    At this stage, we tried to simplify the already simple, as we thought, life of the reviewers. The following tools were in the arsenal of front-end:


    • code style checkers;
    • test run;
    • various nerdy checks in PR itself;
    • revisionist;
    • alerts on mail and task tracker.

    It would seem that everything is there. Take and review!


    The first thing that turned out to be wrong: a different code review process in different repositories. We unified and at the same time proved the affixing of labels for convenient search for PRs through the Github interface.


    The second thing they noticed: mail is not the best tool for quickly reporting on the status of code reviews. Many, in order not to be distracted from work, parses his mail several times a day. Messengers are a completely different matter. The reaction to messages in messengers is much higher. And it was decided to connect the bot to our messenger. The bot sends alerts about the status of a code review for both the author of the pull request and encourages people to review. Very comfortably.


    Step 4. First SLA


    In parallel to actions 2 and 3, we began to work closely with employees and explain that the code review is inseparable from the task itself. They explained that bringing to "Verified" is the responsibility of the developer. Moreover, responsibility is not only to colleagues, but also to users! Fast delivery of features to sales - that's what I wanted to achieve. And the team was sympathetic to the improvement process.


    At this stage, the first idea of ​​the ideal code review was born. Of course, everyone wants it to take place in 5 minutes, but this is not always possible. Taking into account that we have a multi-regional team (with a time difference of four hours), we agreed that our SLA will be one day, i.e. 24 hours. This SLA was announced to all employees and, rubbing their hands, began to wait for the results.


    But the situation has not changed. In the best weeks, half of the pull requests fit into 1 day. The rest got out for him.



    We had a small script that evaluated the time code review on PR. We began to blame them on a daily basis for all PRs in search of "lagging behind." Almost every day there was a pack of such.



    It took a long time to parse them. Most often, the script itself dishonestly calculated the time of the review. He didn’t take into account that some PRs were created outside of working hours (yes, some courageous-skilled people like to work for an hour or two at night, come to us to work!) All this told us that our pull request monitoring tools are not the most effective, as they are dishonest. I have to find new tools.


    Step 5. SLA Tracker


    Help came from where they did not wait. Our colleagues from Tracker announced an amazing thing: now you can install SLA in the Tracker itself. Moreover, you can configure it completely differently. A certain timer is turned on by some event in the task. For some event, it can pause. And stop at some event. Yes, this is what we need!


    They immediately got into a detailed study of the documentation (by the way, here it is ) and set up our queues (there are several of them, since there are several repositories). We set the timer to turn on when switching to the "Need code review" status and end when it switches to any other status except "There are comments" - when it goes, the timer pauses, i.e. did not lose his time.



    It was also cool that the timer took into account working hours and a calendar! We set that 9h is assigned to the code review, i.e. one business day. In this case, an alert is triggered 2h after the start, or if a nine-hour deadline is disrupted. The result was a kind of honest monitoring. At first, we turned on the timer for the sake of the experiment, collected a pack of bugs and exported to the Tracker.


    It is worth noting that the timer was enabled only for tasks that were created after the implementation of the timer itself. Therefore, the instant effect could not be seen. But already at this stage, positive dynamics began. We got the effect a month later, when the flow of new tickets with a timer began to crowd out the old ones. It was noticeable that the approximate time of the code review was concentrated in the area of ​​reminder messages, i.e. marks 2h and 9h.


    At the time of this writing, we have 415 tasks in which the timer has started. Of these, 29 went beyond the nine-hour border. Although, if you take a closer look, you will also encounter such tasks, whose code review was completed within the next hour. If we discard them too, then 17 tasks remain. And this is about 4.1%!



    Total. Samurai constantly sharpening his sword


    Looking back and recalling all our actions, one conclusion can be drawn - all means are good. All our steps led to the desired result: more than 92% of pull requests began to satisfy our SLA ! Less and less torn tasks, code review is faster. Slowly, little by little, 75% of the code review began to fit into 5 hours ! And the dynamics for improvement are still positive. In addition to numerical indicators, we began to receive more positive feedback from subcontractors. Even more pleased with the fact that the team reacted to this whole process. Even a process such as code review acceleration rallied the team even more. Each participant began to understand the responsibility that he bears before others, as a quick review code allows us to benefit our users even faster.


    Of course, 9h is not the ultimate dream, but still success. But on it we do not intend to stop. We continue to monitor the code review process and solve all the technical and even psychological problems that employees face so that our process is as productive as possible and at the same time comfortable for the team.


    Q&A. What if...?


    Q: А что, если я думаю, что за мной следят? К чему этот таймер?
    A: Следим не конкретно за кем-то, а за процессом. В пулл-реквесте две стороны: автор и ревьюеры. Если ревьюеры затягивают с проверкой, то страдает автор. С другой стороны отрывать ревьюеров от работы прямо так сразу тоже негуманно. Нужно найти баланс, чтобы комфортно было обеим сторонам. Таймер нужен не для того, чтобы кого-то наказать, а для того, чтобы фиксировать все отклонения. Большинство таких отклонений случается не по вине ревьюеров, а по вине технических проблем. Задача в том, чтобы эти проблемы устранять. Чтобы другие с ними не столкнулись. Постоянное самосовершенствование


    Q : And what if there are complex PR, 100500+ lines of code, and there are "change the letter." Where's the justice?
    A : Yes, this is true. When we were working on the standard code review, we just took it along the upper boundary, i.e. code review of complex PR should fit into the SLA. But at the same time, we have no goal of driving everyone into these boundaries. We are always sympathetic to pull requests, in which there is a lively, useful discussion, despite the broken bar in one day.


    We have plans for SLA grades on storypoints. 1SP - 1h, 3SP - 5h, 5SP - 2d, for example. Fortunately, the Tracker is already capable of this. We just are not ready for this yet - we still have a long way to go.


    Also popular now: