fix!: activerecord find_by_sql spans on Rails 7.0+ #1184
Conversation
mrsimo
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle,
xuan-cao-swi and
a team
as code owners
|
Sorry about those tests, somehow pushed a last minute thing that broke the build! |
No problem, @mrsimo! Taking a look at this now. It looks like there's one Rubocop offense preventing the |
| super | ||
| if ::ActiveRecord.version >= Gem::Version.new('7.0.0') | ||
| def _query_by_sql(...) | ||
| tracer.in_span("#{self}.find_by_sql") do |
There was a problem hiding this comment.
I get what you're saying about the naming! If we change the name of the span, we should consider marking this update as a breaking change since all 7.0 spans would now have a new name.
I like your suggestion of query for 7.0+ spans since they don't roll up to find_by_sql.
Unfortunately, instrumentation often has to resort to private methods to get the data users want, so _query_by_sql probably wouldn't be too much of a shock and perhaps easier for a user to connect the dots.
I'd love to hear other opinions!
There was a problem hiding this comment.
I feel like query (or similar) would be the most stable for the future. Since we're hooking onto internal AR methods, it's possible we might need to change methods, needing a breaking change every time.
The downside is that all other spans being generated have the name of the method we're hooking into, so it would be a bit inconsistent. We could make it configurable?
As a user of this instrumentation I'd lean towards something more generic and stable. I usually have the mysql2 or trilogy spans with details about the exact query that ran, and as a developer I'd need to open the rails code to know what _query_by_sql even is. Even the find_by_sql was a bit confusing, I didn't know all reads (used to) go through that method.
There was a problem hiding this comment.
Nice, I'm leaning toward query now too!
I hesitate to make it configurable. We have some configuration around span naming for ActiveSupport::Notifications in some of our Rails-related instrumentation gems, but in this scenario, since the span is not related to ActiveSupport::Notifications, I think having a single, consistent name would be best.
There was a problem hiding this comment.
I pushed a change to use query instead of find_by_sql. Spans are now called User query, Account query, etc. I'm liking it!
There was a problem hiding this comment.
Great to hear! Would you mind updating the PR title to mark this as a breaking change? I think this'll help anyone who uses find_by_sql in their observability backends know to update things accordingly.
It can be marked as a breaking change by updating the title to start with fix!: instead of fix:
If you don't think this should be a breaking change, let me know and we can discuss it further.
There was a problem hiding this comment.
Done! Totally agree that it's a breaking change.
instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good to me!
You can ignore this suggestions since it's just a preference
method_name = ::ActiveRecord.version >= Gem::Version.new('7.0.0') ? :_query_by_sql : :find_by_sql
define_method(method_name) do |*args, **kwargs|
tracer.in_span("#{self} query") do
super(*args, **kwargs)
end
endp.s. just checked active_record 3,4,5 and 6, they all use find_by_sql, so only 7+ they made big change, hopefully they won't do it again in 8 😁
|
Hey @xuan-cao-swi! Thanks for the suggestion! An error happens with that version: I'm not very familiar with |
|
Sorry, I should test it before make a comment. It's missing the key-like arguments. I updated my comments (with |
mrsimo
changed the title
|
@xuan-cao-swi cool! Pushed that change! |
For #1183
From my tests, using
_query_by_sqlto instrument is compatible with 7.0, 7.1 and 7.2, and is mostly equivalent tofind_by_sql, with that one calling_query_by_sqlitself. I believe this would be enough to bring back the same behavior we had in 6.1.I am a bit conflicted with the name of these spans, since
find_by_sqlisn't entirely true anymore. Should they be called_query_by_sqlin that case instead? Feels weird using the name of a private method, though. Should we do a more genericreadorquery? Help on this would be appreciated 😸Thank you!!