feat!(action_pack): Use ActiveSupport instead of patches (#703)

* feat!(action_pack): Use ActiveSupport instead of patches * feat!(action_pack): remove comments * feat: drop 6.0 on action_pack to verify the test case * feat: add rails 6.0 back * feat!(action_pack): remove rails 6.0 support + update readme on error handling * Update instrumentation/action_pack/README.md Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/README.md Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/README.md Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> * Update instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers.rb Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --------- Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com> Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com>
open-telemetry · Dec 6, 2023 · ec81777 · ec81777
1 parent c4d33b3
commit ec81777
Show file tree

Hide file tree

Showing 8 changed files with 221 additions and 47 deletions.
diff --git a/instrumentation/action_pack/README.md b/instrumentation/action_pack/README.md
@@ -30,6 +30,25 @@ OpenTelemetry::SDK.configure do |c|
 end
 ```
 
+## Active Support Instrumentation
+
+Earlier versions of this instrumentation relied on patching custom `dispatch` hooks from Rails's [Action Controller](https://github.com/rails/rails/blob/main/actionpack/lib/action_controller/metal.rb#L224) to extract request information.
+
+This instrumentation now relies on `ActiveSupport::Notifications` and registers a custom Subscriber that listens to relevant events to modify the Rack span.
+
+See the table below for details of what [Rails Framework Hook Events](https://guides.rubyonrails.org/active_support_instrumentation.html#action-controller) are recorded by this instrumentation:
+
+| Event Name | Subscribe? | Creates Span? |  Notes |
+| - | - | - | - |
+| `process_action.action_controller` | :white_check_mark: | :x: | It modifies the existing Rack span |
+
+
+### Error Handling for Action Controller
+
+If an error is triggered by Action Controller (such as a 500 internal server error), Action Pack will typically employ the default `ActionDispatch::PublicExceptions.new(Rails.public_path)` as the `exceptions_app`, as detailed in the [documentation](https://guides.rubyonrails.org/configuring.html#config-exceptions-app).
+
+The error object will be retained within `payload[:exception_object]`. Additionally, its storage in `request.env['action_dispatch.exception']` is contingent upon the configuration of `action_dispatch.show_exceptions` in Rails.
+
 ## Examples
 
 Example usage can be seen in the `./example/trace_demonstration.rb` file [here](https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/action_pack/example/trace_demonstration.ru)

diff --git a/instrumentation/action_pack/example/trace_demonstration.ru b/instrumentation/action_pack/example/trace_demonstration.ru
@@ -39,6 +39,6 @@ Rails.application.initialize!
 run Rails.application
 
 # To run this example run the `rackup` command with this file
-# Example: rackup trace_request_demonstration.ru
+# Example: rackup trace_demonstration.ru
 # Navigate to http://localhost:9292/
 # Spans for the requests will appear in the console
diff --git a/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers.rb b/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+# Copyright The OpenTelemetry Authors
+#
+# SPDX-License-Identifier: Apache-2.0
+
+require_relative 'handlers/action_controller'
+
+module OpenTelemetry
+  module Instrumentation
+    module ActionPack
+      # Module that contains custom event handlers, which are used to generate spans per event
+      module Handlers
+        module_function
+
+        def subscribe
+          return unless Array(@subscriptions).empty?
+
+          config = ActionPack::Instrumentation.instance.config
+          handlers_by_pattern = {
+            'process_action.action_controller' => Handlers::ActionController.new(config)
+          }
+
+          @subscriptions = handlers_by_pattern.map do |key, handler|
+            ::ActiveSupport::Notifications.subscribe(key, handler)
+          end
+        end
+
+        # Removes Event Handler Subscriptions for Action Controller notifications
+        # @note this method is not thread-safe and should not be used in a multi-threaded context
+        def unsubscribe
+          @subscriptions&.each { |subscriber| ::ActiveSupport::Notifications.unsubscribe(subscriber) }
+          @subscriptions = nil
+        end
+      end
+    end
+  end
+end
diff --git a/...n/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb b/...n/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb
@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+# Copyright The OpenTelemetry Authors
+#
+# SPDX-License-Identifier: Apache-2.0
+
+module OpenTelemetry
+  module Instrumentation
+    module ActionPack
+      module Handlers
+        # Action Controller handler to handle the notification from Active Support
+        class ActionController
+          # @param config [Hash] of instrumentation options
+          def initialize(config)
+            @config = config
+          end
+
+          # Invoked by ActiveSupport::Notifications at the start of the instrumentation block
+          #
+          # @param _name [String] of the event (unused)
+          # @param _id [String] of the event (unused)
+          # @param payload [Hash] the payload passed as a method argument
+          # @return [Hash] the payload passed as a method argument
+          def start(_name, _id, payload)
+            rack_span = OpenTelemetry::Instrumentation::Rack.current_span
+
+            request = payload[:request]
+
+            rack_span.name = "#{payload[:controller]}##{payload[:action]}" unless request.env['action_dispatch.exception']
+
+            attributes_to_append = {
+              OpenTelemetry::SemanticConventions::Trace::CODE_NAMESPACE => String(payload[:controller]),
+              OpenTelemetry::SemanticConventions::Trace::CODE_FUNCTION => String(payload[:action])
+            }
+
+            attributes_to_append[OpenTelemetry::SemanticConventions::Trace::HTTP_TARGET] = request.filtered_path if request.filtered_path != request.fullpath
+
+            rack_span.add_attributes(attributes_to_append)
+          rescue StandardError => e
+            OpenTelemetry.handle_error(exception: e)
+          end
+
+          # Invoked by ActiveSupport::Notifications at the end of the instrumentation block
+          #
+          # @param _name [String] of the event (unused)
+          # @param _id [String] of the event (unused)
+          # @param payload [Hash] the payload passed as a method argument
+          # @return [Hash] the payload passed as a method argument
+          def finish(_name, _id, payload)
+            rack_span = OpenTelemetry::Instrumentation::Rack.current_span
+            rack_span.record_exception(payload[:exception_object]) if payload[:exception_object]
+          rescue StandardError => e
+            OpenTelemetry.handle_error(exception: e)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/instrumentation.rb b/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/instrumentation.rb
@@ -32,11 +32,11 @@ def gem_version
         end
 
         def patch
-          ::ActionController::Metal.prepend(Patches::ActionController::Metal)
+          Handlers.subscribe
         end
 
         def require_dependencies
-          require_relative 'patches/action_controller/metal'
+          require_relative 'handlers'
         end
 
         def require_railtie

diff --git a/...ion_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb b/...ion_pack/lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal.rb
diff --git a/...k/patches/action_controller/metal_test.rb → ...n_pack/handlers/action_controller_test.rb b/...k/patches/action_controller/metal_test.rb → ...n_pack/handlers/action_controller_test.rb
@@ -6,19 +6,30 @@
 
 require 'test_helper'
 
-require_relative '../../../../../../lib/opentelemetry/instrumentation/action_pack'
-require_relative '../../../../../../lib/opentelemetry/instrumentation/action_pack/patches/action_controller/metal'
+require_relative '../../../../../lib/opentelemetry/instrumentation/action_pack'
+require_relative '../../../../../lib/opentelemetry/instrumentation/action_pack/handlers'
 
-describe OpenTelemetry::Instrumentation::ActionPack::Patches::ActionController::Metal do
+describe OpenTelemetry::Instrumentation::ActionPack::Handlers::ActionController do
   include Rack::Test::Methods
 
+  let(:instrumentation) { OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance }
   let(:exporter) { EXPORTER }
   let(:spans) { exporter.finished_spans }
   let(:span) { exporter.finished_spans.last }
   let(:rails_app) { DEFAULT_RAILS_APP }
+  let(:config) { {} }
 
   # Clear captured spans
-  before { exporter.reset }
+  before do
+    OpenTelemetry::Instrumentation::ActionPack::Handlers.unsubscribe
+
+    instrumentation.instance_variable_set(:@config, config)
+    instrumentation.instance_variable_set(:@installed, false)
+
+    instrumentation.install(config)
+
+    exporter.reset
+  end
 
   it 'sets the span name to the format: ControllerName#action' do
     get '/ok'
@@ -75,18 +86,57 @@
     get 'internal_server_error'
 
     _(span.name).must_equal 'ExampleController#internal_server_error'
+    _(span.kind).must_equal :server
+    _(span.status.ok?).must_equal false
+
+    _(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
+    _(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION
+
+    _(span.attributes['http.method']).must_equal 'GET'
+    _(span.attributes['http.host']).must_equal 'example.org'
+    _(span.attributes['http.scheme']).must_equal 'http'
+    _(span.attributes['http.target']).must_equal '/internal_server_error'
+    _(span.attributes['http.status_code']).must_equal 500
+    _(span.attributes['http.user_agent']).must_be_nil
+    _(span.attributes['code.namespace']).must_equal 'ExampleController'
+    _(span.attributes['code.function']).must_equal 'internal_server_error'
   end
 
   it 'does not set the span name when an exception is raised in middleware' do
     get '/ok?raise_in_middleware'
 
     _(span.name).must_equal 'HTTP GET'
+    _(span.kind).must_equal :server
+    _(span.status.ok?).must_equal false
+
+    _(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
+    _(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION
+
+    _(span.attributes['http.method']).must_equal 'GET'
+    _(span.attributes['http.host']).must_equal 'example.org'
+    _(span.attributes['http.scheme']).must_equal 'http'
+    _(span.attributes['http.target']).must_equal '/ok?raise_in_middleware'
+    _(span.attributes['http.status_code']).must_equal 500
+    _(span.attributes['http.user_agent']).must_be_nil
+    _(span.attributes['code.namespace']).must_be_nil
+    _(span.attributes['code.function']).must_be_nil
   end
 
   it 'does not set the span name when the request is redirected in middleware' do
     get '/ok?redirect_in_middleware'
 
     _(span.name).must_equal 'HTTP GET'
+    _(span.kind).must_equal :server
+    _(span.status.ok?).must_equal true
+
+    _(span.attributes['http.method']).must_equal 'GET'
+    _(span.attributes['http.host']).must_equal 'example.org'
+    _(span.attributes['http.scheme']).must_equal 'http'
+    _(span.attributes['http.target']).must_equal '/ok?redirect_in_middleware'
+    _(span.attributes['http.status_code']).must_equal 307
+    _(span.attributes['http.user_agent']).must_be_nil
+    _(span.attributes['code.namespace']).must_be_nil
+    _(span.attributes['code.function']).must_be_nil
   end
 
   describe 'when the application has exceptions_app configured' do
@@ -96,6 +146,20 @@
       get 'internal_server_error'
 
       _(span.name).must_equal 'ExampleController#internal_server_error'
+      _(span.kind).must_equal :server
+      _(span.status.ok?).must_equal false
+
+      _(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
+      _(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION
+
+      _(span.attributes['http.method']).must_equal 'GET'
+      _(span.attributes['http.host']).must_equal 'example.org'
+      _(span.attributes['http.scheme']).must_equal 'http'
+      _(span.attributes['http.target']).must_equal '/internal_server_error'
+      _(span.attributes['http.status_code']).must_equal 500
+      _(span.attributes['http.user_agent']).must_be_nil
+      _(span.attributes['code.namespace']).must_equal 'ExceptionsController'
+      _(span.attributes['code.function']).must_equal 'show'
     end
   end
 

diff --git a/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/handlers_test.rb b/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/handlers_test.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+# Copyright The OpenTelemetry Authors
+#
+# SPDX-License-Identifier: Apache-2.0
+
+require 'test_helper'
+
+require_relative '../../../../lib/opentelemetry/instrumentation/action_pack'
+
+describe 'OpenTelemetry::Instrumentation::ActionPack::Handlers' do
+  let(:instrumentation) { OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance }
+  let(:config) { {} }
+
+  before do
+    OpenTelemetry::Instrumentation::ActionPack::Handlers.unsubscribe
+    instrumentation.instance_variable_set(:@config, config)
+    instrumentation.instance_variable_set(:@installed, false)
+
+    instrumentation.install(config)
+  end
+
+  it 'success subscribe the notification' do
+    subscriptions = OpenTelemetry::Instrumentation::ActionPack::Handlers.instance_variable_get(:@subscriptions)
+    _(subscriptions.count).must_equal 1
+    _(subscriptions[0].pattern).must_equal 'process_action.action_controller'
+  end
+
+  it 'success unsubscribe the notification' do
+    OpenTelemetry::Instrumentation::ActionPack::Handlers.unsubscribe
+    subscriptions = OpenTelemetry::Instrumentation::ActionPack::Handlers.instance_variable_get(:@subscriptions)
+    _(subscriptions).must_be_nil
+  end
+end