ActiveSupport::Notifications::Instrumenter#instrument should yield #9523

Merged
merged 1 commit into from Mar 27, 2013

3 participants

@stopdropandrew

ActiveSupport::Notifications::Instrumenter#instrument should yield its payload the same way that ActiveSupport::Notifications does.

This allows you to cache the instrumenter (like in ActiveRecord's AbstractAdapter) but still yield the payload to the block.

This works:

ActiveSupport::Notifications.instrument("pull") do |payload|
  payload[:result] = "success"
end

But this doesn't:

ActiveSupport::Notifications.instrumenter.instrument("pull") do |payload|
  payload[:result] = "success"
end
@stopdropandrew stopdropandrew ActiveSupport::Notifications::Instrumenter#instrument should yield
its payload the same way that ActiveSupport::Notifications does.
Fix spelling in test name.
a007800
@schneems
Ruby on Rails member

Pretend i've never used this feature before ( i haven't), why would I ever use ActiveSupport::Notifications.instrumenter.instrument("pull") over ActiveSupport::Notifications.instrument("pull") ? Do you have any clues why the payload wasn't passed in from the original version?

@stopdropandrew

The main reason is so that you can cache the instrumenter, ActiveRecord does it.

My best guess as to why it didn't already yield is that ActiveRecord didn't need to add anything to the payload, and it is the primary place in Rails that caches the instrumenter.

@schneems
Ruby on Rails member

Looks good 👍 from me and @jeremy

@rafaelfranca rafaelfranca merged commit a007800 into rails:master Mar 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment