Skip to content

Commit

Permalink
fix: Drop Rails dependency for ActiveSupport Instrumentation
Browse files Browse the repository at this point in the history
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 open-telemetry#218
  • Loading branch information
arielvalentin committed Dec 27, 2022
1 parent 99026b1 commit 3066879
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 22 deletions.
19 changes: 6 additions & 13 deletions instrumentation/active_support/Appraisals
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -30,12 +33,24 @@ def self.subscribe(

subscriber_object = ::ActiveSupport::Notifications.subscribe(pattern, subscriber)
::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)
active_support_major_version = ::ActiveSupport.version.canonical_segments.first

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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,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'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
2 changes: 0 additions & 2 deletions instrumentation/active_support/test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
require 'webmock/minitest'
require 'pry'

require 'rails'

require 'opentelemetry-instrumentation-active_support'

# global opentelemetry-sdk setup:
Expand Down

0 comments on commit 3066879

Please sign in to comment.