[SDK] Do not use MultiSpanProcessor for single processors by Hailios · Pull Request #4043 · open-telemetry/opentelemetry-cpp

Hailios · 2026-05-02T20:55:49Z

Fixes # (issue)

Changes

do not wrap single processors in a multi span processor. that is, use it when needed, otherwise not. idea here is to save execution time by not needlessly going through the multi span processor.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2026-05-02T21:43:04Z

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.71%. Comparing base (76f0872) to head (6a785f5).

Files with missing lines	Patch %	Lines
sdk/src/trace/tracer_context.cc	94.74%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4043      +/-   ##
==========================================
- Coverage   82.01%   81.71%   -0.29%     
==========================================
  Files         385      385              
  Lines       16023    16036      +13     
==========================================
- Hits        13139    13102      -37     
- Misses       2884     2934      +50

Files with missing lines	Coverage Δ
...ude/opentelemetry/sdk/trace/multi_span_processor.h	`71.06% <ø> (-22.36%)`	⬇️
...k/include/opentelemetry/sdk/trace/tracer_context.h	`100.00% <ø> (ø)`
sdk/src/trace/tracer_context.cc	`97.44% <94.74%> (+12.83%)`	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dbarker

Thanks for the PR! Supporting replacement of the MultiSpanProcessor with a user defined processor is a nice addition. Please see a few comments and request for test cases.

dbarker · 2026-05-07T19:41:06Z

+    // this is the first processor to be added, maybe the MultiSpanProcessor is not needed
+    processor_ = std::move(processor);
+  }
+  else if (num_processors_ == 1)


Please add test cases for these if/else branches

dbarker · 2026-05-07T19:41:23Z

+  else if (num_processors_ == 1)
+  {
+    // if there already is a processor, then make a new MultiSpanProcessor to handle both
+    std::unique_ptr<MultiSpanProcessor> multi_processor(new MultiSpanProcessor({}));


can use std::make_unique

it looks like the rest of the code using the pattern above with first calling new and putting it in a smart pointer. and the specific case of creating an empty MultiSpanProcessor is not as easy with make_unique, since one would need to type out all the letters to make an empty vector of unique pointers of processors, rather than "{}".

owent · 2026-05-13T05:06:50Z

+#else
+  auto processor_typeed = static_cast<MultiSpanProcessor *>(&span_processor);
+#endif
+  ASSERT_NE(nullptr, processor_typeed);


Shoule we check processor_typeed contains all of processor1,processor2 and processor3 here?

yes, but how?

We can add friend class only for test, like OtlpHttpExporterTestPeer in exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h and exporters/otlp/test/otlp_http_exporter_test.cc

marcalff

Thanks for the contribution.

This is a good improvement, and will save overhead in the most frequent cases.

Suggestions for a different implementation:

Have the following attributes:

std::unique_ptr<SpanProcessor> processor_;
MultiSpanProcessor *multi_processor_; // yes, raw pointer.

processor_ will point to a single or multi processor, we don't know looking at this member alone.

multi_processor_ == nullptr means processor_ is a single processor.
multi_processor_ != nullptr means processor_ is a multi processor, and the raw pointer can be used to add more processors to it directly.

This should avoid the need to count processors, and cast back and forth between abstract and more narrow types, which causes confusion.

Drop the processor count, not needed.

Given that this class holds a unique pointer in processors_, the raw multi_processor_ pointer is safe to use.

Hailios · 2026-05-14T07:15:00Z

Thanks for the contribution.

This is a good improvement, and will save overhead in the most frequent cases.

Suggestions for a different implementation:

Have the following attributes:
std::unique_ptr<SpanProcessor> processor_;
MultiSpanProcessor *multi_processor_; // yes, raw pointer.
processor_ will point to a single or multi processor, we don't know looking at this member alone.

multi_processor_ == nullptr means processor_ is a single processor. multi_processor_ != nullptr means processor_ is a multi processor, and the raw pointer can be used to add more processors to it directly.

This should avoid the need to count processors, and cast back and forth between abstract and more narrow types, which causes confusion.

Drop the processor count, not needed.

Given that this class holds a unique pointer in processors_, the raw multi_processor_ pointer is safe to use.

I updated the implementation to follow this advice

lalitb · 2026-05-15T00:26:05Z


-  auto multi_processor = static_cast<MultiSpanProcessor *>(processor_.get());
-  multi_processor->AddProcessor(std::move(processor));
+    processor_ = std::move(multi_processor);


I think this break spans that were already started before AddProcessor() runs? Those spans keep the recordable created by the old single processor, but after this line Span::End() will call MultiSpanProcessor::OnEnd(), which expects a MultiRecordable. Maybe promotion needs to preserve the old processor path for existing spans.

hmm. yes I agree that it breaks currently live spans in they way you describe. unfortunate. do you have a suggestion on how to get around this?

Maybe the cleanest fix is for Span to remember the processor used when it was started.

That should not require shared ownership or another heap allocation per span. It could just store a raw SpanProcessor* captured at start, then use that same processor for OnEnd(). The span already keeps the tracer/context alive, so the processor lifetime should still be covered.

lalitb

As mentioned here. We need to preserver the behavior as documented here -

opentelemetry-cpp/sdk/include/opentelemetry/sdk/trace/tracer_provider.h

Lines 107 to 110 in 76f0872

    
              * Note: This process may not receive any in-flight spans, but will get newly created spans. 
        
              * Note: This method is not thread safe, and should ideally be called from main thread. 
        
              */ 
        
             void AddProcessor(std::unique_ptr<SpanProcessor> processor) noexcept;

Hailios requested a review from a team as a code owner May 2, 2026 20:55

Hailios force-pushed the do_not_use_multispan_for_single_processor branch from 58faa51 to 00e2150 Compare May 3, 2026 06:21

dbarker reviewed May 7, 2026

View reviewed changes

Hailios force-pushed the do_not_use_multispan_for_single_processor branch from 77e0e8e to c2df133 Compare May 12, 2026 19:23

owent reviewed May 13, 2026

View reviewed changes

marcalff reviewed May 13, 2026

View reviewed changes

marcalff changed the title ~~do not use MultiSpanProcessor for single processors~~ [SDK] Do not use MultiSpanProcessor for single processors May 13, 2026

Hailios force-pushed the do_not_use_multispan_for_single_processor branch from 7412f4b to d8ad645 Compare May 13, 2026 19:11

Hailios added 8 commits May 14, 2026 17:08

do not use MultiSpanProcessor for single processors

a553411

add some tests

c6b2bca

format

18a46c7

format again

4a21219

pointer implementation instead of counter

9be6481

format

c74712f

try to get past cppcheck

8470609

verify more in tests

fc50aae

Hailios force-pushed the do_not_use_multispan_for_single_processor branch from 1f2bfe4 to fc50aae Compare May 14, 2026 15:09

Hailios added 3 commits May 14, 2026 21:34

format

8591d82

more formatting

739e24e

format

6a785f5

lalitb reviewed May 15, 2026

View reviewed changes

lalitb requested changes May 15, 2026

View reviewed changes

	* Note: This process may not receive any in-flight spans, but will get newly created spans.
	* Note: This method is not thread safe, and should ideally be called from main thread.
	*/
	void AddProcessor(std::unique_ptr<SpanProcessor> processor) noexcept;

Conversation

Hailios commented May 2, 2026

Changes

Uh oh!

codecov Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dbarker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

Hailios commented May 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented May 2, 2026 •

edited

Loading