feat!: Custom ActiveSupport Span Names (#1014)

* feat!: Custom ActiveSupport Span Names

The current implementation of ActiveSupport instrumentation sets the span name to the reverse tokenized name,
e.g. `render_template.action_view` is converted to `action_view render_template`

This default behavior can sometimes seem counter intuitive for users who use ActiveSupport Notifications to instrument their own code
or users who are familiar with Rails instrumentation names.

This change does a few things to address the issues listed above:

1. Uses the notification name by default as oppossed to the legacy span name
2. Allows users to provide a custom span name formatter lambda
3. Provides a proc with backward compatible span name formatter `OpenTelemetry::Instrumentation::ActiveSupport::LEGACY_NAME_FORMATTER`

See #957

* squash: Bolt on a few things

* squash: would be great if the tests passed

* squash: Linter

Author: arielvalentin

instrumentation/active_support/README.md

@@ -1,12 +1,15 @@
 # OpenTelemetry ActiveSupport Instrumentation
+
 The Active Support instrumentation is a community-maintained instrumentation for the Active Support portion of the [Ruby on Rails][rails-home] web-application framework.
 
 ## How do I get started?
 
 Install the gem using:
 
-```
+```console
+
 gem install opentelemetry-instrumentation-active_support
+
 ```
 
 Or, if you use [bundler][bundler-home], include `opentelemetry-instrumentation-active_support` in your `Gemfile`.
@@ -17,21 +20,24 @@ To use the instrumentation, call `use` with the name of the instrumentation and
 to desired ActiveSupport notification:
 
 ```ruby
+
 OpenTelemetry::SDK.configure do |c|
   c.use 'OpenTelemetry::Instrumentation::ActiveSupport'
 end
 
-
 tracer = OpenTelemetry.tracer_provider.tracer('my_app_or_gem', '0.1.0')
 ::OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, 'bar.foo')
+
 ```
 
 Alternatively, you can also call `use_all` to install all the available instrumentation.
 
 ```ruby
+
 OpenTelemetry::SDK.configure do |c|
   c.use_all
 end
+
 ```
 
 ## Examples

instrumentation/active_support/lib/opentelemetry/instrumentation/active_support/span_subscriber.rb

@@ -8,6 +8,9 @@ module OpenTelemetry
   module Instrumentation
     # rubocop:disable Style/Documentation
     module ActiveSupport
+      LEGACY_NAME_FORMATTER = ->(name) { name.split('.')[0..1].reverse.join(' ') }
+
+      # rubocop:disable Metrics/ParameterLists
       # The SpanSubscriber is a special ActiveSupport::Notification subscription
       # handler which turns notifications into generic spans, taking care to handle
       # context appropriately.
@@ -19,15 +22,17 @@ def self.subscribe(
         tracer,
         pattern,
         notification_payload_transform = nil,
-        disallowed_notification_payload_keys = [],
-        kind: nil
+        disallowed_notification_payload_keys = nil,
+        kind: nil,
+        span_name_formatter: nil
       )
         subscriber = OpenTelemetry::Instrumentation::ActiveSupport::SpanSubscriber.new(
           name: pattern,
           tracer: tracer,
           notification_payload_transform: notification_payload_transform,
           disallowed_notification_payload_keys: disallowed_notification_payload_keys,
-          kind: kind
+          kind: kind,
+          span_name_formatter: span_name_formatter
         )
 
         subscriber_object = ::ActiveSupport::Notifications.subscribe(pattern, subscriber)
@@ -54,16 +59,19 @@ def self.subscribe(
         subscriber_object
       end
 
+      # rubocop:enable Metrics/ParameterLists
       class SpanSubscriber
         ALWAYS_VALID_PAYLOAD_TYPES = [TrueClass, FalseClass, String, Numeric, Symbol].freeze
 
-        def initialize(name:, tracer:, notification_payload_transform: nil, disallowed_notification_payload_keys: [], kind: nil)
-          @span_name = name.split('.')[0..1].reverse.join(' ').freeze
+        # rubocop:disable Metrics/ParameterLists
+        def initialize(name:, tracer:, notification_payload_transform: nil, disallowed_notification_payload_keys: nil, kind: nil, span_name_formatter: nil)
+          @span_name = safe_span_name_for(span_name_formatter, name).dup.freeze
           @tracer = tracer
           @notification_payload_transform = notification_payload_transform
-          @disallowed_notification_payload_keys = disallowed_notification_payload_keys
+          @disallowed_notification_payload_keys = Array(disallowed_notification_payload_keys)
           @kind = kind || :internal
         end
+        # rubocop:enable Metrics/ParameterLists
 
         def start(name, id, payload)
           span = @tracer.start_span(@span_name, kind: @kind)
@@ -128,6 +136,16 @@ def sanitized_value(value)
             value
           end
         end
+
+        # Helper method to try an shield the span name formatter from errors
+        #
+        # It wraps the user supplied formatter in a rescue block and returns the original name if a StandardError is raised by the formatter
+        def safe_span_name_for(span_name_formatter, name)
+          span_name_formatter&.call(name) || name
+        rescue StandardError => e
+          OpenTelemetry.handle_error(exception: e, message: 'Error calling span_name_formatter. Using default span name.')
+          name
+        end
       end
     end
   end

instrumentation/active_support/opentelemetry-instrumentation-active_support.gemspec

@@ -38,6 +38,7 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency 'pry-byebug'
   spec.add_development_dependency 'rails', '>= 6.1'
   spec.add_development_dependency 'rake', '~> 13.0'
+  spec.add_development_dependency 'rspec-mocks'
   spec.add_development_dependency 'rubocop', '~> 1.64.0'
   spec.add_development_dependency 'rubocop-performance', '~> 1.20'
   spec.add_development_dependency 'simplecov', '~> 0.17.1'

instrumentation/active_support/test/opentelemetry/instrumentation/active_support/span_subscriber_test.rb

@@ -12,9 +12,10 @@
   let(:exporter) { EXPORTER }
   let(:last_span) { exporter.finished_spans.last }
   let(:span_kind) { nil }
+  let(:notification_name) { 'bar.foo' }
   let(:subscriber) do
     OpenTelemetry::Instrumentation::ActiveSupport::SpanSubscriber.new(
-      name: 'bar.foo',
+      name: notification_name,
       tracer: tracer,
       kind: span_kind
     )
@@ -36,7 +37,7 @@ def finish(name, id, payload)
 
   it 'memoizes the span name' do
     span, = subscriber.start('oh.hai', 'abc', {})
-    _(span.name).must_equal('foo bar')
+    _(span.name).must_equal(notification_name)
   end
 
   it 'uses the provided tracer' do
@@ -115,7 +116,7 @@ def finish(name, id, payload)
   describe 'instrumentation option - disallowed_notification_payload_keys' do
     let(:subscriber) do
       OpenTelemetry::Instrumentation::ActiveSupport::SpanSubscriber.new(
-        name: 'bar.foo',
+        name: notification_name,
         tracer: tracer,
         notification_payload_transform: nil,
         disallowed_notification_payload_keys: [:foo]
@@ -153,7 +154,7 @@ def finish(name, id, payload)
     let(:transformer_proc) { ->(v) { v.transform_values { 'optimus prime' } } }
     let(:subscriber) do
       OpenTelemetry::Instrumentation::ActiveSupport::SpanSubscriber.new(
-        name: 'bar.foo',
+        name: notification_name,
         tracer: tracer,
         notification_payload_transform: transformer_proc,
         disallowed_notification_payload_keys: [:foo]
@@ -205,58 +206,109 @@ def finish(name, id, payload)
 
   describe 'instrument' do
     before do
-      ActiveSupport::Notifications.unsubscribe('bar.foo')
+      ActiveSupport::Notifications.unsubscribe(notification_name)
     end
 
     it 'does not trace an event by default' do
-      ActiveSupport::Notifications.subscribe('bar.foo') do
+      ActiveSupport::Notifications.subscribe(notification_name) do
         # pass
       end
-      ActiveSupport::Notifications.instrument('bar.foo', extra: 'context')
+      ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
       _(last_span).must_be_nil
     end
 
     it 'traces an event when a span subscriber is used' do
-      OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, 'bar.foo')
-      ActiveSupport::Notifications.instrument('bar.foo', extra: 'context')
+      OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_name)
+      ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
 
       _(last_span).wont_be_nil
-      _(last_span.name).must_equal('foo bar')
+      _(last_span.name).must_equal(notification_name)
       _(last_span.attributes['extra']).must_equal('context')
       _(last_span.kind).must_equal(:internal)
     end
 
+    describe 'when using a custom span name formatter' do
+      describe 'when using the LEGACY_NAME_FORMATTER' do
+        let(:span_name_formatter) { OpenTelemetry::Instrumentation::ActiveSupport::LEGACY_NAME_FORMATTER }
+        it 'uses the user supplied formatter' do
+          OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_name, nil, nil, span_name_formatter: span_name_formatter)
+          ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
+
+          _(last_span).wont_be_nil
+          _(last_span.name).must_equal('foo bar')
+          _(last_span.attributes['extra']).must_equal('context')
+        end
+      end
+
+      describe 'when using a custom formatter' do
+        let(:span_name_formatter) { ->(name) { "custom.#{name}" } }
+
+        it 'uses the user supplied formatter' do
+          OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_name, nil, nil, span_name_formatter: span_name_formatter)
+          ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
+
+          _(last_span).wont_be_nil
+          _(last_span.name).must_equal('custom.bar.foo')
+          _(last_span.attributes['extra']).must_equal('context')
+        end
+      end
+
+      describe 'when using a invalid formatter' do
+        it 'defaults to the notification name' do
+          OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_name, nil, nil, span_name_formatter: ->(_) {})
+          ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
+
+          _(last_span).wont_be_nil
+          _(last_span.name).must_equal(notification_name)
+          _(last_span.attributes['extra']).must_equal('context')
+        end
+      end
+
+      describe 'when using a unstable formatter' do
+        it 'defaults to the notification name' do
+          allow(OpenTelemetry).to receive(:handle_error).with(exception: RuntimeError, message: String)
+
+          OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_name, nil, nil, span_name_formatter: ->(_) { raise 'boom' })
+          ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
+
+          _(last_span).wont_be_nil
+          _(last_span.name).must_equal(notification_name)
+          _(last_span.attributes['extra']).must_equal('context')
+        end
+      end
+    end
+
     it 'finishes spans even when block subscribers blow up' do
-      ActiveSupport::Notifications.subscribe('bar.foo') { raise 'boom' }
-      OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, 'bar.foo')
+      ActiveSupport::Notifications.subscribe(notification_name) { raise 'boom' }
+      OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_name)
 
       expect do
-        ActiveSupport::Notifications.instrument('bar.foo', extra: 'context')
+        ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
       end.must_raise RuntimeError
 
       _(last_span).wont_be_nil
-      _(last_span.name).must_equal('foo bar')
+      _(last_span.name).must_equal(notification_name)
       _(last_span.attributes['extra']).must_equal('context')
     end
 
     it 'finishes spans even when complex subscribers blow up' do
-      ActiveSupport::Notifications.subscribe('bar.foo', CrashingEndSubscriber.new)
-      OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, 'bar.foo')
+      ActiveSupport::Notifications.subscribe(notification_name, CrashingEndSubscriber.new)
+      OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_name)
 
       expect do
-        ActiveSupport::Notifications.instrument('bar.foo', extra: 'context')
+        ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
       end.must_raise RuntimeError
 
       _(last_span).wont_be_nil
-      _(last_span.name).must_equal('foo bar')
+      _(last_span.name).must_equal(notification_name)
       _(last_span.attributes['extra']).must_equal('context')
     end
 
     it 'supports unsubscribe' do
-      obj = OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, 'bar.foo')
+      obj = OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_name)
       ActiveSupport::Notifications.unsubscribe(obj)
 
-      ActiveSupport::Notifications.instrument('bar.foo', extra: 'context')
+      ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
 
       _(obj.class).must_equal(ActiveSupport::Notifications::Fanout::Subscribers::Evented)
       _(last_span).must_be_nil
@@ -267,7 +319,7 @@ def finish(name, id, payload)
       ActiveSupport::Notifications.instrument('bar.foo', extra: 'context')
 
       _(last_span).wont_be_nil
-      _(last_span.name).must_equal('foo bar')
+      _(last_span.name).must_equal('bar.foo')
       _(last_span.attributes['extra']).must_equal('context')
       _(last_span.kind).must_equal(:client)
     end

instrumentation/active_support/test/test_helper.rb

@@ -12,6 +12,7 @@
 require 'opentelemetry-instrumentation-active_support'
 
 require 'minitest/autorun'
+require 'rspec/mocks/minitest_integration'
 require 'webmock/minitest'
 
 # global opentelemetry-sdk setup: