From 859b88c3c8a767cf8e1e721e5d95ca6baf8fc7d7 Mon Sep 17 00:00:00 2001 From: Artur Petrov Date: Sat, 2 Dec 2023 14:23:11 +0300 Subject: [PATCH 1/4] Delay logger init and fortify configuration handling --- lib/futurism.rb | 13 ++++++++++--- lib/futurism/engine.rb | 4 ++++ lib/futurism/helpers.rb | 2 +- test/futurism_test.rb | 29 +++++++++++++++++++---------- test/resolver/controller_test.rb | 9 ++++----- test/support/helpers.rb | 18 ++++++++++++++++++ test/test_helper.rb | 3 +++ 7 files changed, 59 insertions(+), 19 deletions(-) create mode 100644 test/support/helpers.rb diff --git a/lib/futurism.rb b/lib/futurism.rb index b74c0af..194b1f1 100644 --- a/lib/futurism.rb +++ b/lib/futurism.rb @@ -17,16 +17,23 @@ module Futurism autoload :Helpers, "futurism/helpers" mattr_accessor :skip_in_test, default: false + mattr_accessor :instrumentation, default: false + mattr_accessor :logger mattr_writer :default_controller def self.default_controller (@@default_controller || "::ApplicationController").to_s.constantize end + def self.skip_in_test? + skip_in_test.present? + end + + def self.instrumentation? + instrumentation.present? + end + ActiveSupport.on_load(:action_view) do include Futurism::Helpers end - - mattr_accessor :logger - self.logger ||= Rails.logger ? Rails.logger.new : Logger.new($stdout) end diff --git a/lib/futurism/engine.rb b/lib/futurism/engine.rb index f39f246..919115c 100644 --- a/lib/futurism/engine.rb +++ b/lib/futurism/engine.rb @@ -19,5 +19,9 @@ class Engine < ::Rails::Engine app.config.importmap.cache_sweepers << Engine.root.join("app/assets/javascripts") end end + + initializer "futurism.logger", after: "initialize_logger" do + Futurism.logger ||= Rails.logger ? Rails.logger : Logger.new($stdout) + end end end diff --git a/lib/futurism/helpers.rb b/lib/futurism/helpers.rb index 824d153..1e9e422 100644 --- a/lib/futurism/helpers.rb +++ b/lib/futurism/helpers.rb @@ -1,7 +1,7 @@ module Futurism module Helpers def futurize(records_or_string = nil, extends: :div, **options, &block) - if (Rails.env.test? && Futurism.skip_in_test) || options[:unless] + if (Rails.env.test? && Futurism.skip_in_test?) || options[:unless] if records_or_string.nil? return render(**options) else diff --git a/test/futurism_test.rb b/test/futurism_test.rb index 40c763b..4a86763 100644 --- a/test/futurism_test.rb +++ b/test/futurism_test.rb @@ -7,22 +7,31 @@ class Futurism::Test < ActiveSupport::TestCase assert_kind_of Module, Futurism end - test ".skip_in_test" do - assert_equal false, Futurism.skip_in_test + test ".skip_in_test?" do + swap Futurism, skip_in_test: "" do + assert_equal false, Futurism.skip_in_test? + end end - test ".default_controller" do - assert_equal ApplicationController, Futurism.default_controller + test ".instrumentation?" do + swap Futurism, instrumentation: "" do + assert_equal false, Futurism.instrumentation? + end + end - Futurism.default_controller = nil + test ".default_controller" do assert_equal ApplicationController, Futurism.default_controller - Futurism.default_controller = DummyController - assert_equal DummyController, Futurism.default_controller + swap Futurism, default_controller: nil do + assert_equal ApplicationController, Futurism.default_controller + end - Futurism.default_controller = "DummyController" - assert_equal DummyController, Futurism.default_controller + swap Futurism, default_controller: DummyController do + assert_equal DummyController, Futurism.default_controller + end - Futurism.default_controller = nil + swap Futurism, default_controller: "DummyController" do + assert_equal DummyController, Futurism.default_controller + end end end diff --git a/test/resolver/controller_test.rb b/test/resolver/controller_test.rb index 3c3305a..e6cf57c 100644 --- a/test/resolver/controller_test.rb +++ b/test/resolver/controller_test.rb @@ -9,12 +9,11 @@ class Futurism::Resolver::ControllerTest < ActiveSupport::TestCase end test ".from uses Futurism.default_controller" do - Futurism.default_controller = DummyController - controller = Futurism::Resolver::Controller.from(signed_string: nil) - - assert_equal controller, DummyController + swap Futurism, default_controller: DummyController do + controller = Futurism::Resolver::Controller.from(signed_string: nil) - Futurism.default_controller = nil + assert_equal controller, DummyController + end end test ".from lookups up controller via signed_string:" do diff --git a/test/support/helpers.rb b/test/support/helpers.rb new file mode 100644 index 0000000..6bf1f44 --- /dev/null +++ b/test/support/helpers.rb @@ -0,0 +1,18 @@ +require "active_support/test_case" + +class ActiveSupport::TestCase + # Execute the block setting the given values and restoring old values after + # the block is executed. + def swap(object, new_values) + old_values = {} + new_values.each do |key, value| + old_values[key] = object.public_send(key) + object.public_send(:"#{key}=", value) + end + yield + ensure + old_values.each do |key, value| + object.public_send(:"#{key}=", value) + end + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 2c687dc..ea5d064 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -6,6 +6,9 @@ require "minitest/mock" require "nokogiri" +# Load support files +Dir["#{__dir__}/support/**/*.rb"].sort.each { |f| require f } + # Filter out the backtrace from minitest while preserving the one from other libraries. Minitest.backtrace_filter = Minitest::BacktraceFilter.new From 323e4a9d03c39873308cca946a6720cd2e43bfc6 Mon Sep 17 00:00:00 2001 From: Artur Petrov Date: Sat, 2 Dec 2023 14:23:42 +0300 Subject: [PATCH 2/4] Add instrumentation --- lib/futurism.rb | 1 + .../resolver/controller/instrumentation.rb | 33 +++++++++ lib/futurism/resolver/controller/renderer.rb | 6 +- .../controller/instrumentation_test.rb | 69 +++++++++++++++++++ 4 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 lib/futurism/resolver/controller/instrumentation.rb create mode 100644 test/resolver/controller/instrumentation_test.rb diff --git a/lib/futurism.rb b/lib/futurism.rb index 194b1f1..12df682 100644 --- a/lib/futurism.rb +++ b/lib/futurism.rb @@ -9,6 +9,7 @@ require "futurism/resolver/resources" require "futurism/resolver/controller" require "futurism/resolver/controller/renderer" +require "futurism/resolver/controller/instrumentation" require "futurism/helpers" module Futurism diff --git a/lib/futurism/resolver/controller/instrumentation.rb b/lib/futurism/resolver/controller/instrumentation.rb new file mode 100644 index 0000000..7c724b6 --- /dev/null +++ b/lib/futurism/resolver/controller/instrumentation.rb @@ -0,0 +1,33 @@ +require "active_support/notifications" + +module Futurism + module Resolver + class Controller + class Instrumentation < SimpleDelegator + PARAMETERS_KEY = ActionDispatch::Http::Parameters::PARAMETERS_KEY + + def render(*args) + ActiveSupport::Notifications.instrument( + "render.futurism", + channel: get_param(:channel), + controller: get_param(:controller), + action: get_param(:action), + partial: extract_partial_name(*args) + ) do + super(*args) + end + end + + private + + def get_param(key) + __getobj__.instance_variable_get(:@env).dig(PARAMETERS_KEY, key) + end + + def extract_partial_name(opts_or_model, *args) + opts_or_model.is_a?(Hash) ? opts_or_model[:partial] : opts_or_model.to_partial_path + end + end + end + end +end diff --git a/lib/futurism/resolver/controller/renderer.rb b/lib/futurism/resolver/controller/renderer.rb index ef75278..90f9cc4 100644 --- a/lib/futurism/resolver/controller/renderer.rb +++ b/lib/futurism/resolver/controller/renderer.rb @@ -7,7 +7,11 @@ class Renderer HTTP_METHODS = [:get, :post, :put, :patch, :delete] def self.for(controller:, connection:, url:, params:) - new(controller: controller, connection: connection, url: url, params: params).renderer + controller_renderer = new( + controller: controller, connection: connection, url: url, params: params + ).renderer + + Futurism.instrumentation? ? Instrumentation.new(controller_renderer) : controller_renderer end def initialize(controller:, connection:, url:, params:) diff --git a/test/resolver/controller/instrumentation_test.rb b/test/resolver/controller/instrumentation_test.rb new file mode 100644 index 0000000..be6d916 --- /dev/null +++ b/test/resolver/controller/instrumentation_test.rb @@ -0,0 +1,69 @@ +require "test_helper" + +class DummyController < ActionController::Base + def name_helper + "FUTURISM".freeze + end + helper_method :name_helper + + def controller_and_action_helper + [params["controller"], params["action"]].join(":") + end + helper_method :controller_and_action_helper + + def name_from_params_helper + params["name"] + end + helper_method :name_from_params_helper +end + +def dummy_connection + connection = Minitest::Mock.new + connection.expect(:env, {"HTTP_VAR" => "HTTP_VAR_VALUE"}) + connection +end + +class Futurism::Resolver::Controller::InstrumentationTest < ActiveSupport::TestCase + test "invokes ActiveSupport instrumentation on the Futurism render" do + swap Futurism, instrumentation: true do + events = [] + ActiveSupport::Notifications.subscribe("render.futurism") do |*args| + events << ActiveSupport::Notifications::Event.new(*args) + end + + renderer = Futurism::Resolver::Controller::Renderer.for( + controller: DummyController, + connection: dummy_connection, + url: "posts/1", + params: {channel: "Futurism::Channel"} + ) + post = Post.create title: "Lorem" + renderer.render(partial: "posts/card", locals: {post: post}) + + assert_equal 1, events.size + assert_equal "render.futurism", events.last.name + assert_equal "Futurism::Channel", events.last.payload[:channel] + assert_equal "posts", events.last.payload[:controller] + assert_equal "show", events.last.payload[:action] + assert_equal "posts/card", events.last.payload[:partial] + end + end + + test "does not invoke ActiveSupport instrumentation by default" do + events = [] + ActiveSupport::Notifications.subscribe("render.futurism") do |*args| + events << ActiveSupport::Notifications::Event.new(*args) + end + + renderer = Futurism::Resolver::Controller::Renderer.for( + controller: DummyController, + connection: dummy_connection, + url: "posts/1", + params: {channel: "Futurism::Channel"} + ) + post = Post.create title: "Lorem" + renderer.render(partial: "posts/card", locals: {post: post}) + + assert_empty events + end +end From 507dc07172613720883e093f8626fd213268fa16 Mon Sep 17 00:00:00 2001 From: Artur Petrov Date: Sat, 2 Dec 2023 14:49:58 +0300 Subject: [PATCH 3/4] Document the changes --- README.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/README.md b/README.md index 7be4b8c..5b2154d 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,7 @@ Lazy-load Rails partials via CableReady - [Broadcast Partials Individually](#broadcast-partials-individually) - [Contextual Placeholder Arguments](#contextual-placeholder-arguments) - [Events](#events) +- [Instrumentation](#instrumentation) - [Installation](#installation) - [Manual Installation](#manual-installation) - [Authentication](#authentication) @@ -228,6 +229,31 @@ For individual models or arbitrary collections, you can pass `record` and `index Once your futurize element has been rendered, the `futurism:appeared` custom event will be called. +## Instrumentation + +Futurism includes support for instrumenting rendering events. + +To enable ActiveSupport notifications, use the `instrumentation` option: + +```ruby +Futurism.instrumentation = true +``` + +Then subscribe to the `render.futurism` event: + +```ruby +ActiveSupport::Notifications.subscribe("render.futurism") do |*args| + event = ActiveSupport::Notifications::Event.new(*args) + event.name # => "render.futurism" + event.payload[:channel] # => "Futurism::Channel" # ActionCable channel to broadcast + event.payload[:controller] # => "posts" # The controller that invokes `futurize` call + event.payload[:action] # => "show" # The action that invokes `futurize` call + event.payload[:partial] # => "posts/card" # The partial that was rendered +end +``` + +This is useful for performance monitoring, specifically for tracking the source of `futurize` calls. + ## Installation Add this line to your application's Gemfile: From 8c32a8c5fc01cc7a5e279ac0d12c2e4cbbdb89b6 Mon Sep 17 00:00:00 2001 From: Artur Petrov Date: Sat, 2 Dec 2023 14:52:47 +0300 Subject: [PATCH 4/4] Run StandardRB --- lib/futurism/engine.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/futurism/engine.rb b/lib/futurism/engine.rb index 919115c..803d4d0 100644 --- a/lib/futurism/engine.rb +++ b/lib/futurism/engine.rb @@ -21,7 +21,7 @@ class Engine < ::Rails::Engine end initializer "futurism.logger", after: "initialize_logger" do - Futurism.logger ||= Rails.logger ? Rails.logger : Logger.new($stdout) + Futurism.logger ||= Rails.logger || Logger.new($stdout) end end end