palkan / n_plus_one_control

RSpec and Minitest matchers to prevent N+1 queries problem
MIT License
553 stars 20 forks source link

Add an option to give buffer for existing N+1s #37

Closed caalberts closed 3 years ago

caalberts commented 3 years ago

Is your feature request related to a problem? Please describe.

Currently, perform_constant_number_of_queries is very strict on the constant number of queries. In situations where there is an existing N+1 query, but we want to stop it from growing, we cannot use n_plus_one matcher, because it would always fail.

Describe the solution you'd like

Either a chained matcher to create some buffer, or a separate matcher altogether.

Describe alternatives you've considered

A few possible options:

expect { get :index }.to perform_constant_number_of_queries.with_buffer(1)
expect { get :index }.to perform_constant_number_of_queries(buffer: 1)

expect { get :index }.to perform_limited_number_of_queries(buffer: 1)

The idea is that the queries is allowed to increase by buffer for each scale factor. Assume with buffer(1), at n=1, it has 10 queries, at n=2 it is allowed up to 11 queries, at n=3 it is allowed up to 12 queries, and so on.

Alternative naming:

Additional context

I'm not sure if the problem I described is the same as tuning with coef mentioned in the README Probably not as this refers to batches:

# But we can tune it with `coef` and handle such cases as selecting in batches
assert_linear_number_of_queries(coef: 0.1) do
  Post.find_in_batches { some_code }
end
palkan commented 3 years ago

As far as I can see from the PR, that sounds exactly like a assert_linear_number_of_queries matcher.

Your implementation allows the following growth function: N = buffer*scale_factor. The general formulae would be: N = coef*scale_factor + fixed_size. Can we implement this? (And add a new matcher, since we no longer have a constant growth).

Something like:

expect { get :index }.to perform_linear_number_of_queries(slope: 2, intercept: 5) #=> N = 2*scale_factor + 5

Then for each collected count we should verify that the N is less or equal to the expected value:


scales_and_counts = @queries.map { |scale, queries| [scale, queries.size] }

scales_and_counts.all? { |scale, count| count <= slope * scale + intercept }
caalberts commented 3 years ago

As far as I can see from the PR, that sounds exactly like a assert_linear_number_of_queries matcher.

It didn't occur to me earlier, but yes this is what it is.

As a new matcher, I suppose it would also need to have with_scale_factors, matching and with_warming_up chains.