Support :source_location tag option for query log tags
#50969
Conversation
| end | ||
| else | ||
| def query_source_location # :nodoc: | ||
| LogSubscriber.backtrace_cleaner.clean(caller(1).lazy).first |
There was a problem hiding this comment.
I don't think this lazy make much sense. Might as well explicitly do caller_location(1).each.
| if Thread.respond_to?(:each_caller_location) | ||
| def query_source_location # :nodoc: | ||
| Thread.each_caller_location do |location| | ||
| frame = LogSubscriber.backtrace_cleaner.clean_frame(location) |
There was a problem hiding this comment.
| frame = LogSubscriber.backtrace_cleaner.clean_frame(location) | |
| frame = LogSubscriber.backtrace_cleaner.clean_frame(location.path) |
Might as well pass only the path, no?
| @@ -71,7 +72,10 @@ module ActiveRecord | |||
| # | |||
| # config.active_record.cache_query_log_tags = true | |||
| module QueryLogs | |||
| mattr_accessor :taggings, instance_accessor: false, default: {} | |||
| mattr_accessor :taggings, instance_accessor: false, default: { | |||
| line: -> { query_source_location } | |||
There was a problem hiding this comment.
This should be set in activerecord/railtie.rb like the others, otherwise it risks being overidden by users.
fatkodima
force-pushed
the
query_logs-line
branch
from
500ba3f
to
74596c5
Compare
|
Updated, please take a look. |
| @@ -26,6 +26,7 @@ module ActiveRecord | |||
| # * +socket+ | |||
| # * +db_host+ | |||
| # * +database+ | |||
| # * +line+ | |||
There was a problem hiding this comment.
Also I wonder if line really makes sense as a name, as it's not just the line, but the whole location. Maybe location or source_location?
There was a problem hiding this comment.
I named it after the marginalia's :line, but yeah, changed it to :source_location.
CI is still red, but looks like is not related to this PR.
fatkodima
force-pushed
the
query_logs-line
branch
from
74596c5
to
a87668a
Compare
fatkodima
changed the title
:line tag option for query log tags:source_location tag option for query log tags
|
It's weird because main is green, and you got the same failures after your rebase. I'll try to check if main is hosed. |
|
Yeah, similar failures on other PRs... |
Follow up to #42240 (as discussed in #42240 (comment)).
We lost the ability to use the
:lineoption with QueryLogs (#42240 (comment)), while there is one in the marginalia. It was costly to use it before in the gem, but we made it fast (basecamp/marginalia#138), so it is not that costly and people deliberately decide if they want to use it.I would like to have the
:lineoption (upd: decided to name it as:source_location), it is very much useful. Without it, if we have a slow db query log, for example, it is very hard to know from which part of the codebase the query was generated. Even having an:actioncomponent, you need to investigate the unfamiliar codebase to find out where the query was triggered.cc @byroot