New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Marginalia to Rails, via QueryLogs #42240
Conversation
Looking into the CI failures - seems some state has leaked between tests. |
Pushed a change which only attempts to add the comment to the query if the feature is enabled. This is largely to check if anything else is having trouble in CI - currently investigating another approach to |
Back to draft while I look into problems with the railties and AR coverage. |
Hey, thanks for working on this! I'm finding the name This feels conceptually similar to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assorted stuff + some style notes. My earlier comment about log_tags
is more important, but I left this in case it's useful too.
We are also making Active Job and Action Controller depend on Active Record through the their railtie requires. @rafaelfranca knows more about what to do about that.
activerecord/lib/active_record/connection_adapters/abstract/query_comment.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/query_comment.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/query_comment.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/query_comment.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/query_comment.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/query_comment.rb
Outdated
Show resolved
Hide resolved
def escape_sql_comment(str) | ||
return str unless str.include?("*") | ||
while str.include?("/*") || str.include?("*/") | ||
str = str.gsub("/*", "").gsub("*/", "") | ||
end | ||
str | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we own the string at this point? Could we do this?
def escape_sql_comment(str) | |
return str unless str.include?("*") | |
while str.include?("/*") || str.include?("*/") | |
str = str.gsub("/*", "").gsub("*/", "") | |
end | |
str | |
end | |
def escape_sql_comment(comment) | |
comment.gsub!("/*", "") | |
comment.gsub!("*/", "") | |
comment | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the same feature as sanitize_as_sql_comment
. #35660
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've copied over the regexp approach from sanitize_as_sql_comment
- because it's operating on the connection it was getting into a recursive call when establishing the connection, I think.
This could be avoided if the method was on the class, instead of the connection instance - could we consider changing that?
Thank you for the feedback @kaspth ! 🙇♂️ I'm not particularly bound to the naming, happy to adjust to I'll work on your other suggestions along with other CI-related issues I need to address and then swing back to the rename so I can do that in one hit without confusing myself. |
Looking into the railtie integration test failure, as I suspect this is caused by these changes:
|
de3db76
to
0f82995
Compare
a8f0e3d
to
b35b9cc
Compare
I've updated the name of the feature to I think all other comments have been addressed in some shape or form :) Hopefully the decision to inline the SQL sanitisation regex is OK in this instance, otherwise we can look into an alternative approach. |
b35b9cc
to
0a630f4
Compare
Updated to tighten up some of the terminology around comment -> tag in the lib and in tests. Have left in instances where it's referring to an actual SQL comment string. ✅ |
0a630f4
to
53e9f8d
Compare
activerecord/lib/active_record/connection_adapters/abstract/query_log_tags.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/query_log_tags.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/query_log_tags.rb
Outdated
Show resolved
Hide resolved
Thanks for the review @eugeneius - some great points. I'll work on them and keep the commit history around for a bit longer (and clean it up when we're happy) - apologies to everyone for making it harder to track what I've updated. Will think some more on how we can keep close to the original API but also make it easier to add static (for now) components via application configuration vs reopening the module. |
Co-authored-by: Eileen M. Uchitelle <eileencodes@users.noreply.github.com> Co-authored-by: Kasper Timm Hansen <kaspth@gmail.com>
e51f426
to
2408615
Compare
Woop woop, welcome aboard! 🙌🎯 |
🎉 Really excited for this! |
Not sure if this will still be an issue with the port, but with the original gem I ran into some performance issues using the If might be worth adding a note about potential performance impact when using the I had opened an issue to add a note to the docs, but wasn't able to get it merged. Note: Some more rigorous testing might be worthwhile, so it isn't just my say so. :p It's going to be awesome that this is builtin now!!! |
Hey @hmcfletch ! We opted to lose |
when nil then instance_exec(&taggings[key]) if taggings.has_key? key | ||
when Proc then instance_exec(&value_input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm trying to update our app to replace Marginalia, and I'm having a bit of trouble with this instance_exec
. My main issue it that sorbet really doesn't like it, and that's on us to fix, but ultimately I wonder if value_input.call(context)
wouldn't be better.
First it would avoid exposing all private methods as an API, and also it would allow passing anything that responds to call
, not just a proc.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll PR it.
ConnectionAdapters::AbstractAdapter.descendants.each do |klass| | ||
klass.prepend(QueryLogs::ExecutionMethods) if klass.descendants.empty? | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this might be a bit too brittle.
For context we use https://github.com/Shopify/activerecord-pedantmysql2-adapter, which is a subclass of MySQL2Adapter
. This means MySQL2Adapter
isn't tagged at all in our app.
We also used to have a fake adapter for test purpose that was causing trouble.
I also feel that having to patch both exec_query
and execute
is a bit of a smell, there must be a better way we can hook in there with a single entry point. @kaspth any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm interesting! I would have expected the class to be picked up:
irb(main):009:0> ActiveRecord::Base.connection.class
=> ActiveRecord::ConnectionAdapters::PedantMysql2Adapter
irb(main):010:0> ActiveRecord::ConnectionAdapters::AbstractAdapter.descendants
=> [ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter, ActiveRecord::ConnectionAdapters::Mysql2Adapter, ActiveRecord::ConnectionAdapters::PedantMysql2Adapter]
irb(main):011:0> ActiveRecord::ConnectionAdapters::PedantMysql2Adapter.descendants.empty?
=> true
irb(main):012:0> ActiveRecord::QueryLogs.tags = [ :application ]
irb(main):013:0> ActiveRecord::QueryLogs.add_query_log_tags_to_sql("")
=> " /*application:MysqlDevApp*/"
I can't remember exactly why these two entry points were required - I think both are the bare minimum used across multiple adapters for different types of query, but not 100% sure. Would love to bring that down to a single place though!
On holiday right now (hah:), can dig more if needed later tonight :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On holiday right now (hah:), can dig more if needed later tonight :)
Don't worry. I'll have a look. Enjoy your vacations!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my main problem happens because the in the MySQLAdapter exec_query
calls execute_and_free
which calls execute
.
I assume that's also why there's these return if sql.include?(comment)
, that really sound like a big smell to me.
I'll try to work on refactoring that to eliminate that double call.
@keeran 👋 I'm working around this class again, and I'd like to question the usefulness of the |
Hi hi 👋 yes we use them for tagging specific types of queries - I'm not sure what you mean about replacing them with boolean checks though. They're a distinct set of tags used only within a block, outside of the broader / default set. Maybe a rough example would help? |
Sure. What I mean is: QueryLog.push_tag("foo") do
QueryLog.push_tag("bar") do
....
end
end could potentially just be: QueryLog.set_context(foo: true) do
QueryLog.set_contex(bar: true) do
....
end
end I suppose you'd get |
Hmm unless things have changed, that wouldn't necessarily result in new tags within that block unless there were custom tags defined which looked for those context entries. So for any adhoc inline tags we use in an app, we would have to predefine custom tags which look for those context entries and only add the tag when they were present, I think? |
Yes that's what I was suggesting. But again I saw no need for these tags in our app, so I might be missing some of their usefulness. |
OK great thanks for explaining. I'm not sure what the knock-on effects would be of changing that annotation structure as there are observability / tooling systems involved which are outside of my domain. I will report back asap :) |
Sorry for the delay here. Can confirm we would be able to safely adapt to the approach you have suggested without too much trouble. It makes sense to remove this adhoc tagging from QueryLogs if it cleans up the API and implementation, especially given the differences in approach (key=value vs plain string), and that If it's just a case of removing the feature, would you like me to open a PR doing so? |
If you'd like to do it sure! I'll happily merge. And thanks a lot for looking into this :) |
OK cool thanks - I'll prepare our app to use |
Porting Marginalia to Rails
This PR brings Marginalia SQL comments to Rails as a native feature. I have attempted to closely follow the Marginalia approach to solving the problem whilst introducing a new performance benefit for large applications.
The implementation (both in Marginalia and in this PR) can be broken down into 3 key areas:
Comment configuration & construction
Marginalia defines an array of
components
to construct an SQL comment. For each item incomponents
, a method is called onMarginalia::Comment
and the resulting key-value pair is concatenated into a comment string. This array of components can be modified by the application developer.In Marginalia:
In this PR we have adjusted the method-calling approach to work with Procs instead. Default tags are defined as Procs in Railtie callback handlers (added to the
taggings
attribute), and users are able to pass custom tag definitions during application configuration.If a custom comment component is required, a Hash can be added to the
components
array representing the key/value pair for the tag. If a Proc is passed as a value, it will be called when the comment is constructed and can reference internal state via thecontext
Hash.Rails integration - ActionController & ActiveJob
The core benefit of Marginalia is being able to decorate SQL queries with details of the context of the source of the query. By default Marginalia will add comment components representing the controller or job context and this is achieved by making use of callback filters.
Marginalia updates the controller context with
around_*
callbacks:A similar approach is followed in this PR:
In order for these context-specific components to function, Marginalia maintains references to the controller & job in
Thread.current
:Rather than create a potentially unbounded set of keys in
Thread.current
, I have opted to create a hash of context information (to include the controller and job) and only store thatcontext
hash inThread.current
, accessible viaActiveRecord::QueryLogs#update_context
&ActiveRecord::QueryLogs#set_context
.Adapter SQL execution interception
Finally, when the SQL comment is ready to be inserted into the query, Marginalia performs a series of checks before
alias_method
chaining into the appropriate adapter methods.Some of the complexity in this approach results from supporting older versions of Rails. Given we won’t have the same backwards-compatibility requirements here, I have simplified the instrumentation to
prepend
ing the execution methods on any subclass ofAbstractAdapter
:Notable changes
Comment caching
Introduces the ability to cache the component elements of a query comment to avoid needing to rebuild the same string repeatedly during a request / job if the context and content does not change.
The cached comment is stored per thread and is reset whenever the
context
is updated via#update_context
,#set_context
, or via an explicit operation:Migration
Rails 7 upgrade guide content in progress: migrating from Marginalia. (to be rewritten / adjusted as necessary when this feature has been completed)
/cc @eileencodes & @arthurnn