Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use AS::N subscriber for serialize events #1075

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions instrumentation/active_model_serializers/example/Gemfile

This file was deleted.

This file was deleted.

72 changes: 72 additions & 0 deletions instrumentation/active_model_serializers/example/rails_app.rb
Original file line number Diff line number Diff line change
@@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We often output example data to the console. What do you think about doing that here? Or maybe outputting to both OTLP and the console?

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# bug report template for action controller.
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# and subscribe to ActiveSupport notifications
# and subscribe to Active Support 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
55 changes: 55 additions & 0 deletions instrumentation/active_model_serializers/example/standalone.rb
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this extra needed step outside the configure block to get the instrumentation to Actually Instrument when Rails wasn't around to detect and call Railtie hooks. I couldn't get the SpanScriber to have a real, configured Tracer when instantiating the subscriber within the instrumentation install step. I see that ActionMailer and ActionView are relying entirely on a Railtie to subscribe at the end of Rails initialization. I figured I would add this interface for folks who may be using ActiveModelSerializers outside of a full Rails app.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you mean.

Want to do some sort of hacky thing that checks if Rails is defined and use the Railtie, otherwise manually install?


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

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
arielvalentin marked this conversation as resolved.
Show resolved Hide resolved
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'
robbkidd marked this conversation as resolved.
Show resolved Hide resolved
end

def event_name
'render.active_model_serializers'
def default_attribute_transformer
lambda { |payload|
arielvalentin marked this conversation as resolved.
Show resolved Hide resolved
{
'serializer.name' => payload[:serializer].name,
'serializer.renderer' => 'active_model_serializers',
'serializer.format' => payload[:adapter]&.class&.name || 'default'
}
}
end
end
end
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
arielvalentin marked this conversation as resolved.
Show resolved Hide resolved
# 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
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,17 @@
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 }
let(:model) { TestHelper::Model.new(name: 'test object') }

before do
instrumentation.install
instrumentation.subscribe
exporter.reset

# this is currently a noop but this will future proof the test
Expand All @@ -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'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One disadvantage of using the current OTel AS::N SpanSubscriber is that the span_name_formatter callable only receives the notification event name as a parameter. It has no access to the event payload to generate a span name like previously.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to change that about the OTel ASN gem?

What about writing your own span subscriber?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would very much like to change that about the subscriber in the AS::N instrumentation.

We chatted in CNCF Slack briefly around the idea of the OTel AS::N subscriber taking a block that receives a mutable span and the AS::N event payload. The instrumentation developer can perform whatever name and attribute hijinks they desire. (I propose we add to the documentation of this potential future interface a warning that the behavior, performance, and exception handling of that block is entirely the responsibility of the block's author.)

I can picture (vaguely) that any of the instrumentation we write that leverages this block-accepting AS::N subscriber could themselves accept a block to allow end-user customization.

_(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'
Expand All @@ -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'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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