Skip to content

[SDK] Do not use MultiSpanProcessor for single processors #4043

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.

Sign up for GitHub

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

Open
Hailios wants to merge 11 commits into
base: main
Choose a base branch
from
+160 −5
Open

[SDK] Do not use MultiSpanProcessor for single processors #4043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a553411
do not use MultiSpanProcessor for single processors
Hailios May 2, 2026
c6b2bca
add some tests
Hailios May 12, 2026
18a46c7
format
Hailios May 12, 2026
4a21219
format again
Hailios May 12, 2026
9be6481
pointer implementation instead of counter
Hailios May 13, 2026
c74712f
format
Hailios May 13, 2026
8470609
try to get past cppcheck
Hailios May 13, 2026
fc50aae
verify more in tests
Hailios May 14, 2026
8591d82
format
Hailios May 14, 2026
739e24e
more formatting
Hailios May 14, 2026
6a785f5
format
Hailios May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions sdk/include/opentelemetry/sdk/trace/multi_span_processor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,9 @@ class MultiSpanProcessor : public SpanProcessor
ProcessorNode *head_{nullptr};
ProcessorNode *tail_{nullptr};
size_t count_{0};

// For testing
friend class MultiSpanProcessorTestPeer;
};
} // namespace trace
} // namespace sdk
Expand Down
5 changes: 5 additions & 0 deletions sdk/include/opentelemetry/sdk/trace/tracer_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ namespace sdk
namespace trace
{

// forward declare to be able to have a raw pointer to it
class MultiSpanProcessor;

/**
* A class which stores the TracerProvider context.
*
Expand Down Expand Up @@ -120,6 +123,8 @@ class TracerContext
std::unique_ptr<IdGenerator> id_generator_;
std::unique_ptr<SpanProcessor> processor_;
std::unique_ptr<instrumentationscope::ScopeConfigurator<TracerConfig>> tracer_configurator_;
// shares the pointer with processor_ if it is a MultiSpanProcessor, null otherwise
MultiSpanProcessor *multi_processor_;
};

} // namespace trace
Expand Down
45 changes: 40 additions & 5 deletions sdk/src/trace/tracer_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,22 @@ TracerContext::TracerContext(std::vector<std::unique_ptr<SpanProcessor>> &&proce
: resource_(resource),
sampler_(std::move(sampler)),
id_generator_(std::move(id_generator)),
processor_(std::unique_ptr<SpanProcessor>(new MultiSpanProcessor(std::move(processors)))),
tracer_configurator_(std::move(tracer_configurator))
{}
tracer_configurator_(std::move(tracer_configurator)),
multi_processor_(nullptr)
{
if (processors.empty())
{
processor_ = std::unique_ptr<SpanProcessor>(new MultiSpanProcessor(std::move(processors)));
}
else
{
// at least one processor is available here
for (auto &&processor : processors)
{
AddProcessor(std::move(processor));
}
}
}

Sampler &TracerContext::GetSampler() const noexcept
{
Expand All @@ -59,9 +72,31 @@ opentelemetry::sdk::trace::IdGenerator &TracerContext::GetIdGenerator() const no

void TracerContext::AddProcessor(std::unique_ptr<SpanProcessor> processor) noexcept
{
if (!processor)
{
return;
}

if (!processor_)
{
// this is the first processor to be added
processor_ = std::move(processor);
}
else if (multi_processor_ == nullptr)
{
// a processor exists, but it's not a MultiSpanProcessor. make a new MultiSpanProcessor
multi_processor_ = new MultiSpanProcessor({});
std::unique_ptr<MultiSpanProcessor> multi_processor(multi_processor_);
multi_processor->AddProcessor(std::move(processor_));
multi_processor->AddProcessor(std::move(processor));

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

@lalitb lalitb May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@Hailios Hailios May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@lalitb lalitb May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else /*if (multi_processor_ != nullptr)*/
{
// already have a MultiSpanProcessor, add the processor to it
multi_processor_->AddProcessor(std::move(processor));
}
}

SpanProcessor &TracerContext::GetProcessor() const noexcept
Expand Down
112 changes: 112 additions & 0 deletions sdk/test/trace/tracer_provider_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "opentelemetry/sdk/resource/resource.h"
#include "opentelemetry/sdk/trace/exporter.h"
#include "opentelemetry/sdk/trace/id_generator.h"
#include "opentelemetry/sdk/trace/multi_span_processor.h"
#include "opentelemetry/sdk/trace/processor.h"
#include "opentelemetry/sdk/trace/random_id_generator.h"
#include "opentelemetry/sdk/trace/sampler.h"
Expand All @@ -38,6 +39,33 @@
using namespace opentelemetry::sdk::trace;
using namespace opentelemetry::sdk::resource;

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace trace
{
class MultiSpanProcessorTestPeer
{
public:
static std::vector<SpanProcessor *> GetProcessors(MultiSpanProcessor *multi_span_processor)
{
std::vector<SpanProcessor *> res;

MultiSpanProcessor::ProcessorNode *node = multi_span_processor->head_;
while (node != nullptr)
{
auto processor = node->value_.get();
res.emplace_back(processor);
node = node->next_;
}

return res;
}
};
} // namespace trace
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE

TEST(TracerProvider, GetTracer)
{
std::unique_ptr<SpanProcessor> processor(new SimpleSpanProcessor(nullptr));
Expand Down Expand Up @@ -318,6 +346,90 @@ TEST(TracerProvider, GetTracerAbiv2)
}
#endif /* OPENTELEMETRY_ABI_VERSION_NO >= 2 */

// get the same processor back, not wrapped in a MultiSpanProcessor
TEST(TracerProvider, GetProcessor)
{
std::unique_ptr<SpanProcessor> processor(new SimpleSpanProcessor(nullptr));
std::vector<std::unique_ptr<SpanProcessor>> processors;
processors.push_back(std::move(processor));

std::unique_ptr<TracerContext> context1(new TracerContext(std::move(processors)));

auto &span_processor = context1->GetProcessor();

// Should be the SimpleSpanProcessor processor that was created above.
#ifdef OPENTELEMETRY_RTTI_ENABLED
auto processor_typeed = dynamic_cast<SimpleSpanProcessor *>(&span_processor);
#else
auto processor_typeed = static_cast<SimpleSpanProcessor *>(&span_processor);
#endif
ASSERT_NE(nullptr, processor_typeed);
}

// get a MultiSpanProcessor back that wraps both processors
TEST(TracerProvider, GetProcessorsTwo)
{
std::vector<SpanProcessor *> processors_raw(2);
processors_raw[0] = new SimpleSpanProcessor(nullptr); // deleted via unique_ptr
processors_raw[1] = new SimpleSpanProcessor(nullptr); // deleted via unique_ptr

std::unique_ptr<SpanProcessor> processor1(processors_raw[0]);
std::unique_ptr<SpanProcessor> processor2(processors_raw[1]);

std::vector<std::unique_ptr<SpanProcessor>> processors;
processors.push_back(std::move(processor1));
processors.push_back(std::move(processor2));

std::unique_ptr<TracerContext> context1(new TracerContext(std::move(processors)));

auto &span_processor = context1->GetProcessor();

// Should be the SimpleSpanProcessor processor that was created above.
#ifdef OPENTELEMETRY_RTTI_ENABLED
auto processor_typeed = dynamic_cast<MultiSpanProcessor *>(&span_processor);
#else
auto processor_typeed = static_cast<MultiSpanProcessor *>(&span_processor);
#endif
ASSERT_NE(nullptr, processor_typeed);

std::vector<SpanProcessor *> contained_processors =
MultiSpanProcessorTestPeer::GetProcessors(processor_typeed);
EXPECT_EQ(processors_raw, contained_processors);
}

// get a MultiSpanProcessor back that wraps all three processors
TEST(TracerProvider, GetProcessorsThree)
{
std::vector<SpanProcessor *> processors_raw(3);
processors_raw[0] = new SimpleSpanProcessor(nullptr); // deleted via unique_ptr
processors_raw[1] = new SimpleSpanProcessor(nullptr); // deleted via unique_ptr
processors_raw[2] = new SimpleSpanProcessor(nullptr); // deleted via unique_ptr

std::unique_ptr<SpanProcessor> processor1(processors_raw[0]);
std::unique_ptr<SpanProcessor> processor2(processors_raw[1]);
std::unique_ptr<SpanProcessor> processor3(processors_raw[2]);
std::vector<std::unique_ptr<SpanProcessor>> processors;
processors.push_back(std::move(processor1));
processors.push_back(std::move(processor2));
processors.push_back(std::move(processor3));

std::unique_ptr<TracerContext> context1(new TracerContext(std::move(processors)));

auto &span_processor = context1->GetProcessor();

// Should be the SimpleSpanProcessor processor that was created above.
#ifdef OPENTELEMETRY_RTTI_ENABLED
auto processor_typeed = dynamic_cast<MultiSpanProcessor *>(&span_processor);
#else
auto processor_typeed = static_cast<MultiSpanProcessor *>(&span_processor);
#endif
ASSERT_NE(nullptr, processor_typeed);
Copy link
Copy Markdown
Member

@owent owent May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Hailios Hailios May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but how?

Copy link
Copy Markdown
Member

@owent owent May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


std::vector<SpanProcessor *> contained_processors =
MultiSpanProcessorTestPeer::GetProcessors(processor_typeed);
EXPECT_EQ(processors_raw, contained_processors);
}

TEST(TracerProvider, Shutdown)
{
std::unique_ptr<SpanProcessor> processor(new SimpleSpanProcessor(nullptr));
Expand Down
Loading