Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config.active_record.warn_on_records_fetched_greater_than option #18846

Merged
merged 1 commit into from Mar 26, 2015

Conversation

hundredwatt
Copy link
Contributor

When set to an integer, a warning will be logged whenever a result set
larger than the specified size is returned by a query. Fixes #16463

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from 4e52ea1 to 88ba6ec Compare Feb 7, 2015
mattr_accessor :warn_on_result_set_size, instance_writer: false
self.warn_on_result_set_size = nil


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️ this line

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from 88ba6ec to 79e8929 Compare Feb 7, 2015
@hundredwatt
Copy link
Contributor Author

@kaspth ✂️-ed

@kaspth kaspth added this to the 5.0.0 milestone Feb 7, 2015
@hundredwatt
Copy link
Contributor Author

Wondering if we should move the exec_queries method to a module and then use inheritance to add the warning only if the config value is set. That way, this would have 0 cost when not enabled.

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

I don't think you need to touch the existing #exec_queries method. You could try the module approach using #prepend.

Unless #prepend is frowned upon in Rails, which I need someone else to verify.

@hundredwatt
Copy link
Contributor Author

I had the same thought, however there's no other use of Module#prepend in AR. I believe the idea was to start allowing it for Rails 5, when backwards compatibility for 1.9 will be dropped.

@hundredwatt
Copy link
Contributor Author

Gave Module#prepend a try with c5751fe. This will fail CI, but I'm curious to get some feedback on the approach.

@hundredwatt
Copy link
Contributor Author

Nevermind comment about CI, travis-ci matrix is only Ruby 2.2 now.

require 'models/post'
require 'models/comment'
require 'models/author'
require 'models/rating'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the requires for the other models?

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from c5751fe to 7bd96e4 Compare Feb 8, 2015
@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

I think you need to rebase this on master before Travis will try a build.

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

Otherwise the code looks good to me 👍. It depends on whether others are okay with the initializer approach here.

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from 7bd96e4 to 12e2561 Compare Feb 8, 2015
@hundredwatt
Copy link
Contributor Author

Thanks for feedback @kaspth 😄. Will await additional feedback and then can squash before merging.

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

Sure thing 😄

Your test leaks state. The module you prepend is still in Relation after the test, so all the other tests fail because you're comparing x >= nil where x is an integer.

I don't know if unprepending a module is considered a dirty practice, so let's wait for others to chime in 😁

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

What you did just now is probably fine too :)

@hundredwatt
Copy link
Contributor Author

@kaspth 👍

@hundredwatt
Copy link
Contributor Author

Just patched Delayed Job in a production Rails app with this warning and set up a log alert. Already found an instance of Model.each that should be Model.find_each 😄:

Feb 08 15:00:44 GaggleAMP: Result set size exceeded 1001 (model: Authentication::GoogleAuthentication, records: 1507)

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

Cool! I think the error message needs a little tweaking to read a bit more naturally.

Would it be of use to broadcast this as an Active Support notification too?

@@ -102,6 +102,17 @@ class Railtie < Rails::Railtie # :nodoc:
end
end

initializer "active_record.warn_on_result_set_size" do
if config.active_record.warn_on_result_set_size
config.after_initialize do |app|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need the after_initialize block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed, its only there because I copied and pasted from the previous initializer, doh! Will remove

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from 3e9d3da to 6aaf751 Compare Feb 10, 2015
@kaspth
Copy link
Contributor

kaspth commented Feb 10, 2015

I've been thinking about the config option name and result_set doesn't fit with Active Record's terminology.
What about something like warn_on_fetches_above or warn_on_record_fetches_greater_than?

super.tap do
if logger && warn_on_result_set_size
if @records.length >= warn_on_result_set_size
logger.warn "Result set size exceeded #{warn_on_result_set_size} (model: #{@klass}, records: #{@records.size})"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about changing the message to something like:

"Large query detected. Exceeded #{warn_on_result_set_size} threshold by loading #{@records.size} #{@klass} records."

@hundredwatt
Copy link
Contributor Author

@kaspth I like where you're going. How about:

For the config setting: config.active_record.warn_on_records_fetched_greater_than

For the message:

"Large query detected. Exceeded #{warn_on_result_set_size} threshold by fetching #{@records.size} #{@klass} records."

For the module, maybe we can call it LargeQueryDetection instead of WarnOnResultSetSize?

@kaspth
Copy link
Contributor

kaspth commented Feb 12, 2015

I like your suggestions way better than warn_on_result_set_size 😄

I have some more thoughts about the warning message.

There's a part of me that feels fetching is to lightweight a word. Is there a word that better fits we're dealing with a large query?

I think we should explain the threshold just a bit more. Something like this reads a bit broken:
"Large query detected. Exceeded 1001 threshold by fetching 1200 Kitten records."

@hundredwatt
Copy link
Contributor Author

Agreed, I'm struggling with how to phrase this 😄. Here's a number of ideas:

"Query returned large result set. Warning threshold of #{warn_on_result_set_size} exceeded by fetching #{@records.size} #{@klass} records."
"Query fetched #{@records.size} #{@klass} records, exceeding threshold of {warn_on_result_set_size}."
"Query fetched large number of records:  #{@records.size} #{@klass} records (exceeded threshold of #{warn_on_result_set_size})."

Alternatively, do we need to re-state the configured value by including #{warn_on_result_set_size}? Since its explicitly set I'm not sure we need to repeat in the warning message it:

"Query fetched #{@records.size} #{@klass} records"

or maybe have a recommendation?:

"Query fetched #{@records.size} #{@klass} records. Please use batch finder methods to limit memory consumption."

Do any of those options resonate with you?

@hundredwatt
Copy link
Contributor Author

Additionally, we could probably use AS::Notifications to capture the SQL query itself and include it in the warning. I'll play with that.

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from d7d676d to 24d6b9f Compare Feb 14, 2015
@kaspth
Copy link
Contributor

kaspth commented Feb 14, 2015

Well done and thanks for the kind words 👍

I'm going to need someone else to look through the notification stuff, so feel free to jump in.

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from ee2af4b to b1b0cb4 Compare Feb 26, 2015
@hundredwatt
Copy link
Contributor Author

I squashed my previous commits and fixed the documentation as per @fxn's recommendation

# When this module is prepended to ActiveRecord::Relation and
# `config.active_record.warn_on_records_fetched_greater_than` is
# set to an integer, if the number of records a query returns is
# greater than the value of `warn_on_records_fetched_greater_than`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your documentation says greater than but the check is >= 😉

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from 1c6f28f to 3c6675c Compare Mar 16, 2015
@kaspth
Copy link
Contributor

kaspth commented Mar 25, 2015

@hundredwatt hey, completely forgot about this until today 😄

Can you rebase and squash your commits? Then this gets my 👍

Great work, ❤️

@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from 3c6675c to 613abe2 Compare Mar 25, 2015
When set to an integer, a warning will be logged whenever a result set
larger than the specified size is returned by a query. Fixes rails#16463

The warning is outputed a module which is prepended in an initializer,
so there will be no performance impact if
`config.active_record.warn_on_records_fetched_greater_than` is not set.
@hundredwatt hundredwatt force-pushed the feat/warn-on-result-set-size branch from 613abe2 to 4d6fbe2 Compare Mar 25, 2015
@hundredwatt
Copy link
Contributor Author

@kaspth No worries, I know there's a lot of other PRs out there 😉

I rebased/squashed, but the Travis build didn't kick off. Any ideas?

@hundredwatt
Copy link
Contributor Author

@kaspth Nevermind, its building now

payload = args.last

QueryRegistry.queries << payload[:sql]
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could accomplish the same thing with:

ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload|
  QueryRegistry.queries << payload[:sql]
end

@kaspth
Copy link
Contributor

kaspth commented Mar 25, 2015

The build passed, but I don't know why GitHub didn't signal anything here.

@hundredwatt
Copy link
Contributor Author

The build passed, but I don't know why GitHub didn't signal anything here.

¯_(ツ)_/¯

rafaelfranca added a commit that referenced this pull request Mar 26, 2015
Add `config.active_record.warn_on_result_set_size` option
@rafaelfranca rafaelfranca merged commit 8b451e3 into rails:master Mar 26, 2015
@kaspth
Copy link
Contributor

kaspth commented Mar 26, 2015

@hundredwatt @rafaelfranca ❤️

@kaspth kaspth changed the title Add config.active_record.warn_on_result_set_size option Add config.active_record.warn_on_records_fetched_greater_than option Mar 26, 2015
jonatack added a commit to jonatack/rails that referenced this pull request Mar 27, 2015
Add `config.active_record.warn_on_records_fetched_greater_than` to the
Configuring Rails Guide.
schneems added a commit that referenced this pull request Mar 27, 2015
@hundredwatt hundredwatt deleted the feat/warn-on-result-set-size branch Mar 27, 2015
@hundredwatt
Copy link
Contributor Author

@kaspth @rafaelfranca Thanks so much for your feedback! 😄 💙

# `config.active_record.warn_on_records_fetched_greater_than` is
# set to an integer, if the number of records a query returns is
# greater than the value of `warn_on_records_fetched_greater_than`,
# a warning is logged. This allows for the dection of queries that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/dection/detection/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was merged a while ago and after looking at the master branch I see it's already fixed: fe6de0f 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to have ActiveRecord warn on big result sets
4 participants