summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJames Edwards-Jones <jedwardsjones@gitlab.com>2017-03-09 14:54:26 +0000
committerJames Edwards-Jones <jedwardsjones@gitlab.com>2017-03-13 18:59:36 +0000
commit4dc99f326abcd73ba3564e8294c823d0a07a2f44 (patch)
treee9c72dfdcb95a7cd1fbe087dfcb0a700af0083e1
parent1dc2e185b8dfb7dfdccabe28998b30265f4ef39d (diff)
downloadgitlab-ce-4dc99f326abcd73ba3564e8294c823d0a07a2f44.tar.gz
-rw-r--r--doc/development/merge_request_performance_guidelines.md4
-rw-r--r--doc/development/performance.md1
-rw-r--r--doc/development/profiling.md2
-rw-r--r--doc/development/query_recorder.md29
4 files changed, 35 insertions, 1 deletions
diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md
index 8232a0a113c..2b4126b43ef 100644
--- a/doc/development/merge_request_performance_guidelines.md
+++ b/doc/development/merge_request_performance_guidelines.md
@@ -68,7 +68,7 @@ end
This will end up running one query for every object to update. This code can
easily overload a database given enough rows to update or many instances of this
code running in parallel. This particular problem is known as the
-["N+1 query problem"](http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations).
+["N+1 query problem"](http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations). You can write a test with [QueryRecoder](query_recorder.md) to detect this and prevent regressions.
In this particular case the workaround is fairly easy:
@@ -117,6 +117,8 @@ Post.all.includes(:author).each do |post|
end
```
+Also consider using [QueryRecoder tests](query_recorder.md) to prevent a regression when eager loading.
+
## Memory Usage
**Summary:** merge requests **must not** increase memory usage unless absolutely
diff --git a/doc/development/performance.md b/doc/development/performance.md
index c1f129e576c..04419650b12 100644
--- a/doc/development/performance.md
+++ b/doc/development/performance.md
@@ -39,6 +39,7 @@ GitLab provides built-in tools to aid the process of improving performance:
* [Sherlock](profiling.md#sherlock)
* [GitLab Performance Monitoring](../administration/monitoring/performance/introduction.md)
* [Request Profiling](../administration/monitoring/performance/request_profiling.md)
+* [QueryRecoder](query_recorder.md) for preventing `N+1` regressions
GitLab employees can use GitLab.com's performance monitoring systems located at
<http://performance.gitlab.net>, this requires you to log in using your
diff --git a/doc/development/profiling.md b/doc/development/profiling.md
index e244ad4e881..933033a09e0 100644
--- a/doc/development/profiling.md
+++ b/doc/development/profiling.md
@@ -25,3 +25,5 @@ starting GitLab. For example:
Bullet will log query problems to both the Rails log as well as the Chrome
console.
+
+As a follow up to finding `N+1` queries with Bullet, consider writing a [QueryRecoder test](query_recorder.md) to prevent a regression.
diff --git a/doc/development/query_recorder.md b/doc/development/query_recorder.md
new file mode 100644
index 00000000000..e0127aaed4c
--- /dev/null
+++ b/doc/development/query_recorder.md
@@ -0,0 +1,29 @@
+# QueryRecorder
+
+QueryRecorder is a tool for detecting the [N+1 queries problem](http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations) from tests.
+
+> Implemented in [spec/support/query_recorder.rb](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/support/query_recorder.rb) via [9c623e3e](https://gitlab.com/gitlab-org/gitlab-ce/commit/9c623e3e5d7434f2e30f7c389d13e5af4ede770a)
+
+As a rule, merge requests [should not increase query counts](merge_request_performance_guidelines.md#query-counts). If you find yourself adding something like `.includes(:author, :assignee)` to avoid having `N+1` queries, consider using QueryRecorder to enforce this with a test. Without this, a new feature which causes an additional model to be accessed will silently reintroduce the problem.
+
+## How it works
+
+This style of test works by counting the number of SQL queries executed by ActiveRecord. First a control count is taken, then you add new records to the database and rerun the count. If the number of queries has significantly increased then an `N+1` queries problem exists.
+
+```ruby
+it "avoids N+1 database queries" do
+ control_count = ActiveRecord::QueryRecorder.new { visit_some_page }.count
+ create_list(:issue, 5)
+ expect { visit_some_page }.not_to exceed_query_limit(control_count)
+end
+```
+
+As an example you might create 5 issues in between counts, which would cause the query count to increase by 5 if an N+1 problem exists.
+
+> **Note:** In some cases the query count might change slightly between runs for unrelated reasons. In this case you might need to test `exceed_query_limit(control_count + acceptable_change)`, but this should be avoided if possible.
+
+## See also
+
+- [Bullet](profiling.md#Bullet) For finding `N+1` query problems
+- [Performance guidelines](performance.md)
+- [Merge request performance guidelines](merge_request_performance_guidelines.md#query-counts) \ No newline at end of file