WIP: display activerecord query cache hits #393
Conversation
Codecov Report
@@ Coverage Diff @@
## master #393 +/- ##
==========================================
- Coverage 88.76% 88.01% -0.75%
==========================================
Files 21 21
Lines 1237 1252 +15
==========================================
+ Hits 1098 1102 +4
- Misses 139 150 +11
Continue to review full report at Codecov.
|
gregmolnar
changed the title
gregmolnar
force-pushed
the
query_cache
branch
2 times, most recently
from
c672c26
to
b8b7517
Compare
| @@ -76,6 +76,35 @@ def initialize(app, config = nil) | |||
| @config.storage_instance = @config.storage.new(@config.storage_options) | |||
| end | |||
| @storage = @config.storage_instance | |||
| if defined?(ActiveSupport) | |||
| ActiveSupport::Notifications.subscribe "sql.active_record" do |*args| | |||
There was a problem hiding this comment.
sadly this is far from a zero cost thing @nateberkopec , in fact at Discourse we do stuff like this:
I am ok with adding this stuff to rack-mini-profiler, but we are going to new a setting here and to default it off explaining that this adds cost if you turn it on.
Ideally we should measure what the actual cost of this extra subscriber is.
There was a problem hiding this comment.
Interesting!
I wonder if there's a better way to get this stuff, because all we care about is the caller locations when QueryCache is hit. We don't actually have to subscribe to all AR notifications.
There was a problem hiding this comment.
I am ok with adding this stuff to rack-mini-profiler, but we are going to new a setting here and to default it off explaining that this adds cost if you turn it on.
I mean this is fine with me too tbh
There was a problem hiding this comment.
I will add a configuration option and default it to be off.
There was a problem hiding this comment.
I think ideally it would be in pp=help and work the same way no-backtrace, normal-backtrace and full-backtrace does. Maybe call it no-ar-cache (default), show-ar-cache (option).
That way you could still use it in production and just flip it back off when you're done. You can unsubscribe from ActiveSupport notifications, right?
|
Yes you can unsubscribe , sure that sounds like a great plan !
…On Thu, 1 Aug 2019 at 12:11 am, Nate Berkopec ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/mini_profiler/profiler.rb
<#393 (comment)>
:
> @@ -76,6 +76,35 @@ def initialize(app, config = nil)
@config.storage_instance = @***@***.***_options)
end
@storage = @config.storage_instance
+ if defined?(ActiveSupport)
+ ActiveSupport::Notifications.subscribe "sql.active_record" do |*args|
I think ideally it would be in pp=help and work the same way no-backtrace,
normal-backtrace and full-backtrace does. Maybe call it no-ar-cache
(default), show-ar-cache (option).
That way you could still use it in production and just flip it back off
when you're done. You can unsubscribe from ActiveSupport notifications,
right?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#393?email_source=notifications&email_token=AAABIXO4AY4P2XLFU3TPWJ3QCGMS7A5CNFSM4IEMTCSKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCAETXWA#discussion_r309242703>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAABIXJMJMAISATAHV7RPB3QCGMS7ANCNFSM4IEMTCSA>
.
|
|
Hi! I see that this PR is 3 years old, should it be closed ? I just started to use the profiler and I was looking for a way to distinguish queries that have hit or no hit on the cache. |
|
Now that RMP subscribes to sql notifications anyway, maybe this PR has new legs... |
|
Ship it! :) |
|
Closing as stale but this is a good idea so I'm moving it to an issue. |
Based on @nateberkopec 's recommendation, this change would display the Active Record queries hitting its query cache.