From 13f2b7fe08a313a9eb238b8a96e3dff902471339 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Fri, 19 Jul 2024 11:38:53 -0400 Subject: [PATCH] fix: use AS::N subscriber for serialize events The details for Context management (i.e. setting current span) are already handled by the OTel ActiveSupport instrumentation. Reuse the notifications subscriber here for ActiveModel serialization events. Reworked the example app into two: one Rails which works with the usual SDK configuration and one standalone (no Rails) to demonstrate that the subscription needs to be made after the SDK configuration is complete. If the subscription is created during instrumentation install, the subscription's tracer will be a NO-OP API tracer and won't produce spans. --- .../active_model_serializers/example/Gemfile | 9 --- .../example/active_model_serializers.rb | 26 ------- .../example/rails_app.rb | 72 +++++++++++++++++++ .../example/standalone.rb | 55 ++++++++++++++ .../active_model_serializers/event_handler.rb | 48 ------------- .../instrumentation.rb | 41 ++++++++--- .../active_model_serializers/railtie.rb | 24 +++++++ ...mentation-active_model_serializers.gemspec | 1 + ...andler_test.rb => instrumentation_test.rb} | 9 +-- .../active_model_serializers_test.rb | 8 ++- 10 files changed, 195 insertions(+), 98 deletions(-) delete mode 100644 instrumentation/active_model_serializers/example/Gemfile delete mode 100644 instrumentation/active_model_serializers/example/active_model_serializers.rb create mode 100644 instrumentation/active_model_serializers/example/rails_app.rb create mode 100644 instrumentation/active_model_serializers/example/standalone.rb delete mode 100644 instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/event_handler.rb create mode 100644 instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/railtie.rb rename instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers/{event_handler_test.rb => instrumentation_test.rb} (91%) diff --git a/instrumentation/active_model_serializers/example/Gemfile b/instrumentation/active_model_serializers/example/Gemfile deleted file mode 100644 index f2e069165..000000000 --- a/instrumentation/active_model_serializers/example/Gemfile +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -source 'https://rubygems.org' - -gem 'active_model_serializers' -gem 'opentelemetry-api' -gem 'opentelemetry-common' -gem 'opentelemetry-instrumentation-active_model_serializers' -gem 'opentelemetry-sdk' diff --git a/instrumentation/active_model_serializers/example/active_model_serializers.rb b/instrumentation/active_model_serializers/example/active_model_serializers.rb deleted file mode 100644 index d145f240d..000000000 --- a/instrumentation/active_model_serializers/example/active_model_serializers.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -# Copyright The OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -require 'rubygems' -require 'bundler/setup' - -Bundler.require - -OpenTelemetry::SDK.configure do |c| - c.use 'OpenTelemetry::Instrumentation::ActiveModelSerializers' -end - -class TestModel < ActiveModelSerializers::Model #:nodoc: - attr_accessor :name -end - -class TestModelSerializer < ActiveModel::Serializer #:nodoc: - attributes :name -end - -model = TestModel.new(name: 'test object') - -ActiveModelSerializers::SerializableResource.new(model).serializable_hash diff --git a/instrumentation/active_model_serializers/example/rails_app.rb b/instrumentation/active_model_serializers/example/rails_app.rb new file mode 100644 index 000000000..7962501ae --- /dev/null +++ b/instrumentation/active_model_serializers/example/rails_app.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +# Copyright The OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'bundler/inline' + +gemfile(true) do + source 'https://rubygems.org' + + gem 'rails' + gem 'active_model_serializers' + gem 'opentelemetry-api' + gem 'opentelemetry-common' + gem 'opentelemetry-instrumentation-active_model_serializers', path: '../' + gem 'opentelemetry-sdk' + gem 'opentelemetry-exporter-otlp' +end + +OpenTelemetry::SDK.configure do |c| + c.service_name = 'active_model_serializers_example' + c.use 'OpenTelemetry::Instrumentation::ActiveModelSerializers' +end + +# no manual subscription trigger + +at_exit do + OpenTelemetry.tracer_provider.shutdown +end + +# TraceRequestApp is a minimal Rails application inspired by the Rails +# bug report template for action controller. +# The configuration is compatible with Rails 6.0 +class TraceRequestApp < Rails::Application + config.root = __dir__ + config.hosts << 'example.org' + credentials.secret_key_base = 'secret_key_base' + + config.eager_load = false + + config.logger = Logger.new($stdout) + Rails.logger = config.logger +end + +# Rails app initialization will pick up the instrumentation Railtie +# and subscribe to ActiveSupport notifications +TraceRequestApp.initialize! + +ExampleAppTracer = OpenTelemetry.tracer_provider.tracer('example_app') + +class TestModel + include ActiveModel::API + include ActiveModel::Serialization + + attr_accessor :name + + def attributes + { 'name' => nil, + 'screaming_name' => nil } + end + + def screaming_name + ExampleAppTracer.in_span('screaming_name transform') do |span| + name.upcase + end + end +end + +model = TestModel.new(name: 'test object') + +puts ActiveModelSerializers::SerializableResource.new(model).serializable_hash diff --git a/instrumentation/active_model_serializers/example/standalone.rb b/instrumentation/active_model_serializers/example/standalone.rb new file mode 100644 index 000000000..a5ca64354 --- /dev/null +++ b/instrumentation/active_model_serializers/example/standalone.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +# Copyright The OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +require 'bundler/inline' + +gemfile(true) do + source 'https://rubygems.org' + + gem 'active_model_serializers' + gem 'opentelemetry-api' + gem 'opentelemetry-common' + gem 'opentelemetry-instrumentation-active_model_serializers', path: '../' + gem 'opentelemetry-sdk' + gem 'opentelemetry-exporter-otlp' +end + +OpenTelemetry::SDK.configure do |c| + c.service_name = 'active_model_serializers_example' + c.use_all +end + +# without Rails and the Railtie automation, must manually trigger +# instrumentation subscription after SDK is configured +OpenTelemetry::Instrumentation::ActiveModelSerializers.subscribe + +at_exit do + OpenTelemetry.tracer_provider.shutdown +end + +ExampleAppTracer = OpenTelemetry.tracer_provider.tracer('example_app') + +class TestModel + include ActiveModel::API + include ActiveModel::Serialization + + attr_accessor :name + + def attributes + { 'name' => nil, + 'screaming_name' => nil } + end + + def screaming_name + ExampleAppTracer.in_span('screaming_name transform') do |span| + name.upcase + end + end +end + +model = TestModel.new(name: 'test object') + +puts ActiveModelSerializers::SerializableResource.new(model).serializable_hash diff --git a/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/event_handler.rb b/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/event_handler.rb deleted file mode 100644 index 81bcfd9bb..000000000 --- a/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/event_handler.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -# Copyright The OpenTelemetry Authors -# -# SPDX-License-Identifier: Apache-2.0 - -module OpenTelemetry - module Instrumentation - module ActiveModelSerializers - # Event handler singleton for ActiveModelSerializers - module EventHandler - extend self - - def handle(start_timestamp, end_timestamp, payload) - tracer.start_span(span_name(payload), - start_timestamp: start_timestamp, - attributes: build_attributes(payload), - kind: :internal) - .finish(end_timestamp: end_timestamp) - end - - protected - - def span_name(payload) - "#{demodulize(payload[:serializer].name)} render" - end - - def build_attributes(payload) - { - 'serializer.name' => payload[:serializer].name, - 'serializer.renderer' => 'active_model_serializers', - 'serializer.format' => payload[:adapter]&.class&.name || 'default' - } - end - - def tracer - ActiveModelSerializers::Instrumentation.instance.tracer - end - - def demodulize(string) - string = string.to_s - i = string.rindex('::') - i ? string[(i + 2)..-1] : string - end - end - end - end -end diff --git a/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/instrumentation.rb b/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/instrumentation.rb index 453f9e092..8aa538860 100644 --- a/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/instrumentation.rb +++ b/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/instrumentation.rb @@ -4,16 +4,24 @@ # # SPDX-License-Identifier: Apache-2.0 +require 'opentelemetry-instrumentation-active_support' + module OpenTelemetry module Instrumentation module ActiveModelSerializers # Instrumentation class that detects and installs the ActiveModelSerializers instrumentation class Instrumentation < OpenTelemetry::Instrumentation::Base + # Minimum supported version of the `active_model_serializers` gem MINIMUM_VERSION = Gem::Version.new('0.10.0') + # ActiveSupport::Notification topics to which the instrumentation subscribes + SUBSCRIPTIONS = %w[ + render.active_model_serializers + ].freeze + install do |_config| + install_active_support_instrumenation require_dependencies - register_event_handler end present do @@ -24,24 +32,39 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base !defined?(::ActiveSupport::Notifications).nil? && gem_version >= MINIMUM_VERSION end + def subscribe + SUBSCRIPTIONS.each do |subscription_name| + OpenTelemetry.logger.debug("Subscribing to #{subscription_name} notifications with #{_tracer}") + OpenTelemetry::Instrumentation::ActiveSupport.subscribe(_tracer, subscription_name, default_attribute_transformer) + end + end + private + def _tracer + self.class.instance.tracer + end + def gem_version Gem::Version.new(::ActiveModel::Serializer::VERSION) end - def require_dependencies - require_relative 'event_handler' + def install_active_support_instrumenation + OpenTelemetry::Instrumentation::ActiveSupport::Instrumentation.instance.install({}) end - def register_event_handler - ::ActiveSupport::Notifications.subscribe(event_name) do |_name, start, finish, _id, payload| - EventHandler.handle(start, finish, payload) - end + def require_dependencies + require_relative 'railtie' end - def event_name - 'render.active_model_serializers' + def default_attribute_transformer + lambda { |payload| + { + 'serializer.name' => payload[:serializer].name, + 'serializer.renderer' => 'active_model_serializers', + 'serializer.format' => payload[:adapter]&.class&.name || 'default' + } + } end end end diff --git a/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/railtie.rb b/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/railtie.rb new file mode 100644 index 000000000..9c1482743 --- /dev/null +++ b/instrumentation/active_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/railtie.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +# Copyright The OpenTelemetry Authors +# +# SPDX-License-Identifier: Apache-2.0 + +module OpenTelemetry + module Instrumentation + module ActiveModelSerializers # :nodoc: + def self.subscribe + Instrumentation.instance.subscribe + end + + if defined?(::Rails::Railtie) + # This Railtie sets up subscriptions to relevant ActiveModelSerializers notifications + class Railtie < ::Rails::Railtie + config.after_initialize do + ::OpenTelemetry::Instrumentation::ActiveModelSerializers.subscribe + end + end + end + end + end +end diff --git a/instrumentation/active_model_serializers/opentelemetry-instrumentation-active_model_serializers.gemspec b/instrumentation/active_model_serializers/opentelemetry-instrumentation-active_model_serializers.gemspec index dd76de1fa..82ea47d8d 100644 --- a/instrumentation/active_model_serializers/opentelemetry-instrumentation-active_model_serializers.gemspec +++ b/instrumentation/active_model_serializers/opentelemetry-instrumentation-active_model_serializers.gemspec @@ -26,6 +26,7 @@ Gem::Specification.new do |spec| spec.required_ruby_version = '>= 3.0' spec.add_dependency 'opentelemetry-api', '~> 1.0' + spec.add_dependency 'opentelemetry-instrumentation-active_support', '>= 0.6.0' spec.add_dependency 'opentelemetry-instrumentation-base', '~> 0.22.1' spec.add_development_dependency 'active_model_serializers', '>= 0.10.0' diff --git a/instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers/event_handler_test.rb b/instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers/instrumentation_test.rb similarity index 91% rename from instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers/event_handler_test.rb rename to instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers/instrumentation_test.rb index d029af5d5..a33ab9158 100644 --- a/instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers/event_handler_test.rb +++ b/instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers/instrumentation_test.rb @@ -7,9 +7,9 @@ require_relative '../../../test_helper' # require instrumentation so we do not have to depend on the install hook being called -require_relative '../../../../lib/opentelemetry/instrumentation/active_model_serializers/event_handler' +require_relative '../../../../lib/opentelemetry/instrumentation/active_model_serializers/instrumentation' -describe OpenTelemetry::Instrumentation::ActiveModelSerializers::EventHandler do +describe OpenTelemetry::Instrumentation::ActiveModelSerializers::Instrumentation do let(:instrumentation) { OpenTelemetry::Instrumentation::ActiveModelSerializers::Instrumentation.instance } let(:exporter) { EXPORTER } let(:span) { exporter.finished_spans.first } @@ -17,6 +17,7 @@ before do instrumentation.install + instrumentation.subscribe exporter.reset # this is currently a noop but this will future proof the test @@ -38,7 +39,7 @@ _(exporter.finished_spans.size).must_equal 1 _(span).must_be_kind_of OpenTelemetry::SDK::Trace::SpanData - _(span.name).must_equal 'ModelSerializer render' + _(span.name).must_equal 'render.active_model_serializers' _(span.attributes['serializer.name']).must_equal 'TestHelper::ModelSerializer' _(span.attributes['serializer.renderer']).must_equal 'active_model_serializers' _(span.attributes['serializer.format']).must_equal 'ActiveModelSerializers::Adapter::Attributes' @@ -54,7 +55,7 @@ _(exporter.finished_spans.size).must_equal 1 _(span).must_be_kind_of OpenTelemetry::SDK::Trace::SpanData - _(span.name).must_equal 'ModelSerializer render' + _(span.name).must_equal 'render.active_model_serializers' _(span.attributes['serializer.name']).must_equal 'TestHelper::ModelSerializer' _(span.attributes['serializer.renderer']).must_equal 'active_model_serializers' _(span.attributes['serializer.format']).must_equal 'TestHelper::Model' diff --git a/instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers_test.rb b/instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers_test.rb index 36f97dfa6..80e5ef812 100644 --- a/instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers_test.rb +++ b/instrumentation/active_model_serializers/test/opentelemetry/instrumentation/active_model_serializers_test.rb @@ -37,11 +37,15 @@ end end - describe 'install' do + describe 'subscribe' do + before do + instrumentation.subscribe + end + it 'subscribes to ActiveSupport::Notifications' do subscriptions = ActiveSupport::Notifications.notifier.instance_variable_get(:@string_subscribers) subscriptions = subscriptions['render.active_model_serializers'] - assert(subscriptions.detect { |s| s.is_a?(ActiveSupport::Notifications::Fanout::Subscribers::Timed) }) + assert(subscriptions.detect { |s| s.is_a?(ActiveSupport::Notifications::Fanout::Subscribers::Evented) }) end end end