-
Notifications
You must be signed in to change notification settings - Fork 172
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
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' | ||||||
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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
# 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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 |
---|---|---|
@@ -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 |
---|---|---|
|
@@ -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 | ||
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
|
@@ -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' | ||
|
There was a problem hiding this comment.
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?