From 4dc99f326abcd73ba3564e8294c823d0a07a2f44 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Thu, 9 Mar 2017 14:54:26 +0000 Subject: Adds docs for QueryRecorder tests --- .../merge_request_performance_guidelines.md | 4 ++- doc/development/performance.md | 1 + doc/development/profiling.md | 2 ++ doc/development/query_recorder.md | 29 ++++++++++++++++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 doc/development/query_recorder.md 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 , 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 -- cgit v1.2.1