Reinstate the TracerContext constructor for single processor

[maxgolov]

Changes

Recent refactor removed the constructor for single processor, assuming that there is always a vector of processors #692 . However, the most common trivial scenario is when a customer provides a single processor rather than a vector of them. The intent of this change is to reinstate the previous constructor that got (inadvertently?) removed, keeping the simpler constructor in place where it was before.

No significant changes. Just reinstating what was accidentally removed.

Since the constructor is rather trivial (empty), implementation is included inline in the header, not added to separate .cc file. I do not have objection if there is an ask to move the empty body into separate compilation unit. But it seems okay to me as-is.

[codecov]

Codecov Report

Merging #814 (8bc0a3f) into main (5767f8e) will decrease coverage by 1.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #814      +/-   ##
==========================================
- Coverage   96.19%   95.15%   -1.03%     
==========================================
  Files         153      173      +20     
  Lines        6444     8449    +2005     
==========================================
+ Hits         6198     8039    +1841     
- Misses        246      410     +164     

Impacted Files	Coverage Δ
...pentelemetry/context/propagation/noop_propagator.h	60.00% <0.00%> (-40.00%)	⬇️
sdk/src/trace/span.cc	84.50% <0.00%> (-6.96%)	⬇️
api/test/trace/provider_test.cc	73.34% <0.00%> (-6.66%)	⬇️
...de/opentelemetry/trace/propagation/b3_propagator.h	94.03% <0.00%> (-2.46%)	⬇️
...ude/opentelemetry/common/key_value_iterable_view.h	86.67% <0.00%> (-2.22%)	⬇️
...lude/opentelemetry/sdk/trace/samplers/always_off.h	77.78% <0.00%> (-2.22%)	⬇️
sdk/test/trace/always_off_sampler_test.cc	84.62% <0.00%> (-2.05%)	⬇️
sdk/test/trace/always_on_sampler_test.cc	89.75% <0.00%> (-1.92%)	⬇️
...i/include/opentelemetry/trace/propagation/jaeger.h	96.50% <0.00%> (-1.06%)	⬇️
sdk/include/opentelemetry/sdk/trace/span_data.h	98.97% <0.00%> (-1.03%)	⬇️
... and 71 more

[lalitb]

the most common trivial scenario is when a customer provides a single processor rather than a vector of them

We can recommend customers to use the TracerProvider interface as it supports both single and multiple processors unless they are directly using the Tracer object.
The problem with the current change is that TracerContext maintains object of type MultiSpanProcessor internally, and so calling TracerContext::AddProcessor() will fail after this change.

[lalitb]

This will fail TracerContext::AddProcessor as mentioned in comments.

[maxgolov]

The problem with the current change is that TracerContext maintains object of type MultiSpanProcessor internally, and so calling TracerContext::AddProcessor() will fail after this change.

@lalitb - can you elaborate on a meaning of AddProcessor API in case if the system is configured to run with a single span processor? How often it's called, and what's the need of that API? The only use-case I see is in a test case for the Multi Processor.

[lalitb]

@lalitb - can you elaborate on a meaning of AddProcessor API in case if the system is configured to run with a single span processor? How often it's called, and what's the need of that API? The only use-case I see is in a test case for the Multi Processor.

AddProcessor() should add the processor passed as an argument to the list of configured processors irrespective of where it is called. And below example will now fail for a multi-processor system ( as the customer may now want to add processors one by one ).

  auto processor1 = std::unique_ptr<SpanProcessor>(new SimpleSpanProcessor(std::move(exporter1)));
  auto context = std::make_shared<TracerContext>(std::move(processor1));
  auto processor2 = std::unique_ptr<SpanProcessor>(new SimpleSpanProcessor(std::move(exporter2)));
  context->AddProcessor(std::move(processor2));

[maxgolov]

I don't think it fails if you invoke the constructor intended to be used for multi-processor scenario. Also assuming that if customer used the first constructor (single-processor), they are not going to attempt to call AddProcessor. We can possibly rework the single processor implementation to populate a vector with one single entry in it. Otherwise it appears on API surface of TracerContext that we operate everywhere on base class of SpanProcessor. Then somewhere in internal implementation detail we do a static_cast to child, which seems dangerous. It would have been nicer to have this done with dynamic_cast, and auto-rewire from single- to multi- processor only when necessary. Thus, not dealing with walking the list of processor in a trivial case... Unfortunately dynamic_cast requires RTTI, but in most modern scenarios we should have it?

[lalitb]

Then somewhere in internal implementation detail we do a static_cast to child, which seems dangerous. It would have been nicer to have this done with dynamic_cast, and auto-rewire from single- to multi- processor only when necessary.

I don't see static_cast conversion here dangerous as long as we know that the object being converted is of child type. It's used internally and should be safe. As you rightly said, dynamic_cast uses RTTI and we shouldn't use it within the library. Not every application would want to enable RTTI.
I did try earlier to have approach like this, somehow didn't work, but would be worth trying out:

TracerContext::TracerContext(sstd::unique_ptr<SpanProcessor> &processor,
                             opentelemetry::sdk::resource::Resource resource,
                             std::unique_ptr<Sampler> sampler,
                             std::unique_ptr<IdGenerator> id_generator) noexcept
    : processor_(std::unique_ptr<SpanProcessor>(new MultiSpanProcessor(<somehow convert to vector of processor>))),
      resource_(resource),
      sampler_(std::move(sampler)),
      id_generator_(std::move(id_generator))
{}
``

[lalitb]

incorrectly closed by bot.

[lalitb]

Closing. The changes are not correct as discussed in the review comments.