Skip to content
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 automatic instrumentation support for callbacks #760

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
require_relative "callbacks_instrumentation/backport"

module ActiveSupport::Callbacks
ClassMethods.prepend Module.new {
def set_callback(name, *filter_list, &block)
*, options = normalize_callback_params(filter_list.dup, block)
Instrumenter.options = options.merge(caller_locations:)
super
ensure
Instrumenter.options = nil
end
}

Callback.prepend Module.new {
def compiled
@compiled = super.then { Instrumenter.build self, _1 } unless defined?(@compiled)
@compiled
end
}

class Instrumenter < Data.define(:callback, :callable, :options)
singleton_class.attr_accessor :options

def self.build(callback, callable)
options ? new(callback, callable, options) : callable
end

def initialize(callback:, callable:, options:)
caller_locations = options.delete(:caller_locations)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got this error when trying to migrate the database on a fresh new application:

Loading application environment...
Loading code in search of Active Record models...
bin/rails aborted!
NoMethodError: undefined method `first' for nil (NoMethodError)

      @full_name = (callback.name == :commit) ? caller_locations.first.base_label : "#{callback.kind}_#{callback.name}"
                                                                ^^^^^^
/home/gazayas/work/bt/bullet_train/local/bullet_train-core/bullet_train/lib/bullet_train/core_ext/callbacks_instrumentation.rb:31:in `initialize'

Since caller_locations is nil here, I wonder if we should simply return the method in that case? I can't say I know too much about this Instrumenter class to say that that's the right decision.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, the Loading code in search of Active Record models comes from rails-erd. Dropping this for reference because we do some eager loading here: bullet-train-co/bullet_train#1200

@location = Rails.backtrace_cleaner.clean(caller_locations.map(&:to_s)).first if caller_locations
@full_name = (callback.name == :commit) ? caller_locations.first.base_label : "#{callback.kind}_#{callback.name}"
super
end
attr_reader :options, :location, :full_name
delegate :name, :kind, :filter, to: :callback

def apply(sequence)
case kind
in :before then sequence.before(self)
in :after then sequence.after(self)
in :around then sequence.around(self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After migrating the database against main and switching over to this branch, I got the following error:

image

Looking at the first commit that you posted in backport.rb, I saw that they started using this in #apply:

callback_sequence.around(@user_callback, @user_conditions)

I went ahead and used that and then everything started working for me properly, but I don't know enough about this class to say if that's the proper fix or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's a test failure that's complaining about the same thing: https://github.com/bullet-train-co/bullet_train-core/actions/runs/8115440263/job/22183312092?pr=760#step:7:34

CleanShot 2024-03-18 at 13 20 11

end
end

def call(env)
ActiveSupport::Notifications.instrument "callback.active_support", callback: self do
callable.call(env)
end
end
end

class LogSubscriber < ActiveSupport::LogSubscriber
attach_to :active_support

def callback(event)
event.payload => {callback:}
info { "Callback #{callback.full_name}: #{callback.filter} #{callback.options} from #{callback.location}".squish }
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
return if ActiveSupport::VERSION.to_s.start_with?("7.2")

# Backports a few commits from Rails 7.2 to make our debugging extension easier:
#
# - https://github.com/rails/rails/commit/6c5a042824fdaf630ebaa7fb293b8c2543f20d00
# - https://github.com/rails/rails/commit/42ad4d6b0bfc14e16050f9945a8b2ac5c87c294f
Comment on lines +1 to +6
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a few commits to make our instrumentation a little easier, so we backport these commits …except if users are running on Rails 7.2 where they already are.

module ActiveSupport::Callbacks
Callback.prepend Module.new {
def initialize(...)
super
compiled # Eager load ActiveSupport::Callback procs
end

def compiled
@compiled ||=
begin
user_conditions = conditions_lambdas
user_callback = CallTemplate.build(@filter, self)

case kind
when :before
Filters::Before.new(user_callback.make_lambda, user_conditions, chain_config, @filter, name)
when :after
Filters::After.new(user_callback.make_lambda, user_conditions, chain_config)
when :around
Filters::Around.new(user_callback, user_conditions)
end
end
end

def apply(callback_sequence)
compiled.apply(callback_sequence)
end
}

class CallbackSequence
def before(before)
@before.unshift(before)
self
end

def after(after)
@after.push(after)
self
end
end

module Filters
# Around wasn't defined before our commit backport, so we don't need to remove that.
remove_const :Before
remove_const :After

class Before
def initialize(user_callback, user_conditions, chain_config, filter, name)
halted_lambda = chain_config[:terminator]
@user_callback, @user_conditions, @halted_lambda, @filter, @name = user_callback, user_conditions, halted_lambda, filter, name
freeze
end
attr_reader :user_callback, :user_conditions, :halted_lambda, :filter, :name

def call(env)
target = env.target
value = env.value
halted = env.halted

if !halted && user_conditions.all? { |c| c.call(target, value) }
result_lambda = -> { user_callback.call target, value }
env.halted = halted_lambda.call(target, result_lambda)
if env.halted
target.send :halted_callback_hook, filter, name
end
end

env
end

def apply(callback_sequence)
callback_sequence.before(self)
end
end

class After
attr_reader :user_callback, :user_conditions, :halting
def initialize(user_callback, user_conditions, chain_config)
halting = chain_config[:skip_after_callbacks_if_terminated]
@user_callback, @user_conditions, @halting = user_callback, user_conditions, halting
freeze
end

def call(env)
target = env.target
value = env.value
halted = env.halted

if (!halted || !@halting) && user_conditions.all? { |c| c.call(target, value) }
user_callback.call target, value
end

env
end

def apply(callback_sequence)
callback_sequence.after(self)
end
end

class Around
def initialize(user_callback, user_conditions)
@user_callback, @user_conditions = user_callback, user_conditions
freeze
end

def apply(callback_sequence)
callback_sequence.around(@user_callback, @user_conditions)
end
end
end
end
10 changes: 10 additions & 0 deletions bullet_train/lib/bullet_train/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,15 @@

module BulletTrain
class Engine < ::Rails::Engine
config.bullet_train = ActiveSupport::OrderedOptions.new
config.bullet_train.instrument_callbacks = Rails.env.development? || Rails.env.test?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure of a better way to ensure we run this instrumentation in our CI tests than to just make it a default in Bullet Train apps' tests too, but that may be a bit too aggressive. @jagthedrummer would you happen to have a better idea?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, so sorry for the delay on this! Can we use a config option to toggle this on or off, and then in the starter repo we default to it being on? Directly checking Rails.env seems less than ideal.


initializer "callbacks.instrumentation" do
if config.bullet_train.instrument_callbacks
config.after_initialize do
require_relative "../bullet_train/core_ext/callbacks_instrumentation"
end
end
end
end
end
Loading