Skip to content

Limit the number of commits in push merge request emails

Quang-Minh Nguyen requested to merge 351293-large-pushes-to-merge-requests-can-cause-emails-not-to-be-delivered into master

What does this MR do and why?

For #351293 (closed)

Recent, we noticed some Gitlab::SidekiqMiddleware::SizeLimiter::ExceedLimitError exceptions after Sidekiq size limiter was enabled. There are multiple code paths leading to that exception. One of them is Notification#push_to_merge_request. This method pushes an email notification with corresponding changed commits to the user. At the moment, we don't have any limitation for the commits. If the change contains a huge number of commits, for example when a merge request of gitlab-org/gitlab is rebased, the email content may grow very fast. That exceeds the size limit of Sidekiq job payload.

Therefore two optimization opportunities here:

  • We can truncate the number of displayed commits in the email. The whole purpose of email notification is for human to read and react. I highly doubt that there would be anyone who relies on the full list of commits in the emails. In fact, to make the email exceed the limit, the commit list must be super long, maybe thousands of commits. That number is overwhelming for anyone to make sense from. This MR limits the latest 20 commits.
  • We don't really use the full list of @existing_commits in the email content. The content requires the first, the last, and the number of existing commits to generate the following message: * ffa2983e...deafbf11 - 214 commits from branch master. So, we don't properly need the full populated list. We can simply replace that list by three equivalent parameters.

After applying those two points, the size of email payload is always kept small. As a result, the aforementioned exception would never raise again.

Screenshots or screen recordings

Case 1: The number of new commits is smaller than 20

Expected result: Nothing changes Screen_Shot_2022-03-14_at_16.44.06

Case 2: The number of new commits is more than 20

Expected result: The email show the latest 20 commits and "And ... more" at the end. It also shows compare link of first and last existing commits Screen_Shot_2022-03-14_at_16.45.08

Case 3: When where is only one existing commit and some new commits

Expected result: The email show the latest 20 commits and "And ... more" at the end. It also shows the only existing commit and corresponding link. Screen_Shot_2022-03-14_at_16.46.39

Case 4: When where is only one existing commit and no new commit

Expected result: The email show the only existing commit and corresponding link. Screen_Shot_2022-03-14_at_16.47.11

Case 5: When there is no new commit and no existing commits

Expected result: the email message only. Screen_Shot_2022-03-14_at_16.47.28

How to set up and validate locally

This is the basic functionality of Gitlab. To test on local environment, we can open a merge request. Then, push (force) to the branch correspondingly to the testing scenarios. The email notifications are captured at http://127.0.0.1:3000/rails/letter_opener/.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #351293 (closed)

Edited by Quang-Minh Nguyen

Merge request reports