Adding custom SpanProcessors considered impossible without private method calls

[0robustus1]

We've recently looked into adding a custom SpanProcessor to most of our applications. We need integrate with a particular vendor that doesn't fully/truly support OpenTelemetry specifications and relies on setting custom attributes (e.g. operation.name, resource.name and even adjusting the Span Kind (to avoid "internal")). The SpanProcessor itself is easily written, but adding it to an otherwise default setup of OpenTelemetry in Ruby is not:

require 'rake'
require 'opentelemetry/sdk'
require 'opentelemetry/exporter/otlp'
require 'opentelemetry/instrumentation/all'

class RakeTaskSpanProcessor < ::OpenTelemetry::SDK::Trace::SpanProcessor
  def on_start(span, _parent_context)
    return unless span.name =~ /^rake\.(invoke|execute)/

    task = span.attributes['rake.task']

    span.set_attribute('operation.name', span.name)
    span.set_attribute('resource.name', task || 'unknown')
  end
end

::OpenTelemetry::SDK.configure do |c|
  c.add_span_processor(RakeTaskSpanProcessor.new)
  c.use_all
end

desc 'Dummy task'
task :dummy do
  puts 'foo'
end

# write to a file: Rakefile.custom
# run: env OTEL_RESOURCE_ATTRIBUTES="service.namespace=rake-tester,service.name=test" rake -f Rakefile.custom dummy

Adding a SpanProcessor in this way will result in no exporting SpanProcessors being added. This seems to be due to https://github.com/open-telemetry/opentelemetry-ruby/blob/opentelemetry-sdk/v1.9.0/sdk/lib/opentelemetry/sdk/configurator.rb#L180-L183 which only adds the default config from the environment if no other SpanProcessors are configured. That seems to be due to Exporters being implemented as SpanProcessors themselves with no other clearly identifiable markers (e.g. a shared parent class or a specific attribute). So while it is unfortunate that just "adding" a SpanProcessor breaks default behaviour it does seem consistent.

However, there is no built in way for somebody who adds a SpanProcessor to opt in to also getting the default SpanProcessors (exporters), because wrapped_exporters_from_env is a private method.

In our testing the following setup works as intended:

require 'rake'
require 'opentelemetry/sdk'
require 'opentelemetry/exporter/otlp'
require 'opentelemetry/instrumentation/all'

class RakeTaskSpanProcessor < ::OpenTelemetry::SDK::Trace::SpanProcessor
  def on_start(span, _parent_context)
    return unless span.name =~ /^rake\.(invoke|execute)/

    task = span.attributes['rake.task']

    span.set_attribute('operation.name', span.name)
    span.set_attribute('resource.name', task || 'unknown')
  end
end

::OpenTelemetry::SDK.configure do |c|
  c.add_span_processor(RakeTaskSpanProcessor.new)
  c.send(:wrapped_exporters_from_env).compact.each { |p| c.add_span_processor(p) }
  c.use_all
end

desc 'Dummy task'
task :dummy do
  puts 'foo'
end

But it requires calling a private method (c.send(:wrapped_exporters_from_env).compact.each { |p| c.add_span_processor(p) }), which is inherently undesirable.

Instead it would be preferable if obtaining the environment based default exporters would be a documented method on the public interface or if there would be a method to add span processors without deactivating the default behaviour.

[robbkidd]

Yep. I've run into this frustration, too. In applications where I have added a custom span processor, I opted to also add a batch span processor with my desired exporter there in the configure block. Instantiating those in code will have them read the rest of their configuration from environment variables.

::OpenTelemetry::SDK.configure do |c|
  c.add_span_processor(RakeTaskSpanProcessor.new)
  c.add_span_processor(
      OpenTelemetry::SDK::Trace::Export::BatchSpanProcessor.new(
        OpenTelemetry::Exporter::OTLP::Exporter.new()
      )
    )
  c.use_all
end

… which I agree is A Bit Much™.

I wonder if a maintainer from the days when configure_span_processors() was written can talk me out of opening a PR to make that concatenate any processors returned from wrapped_exporters_from_env() to the ones that may have been added during SDK.configure(). I'm picturing that as a solution that does not require application developers know that they need to call something else after adding span processors in code. Would that achieve your desired goal, @0robustus1?

Update: upon a moment's further reflection, I think changing the behavior of configure_span_processors() to concatenate would be a breaking/surprising change to everyone dealing with the current behavior by adding a BSP+exporter or backdoor-calling wrapped_exporters_from_env(). They would end up with duplicate exporters. 🤔

[0robustus1]

@robbkidd Yes I agree optimally the behaviour would've been to concatenate in the first place, especially considering the naming of the method add_span_processor(). But by now it would be a breaking change. Hence my suggestion to either opt for an explicit way to add those default processors (that is part of the API) or to add a new prepend_span_processor() method (naming not final 😆 ) that would actually include the concatentation behaviour.

Regarding your suggestion to use OpenTelemetry::SDK::Trace::Export::BatchSpanProcessor.new(OpenTelemetry::Exporter::OTLP::Exporter.new()): I wasn't aware that this defaulted to also pulling config from the environment variables. It might be a good workaround unless one relies on the ability of wrapped_exporters_from_env() to return multiple processors/exporters.

[dmathieu]

Two alternatives could be:

Introduce a OpenTelemetry::SDK::DefaultProcessors array that returns all the default processors.

Then, you could do something along the lines of:

::OpenTelemetry::SDK.configure do |c|
  OpenTelemetry::SDK::DefaultProcessors.each do |sp|
    c.add_span_processor(sp.new)
  end
  c.add_span_processor(RakeTaskSpanProcessor.new)
  c.use_all
end

Introduce a c.auto_detect_processors

Then, you could do something along the lines of:

::OpenTelemetry::SDK.configure do |c|
  c.auto_detect_processors
  c.add_span_processor(RakeTaskSpanProcessor.new)
  c.use_all
end

Neither are breaking changes, and as far as I can see, they should both be spec-compliant.

[robbkidd]

Introduce a c.auto_detect_processors

🤔 I'm cautiously optimistic about this idea (naming negotiable). It would probably look an awful lot like the private-send workaround.

def auto_detect_processors
  wrapped_exporters_from_env.compact.each { |p| tracer_provider.add_span_processor(p) }
end

Also, maybe some way to communicate a warning that in a future major version we'll change the internal behavior to concatenate so that the user does not have to explicitly call an "auto" detection.

[dmathieu]

Looking again at the configuration specification, there is definitely no mention of manual configuration preventing the additional use of configuration variables. So while it's a breaking change, we could change the internal behavior to concatenate both configuration styles.

[github-actions[bot]]

👋 This issue 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 keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.