From 1117d8e2eac11fca47cfe25b643f8e7f41a0eb57 Mon Sep 17 00:00:00 2001 From: Ariel Valentin Date: Tue, 27 Dec 2022 12:10:21 -0600 Subject: [PATCH] fix: Drop Rails dependency for ActiveSupport Instrumentation This makes it possible to use this instrumentation in non-Rails applications that leverage AS::Notifications. In addition to that it mitigates a race condition where a subscription may be registered by a separate thread before obtaining a lock on the notifications object. See https://github.com/open-telemetry/opentelemetry-ruby-contrib/issues/218 --- instrumentation/active_support/Appraisals | 19 +++++--------- .../active_support/span_subscriber.rb | 25 +++++++++++++++---- ...try-instrumentation-active_support.gemspec | 2 +- .../active_support/span_subscriber_test.rb | 2 +- .../active_support/test/test_helper.rb | 9 +++---- 5 files changed, 31 insertions(+), 26 deletions(-) diff --git a/instrumentation/active_support/Appraisals b/instrumentation/active_support/Appraisals index d358e21c82..c4e1779817 100644 --- a/instrumentation/active_support/Appraisals +++ b/instrumentation/active_support/Appraisals @@ -4,23 +4,16 @@ # # SPDX-License-Identifier: Apache-2.0 -# Rails 5.2 is incompatible with Ruby 3. -if RUBY_VERSION < '3' - appraise 'rails-5.2' do - gem 'rails', '~> 5.2.0' - end -end - -appraise 'rails-6.0' do - gem 'rails', '~> 6.0.0' +appraise 'activesupport-6.0' do + gem 'activesupport', '~> 6.0.0' end -appraise 'rails-6.1' do - gem 'rails', '~> 6.1.0' +appraise 'activesupport-6.1' do + gem 'activesupport', '~> 6.1.0' end if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0') - appraise 'rails-7.0' do - gem 'rails', '~> 7.0.0' + appraise 'activesupport-7.0' do + gem 'activesupport', '~> 7.0.0' end end diff --git a/instrumentation/active_support/lib/opentelemetry/instrumentation/active_support/span_subscriber.rb b/instrumentation/active_support/lib/opentelemetry/instrumentation/active_support/span_subscriber.rb index be986f9978..0952c19471 100644 --- a/instrumentation/active_support/lib/opentelemetry/instrumentation/active_support/span_subscriber.rb +++ b/instrumentation/active_support/lib/opentelemetry/instrumentation/active_support/span_subscriber.rb @@ -8,12 +8,15 @@ module OpenTelemetry module Instrumentation # rubocop:disable Style/Documentation module ActiveSupport + NOTIFIER_MAJOR_VERSION_6 = Gem::Version.new("6").canonical_segments.first + # The SpanSubscriber is a special ActiveSupport::Notification subscription # handler which turns notifications into generic spans, taking care to handle # context appropriately. # A very hacky way to make sure that OpenTelemetry::Instrumentation::ActiveSupport::SpanSubscriber # gets invoked first + # Rails 6+ https://github.com/rails/rails/blob/0f0ec9908e25af36df2d937dc431f626a4102b3d/activesupport/lib/active_support/notifications/fanout.rb#L51 # def self.subscribe( tracer, @@ -29,13 +32,25 @@ def self.subscribe( ) subscriber_object = ::ActiveSupport::Notifications.subscribe(pattern, subscriber) + active_support_major_version = ::ActiveSupport.version.canonical_segments.first + ::ActiveSupport::Notifications.notifier.synchronize do - if ::Rails::VERSION::MAJOR >= 6 - s = ::ActiveSupport::Notifications.notifier.instance_variable_get(:@string_subscribers)[pattern].pop - ::ActiveSupport::Notifications.notifier.instance_variable_get(:@string_subscribers)[pattern].unshift(s) + if active_support_major_version >= NOTIFIER_MAJOR_VERSION_6 + subscribers = ::ActiveSupport::Notifications.notifier.instance_variable_get(:@string_subscribers)[:pattern] + else + subscribers = ::ActiveSupport::Notifications.notifier.instance_variable_get(:@subscribers) + end + + if subscribers.nil? + OpenTelemetry.handle_error( + message: "Unable to move OTEL ActiveSupport Notifications subscriber to the front of the notifications list which may cause incomplete traces." + + "Please report an issue here: " + + "https://github.com/open-telemetry/opentelemetry-ruby-contrib/issues/new?labels=bug&template=bug_report.md&title=ActiveSupport%20Notifications%20subscribers%20list%20is%20nil" + ) else - s = ::ActiveSupport::Notifications.notifier.instance_variable_get(:@subscribers).pop - ::ActiveSupport::Notifications.notifier.instance_variable_get(:@subscribers).unshift(s) + subscribers.unshift( + subscribers.delete(subscriber_object) + ) end end subscriber_object diff --git a/instrumentation/active_support/opentelemetry-instrumentation-active_support.gemspec b/instrumentation/active_support/opentelemetry-instrumentation-active_support.gemspec index f0793c6871..ed9ffecb5d 100644 --- a/instrumentation/active_support/opentelemetry-instrumentation-active_support.gemspec +++ b/instrumentation/active_support/opentelemetry-instrumentation-active_support.gemspec @@ -28,6 +28,7 @@ Gem::Specification.new do |spec| spec.add_dependency 'opentelemetry-api', '~> 1.0' spec.add_dependency 'opentelemetry-instrumentation-base', '~> 0.21.0' + spec.add_development_dependency 'activesupport' spec.add_development_dependency 'appraisal', '~> 2.2.0' spec.add_development_dependency 'bundler', '>= 1.17' spec.add_development_dependency 'minitest', '~> 5.0' @@ -35,7 +36,6 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'opentelemetry-test-helpers' spec.add_development_dependency 'pry' spec.add_development_dependency 'pry-byebug' - spec.add_development_dependency 'rails' spec.add_development_dependency 'rake', '~> 12.3.3' spec.add_development_dependency 'rubocop', '~> 1.3.0' spec.add_development_dependency 'simplecov', '~> 0.17.1' diff --git a/instrumentation/active_support/test/opentelemetry/instrumentation/active_support/span_subscriber_test.rb b/instrumentation/active_support/test/opentelemetry/instrumentation/active_support/span_subscriber_test.rb index 91bd842a46..c0eb97f670 100644 --- a/instrumentation/active_support/test/opentelemetry/instrumentation/active_support/span_subscriber_test.rb +++ b/instrumentation/active_support/test/opentelemetry/instrumentation/active_support/span_subscriber_test.rb @@ -6,7 +6,7 @@ require 'test_helper' -describe OpenTelemetry::Instrumentation::ActiveSupport::SpanSubscriber do +describe 'OpenTelemetry::Instrumentation::ActiveSupport::SpanSubscriber' do let(:instrumentation) { OpenTelemetry::Instrumentation::ActiveSupport::Instrumentation.instance } let(:tracer) { instrumentation.tracer } let(:exporter) { EXPORTER } diff --git a/instrumentation/active_support/test/test_helper.rb b/instrumentation/active_support/test/test_helper.rb index d9bd11ae78..6f6bc0248a 100644 --- a/instrumentation/active_support/test/test_helper.rb +++ b/instrumentation/active_support/test/test_helper.rb @@ -4,15 +4,12 @@ # # SPDX-License-Identifier: Apache-2.0 -require 'opentelemetry/sdk' -require 'opentelemetry-test-helpers' +require 'bundler/setup' +Bundler.require(:default, :development) require 'minitest/autorun' require 'webmock/minitest' -require 'pry' - -require 'rails' - +require 'active_support' require 'opentelemetry-instrumentation-active_support' # global opentelemetry-sdk setup: