From 536f475027497792f7bcc963767b1608e2d1009a Mon Sep 17 00:00:00 2001 From: Akshat Lakhera Date: Mon, 19 Oct 2026 22:01:52 +0530 Subject: [PATCH] [SDK] Return a null recordable when MakeRecordable() fails to allocate Signed-off-by: Akshat Lakhera --- CHANGELOG.md | 4 + .../memory/in_memory_span_exporter.h | 13 +- exporters/ostream/src/span_exporter.cc | 13 +- exporters/otlp/src/otlp_file_exporter.cc | 13 +- exporters/otlp/src/otlp_grpc_exporter.cc | 17 ++- exporters/otlp/src/otlp_http_exporter.cc | 17 ++- exporters/zipkin/src/zipkin_exporter.cc | 13 +- .../sdk/trace/multi_span_processor.h | 32 +++- sdk/src/trace/multi_recordable.cc | 13 +- sdk/test/trace/multi_span_processor_test.cc | 43 ++++++ sdk/test/trace/tracer_test.cc | 141 ++++++++++++++++++ 11 files changed, 300 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a3671abae..dc7bfae62c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ Increment the: * [SDK] Fix disabled Tracer StartSpan noop behavior [#4710](https://github.com/open-telemetry/opentelemetry-cpp/pull/4710) +* [SDK] Return a null recordable when MakeRecordable() fails to allocate, + instead of terminating the process from a noexcept function. + [#4700](https://github.com/open-telemetry/opentelemetry-cpp/pull/4700) + * [SDK] Preserve the default unlimited timeout when batch span and log record processors shut down their exporters. [#4686](https://github.com/open-telemetry/opentelemetry-cpp/pull/4686) diff --git a/exporters/memory/include/opentelemetry/exporters/memory/in_memory_span_exporter.h b/exporters/memory/include/opentelemetry/exporters/memory/in_memory_span_exporter.h index 7c187852ae..4a8dd51c18 100644 --- a/exporters/memory/include/opentelemetry/exporters/memory/in_memory_span_exporter.h +++ b/exporters/memory/include/opentelemetry/exporters/memory/in_memory_span_exporter.h @@ -45,7 +45,18 @@ class InMemorySpanExporter final : public opentelemetry::sdk::trace::SpanExporte */ std::unique_ptr MakeRecordable() noexcept override { - return std::unique_ptr(new sdk::trace::SpanData()); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + return std::unique_ptr(new sdk::trace::SpanData()); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } + catch (...) + { + return nullptr; + } +#endif } /** diff --git a/exporters/ostream/src/span_exporter.cc b/exporters/ostream/src/span_exporter.cc index 896a342c71..8a4c34abaa 100644 --- a/exporters/ostream/src/span_exporter.cc +++ b/exporters/ostream/src/span_exporter.cc @@ -68,7 +68,18 @@ OStreamSpanExporter::OStreamSpanExporter(std::ostream &sout) noexcept : sout_(so std::unique_ptr OStreamSpanExporter::MakeRecordable() noexcept { - return std::unique_ptr(new trace_sdk::SpanData); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + return std::unique_ptr(new trace_sdk::SpanData); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } + catch (...) + { + return nullptr; + } +#endif } sdk::common::ExportResult OStreamSpanExporter::Export( diff --git a/exporters/otlp/src/otlp_file_exporter.cc b/exporters/otlp/src/otlp_file_exporter.cc index 862dbed9f7..0cb916d064 100644 --- a/exporters/otlp/src/otlp_file_exporter.cc +++ b/exporters/otlp/src/otlp_file_exporter.cc @@ -50,7 +50,18 @@ OtlpFileExporter::OtlpFileExporter(const OtlpFileExporterOptions &options, std::unique_ptr OtlpFileExporter::MakeRecordable() noexcept { - return std::make_unique(); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + return std::make_unique(); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } + catch (...) + { + return nullptr; + } +#endif } opentelemetry::sdk::common::ExportResult OtlpFileExporter::Export( diff --git a/exporters/otlp/src/otlp_grpc_exporter.cc b/exporters/otlp/src/otlp_grpc_exporter.cc index c275798b81..7bf991d7db 100644 --- a/exporters/otlp/src/otlp_grpc_exporter.cc +++ b/exporters/otlp/src/otlp_grpc_exporter.cc @@ -92,9 +92,20 @@ OtlpGrpcExporter::~OtlpGrpcExporter() std::unique_ptr OtlpGrpcExporter::MakeRecordable() noexcept { - return std::make_unique(options_.max_attributes, options_.max_events, - options_.max_links, options_.max_attributes_per_event, - options_.max_attributes_per_link); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + return std::make_unique(options_.max_attributes, options_.max_events, + options_.max_links, options_.max_attributes_per_event, + options_.max_attributes_per_link); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } + catch (...) + { + return nullptr; + } +#endif } sdk::common::ExportResult OtlpGrpcExporter::Export( diff --git a/exporters/otlp/src/otlp_http_exporter.cc b/exporters/otlp/src/otlp_http_exporter.cc index 1155b683bd..b9633c161b 100644 --- a/exporters/otlp/src/otlp_http_exporter.cc +++ b/exporters/otlp/src/otlp_http_exporter.cc @@ -301,9 +301,20 @@ OtlpHttpExporter::OtlpHttpExporter(std::unique_ptr http_client) std::unique_ptr OtlpHttpExporter::MakeRecordable() noexcept { - return std::make_unique( - options_.max_attributes, options_.max_events, options_.max_links, - options_.max_attributes_per_event, options_.max_attributes_per_link); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + return std::make_unique( + options_.max_attributes, options_.max_events, options_.max_links, + options_.max_attributes_per_event, options_.max_attributes_per_link); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } + catch (...) + { + return nullptr; + } +#endif } opentelemetry::sdk::common::ExportResult OtlpHttpExporter::Export( diff --git a/exporters/zipkin/src/zipkin_exporter.cc b/exporters/zipkin/src/zipkin_exporter.cc index 308d8081c6..5f30982199 100644 --- a/exporters/zipkin/src/zipkin_exporter.cc +++ b/exporters/zipkin/src/zipkin_exporter.cc @@ -66,7 +66,18 @@ ZipkinExporter::ZipkinExporter( std::unique_ptr ZipkinExporter::MakeRecordable() noexcept { - return std::unique_ptr(new Recordable); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + return std::unique_ptr(new Recordable); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } + catch (...) + { + return nullptr; + } +#endif } sdk::common::ExportResult ZipkinExporter::Export( diff --git a/sdk/include/opentelemetry/sdk/trace/multi_span_processor.h b/sdk/include/opentelemetry/sdk/trace/multi_span_processor.h index 6d7e6ffbc4..34aa80490b 100644 --- a/sdk/include/opentelemetry/sdk/trace/multi_span_processor.h +++ b/sdk/include/opentelemetry/sdk/trace/multi_span_processor.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include "opentelemetry/sdk/trace/multi_recordable.h" @@ -65,16 +66,31 @@ class MultiSpanProcessor : public SpanProcessor std::unique_ptr MakeRecordable() noexcept override { - auto recordable = std::unique_ptr(new MultiRecordable); - auto multi_recordable = static_cast(recordable.get()); - ProcessorNode *node = head_; - while (node != nullptr) +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try { - auto processor = node->value_.get(); - multi_recordable->AddRecordable(*processor, processor->MakeRecordable()); - node = node->next_; +#endif + auto recordable = std::unique_ptr(new MultiRecordable); + auto multi_recordable = static_cast(recordable.get()); + ProcessorNode *node = head_; + while (node != nullptr) + { + auto processor = node->value_.get(); + auto sub_recordable = processor->MakeRecordable(); + if (sub_recordable != nullptr) + { + multi_recordable->AddRecordable(*processor, std::move(sub_recordable)); + } + node = node->next_; + } + return recordable; +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } + catch (...) + { + return nullptr; } - return recordable; +#endif } void OnStart(Recordable &span, diff --git a/sdk/src/trace/multi_recordable.cc b/sdk/src/trace/multi_recordable.cc index 37959a8e1f..2d2bd1ad24 100644 --- a/sdk/src/trace/multi_recordable.cc +++ b/sdk/src/trace/multi_recordable.cc @@ -31,7 +31,18 @@ std::size_t MultiRecordable::MakeKey(const SpanProcessor &processor) noexcept void MultiRecordable::AddRecordable(const SpanProcessor &processor, std::unique_ptr recordable) noexcept { - recordables_[MakeKey(processor)] = std::move(recordable); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + recordables_[MakeKey(processor)] = std::move(recordable); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } + catch (...) // NOLINT(bugprone-empty-catch) + { + // Drop the recordable, the processor will skip this span. + } +#endif } const std::unique_ptr &MultiRecordable::GetRecordable( diff --git a/sdk/test/trace/multi_span_processor_test.cc b/sdk/test/trace/multi_span_processor_test.cc index c382c0d9b6..5e848a4e7b 100644 --- a/sdk/test/trace/multi_span_processor_test.cc +++ b/sdk/test/trace/multi_span_processor_test.cc @@ -12,6 +12,7 @@ #include "opentelemetry/sdk/trace/processor.h" #include "opentelemetry/sdk/trace/recordable.h" #include "opentelemetry/sdk/trace/span_data.h" +#include "opentelemetry/trace/span_context.h" using namespace opentelemetry::sdk::trace; using opentelemetry::trace::SpanContext; @@ -47,6 +48,28 @@ class FixedResultProcessor final : public SpanProcessor bool result_; }; +// A processor that fails to create a recordable and counts the spans it receives. +class NullRecordableProcessor final : public SpanProcessor +{ +public: + std::unique_ptr MakeRecordable() noexcept override { return nullptr; } + + void OnStart(Recordable & /* span */, const SpanContext & /* parent_context */) noexcept override + { + ++started_; + } + + // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved) + void OnEnd(std::unique_ptr && /* span */) noexcept override { ++ended_; } + + bool ForceFlush(std::chrono::microseconds /* timeout */) noexcept override { return true; } + + bool Shutdown(std::chrono::microseconds /* timeout */) noexcept override { return true; } + + int started_ = 0; + int ended_ = 0; +}; + std::unique_ptr MakeMultiProcessor(bool first, bool second) { std::vector> processors; @@ -71,4 +94,24 @@ TEST(MultiSpanProcessorTest, ShutdownFailsWhenAnyChildFails) EXPECT_FALSE(MakeMultiProcessor(false, true)->Shutdown()); } +TEST(MultiSpanProcessorTest, MakeRecordableSkipsChildWithoutRecordable) +{ + auto *null_processor = new NullRecordableProcessor(); + std::vector> processors; + processors.emplace_back(null_processor); + processors.emplace_back(new FixedResultProcessor(true)); + MultiSpanProcessor processor(std::move(processors)); + + auto recordable = processor.MakeRecordable(); + ASSERT_NE(recordable, nullptr); + + recordable->SetName("span"); + recordable->SetAttribute("key", "value"); + processor.OnStart(*recordable, SpanContext::GetInvalid()); + processor.OnEnd(std::move(recordable)); + + EXPECT_EQ(null_processor->started_, 0); + EXPECT_EQ(null_processor->ended_, 0); +} + } // namespace diff --git a/sdk/test/trace/tracer_test.cc b/sdk/test/trace/tracer_test.cc index a04177c913..119066ee4c 100644 --- a/sdk/test/trace/tracer_test.cc +++ b/sdk/test/trace/tracer_test.cc @@ -2,12 +2,15 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include #include #include +#include #include #include #include +#include #include #include #include @@ -17,6 +20,7 @@ #include "opentelemetry/common/macros.h" #include "opentelemetry/common/timestamp.h" #include "opentelemetry/context/context.h" +#include "opentelemetry/context/propagation/text_map_propagator.h" #include "opentelemetry/exporters/memory/in_memory_span_data.h" #include "opentelemetry/exporters/memory/in_memory_span_exporter.h" #include "opentelemetry/nostd/shared_ptr.h" @@ -30,6 +34,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_recordable.h" #include "opentelemetry/sdk/trace/processor.h" #include "opentelemetry/sdk/trace/random_id_generator.h" #include "opentelemetry/sdk/trace/sampler.h" @@ -44,6 +49,7 @@ #include "opentelemetry/sdk/trace/tracer_context.h" #include "opentelemetry/trace/context.h" #include "opentelemetry/trace/default_span.h" +#include "opentelemetry/trace/propagation/http_trace_context.h" #include "opentelemetry/trace/scope.h" #include "opentelemetry/trace/span.h" #include "opentelemetry/trace/span_context.h" @@ -204,8 +210,83 @@ std::shared_ptr initTracer( return std::shared_ptr(new Tracer(context, std::move(scope))); } +#if defined(__SANITIZE_ADDRESS__) +# define OTEL_TEST_ASAN 1 +#elif defined(__has_feature) +# if __has_feature(address_sanitizer) +# define OTEL_TEST_ASAN 1 +# endif +#endif +#ifndef OTEL_TEST_ASAN +# define OTEL_TEST_ASAN 0 +#endif + +#if OPENTELEMETRY_HAVE_EXCEPTIONS && !OTEL_TEST_ASAN +// Allocation failure injection: while armed, the next allocation of a MultiRecordable throws +// std::bad_alloc. MultiRecordable is what MultiSpanProcessor::MakeRecordable() allocates for +// every SDK span, so this simulates the span allocation failing. +std::atomic g_fail_multi_recordable_allocation{false}; +std::atomic g_failed_allocations{0}; +// Counts calls into the replacement operator new. Tools such as valgrind substitute their own +// operator new, in which case this stays at zero and the test is skipped. +std::atomic g_replaced_new_calls{0}; + +class MapCarrier : public opentelemetry::context::propagation::TextMapCarrier +{ +public: + nostd::string_view Get(nostd::string_view key) const noexcept override + { + auto it = headers_.find(std::string(key)); + return it != headers_.end() ? nostd::string_view(it->second) : nostd::string_view(); + } + + void Set(nostd::string_view key, nostd::string_view value) noexcept override + { + headers_[std::string(key)] = std::string(value); + } + + std::map headers_; +}; +#endif + } // namespace +#if OPENTELEMETRY_HAVE_EXCEPTIONS && !OTEL_TEST_ASAN +void *operator new(std::size_t size) +{ + ++g_replaced_new_calls; + if (size == sizeof(MultiRecordable) && g_fail_multi_recordable_allocation.exchange(false)) + { + ++g_failed_allocations; + throw std::bad_alloc(); + } + void *ptr = std::malloc(size); // NOLINT(cppcoreguidelines-no-malloc) + if (ptr == nullptr) + { + throw std::bad_alloc(); + } + return ptr; +} + +// free() pairs with the malloc() in the replacement operator new above. +# if defined(__GNUC__) && !defined(__clang__) +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wmismatched-new-delete" +# endif +void operator delete(void *ptr) noexcept +{ + std::free(ptr); // NOLINT(cppcoreguidelines-no-malloc) +} + +void operator delete(void *ptr, std::size_t /* size */) noexcept +{ + std::free(ptr); // NOLINT(cppcoreguidelines-no-malloc) +} +# if defined(__GNUC__) && !defined(__clang__) +# pragma GCC diagnostic pop +# endif +#endif + TEST(Tracer, ToInMemorySpanExporter) { InMemorySpanExporter *exporter = new InMemorySpanExporter(); @@ -1750,3 +1831,63 @@ TEST(Tracer, SpanLimits) EXPECT_EQ(limits.attribute_value_length_limit, nostd::get(events.at(0).GetAttributes().at("one")).size()); } + +#if OPENTELEMETRY_HAVE_EXCEPTIONS && !OTEL_TEST_ASAN +TEST(Tracer, FailedSpanAllocationPreservesContextOnExtract) +{ + InMemorySpanExporter *exporter = new InMemorySpanExporter(); + auto tracer = initTracer(std::unique_ptr{exporter}); + + const std::uint8_t trace_id_bytes[trace_api::TraceId::kSize] = {1, 2, 3, 4, 5, 6, 7, 8, + 9, 10, 11, 12, 13, 14, 15, 16}; + const std::uint8_t span_id_bytes[trace_api::SpanId::kSize] = {1, 2, 3, 4, 5, 6, 7, 8}; + const SpanContext parent(trace_api::TraceId(trace_id_bytes), trace_api::SpanId(span_id_bytes), + trace_api::TraceFlags(trace_api::TraceFlags::kIsSampled), true); + + trace_api::StartSpanOptions options; + options.parent = parent; + + g_failed_allocations = 0; + g_replaced_new_calls = 0; + g_fail_multi_recordable_allocation = true; + auto span = tracer->StartSpan("span", options); + g_fail_multi_recordable_allocation = false; + + if (g_replaced_new_calls.load() == 0) + { + GTEST_SKIP() << "global operator new is not replaceable in this environment"; + } + + // The injected allocation failure was hit and handled, and the span is not recording ... + ASSERT_EQ(1, g_failed_allocations.load()); + EXPECT_FALSE(span->IsRecording()); + + // ... but it still carries the parent trace and a fresh span id. + const SpanContext span_context = span->GetContext(); + EXPECT_TRUE(span_context.IsValid()); + EXPECT_EQ(parent.trace_id(), span_context.trace_id()); + EXPECT_NE(parent.span_id(), span_context.span_id()); + EXPECT_TRUE(span_context.IsSampled()); + + // Injecting the failed span into a carrier and extracting it back keeps the original context. + MapCarrier carrier; + opentelemetry::trace::propagation::HttpTraceContext propagator; + opentelemetry::context::Context inject_context; + propagator.Inject(carrier, trace_api::SetSpan(inject_context, span)); + + opentelemetry::context::Context extract_context; + const auto extracted = trace_api::GetSpanContext(propagator.Extract(carrier, extract_context)); + EXPECT_TRUE(extracted.IsValid()); + EXPECT_TRUE(extracted.IsRemote()); + EXPECT_EQ(parent.trace_id(), extracted.trace_id()); + EXPECT_EQ(span_context.span_id(), extracted.span_id()); + EXPECT_EQ(span_context.trace_flags(), extracted.trace_flags()); + + span->End(); + + // The failure is transient: the next span on the same tracer records normally. + auto next_span = tracer->StartSpan("next", options); + EXPECT_TRUE(next_span->IsRecording()); + next_span->End(); +} +#endif