charkost / prosopite

:mag: Rails N+1 queries auto-detection with zero false positives / false negatives
Apache License 2.0
1.48k stars 45 forks source link

Use ActiveSupport::BacktraceCleaner #54

Closed technicalpickles closed 1 year ago

technicalpickles commented 1 year ago

Fixes https://github.com/charkost/prosopite/issues/53

We've added a Prosopite.backtrace_cleaner which defaults to Rails.backtrace_cleaner if it's available. It can also be configured, as mentioned in the README.md.

The original filtering was limited to just items in the bundled path. The default Rails.backtrace_cleaner includes that plus stdlib stuff, and also cleans up files relative to the current directory (ie /Users/technicalpickles/my_app/lib/foo.rb becomes lib/foo.rb). A user can also other filters, which we've done for things like middleware that is included everywhere.

technicalpickles commented 1 year ago

That will also fix the failing tests due to Rails' constant absence.

Looks like removing this fails with:

TestQueries#test_ignore_queries_with_incorrect_query_match [/home/runner/work/prosopite/prosopite/test/test_queries.rb:367]:
[Prosopite::NPlusOneQueriesError] exception expected, not
Class: <NameError>
Message: <"uninitialized constant Rails">
---Backtrace---
/home/runner/work/prosopite/prosopite/lib/prosopite.rb:27:in `backtrace_cleaner'
/home/runner/work/prosopite/prosopite/lib/prosopite.rb:214:in `block in send_notifications'
/home/runner/work/prosopite/prosopite/lib/prosopite.rb:208:in `each'
/home/runner/work/prosopite/prosopite/lib/prosopite.rb:208:in `send_notifications'
/home/runner/work/prosopite/prosopite/lib/prosopite.rb:101:in `finish'
/home/runner/work/prosopite/prosopite/test/test_queries.rb:387:in `block in assert_n_plus_one'
---------------

I think maybe we need to require Rails at some point for the tests? Currently, it's only pulling in ActiveRecord.

technicalpickles commented 1 year ago

I think maybe we need to require Rails at some point for the tests? Currently, it's only pulling in ActiveRecord.

Pushed a change to do this, and it seems to pass locally now. Let me know if you'd prefer that in a separate PR.

technicalpickles commented 1 year ago

Current failure:

  nokogiri-1.14.0-x86_64-linux requires ruby version >= 2.7, < 3.3.dev, which is
  incompatible with the current version, ruby 2.6.10p210

Looks like the rails dependency brought that in. I think 7.0 dropped Ruby 2.6 support, so maybe we can use an earlier Rails release? Or drop 2.6 support?

charkost commented 1 year ago

Squashed & merged as 81d909b4bf7e963cda60c1ab79a02ac261937e0d Thanks!

technicalpickles commented 1 year ago

@charkost great, thanks for merging and releasing!