Introduce caching on Projects::MergeRequestsController#discussions
Problem to solve
During investigation in #330893 (closed), noticed that Projects::MergeRequestController#dicsussions
can take a while for a large MR (example: https://staging.gitlab.com/gpt/large_projects/gitlabhq1/-/merge_requests/8785).
Locally, it can take ~9s to load the discussions. Profiling the endpoint shows that it takes most of the time serializing the response.
Proposal
While working on a PoC on how we can cache this endpoint (!63644 (closed)), noticed that it can go down up to ~1.5s. That's ~80% improvement.
In the PoC, we cache the serialized response via Rails.cache
. It's utilizing the Api::Helpers::Caching
module with some modification to work with Rails controllers instead of Grape API (see #render_cached
). This will cache each discussion on a MR.
To implement this, we can:
- Implement
#render_cached
or name it something else. Not exactly how it's written in the PoC MR as it can be improved but it should behave the same way. - Define appropriate cache key for a discussion. In the PoC, used the discussion ID and the latest updated at of notes under a discussion.
- Define a cache expiration. 1 day seems fine as it is possible to have a discussion that gets untouched in a day.
- Implement it behind a feature flag then roll it out in a separate roll out issue. This way we can enable it on production and monitor how it affects our Redis cache usage.
Be on the look out for possible bug due to cache response is still being returned while it shouldn't.