feat: Enable support for `Regexp` patterns when subscribing to Active Support's instrumentation Events #1296

erichmachado · 2024-12-09T23:30:29Z

This PR aims to introduce support for Regexp patterns to the OpenTelemetry::Instrumentation::ActiveSupport.subscribe method.

This should be in line with a similar feature available on ActiveSupport::Notifications.subscribe.

With this update, users will be able to:

Subscribe to multiple events at once, if desired.
Use the Regexp pattern directly without having to resort to a custom span_name_formatter to work around crashes when processing the event name.
Process the incoming event name if span_name_formatter is set, since the name parameter in the initializer includes a reference to the Regexp pattern in this case.

…'s instrumentation Events

linux-foundation-easycla · 2024-12-09T23:30:33Z

The committers listed above are authorized under a signed CLA.

✅ login: erichmachado / name: Erich Soares Machado (b8c0fca, b1d707f, 835edc3, ae89d18, ed978b2, f541dfb)
✅ login: arielvalentin / name: Ariel Valentin (95223a8)

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

erichmachado · 2024-12-09T23:40:03Z

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

+      ActiveSupport::Notifications.notifier.all_listeners_for(notification_name).each do |listener|
+        ActiveSupport::Notifications.unsubscribe(listener)
+      end


This prevents subscriptions from leaking across tests. Just using ActiveSupport::Notifications.unsubscribe with a Regexp won't work as intended.

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

arielvalentin · 2024-12-10T00:54:49Z

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

@@ -137,11 +138,20 @@ def sanitized_value(value)
          end
        end

+        def span_name(name)


I see what you meant now. Duping the string will now result in a separate string allocation and an additional case statement per subscription call. The difference in use between the regexp matcher and string based subscriber is only for span names right?

I think maybe it would be best in this case then to use the span name found on the payload and then keep the name instance variable as more as a indexing value that we can use to unsubscribe registrations or inspect the object to know what it's subscribed to but not use it for span naming. Does that help?

Thank you for your review @arielvalentin!

I think maybe it would be best in this case then to use the span name found on the payload and then keep the name instance variable as more as a indexing value that we can use to unsubscribe registrations or inspect the object to know what it's subscribed to but not use it for span naming. Does that help?

It does help. It's also a behavioural change, meaning this test will break:

opentelemetry-ruby-contrib/instrumentation/active_support/test/opentelemetry/instrumentation/active_support/span_subscriber_test.rb

Lines 38 to 41 in 7bda424

it 'memoizes the span name' do

span, = subscriber.start('oh.hai', 'abc', {})

_(span.name).must_equal(notification_name)

end

Which is the behaviour I was trying to preserve there. In this case, would it make sense to just remove that test?

I think that should be ok.

That test is a bit strange because I was guarding against the very unlikely bug that AS Notifications to invoke the subscriber with an event that it was not subscribed to.

You can probably delete it.

Thank you! I've updated that on b8c0fca. Can you please check if this is what you were looking for?

erichmachado · 2024-12-10T23:41:46Z

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

+      it 'finishes spans even when block subscribers blow up' do
+        # This scenario cannot be exercised reliably on Active Support < 7.0 since the #finish method
+        # will never be called by the notifier if another subscriber raises an error.
+        #
+        # See this PR for additional details: https://github.com/rails/rails/pull/43282
+        skip 'Notifications will be broken in this scenario on Active Support < 7.0' if ActiveSupport.version < Gem::Version.new("7.0")
+
+        ActiveSupport::Notifications.subscribe(notification_pattern) { raise 'boom' }
+        OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_pattern)
+
+        expect do
+          ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
+        end.must_raise RuntimeError
+
+        _(last_span).wont_be_nil
+        _(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
+        # This scenario cannot be exercised reliably on Active Support < 7.0 since the #finish method
+        # will never be called by the notifier if another subscriber raises an error.
+        #
+        # See this PR for additional details: https://github.com/rails/rails/pull/43282
+        skip 'Notifications will be broken in this scenario on Active Support < 7.0' if ActiveSupport.version < Gem::Version.new("7.0")
+
+        ActiveSupport::Notifications.subscribe(notification_pattern, CrashingEndSubscriber.new)
+        OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_pattern)
+
+        expect do
+          ActiveSupport::Notifications.instrument(notification_name, extra: 'context')
+        end.must_raise RuntimeError
+
+        _(last_span).wont_be_nil
+        _(last_span.name).must_equal(notification_name)
+        _(last_span.attributes['extra']).must_equal('context')
+      end


I had to skip these two tests on Active Support 6.1 since there's a bug in this version that prevents the subscriber from properly finishing the spans if another subscriber raises an error.

See rails/rails#43282 for further details.

…p-subscription

arielvalentin · 2024-12-12T15:44:49Z

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

@@ -65,16 +65,17 @@ class SpanSubscriber

        # 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
+          @name = name


Please rename this attribute to pattern now that it will match the argument passed into AS notification subscribe. This will help us disambiguate the shadowed name parameter passed into the start and finish methods as well.

https://api.rubyonrails.org/classes/ActiveSupport/Notifications.html#method-c-subscribe

I've updated the attribute name to @pattern on ae89d18 @arielvalentin, as requested.

I didn't touch the name parameter to avoid disrupting the interface. Let me know if you want this to be renamed as well.

Yes please. This Subscriber is part of the internal API of this gem and i don't think it is used externally at the moment.

Thank you for confirming. It should be fixed on ed978b2.

arielvalentin · 2024-12-12T15:48:30Z

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

+          it 'uses the user supplied formatter' do
+            OpenTelemetry::Instrumentation::ActiveSupport.subscribe(tracer, notification_pattern, nil, nil, span_name_formatter: span_name_formatter)
+            ActiveSupport::Notifications.instrument(notification_name, extra: 'context')


Please confirm that the indentation changes are using spaces and not tabs. There are a few other sections where it looks like indentation changed and I want to make sure that you are using spaces there as well.

I've double checked it here and it's all spaces. 👍

…ber name parameter to pattern

Enable support for Regexp patterns when subscribing to Active Support…

b1d707f

…'s instrumentation Events

erichmachado commented Dec 9, 2024

View reviewed changes

...ion/active_support/test/opentelemetry/instrumentation/active_support/span_subscriber_test.rb Show resolved Hide resolved

erichmachado commented Dec 9, 2024

View reviewed changes

...ion/active_support/test/opentelemetry/instrumentation/active_support/span_subscriber_test.rb Show resolved Hide resolved

arielvalentin reviewed Dec 10, 2024

View reviewed changes

Stop memoizing the span name

b8c0fca

erichmachado force-pushed the em-feat-active-support-instrumentation-regexp-subscription branch 2 times, most recently from 6cf922f to 9791e56 Compare December 10, 2024 23:35

Ensure that tests pass on Active Support 6.1

835edc3

erichmachado force-pushed the em-feat-active-support-instrumentation-regexp-subscription branch from 9791e56 to 835edc3 Compare December 10, 2024 23:40

erichmachado commented Dec 10, 2024

View reviewed changes

Fix RuboCop offenses

f541dfb

erichmachado marked this pull request as ready for review December 11, 2024 19:31

erichmachado requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, ahayworth, plantfansam, robbkidd, simi, kaylareopelle, xuan-cao-swi and a team as code owners December 11, 2024 19:31

Merge branch 'main' into em-feat-active-support-instrumentation-regex…

95223a8

…p-subscription

arielvalentin requested changes Dec 12, 2024

View reviewed changes

Rename @name to @pattern to match the Active Support nomenclature

ae89d18

erichmachado requested a review from arielvalentin December 12, 2024 21:01

Rename the OpenTelemetry::Instrumentation::ActiveSupport::SpanSubscri…

ed978b2

…ber name parameter to pattern

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable support for `Regexp` patterns when subscribing to Active Support's instrumentation Events #1296

feat: Enable support for `Regexp` patterns when subscribing to Active Support's instrumentation Events #1296

erichmachado commented Dec 9, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 9, 2024 •

edited

Loading

erichmachado Dec 9, 2024

arielvalentin Dec 10, 2024

erichmachado Dec 10, 2024

arielvalentin Dec 10, 2024

erichmachado Dec 10, 2024

erichmachado Dec 10, 2024

arielvalentin Dec 12, 2024

erichmachado Dec 12, 2024

arielvalentin Dec 13, 2024

erichmachado Dec 13, 2024

arielvalentin Dec 12, 2024

erichmachado Dec 12, 2024

	it 'memoizes the span name' do
	span, = subscriber.start('oh.hai', 'abc', {})
	_(span.name).must_equal(notification_name)
	end

feat: Enable support for Regexp patterns when subscribing to Active Support's instrumentation Events #1296

Are you sure you want to change the base?

feat: Enable support for Regexp patterns when subscribing to Active Support's instrumentation Events #1296

Conversation

erichmachado commented Dec 9, 2024 • edited Loading

linux-foundation-easycla bot commented Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Enable support for `Regexp` patterns when subscribing to Active Support's instrumentation Events #1296

feat: Enable support for `Regexp` patterns when subscribing to Active Support's instrumentation Events #1296

erichmachado commented Dec 9, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 9, 2024 •

edited

Loading