fix: use AS::N subscriber for serialize events #1075
Conversation
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.
robbkidd
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
simi,
kaylareopelle and
xuan-cao-swi
as code owners
| @@ -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.
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.
There was a problem hiding this comment.
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.
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.
|
|
||
| # 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.
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.
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?
...el_serializers/lib/opentelemetry/instrumentation/active_model_serializers/instrumentation.rb
Show resolved
Hide resolved
...el_serializers/lib/opentelemetry/instrumentation/active_model_serializers/instrumentation.rb
Show resolved
Hide resolved
...el_serializers/lib/opentelemetry/instrumentation/active_model_serializers/instrumentation.rb
Show resolved
Hide resolved
...tive_model_serializers/lib/opentelemetry/instrumentation/active_model_serializers/railtie.rb
Show resolved
Hide resolved
| gem 'opentelemetry-common' | ||
| gem 'opentelemetry-instrumentation-active_model_serializers', path: '../' | ||
| gem 'opentelemetry-sdk' | ||
| gem 'opentelemetry-exporter-otlp' |
There was a problem hiding this comment.
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 | ||
|
|
||
| # TraceRequestApp is a minimal Rails application inspired by the Rails | ||
| # bug report template for action controller. |
There was a problem hiding this comment.
| # bug report template for action controller. | |
| # bug report template for Action Controller. |
| end | ||
|
|
||
| # Rails app initialization will pick up the instrumentation Railtie | ||
| # and subscribe to ActiveSupport notifications |
There was a problem hiding this comment.
| # and subscribe to ActiveSupport notifications | |
| # and subscribe to Active Support notifications |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
|
@robbkidd mind taking a look at @kaylareopelle comments? |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
arielvalentin
added
keep
Resolves #992
This mimicks a fair amount of the current ActionMailer and ActionView instrumentation.
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.
TODO: