Skip to content

Improve HTTP caching for issue discussions

Heinrich Lee Yu requested to merge issue-discussions-http-cache into master

What does this MR do and why?

This skips rendering of notes to JSON and returns a 304 early when the notes have not changed.

Even before this change, we already return 304s because Rack::ETag does this automatically by computing a digest from the response body and using that as the ETag. The problem with that is the whole response still needs to be computed so we don't save any server processing time but we save on network transfer time because we'll be returning an empty body.

With this change, we explicitly pass in the discussion cache keys so that they will be used to compute the ETag and we would skip the JSON serialization part. This leverages the discussion cache keys introduced by #332967 (closed) but this just does HTTP caching while MRs cache discussions in Redis.

There are pros and cons to both approaches and this will be a way to find out which one we should use or if we should use both. Because they're not really mutually exclusive.

How to set up and validate locally

  1. Enable the issue_discussions_http_cache feature flag.
  2. Open an issue.
  3. Subsequent requests for discussion.json should return a 304 unless something is modified.

I tested this out locally on an issue with 500 notes.

Logs before this change
{"method":"GET","path":"/flightjs/flight_moved/-/issues/35/discussions.json","format":"json","controller":"Projects::IssuesController","action":"discussions","status":200,"time":"2021-10-19T06:27:43.275Z","params":[{"key":"namespace_id","value":"flightjs"},{"key":"project_id","value":"flight_moved"},{"key":"id","value":"35"}],"remote_ip":"192.168.68.103","user_id":1,"username":"root","ua":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:93.0) Gecko/20100101 Firefox/93.0","correlation_id":"01FJBKQV0NW782FH523TADYQF8","meta.user":"root","meta.project":"flightjs/flight_moved","meta.root_namespace":"flightjs","meta.caller_id":"Projects::IssuesController#discussions","meta.remote_ip":"192.168.68.103","meta.feature_category":"issue_tracking","meta.client_id":"user/1","redis_calls":8,"redis_duration_s":0.00216,"redis_read_bytes":451,"redis_write_bytes":84023,"redis_cache_calls":2,"redis_cache_duration_s":0.000742,"redis_cache_read_bytes":205,"redis_cache_write_bytes":83075,"redis_shared_state_calls":6,"redis_shared_state_duration_s":0.001418,"redis_shared_state_read_bytes":246,"redis_shared_state_write_bytes":948,"db_count":32,"db_write_count":0,"db_cached_count":5,"db_replica_count":32,"db_replica_cached_count":5,"db_replica_wal_count":0,"db_replica_wal_cached_count":0,"db_primary_count":0,"db_primary_cached_count":0,"db_primary_wal_count":0,"db_primary_wal_cached_count":0,"db_replica_duration_s":0.079,"db_primary_duration_s":0.0,"cpu_s":3.285327,"mem_objects":1893655,"mem_bytes":192587464,"mem_mallocs":816495,"mem_total_bytes":268333664,"pid":63,"queue_duration_s":1.475065,"db_duration_s":0.01876,"view_duration_s":0.14936,"duration_s":2.63021}
{"method":"GET","path":"/flightjs/flight_moved/-/issues/35/discussions.json","format":"json","controller":"Projects::IssuesController","action":"discussions","status":200,"time":"2021-10-19T06:29:05.518Z","params":[{"key":"namespace_id","value":"flightjs"},{"key":"project_id","value":"flight_moved"},{"key":"id","value":"35"}],"remote_ip":"192.168.68.103","user_id":1,"username":"root","ua":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:93.0) Gecko/20100101 Firefox/93.0","correlation_id":"01FJBKTC8DT9QX5PG8JD5QNSCW","meta.user":"root","meta.project":"flightjs/flight_moved","meta.root_namespace":"flightjs","meta.caller_id":"Projects::IssuesController#discussions","meta.remote_ip":"192.168.68.103","meta.feature_category":"issue_tracking","meta.client_id":"user/1","redis_calls":9,"redis_duration_s":0.002439,"redis_read_bytes":653,"redis_write_bytes":85775,"redis_cache_calls":3,"redis_cache_duration_s":0.001077,"redis_cache_read_bytes":407,"redis_cache_write_bytes":84830,"redis_shared_state_calls":6,"redis_shared_state_duration_s":0.001362,"redis_shared_state_read_bytes":246,"redis_shared_state_write_bytes":945,"db_count":32,"db_write_count":0,"db_cached_count":5,"db_replica_count":32,"db_replica_cached_count":5,"db_replica_wal_count":0,"db_replica_wal_cached_count":0,"db_primary_count":0,"db_primary_cached_count":0,"db_primary_wal_count":0,"db_primary_wal_cached_count":0,"db_replica_duration_s":0.072,"db_primary_duration_s":0.0,"cpu_s":2.88176,"mem_objects":1895331,"mem_bytes":196813656,"mem_mallocs":814506,"mem_total_bytes":272626896,"pid":64,"queue_duration_s":0.686475,"db_duration_s":0.01654,"view_duration_s":0.12787,"duration_s":2.4715}
{"method":"GET","path":"/flightjs/flight_moved/-/issues/35/discussions.json","format":"json","controller":"Projects::IssuesController","action":"discussions","status":200,"time":"2021-10-19T06:31:10.686Z","params":[{"key":"namespace_id","value":"flightjs"},{"key":"project_id","value":"flight_moved"},{"key":"id","value":"35"}],"remote_ip":"192.168.68.103","user_id":1,"username":"root","ua":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:93.0) Gecko/20100101 Firefox/93.0","correlation_id":"01FJBKY6HZM1B9E0R1SDS6DTJ5","meta.user":"root","meta.project":"flightjs/flight_moved","meta.root_namespace":"flightjs","meta.caller_id":"Projects::IssuesController#discussions","meta.remote_ip":"192.168.68.103","meta.feature_category":"issue_tracking","meta.client_id":"user/1","redis_calls":8,"redis_duration_s":0.002385,"redis_read_bytes":451,"redis_write_bytes":84014,"redis_cache_calls":2,"redis_cache_duration_s":0.000788,"redis_cache_read_bytes":205,"redis_cache_write_bytes":83069,"redis_shared_state_calls":6,"redis_shared_state_duration_s":0.001597,"redis_shared_state_read_bytes":246,"redis_shared_state_write_bytes":945,"db_count":32,"db_write_count":0,"db_cached_count":5,"db_replica_count":32,"db_replica_cached_count":5,"db_replica_wal_count":0,"db_replica_wal_cached_count":0,"db_primary_count":0,"db_primary_cached_count":0,"db_primary_wal_count":0,"db_primary_wal_cached_count":0,"db_replica_duration_s":0.075,"db_primary_duration_s":0.0,"cpu_s":2.987833,"mem_objects":1893664,"mem_bytes":191166472,"mem_mallocs":797473,"mem_total_bytes":266913032,"pid":63,"queue_duration_s":0.174229,"db_duration_s":0.01754,"view_duration_s":0.12758,"duration_s":2.91563}
Logs after this change
{"method":"GET","path":"/flightjs/flight_moved/-/issues/35/discussions.json","format":"json","controller":"Projects::IssuesController","action":"discussions","status":304,"time":"2021-10-19T06:21:39.194Z","params":[{"key":"namespace_id","value":"flightjs"},{"key":"project_id","value":"flight_moved"},{"key":"id","value":"35"}],"remote_ip":"192.168.68.103","user_id":1,"username":"root","ua":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:93.0) Gecko/20100101 Firefox/93.0","correlation_id":"01FJBKCS172YHVTRBTBA4DGAY1","meta.user":"root","meta.project":"flightjs/flight_moved","meta.root_namespace":"flightjs","meta.caller_id":"Projects::IssuesController#discussions","meta.remote_ip":"192.168.68.103","meta.feature_category":"issue_tracking","meta.client_id":"user/1","redis_calls":12,"redis_duration_s":0.0050290000000000005,"redis_read_bytes":1676,"redis_write_bytes":78571,"redis_cache_calls":6,"redis_cache_duration_s":0.002284,"redis_cache_read_bytes":1430,"redis_cache_write_bytes":77622,"redis_shared_state_calls":6,"redis_shared_state_duration_s":0.002745,"redis_shared_state_read_bytes":246,"redis_shared_state_write_bytes":949,"db_count":28,"db_write_count":0,"db_cached_count":3,"db_replica_count":28,"db_replica_cached_count":3,"db_replica_wal_count":0,"db_replica_wal_cached_count":0,"db_primary_count":0,"db_primary_cached_count":0,"db_primary_wal_count":0,"db_primary_wal_cached_count":0,"db_replica_duration_s":0.091,"db_primary_duration_s":0.0,"cpu_s":2.093836,"mem_objects":653618,"mem_bytes":119452184,"mem_mallocs":510321,"mem_total_bytes":145596904,"pid":63,"queue_duration_s":1.534067,"db_duration_s":0.02592,"view_duration_s":0.0,"duration_s":0.96758}
{"method":"GET","path":"/flightjs/flight_moved/-/issues/35/discussions.json","format":"json","controller":"Projects::IssuesController","action":"discussions","status":304,"time":"2021-10-19T06:22:20.906Z","params":[{"key":"namespace_id","value":"flightjs"},{"key":"project_id","value":"flight_moved"},{"key":"id","value":"35"}],"remote_ip":"192.168.68.103","user_id":1,"username":"root","ua":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:93.0) Gecko/20100101 Firefox/93.0","correlation_id":"01FJBKE1JQKCFS33BGDZ83J440","meta.user":"root","meta.project":"flightjs/flight_moved","meta.root_namespace":"flightjs","meta.caller_id":"Projects::IssuesController#discussions","meta.remote_ip":"192.168.68.103","meta.feature_category":"issue_tracking","meta.client_id":"user/1","redis_calls":7,"redis_duration_s":0.001931,"redis_read_bytes":248,"redis_write_bytes":70335,"redis_cache_calls":1,"redis_cache_duration_s":0.000357,"redis_cache_read_bytes":2,"redis_cache_write_bytes":69392,"redis_shared_state_calls":6,"redis_shared_state_duration_s":0.001574,"redis_shared_state_read_bytes":246,"redis_shared_state_write_bytes":943,"db_count":27,"db_write_count":0,"db_cached_count":3,"db_replica_count":27,"db_replica_cached_count":3,"db_replica_wal_count":0,"db_replica_wal_cached_count":0,"db_primary_count":0,"db_primary_cached_count":0,"db_primary_wal_count":0,"db_primary_wal_cached_count":0,"db_replica_duration_s":0.064,"db_primary_duration_s":0.0,"cpu_s":1.625618,"mem_objects":645190,"mem_bytes":118166888,"mem_mallocs":509794,"mem_total_bytes":143974488,"pid":63,"queue_duration_s":1.801715,"db_duration_s":0.01576,"view_duration_s":0.0,"duration_s":0.89308}
{"method":"GET","path":"/flightjs/flight_moved/-/issues/35/discussions.json","format":"json","controller":"Projects::IssuesController","action":"discussions","status":304,"time":"2021-10-19T06:23:36.190Z","params":[{"key":"namespace_id","value":"flightjs"},{"key":"project_id","value":"flight_moved"},{"key":"id","value":"35"}],"remote_ip":"192.168.68.103","user_id":1,"username":"root","ua":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:93.0) Gecko/20100101 Firefox/93.0","correlation_id":"01FJBKGB1DN5EDNSQGQKSFK1GE","meta.user":"root","meta.project":"flightjs/flight_moved","meta.root_namespace":"flightjs","meta.caller_id":"Projects::IssuesController#discussions","meta.remote_ip":"192.168.68.103","meta.feature_category":"issue_tracking","meta.client_id":"user/1","redis_calls":8,"redis_duration_s":0.090257,"redis_read_bytes":462,"redis_write_bytes":71468,"redis_cache_calls":2,"redis_cache_duration_s":0.001461,"redis_cache_read_bytes":216,"redis_cache_write_bytes":70519,"redis_shared_state_calls":6,"redis_shared_state_duration_s":0.088796,"redis_shared_state_read_bytes":246,"redis_shared_state_write_bytes":949,"db_count":27,"db_write_count":0,"db_cached_count":3,"db_replica_count":27,"db_replica_cached_count":3,"db_replica_wal_count":0,"db_replica_wal_cached_count":0,"db_primary_count":0,"db_primary_cached_count":0,"db_primary_wal_count":0,"db_primary_wal_cached_count":0,"db_replica_duration_s":0.077,"db_primary_duration_s":0.0,"cpu_s":1.74552,"mem_objects":646403,"mem_bytes":117628616,"mem_mallocs":506146,"mem_total_bytes":143484736,"pid":64,"queue_duration_s":1.828617,"db_duration_s":0.01605,"view_duration_s":0.0,"duration_s":0.92867}

You can see the reduced duration_s and mem_* numbers.

Some notes:

  • Before this change (when Rack::Etag converts the 200 into 304s), the status that is logged is 200 even though 304 is actually sent and received by the browser. I think this is because Rack::Etag is executed after the logging.

MR acceptance checklist

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

Edited by Heinrich Lee Yu

Merge request reports

Loading