Allow Mini Profiler patches to be optional in Rails applications #418
Conversation
Codecov Report
@@ Coverage Diff @@
## master #418 +/- ##
=========================================
+ Coverage 88.23% 88.53% +0.3%
=========================================
Files 21 22 +1
Lines 1249 1335 +86
=========================================
+ Hits 1102 1182 +80
- Misses 147 153 +6
Continue to review full report at Codecov.
|
|
I think we should be a bit more official about this support including changes to the README.md Our default I think is correct, we much prefer to get rich information vs limited. But we should be transparent about it. After
Something like that... Ideally we can also support Sequel here using existing Sequel hooks, perhaps @jeremyevans can help point you at what hooks you can lean on there? |
|
Sure, I can add a note about this change in README.md and changelog, however I wanted to point out (and make sure you noticed) that I'm not using the The |
|
Tricky business! One possible idea is to codify this into the The nice thing about this is that it is super clear what is going on and how to move to the "default" way it works if you don't like the money patches. I am kind of liking this cause it is super explicit about what is going on, and works with straight rack or rails and can easily be documented. @rafaelfranca what are your thoughts? |
|
I really like this idea as well (didn't know If people want to disable patches and want to have gem 'rack-mini-profiler', require: ['disable_method_patches']If they want to disable patches but don't want to gem 'rack-mini-profiler', require: ['disable_method_patches', 'rack-mini-profiler']And no change is needed for existing users. |
|
Having this on gemfile of Rails application is a no-go for me. It is aesthetically unpleasant and still give the message that patching Rails is ok. Even adding a config to disable monkey patch Rails is a no-go to me. It needs to not patch Rails at all by default to be in the Rails Gemfile. BTW, I don't have a problem with matching Net::HTTP, I just don't want to add by default a gem to Rails applications that can break every time we change private API that are being patched by the gem. |
|
Understood, I am fine to amend the gem not to patch rails by default, it is
the other patches that I need on by default
We can work out the rails missing APIs.
The performance and richness of direct patches to db drivers is not
something I want to give up, I don’t mind working with pg and MySQL long
term to get hooks direct in the db drivers so we can avoid this, but short
term this is tricky
I think the only issue is the action view patch? What changes do we need to
make in rails to avoid the need for it?
…On Sat, 16 Nov 2019 at 6:47 am, Rafael França ***@***.***> wrote:
Having this on gemfile of Rails application is a no-go for me. It is
aesthetically unpleasant and still give the message that patching Rails is
ok. Even adding a config to disable monkey patch Rails is a no-go to me. It
needs to not patch Rails at all by default to be in the Rails Gemfile.
BTW, I don't have a problem with matching Net::HTTP, I just don't want to
add by default a gem to Rails applications that can break every time we
change private API that are being patched by the gem.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#418?email_source=notifications&email_token=AAABIXJNNM6JGVLNZ7EQF6TQT34FBA5CNFSM4JM72EWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEGQS6Q#issuecomment-554502522>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAABIXP3ATKV6ELZ4XITTEDQT34FBANCNFSM4JM72EWA>
.
|
|
Where are the action view patches? I only saw commented code related to Action View. |
|
I think this is the sticking point: I am glad to redo this thing so our fallback is less rich if we must https://github.com/MiniProfiler/rack-mini-profiler/blob/master/lib/patches/db/activerecord.rb and then by default we have "direct db driver" patches and a fallback of a limited notification. |
|
I think both of those methods send notifications through |
|
The trouble with the notifications API is that we are building a tree of events, we ideally want a "before" and "after" call so we can cleanly construct a tree. AS notifications only gives us an after call. We can reverse it into the tree with a bit more work, provided we are lucky and times line up. I did want to spend a bit of time writing a proposal for a more efficient AS notifications API that can handle these use cases better, will hack away at something. But yeah current API should in theory be somewhat workable, albeit less efficient than the method patching. |
|
Ok, sorry for the delay here 😅 I've changed the default here so that Rails is not patched at all, but the DB drivers (e.g. Note I've used I've tested this with a brand new Rails app based off Rails master and it worked great (will double test this tomorrow because I've made some small changes while I was writing tests). |
|
@rafaelfranca thoughts? is this in line with what you were thinking? |
|
Yes. Great work on this! I think you will have to deal with |
|
Thank you @rafaelfranca! I've updated the PR so that Mini Profiler will fallback to |
background: MiniProfiler#418 By default, rack mini profiler is expected to use active support notifications Before ====== Oracle is working as described, but pg and mysql are not. SqlPatches.sql_patches returns ["pg"] for postgres and does not patch notifications verification: After ===== Like oracle, postgres and mysql are double checking with patch_rails? to determine if the rails code should be patched. SqlPatches.sql_patches returns [] for postgres
background: MiniProfiler#418 By default, rack mini profiler is expected to use active support notifications Before ====== Oracle is working as described, but pg and mysql are not. SqlPatches.sql_patches returns ["pg"] for postgres, patches postgres, and does not leverage ActiveSupport::Notifications After ===== Like oracle, postgres and mysql are double checking with patch_rails? to determine if the rails code should be patched. SqlPatches.sql_patches returns [] for postgres
background: #418 By default, rack mini profiler is expected to use active support notifications Before ====== Oracle is working as described, but pg and mysql are not. SqlPatches.sql_patches returns ["pg"] for postgres, patches postgres, and does not leverage ActiveSupport::Notifications After ===== Like oracle, postgres and mysql are double checking with patch_rails? to determine if the rails code should be patched. SqlPatches.sql_patches returns [] for postgres
This PR allows Rails application to use mini profiler without any of the monkey patches that mini profiler ships with, instead mini profiler will rely completely on
ActiveSupport::Notifications.To use the non-patches version in your Rails app, add
config.mini_profiler_without_patches = trueto your environment configurations file(s).There will be some differences between the patches and no-patches versions, for example most importantly measuring SQL/actions/render time won't be as accurate, and there won't be a
Net::HTTP"step" because there is no official API that we can use to replicate what this patch does https://github.com/MiniProfiler/rack-mini-profiler/blob/master/lib/patches/net_patches.rb.