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?
| 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.
500ba3f to
74596c5
Compare
|
Updated, please take a look. |
byroot
left a comment
byroot
left a comment
There was a problem hiding this comment.
Looks OK now, but there is a surprising amount of tests failures. I suspect they're not related but might be worth trying a rebase?
| # * +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.
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... |
|
🎉 |
|
Having line number logging is hugely helpful with Marginalia, so it's great to have this within Rails! I added this to my Rideshare app I used for testing and for my book, Rails 8,
Here's an example annotation from the end of one of the result rows:
Here it's unescaped: Works great! Thanks for your work on this @fatkodima @byroot! |
3 participants
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