Stop building AS::Notifications::Event manually

It's possible since Rails 6 (3ea2857) to let the framework create Event objects, but the guides and docs weren't updated to lead with this example. Manually instantiating an Event doesn't record CPU time and allocations, I've seen it more than once that people copy-pasting the example code get confused about these stats returning 0. The tests here show that - just like the apps I've worked on - the old pattern keeps getting copy-pasted.
rails · Sep 29, 2023 · 95b6fbd · 95b6fbd
1 parent d38dcdc
commit 95b6fbd
Show file tree

Hide file tree

Showing 17 changed files with 108 additions and 164 deletions.
diff --git a/actioncable/test/channel/base_test.rb b/actioncable/test/channel/base_test.rb
@@ -200,9 +200,7 @@ def error_handler
 
   test "notification for perform_action" do
     events = []
-    ActiveSupport::Notifications.subscribe "perform_action.action_cable" do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("perform_action.action_cable") { |event| events << event }
 
     data = { "action" => :speak, "content" => "hello" }
     @channel.perform_action data
@@ -218,9 +216,7 @@ def error_handler
 
   test "notification for transmit" do
     events = []
-    ActiveSupport::Notifications.subscribe "transmit.action_cable" do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("transmit.action_cable") { |event| events << event }
 
     @channel.perform_action "action" => :get_latest
     expected_data = { data: "latest" }
@@ -238,9 +234,7 @@ def error_handler
     @channel.subscribe_to_channel
 
     events = []
-    ActiveSupport::Notifications.subscribe "transmit_subscription_confirmation.action_cable" do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("transmit_subscription_confirmation.action_cable") { |e| events << e }
 
     @channel.stub(:subscription_confirmation_sent?, false) do
       @channel.send(:transmit_subscription_confirmation)
@@ -255,9 +249,7 @@ def error_handler
 
   test "notification for transmit_subscription_rejection" do
     events = []
-    ActiveSupport::Notifications.subscribe "transmit_subscription_rejection.action_cable" do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("transmit_subscription_rejection.action_cable") { |event| events << event }
 
     @channel.send(:transmit_subscription_rejection)
 

diff --git a/actioncable/test/server/broadcasting_test.rb b/actioncable/test/server/broadcasting_test.rb
@@ -16,9 +16,7 @@ class BroadcastingTest < ActionCable::TestCase
     server = TestServer.new
 
     events = []
-    ActiveSupport::Notifications.subscribe "broadcast.action_cable" do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("broadcast.action_cable") { |event| events << event }
 
     broadcasting = "test_queue"
     message = { body: "test message" }
@@ -37,9 +35,7 @@ class BroadcastingTest < ActionCable::TestCase
     server = TestServer.new
 
     events = []
-    ActiveSupport::Notifications.subscribe "broadcast.action_cable" do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("broadcast.action_cable") { |event| events << event }
 
     broadcasting = "test_queue"
     message = { body: "test message" }

diff --git a/actionmailbox/test/unit/mailbox/notifications_test.rb b/actionmailbox/test/unit/mailbox/notifications_test.rb
@@ -8,9 +8,7 @@ class RepliesMailbox < ActionMailbox::Base
 class ActionMailbox::Base::NotificationsTest < ActiveSupport::TestCase
   test "instruments processing" do
     events = []
-    ActiveSupport::Notifications.subscribe("process.action_mailbox") do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("process.action_mailbox") { |event| events << event }
 
     mailbox = RepliesMailbox.new(create_inbound_email_from_fixture("welcome.eml"))
     mailbox.perform_processing

diff --git a/actionmailer/test/base_test.rb b/actionmailer/test/base_test.rb
@@ -965,9 +965,7 @@ def a_callback
 
   test "notification for process" do
     events = []
-    ActiveSupport::Notifications.subscribe("process.action_mailer") do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("process.action_mailer") { |event| events << event }
 
     BaseMailer.welcome(body: "Hello there").deliver_now
 
@@ -982,9 +980,7 @@ def a_callback
 
   test "notification for deliver" do
     events = []
-    ActiveSupport::Notifications.subscribe("deliver.action_mailer") do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    ActiveSupport::Notifications.subscribe("deliver.action_mailer") { |event| events << event }
 
     BaseMailer.welcome(body: "Hello there").deliver_now
 

diff --git a/actionmailer/test/caching_test.rb b/actionmailer/test/caching_test.rb
@@ -173,9 +173,8 @@ def test_fragment_cache_instrumentation
     @mailer.enable_fragment_cache_logging = true
     payload = nil
 
-    subscriber = proc do |*args|
-      event = ActiveSupport::Notifications::Event.new(*args)
-      payload = event.payload
+    subscriber = proc do |_, _, _, _, event_payload|
+      payload = event_payload
     end
 
     ActiveSupport::Notifications.subscribed(subscriber, "read_fragment.action_mailer") do

diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb
@@ -257,9 +257,8 @@ def test_render_inline_before_fragment_caching
   def test_fragment_cache_instrumentation
     payload = nil
 
-    subscriber = proc do |*args|
-      event = ActiveSupport::Notifications::Event.new(*args)
-      payload = event.payload
+    subscriber = proc do |_, _, _, _, event_payload|
+      payload = event_payload
     end
 
     ActiveSupport::Notifications.subscribed(subscriber, "read_fragment.action_controller") do

diff --git a/actionpack/test/dispatch/middleware_stack_test.rb b/actionpack/test/dispatch/middleware_stack_test.rb
@@ -195,9 +195,7 @@ def test_delete_works
   test "instruments the execution of middlewares" do
     events = []
 
-    subscriber = proc do |*args|
-      events << ActiveSupport::Notifications::Event.new(*args)
-    end
+    subscriber = proc { |event| events << event }
 
     ActiveSupport::Notifications.subscribed(subscriber, "process_middleware.action_dispatch") do
       app = Rack::Lint.new(

diff --git a/activejob/test/cases/queuing_test.rb b/activejob/test/cases/queuing_test.rb
@@ -89,10 +89,8 @@ class QueuingTest < ActiveSupport::TestCase
     jobs = HelloJob.new("Jamie"), HelloJob.new("John")
     called = false
 
-    subscriber = lambda do |*args|
+    subscriber = proc do |_, _, _, _, payload|
       called = true
-      event = ActiveSupport::Notifications::Event.new(*args)
-      payload = event.payload
       assert payload[:adapter]
       assert_equal jobs, payload[:jobs]
       assert_equal 2, payload[:enqueued_count]

diff --git a/activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb b/activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb
@@ -358,14 +358,13 @@ class TrilogyAdapterTest < ActiveRecord::TrilogyTestCase
   def assert_notification(notification, expected_payload = {}, &block)
     notification_sent = false
 
-    subscription = lambda do |*args|
+    subscription = lambda do |_, _, _, _, payload|
       notification_sent = true
-      event = ActiveSupport::Notifications::Event.new(*args)
 
       expected_payload.each do |key, value|
         assert(
-          value === event.payload[key],
-          "Expected notification payload[:#{key}] to match #{value.inspect}, but got #{event.payload[key].inspect}."
+          value === payload[key],
+          "Expected notification payload[:#{key}] to match #{value.inspect}, but got #{payload[key].inspect}."
         )
       end
     end

diff --git a/activerecord/test/cases/instrumentation_test.rb b/activerecord/test/cases/instrumentation_test.rb
@@ -11,10 +11,9 @@ def setup
 
     def test_payload_name_on_load
       Book.create(name: "test book")
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("SELECT")
-          assert_equal "Book Load", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("SELECT")
+          assert_equal "Book Load", payload[:name]
         end
       end
       Book.first
@@ -23,10 +22,9 @@ def test_payload_name_on_load
     end
 
     def test_payload_name_on_create
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("INSERT")
-          assert_equal "Book Create", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("INSERT")
+          assert_equal "Book Create", payload[:name]
         end
       end
       Book.create(name: "test book")
@@ -35,10 +33,9 @@ def test_payload_name_on_create
     end
 
     def test_payload_name_on_update
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("UPDATE")
-          assert_equal "Book Update", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("UPDATE")
+          assert_equal "Book Update", payload[:name]
         end
       end
       book = Book.create(name: "test book", format: "paperback")
@@ -48,10 +45,9 @@ def test_payload_name_on_update
     end
 
     def test_payload_name_on_update_all
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("UPDATE")
-          assert_equal "Book Update All", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("UPDATE")
+          assert_equal "Book Update All", payload[:name]
         end
       end
       Book.update_all(format: "ebook")
@@ -60,10 +56,9 @@ def test_payload_name_on_update_all
     end
 
     def test_payload_name_on_destroy
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("DELETE")
-          assert_equal "Book Destroy", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("DELETE")
+          assert_equal "Book Destroy", payload[:name]
         end
       end
       book = Book.create(name: "test book")
@@ -73,10 +68,9 @@ def test_payload_name_on_destroy
     end
 
     def test_payload_name_on_delete_all
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("DELETE")
-          assert_equal "Book Delete All", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("DELETE")
+          assert_equal "Book Delete All", payload[:name]
         end
       end
       Book.delete_all
@@ -85,10 +79,9 @@ def test_payload_name_on_delete_all
     end
 
     def test_payload_name_on_pluck
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("SELECT")
-          assert_equal "Book Pluck", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("SELECT")
+          assert_equal "Book Pluck", payload[:name]
         end
       end
       Book.pluck(:name)
@@ -97,10 +90,9 @@ def test_payload_name_on_pluck
     end
 
     def test_payload_name_on_count
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("SELECT")
-          assert_equal "Book Count", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("SELECT")
+          assert_equal "Book Count", payload[:name]
         end
       end
       Book.count
@@ -109,10 +101,9 @@ def test_payload_name_on_count
     end
 
     def test_payload_name_on_grouped_count
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        if event.payload[:sql].match?("SELECT")
-          assert_equal "Book Count", event.payload[:name]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        if payload[:sql].match?("SELECT")
+          assert_equal "Book Count", payload[:name]
         end
       end
       Book.group(:status).count
@@ -122,9 +113,8 @@ def test_payload_name_on_grouped_count
 
     def test_payload_connection_with_query_cache_disabled
       connection = Book.connection
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        assert_equal connection, event.payload[:connection]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        assert_equal connection, payload[:connection]
       end
       Book.first
     ensure
@@ -133,9 +123,8 @@ def test_payload_connection_with_query_cache_disabled
 
     def test_payload_connection_with_query_cache_enabled
       connection = Book.connection
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
-        event = ActiveSupport::Notifications::Event.new(*args)
-        assert_equal connection, event.payload[:connection]
+      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |_, _, _, _, payload|
+        assert_equal connection, payload[:connection]
       end
       Book.cache do
         Book.first

diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb
@@ -129,9 +129,7 @@ def with_raise_on_open_redirects(service)
 
     def subscribe_events_from(name)
       events = []
-      ActiveSupport::Notifications.subscribe(name) do |*args|
-        events << ActiveSupport::Notifications::Event.new(*args)
-      end
+      ActiveSupport::Notifications.subscribe(name) { |event| events << event }
       events
     end
 end