fix: Use Active Support Lazy Load Hooks to avoid prematurely initializing ActiveRecord::Base and ActiveJob::Base #1104
Conversation
bensheldon
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle and
xuan-cao-swi
as code owners
| require_relative 'patches/base' | ||
| require_relative 'handlers' | ||
| end | ||
|
|
||
| def patch_activejob | ||
| ::ActiveJob::Base.prepend(Patches::Base) unless ::ActiveJob::Base <= Patches::Base | ||
|
|
||
| Handlers.subscribe |
There was a problem hiding this comment.
Should we also defer these subscriptions until an explicit point in the Rails initialization process? Or are we assuming folks are already registering this in config/initializers themselves?
There was a problem hiding this comment.
I'm not certain this is the right approach, but when New Relic detects the presence of Rails, it will load instrumentation within a Rails::Configuration.after_initialize block. It would be the equivalent of wrapping OpenTelemetry::SDK::Configurator#install_instrumentation in an after_initialize block.
This could probably be done individually in instrumentation if we didn't want to tie anything about Rails into the main SDK.
There was a problem hiding this comment.
I think that's right that Instrumentation subscriptions should happen in an after_initialize block. I was trying to avoid adding a full-blown Railtie, but that's the fully correct way to do it.
Though from poking around Rails, it seems like simply require "activesupport/notifications" might be totally load-order independent. e.g.
There was a problem hiding this comment.
I think then we should have a follow up PR and use a Railtie. I am ok with incremental improvements.
| # module that they are defined in as they are included into ActiveRecord::Base | ||
| # Example: Patches::PersistenceClassMethods refers to https://github.com/rails/rails/blob/v6.1.0/activerecord/lib/active_record/persistence.rb#L10 | ||
| # which is included into ActiveRecord::Base in https://github.com/rails/rails/blob/914caca2d31bd753f47f9168f2a375921d9e91cc/activerecord/lib/active_record/base.rb#L283 | ||
| ::ActiveRecord::Base.prepend(Patches::Querying) |
There was a problem hiding this comment.
It occurs to me now that none of these use active support concerns.
Is there any issue related to code reloading that would be alleviated by using concerns?
There was a problem hiding this comment.
None of the concerns have lazy-load-hooks, so they end up just being loaded at the same time.
I had to read the original comment like 5 times, but I think what it means is that the Rails code being patched here has been extracted into a concern by Rails, and OTel is patching it, with a similar naming convention, when it's included back into ActiveRecord::Base.
Also Concern is waaaay too overloaded as a word cause it can both mean "includes ActiveSupport::Concern which has some nice helper methods for extraction/inclusion" but also just like "a module with some behavior that's been extracted that's intended to be included back". ¯\_(ツ)_/¯
| end | ||
|
|
||
| def require_dependencies | ||
| require 'active_support/lazy_load_hooks' | ||
| require_relative 'patches/querying' |
There was a problem hiding this comment.
Though the hooks are lazy, shouldn't the modules or references be loaded closer to where they are used?
There was a problem hiding this comment.
I was pattern-matching the structure 😁 They could just as easily all live together right where they're called. I'll do some cleanup.
…zing ActiveRecord::Base and ActiveJob::Base
bensheldon
force-pushed
the
bensheldon/lazyload-hooks
branch
from
35be70b
to
22eba15
Compare
This PR ensures that requiring OpenTelemetry Rails will not prematurely initialize ActiveRecord::Base and ActiveJob::Base classes, which are autoloaded by Rails:
https://api.rubyonrails.org/v7.0.0/classes/ActiveSupport/LazyLoadHooks.html
The benefit is that Rails (tests / development) can potentially boot faster because these constants aren't being initialized unnecessarily.