From ea6a0b8137cad046ee21b3680fa47f22d30f1692 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 12 Feb 2021 14:35:15 -0500 Subject: [PATCH 1/8] Add SetResourceRef to trace Recordable interface. --- .../include/opentelemetry/exporters/etw/etw_data.h | 4 ++++ .../opentelemetry/exporters/otlp/recordable.h | 2 ++ exporters/otlp/src/recordable.cc | 5 +++++ .../opentelemetry/ext/zpages/threadsafe_span_data.h | 4 ++++ sdk/include/opentelemetry/sdk/trace/recordable.h | 12 ++++++++++++ sdk/include/opentelemetry/sdk/trace/span_data.h | 5 +++++ 6 files changed, 32 insertions(+) diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h index a60c2517af..4ed6c8b932 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h @@ -465,6 +465,10 @@ class ETWSpanData final : public sdk::trace::Recordable parent_span_id_ = parent_span_id; } + void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept override { + // TODO + } + void SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept override { diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h index c37a68aba5..8b3092ed91 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h @@ -18,6 +18,8 @@ class Recordable final : public sdk::trace::Recordable trace::SpanId span_id, trace::SpanId parent_span_id) noexcept override; + void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept override; + void SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept override; diff --git a/exporters/otlp/src/recordable.cc b/exporters/otlp/src/recordable.cc index ebc9c3047b..635cf17807 100644 --- a/exporters/otlp/src/recordable.cc +++ b/exporters/otlp/src/recordable.cc @@ -217,6 +217,11 @@ void Recordable::SetDuration(std::chrono::nanoseconds duration) noexcept const uint64_t unix_end_time = span_.start_time_unix_nano() + duration.count(); span_.set_end_time_unix_nano(unix_end_time); } + +void Recordable::SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept { + // TODO - Do something. +} + } // namespace otlp } // namespace exporter OPENTELEMETRY_END_NAMESPACE diff --git a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h index 3bb0326130..4fc2435788 100644 --- a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h +++ b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h @@ -127,6 +127,10 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable parent_span_id_ = parent_span_id; } + void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept override { + // TODO + } + void SetAttribute(nostd::string_view key, const common::AttributeValue &value) noexcept override { std::lock_guard lock(mutex_); diff --git a/sdk/include/opentelemetry/sdk/trace/recordable.h b/sdk/include/opentelemetry/sdk/trace/recordable.h index e179f440bb..779873c928 100644 --- a/sdk/include/opentelemetry/sdk/trace/recordable.h +++ b/sdk/include/opentelemetry/sdk/trace/recordable.h @@ -5,6 +5,7 @@ #include "opentelemetry/core/timestamp.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/empty_attributes.h" +#include "opentelemetry/sdk/resource/resource.h" #include "opentelemetry/trace/canonical_code.h" #include "opentelemetry/trace/span.h" #include "opentelemetry/trace/span_context.h" @@ -41,6 +42,17 @@ class Recordable opentelemetry::trace::SpanId span_id, opentelemetry::trace::SpanId parent_span_id) noexcept = 0; + /** + * Sets the resource associated with a span. + * + *

The resource is guaranteed to have a lifetime longer than this recordable. It is + * owned by the `TracerProvider` which (indirectly) owns the `Exporter` that generates a + * `Recordable`. + * + * @param resource pointer to the Resource for this span. + */ + virtual void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept = 0; + /** * Set an attribute of a span. * @param name the name of the attribute diff --git a/sdk/include/opentelemetry/sdk/trace/span_data.h b/sdk/include/opentelemetry/sdk/trace/span_data.h index 629d68d7f0..2080c9d9a8 100644 --- a/sdk/include/opentelemetry/sdk/trace/span_data.h +++ b/sdk/include/opentelemetry/sdk/trace/span_data.h @@ -7,6 +7,7 @@ #include "opentelemetry/core/timestamp.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/attribute_utils.h" +#include "opentelemetry/sdk/resource/resource.h" #include "opentelemetry/sdk/trace/recordable.h" #include "opentelemetry/trace/canonical_code.h" #include "opentelemetry/trace/span.h" @@ -220,6 +221,10 @@ class SpanData final : public Recordable void SetDuration(std::chrono::nanoseconds duration) noexcept override { duration_ = duration; } + void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept override { + // TODO + } + private: opentelemetry::trace::TraceId trace_id_; opentelemetry::trace::SpanId span_id_; From c37058b60fbd330886339cd650e3e6992e661e4e Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 12 Feb 2021 15:51:10 -0500 Subject: [PATCH 2/8] Remove Tracer methods not in spec. - Tracer now pulls its processor from TracerProvider (as per spec) - Remove `GetProcessor()` and friends from Tracer - Update all tests to reflect the pipeline is now on TracerProvider --- .../zpages/tracez_data_aggregator_test.cc | 7 +- ext/test/zpages/tracez_processor_test.cc | 10 ++- sdk/include/opentelemetry/sdk/trace/tracer.h | 34 ++------- .../opentelemetry/sdk/trace/tracer_provider.h | 3 +- sdk/src/trace/tracer.cc | 28 ++----- sdk/src/trace/tracer_provider.cc | 7 +- sdk/test/trace/sampler_benchmark.cc | 6 +- sdk/test/trace/tracer_provider_test.cc | 6 +- sdk/test/trace/tracer_test.cc | 76 +++++++++---------- 9 files changed, 71 insertions(+), 106 deletions(-) diff --git a/ext/test/zpages/tracez_data_aggregator_test.cc b/ext/test/zpages/tracez_data_aggregator_test.cc index eb077d636a..0ff1892420 100644 --- a/ext/test/zpages/tracez_data_aggregator_test.cc +++ b/ext/test/zpages/tracez_data_aggregator_test.cc @@ -6,6 +6,7 @@ #include "opentelemetry/sdk/resource/resource.h" #include "opentelemetry/sdk/trace/recordable.h" #include "opentelemetry/sdk/trace/tracer.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" using namespace opentelemetry::sdk::trace; using namespace opentelemetry::ext::zpages; @@ -36,13 +37,15 @@ class TracezDataAggregatorTest : public ::testing::Test { std::shared_ptr processor(new TracezSpanProcessor()); auto resource = opentelemetry::sdk::resource::Resource::Create({}); - tracer = std::shared_ptr(new Tracer(processor, resource)); + tracer_provider = std::shared_ptr(new TracerProvider(processor, std::move(resource))); + tracer = tracer_provider->GetTracer("test", "1.0"); tracez_data_aggregator = std::unique_ptr( new TracezDataAggregator(processor, milliseconds(10))); } std::unique_ptr tracez_data_aggregator; - std::shared_ptr tracer; + std::shared_ptr tracer_provider; + opentelemetry::nostd::shared_ptr tracer; }; /** diff --git a/ext/test/zpages/tracez_processor_test.cc b/ext/test/zpages/tracez_processor_test.cc index 2d9246495a..7d9eebd059 100644 --- a/ext/test/zpages/tracez_processor_test.cc +++ b/ext/test/zpages/tracez_processor_test.cc @@ -8,6 +8,7 @@ #include "opentelemetry/nostd/span.h" #include "opentelemetry/sdk/resource/resource.h" #include "opentelemetry/sdk/trace/tracer.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" using namespace opentelemetry::sdk::trace; using namespace opentelemetry::ext::zpages; @@ -148,7 +149,7 @@ void GetManySnapshots(std::shared_ptr &processor, int i) */ void StartManySpans( std::vector> &spans, - std::shared_ptr tracer, + opentelemetry::nostd::shared_ptr tracer, int i) { for (; i > 0; i--) @@ -177,8 +178,8 @@ class TracezProcessor : public ::testing::Test { processor = std::shared_ptr(new TracezSpanProcessor()); auto resource = opentelemetry::sdk::resource::Resource::Create({}); - - tracer = std::shared_ptr(new Tracer(processor, resource)); + tracer_provider = std::shared_ptr(new TracerProvider(processor, std::move(resource))); + tracer = tracer_provider->GetTracer("test", "1.0"); auto spans = processor->GetSpanSnapshot(); running = spans.running; completed = std::move(spans.completed); @@ -187,7 +188,8 @@ class TracezProcessor : public ::testing::Test } std::shared_ptr processor; - std::shared_ptr tracer; + std::shared_ptr tracer_provider; + opentelemetry::nostd::shared_ptr tracer; std::vector span_names; std::vector> span_vars; diff --git a/sdk/include/opentelemetry/sdk/trace/tracer.h b/sdk/include/opentelemetry/sdk/trace/tracer.h index 8a9d955b67..4d57b4bdce 100644 --- a/sdk/include/opentelemetry/sdk/trace/tracer.h +++ b/sdk/include/opentelemetry/sdk/trace/tracer.h @@ -15,36 +15,18 @@ namespace sdk { namespace trace { + +// Forward Declaration due to bidirectional dependency +class TracerProvider; + class Tracer final : public trace_api::Tracer, public std::enable_shared_from_this { public: /** * Initialize a new tracer. - * @param processor The span processor for this tracer. This must not be a - * nullptr. - */ - explicit Tracer(std::shared_ptr processor, - const opentelemetry::sdk::resource::Resource &resource, - std::shared_ptr sampler = std::make_shared()) noexcept; - - /** - * Set the span processor associated with this tracer. - * @param processor The new span processor for this tracer. This must not be - * a nullptr. - */ - void SetProcessor(std::shared_ptr processor) noexcept; - - /** - * Obtain the span processor associated with this tracer. - * @return The span processor for this tracer. - */ - std::shared_ptr GetProcessor() const noexcept; - - /** - * Obtain the sampler associated with this tracer. - * @return The sampler for this tracer. + * @param parent The `TraceProvider` which owns resource + pipeline for traces. */ - std::shared_ptr GetSampler() const noexcept; + explicit Tracer(const TracerProvider& provider) noexcept; nostd::shared_ptr StartSpan( nostd::string_view name, @@ -57,9 +39,7 @@ class Tracer final : public trace_api::Tracer, public std::enable_shared_from_th void CloseWithMicroseconds(uint64_t timeout) noexcept override; private: - opentelemetry::sdk::AtomicSharedPtr processor_; - const std::shared_ptr sampler_; - const opentelemetry::sdk::resource::Resource &resource_; + const TracerProvider& provider_; }; } // namespace trace } // namespace sdk diff --git a/sdk/include/opentelemetry/sdk/trace/tracer_provider.h b/sdk/include/opentelemetry/sdk/trace/tracer_provider.h index a6de0a998d..4a5c39cc91 100644 --- a/sdk/include/opentelemetry/sdk/trace/tracer_provider.h +++ b/sdk/include/opentelemetry/sdk/trace/tracer_provider.h @@ -68,9 +68,10 @@ class TracerProvider final : public opentelemetry::trace::TracerProvider private: opentelemetry::sdk::AtomicSharedPtr processor_; - std::shared_ptr tracer_; const std::shared_ptr sampler_; const opentelemetry::sdk::resource::Resource resource_; + // TODO: Tracer should be per-instrumentation library + std::shared_ptr tracer_; }; } // namespace trace } // namespace sdk diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc index 27e3aad58c..8183e81424 100644 --- a/sdk/src/trace/tracer.cc +++ b/sdk/src/trace/tracer.cc @@ -1,4 +1,5 @@ #include "opentelemetry/sdk/trace/tracer.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" #include "opentelemetry/context/runtime_context.h" #include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/sdk/common/atomic_shared_ptr.h" @@ -10,27 +11,10 @@ namespace sdk { namespace trace { -Tracer::Tracer(std::shared_ptr processor, - const opentelemetry::sdk::resource::Resource &resource, - std::shared_ptr sampler) noexcept - : processor_{processor}, sampler_{sampler}, resource_{resource} +Tracer::Tracer(const opentelemetry::sdk::trace::TracerProvider& provider) noexcept + : provider_ { provider } {} -void Tracer::SetProcessor(std::shared_ptr processor) noexcept -{ - processor_.store(processor); -} - -std::shared_ptr Tracer::GetProcessor() const noexcept -{ - return processor_.load(); -} - -std::shared_ptr Tracer::GetSampler() const noexcept -{ - return sampler_; -} - trace_api::SpanContext GetCurrentSpanContext(const trace_api::SpanContext &explicit_parent) { // Use the explicit parent, if it's valid. @@ -61,7 +45,7 @@ nostd::shared_ptr Tracer::StartSpan( trace_api::SpanContext parent = GetCurrentSpanContext(options.parent); auto sampling_result = - sampler_->ShouldSample(parent, parent.trace_id(), name, options.kind, attributes, links); + provider_.GetSampler()->ShouldSample(parent, parent.trace_id(), name, options.kind, attributes, links); if (sampling_result.decision == Decision::DROP) { // Don't allocate a no-op span for every DROP decision, but use a static @@ -74,8 +58,8 @@ nostd::shared_ptr Tracer::StartSpan( else { auto span = nostd::shared_ptr{ - new (std::nothrow) Span{this->shared_from_this(), processor_.load(), name, attributes, - links, options, parent, resource_}}; + new (std::nothrow) Span{this->shared_from_this(), provider_.GetProcessor(), name, attributes, + links, options, parent, provider_.GetResource()}}; // if the attributes is not nullptr, add attributes to the span. if (sampling_result.attributes) diff --git a/sdk/src/trace/tracer_provider.cc b/sdk/src/trace/tracer_provider.cc index b37ae5de56..6803272947 100644 --- a/sdk/src/trace/tracer_provider.cc +++ b/sdk/src/trace/tracer_provider.cc @@ -9,9 +9,9 @@ TracerProvider::TracerProvider(std::shared_ptr processor, opentelemetry::sdk::resource::Resource &&resource, std::shared_ptr sampler) noexcept : processor_{processor}, - tracer_(new Tracer(std::move(processor), resource, sampler)), sampler_(sampler), - resource_(resource) + resource_(resource), + tracer_(new Tracer(*this)) {} opentelemetry::nostd::shared_ptr TracerProvider::GetTracer( @@ -24,9 +24,6 @@ opentelemetry::nostd::shared_ptr TracerProvider::G void TracerProvider::SetProcessor(std::shared_ptr processor) noexcept { processor_.store(processor); - - auto sdkTracer = static_cast(tracer_.get()); - sdkTracer->SetProcessor(processor); } std::shared_ptr TracerProvider::GetProcessor() const noexcept diff --git a/sdk/test/trace/sampler_benchmark.cc b/sdk/test/trace/sampler_benchmark.cc index 5606206ce6..3a01803859 100644 --- a/sdk/test/trace/sampler_benchmark.cc +++ b/sdk/test/trace/sampler_benchmark.cc @@ -8,6 +8,7 @@ #include "opentelemetry/sdk/trace/simple_processor.h" #include "opentelemetry/sdk/trace/span_data.h" #include "opentelemetry/sdk/trace/tracer.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" #include @@ -120,8 +121,9 @@ void BenchmarkSpanCreation(std::shared_ptr sampler, benchmark::State &s std::unique_ptr exporter(new InMemorySpanExporter()); auto processor = std::make_shared(std::move(exporter)); auto resource = opentelemetry::sdk::resource::Resource::Create({}); - auto tracer = - std::shared_ptr(new Tracer(processor, resource, sampler)); + auto tracer_provider = + std::shared_ptr(new TracerProvider(processor, std::move(resource), sampler)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); while (state.KeepRunning()) { diff --git a/sdk/test/trace/tracer_provider_test.cc b/sdk/test/trace/tracer_provider_test.cc index 47c7ccc827..320f85c673 100644 --- a/sdk/test/trace/tracer_provider_test.cc +++ b/sdk/test/trace/tracer_provider_test.cc @@ -24,17 +24,17 @@ TEST(TracerProvider, GetTracer) // Should return the same instance each time. ASSERT_EQ(t1, t2); + // TODO: These should be different, InstrumentationLibrary is different. ASSERT_EQ(t1, t3); // Should be an sdk::trace::Tracer with the processor attached. auto sdkTracer1 = dynamic_cast(t1.get()); ASSERT_NE(nullptr, sdkTracer1); - ASSERT_EQ(processor, sdkTracer1->GetProcessor()); - ASSERT_EQ("AlwaysOnSampler", sdkTracer1->GetSampler()->GetDescription()); + // TODO - figure out how to ensure tracer is attached to this instance. TracerProvider tp2(processor, Resource::Create({}), std::make_shared()); auto sdkTracer2 = dynamic_cast(tp2.GetTracer("test").get()); - ASSERT_EQ("AlwaysOffSampler", sdkTracer2->GetSampler()->GetDescription()); + // TODO - figure out how to ensure tracer is attached to this instance. } TEST(TracerProvider, GetSampler) diff --git a/sdk/test/trace/tracer_test.cc b/sdk/test/trace/tracer_test.cc index d244ef2528..2844928bac 100644 --- a/sdk/test/trace/tracer_test.cc +++ b/sdk/test/trace/tracer_test.cc @@ -1,4 +1,5 @@ #include "opentelemetry/sdk/trace/tracer.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" #include "opentelemetry/exporters/memory/in_memory_span_exporter.h" #include "opentelemetry/sdk/resource/resource.h" #include "opentelemetry/sdk/trace/samplers/always_off.h" @@ -47,22 +48,22 @@ class MockSampler final : public Sampler namespace { -std::shared_ptr initTracer( +std::shared_ptr initTracerProvider( std::unique_ptr &&exporter) { auto processor = std::make_shared(std::move(exporter)); auto resource = Resource::Create({}); - return std::shared_ptr(new Tracer(processor, resource)); + return std::shared_ptr(new TracerProvider(processor, std::move(resource))); } -std::shared_ptr initTracer( +std::shared_ptr initTracerProvider( std::unique_ptr &&exporter, std::shared_ptr sampler) { auto processor = std::make_shared(std::move(exporter)); auto resource = Resource::Create({}); - return std::shared_ptr(new Tracer(processor, resource, sampler)); + return std::shared_ptr(new TracerProvider(processor, std::move(resource), sampler)); } } // namespace @@ -70,7 +71,8 @@ TEST(Tracer, ToInMemorySpanExporter) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); auto span_first = tracer->StartSpan("span 1"); auto scope_first = tracer->WithActiveSpan(span_first); @@ -105,7 +107,8 @@ TEST(Tracer, StartSpanSampleOn) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer_on = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer_on = tracer_provider->GetTracer("test", "1.0"); tracer_on->StartSpan("span 1")->End(); @@ -121,7 +124,8 @@ TEST(Tracer, StartSpanSampleOff) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer_off = initTracer(std::move(exporter), std::make_shared()); + auto tracer_provider = initTracerProvider(std::move(exporter), std::make_shared()); + auto tracer_off = tracer_provider->GetTracer("test", "1.0"); // This span will not be recorded. tracer_off->StartSpan("span 2")->End(); @@ -135,7 +139,8 @@ TEST(Tracer, StartSpanWithOptionsTime) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); opentelemetry::trace::StartSpanOptions start; start.start_system_time = SystemTimestamp(std::chrono::nanoseconds(300)); @@ -158,7 +163,8 @@ TEST(Tracer, StartSpanWithAttributes) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); // Start a span with all supported scalar attribute types. tracer @@ -238,7 +244,8 @@ TEST(Tracer, StartSpanWithAttributesCopy) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); { std::unique_ptr> numbers(new std::vector); @@ -278,30 +285,13 @@ TEST(Tracer, StartSpanWithAttributesCopy) ASSERT_EQ("c", strings[2]); } -TEST(Tracer, GetSampler) -{ - auto resource = Resource::Create({}); - // Create a Tracer with a default AlwaysOnSampler - std::shared_ptr processor_1(new SimpleSpanProcessor(nullptr)); - std::shared_ptr tracer_on(new Tracer(std::move(processor_1), resource)); - - auto t1 = tracer_on->GetSampler(); - ASSERT_EQ("AlwaysOnSampler", t1->GetDescription()); - - // Create a Tracer with a AlwaysOffSampler - std::shared_ptr processor_2(new SimpleSpanProcessor(nullptr)); - std::shared_ptr tracer_off( - new Tracer(std::move(processor_2), resource, std::make_shared())); - - auto t2 = tracer_off->GetSampler(); - ASSERT_EQ("AlwaysOffSampler", t2->GetDescription()); -} TEST(Tracer, SpanSetAttribute) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); auto span = tracer->StartSpan("span 1"); @@ -319,7 +309,8 @@ TEST(Tracer, SpanSetEvents) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); auto span = tracer->StartSpan("span 1"); span->AddEvent("event 1"); @@ -344,7 +335,8 @@ TEST(Tracer, SpanSetLinks) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); { @@ -421,7 +413,8 @@ TEST(Tracer, TestAlwaysOnSampler) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer_on = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer_on = tracer_provider->GetTracer("test", "1.0"); // Testing AlwaysOn sampler. // Create two spans for each tracer. Check the exported result. @@ -440,7 +433,8 @@ TEST(Tracer, TestAlwaysOffSampler) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer_off = initTracer(std::move(exporter), std::make_shared()); + auto tracer_provider = initTracerProvider(std::move(exporter), std::make_shared()); + auto tracer_off = tracer_provider->GetTracer("test", "1.0"); auto span_off_1 = tracer_off->StartSpan("span 1"); auto span_off_2 = tracer_off->StartSpan("span 2"); @@ -459,9 +453,8 @@ TEST(Tracer, TestParentBasedSampler) // so this sampler will work as an AlwaysOnSampler. std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data_parent_on = exporter->GetData(); - auto tracer_parent_on = - initTracer(std::move(exporter), - std::make_shared(std::make_shared())); + auto tracer_provider = initTracerProvider(std::move(exporter), std::make_shared()); + auto tracer_parent_on = tracer_provider->GetTracer("test", "1.0"); auto span_parent_on_1 = tracer_parent_on->StartSpan("span 1"); auto span_parent_on_2 = tracer_parent_on->StartSpan("span 2"); @@ -480,9 +473,10 @@ TEST(Tracer, TestParentBasedSampler) // so this sampler will work as an AlwaysOnSampler. std::unique_ptr exporter2(new InMemorySpanExporter()); std::shared_ptr span_data_parent_off = exporter2->GetData(); - auto tracer_parent_off = - initTracer(std::move(exporter2), + auto tracer_provider_parent_off = + initTracerProvider(std::move(exporter2), std::make_shared(std::make_shared())); + auto tracer_parent_off = tracer_provider_parent_off->GetTracer("test", "1.0"); auto span_parent_off_1 = tracer_parent_off->StartSpan("span 1"); auto span_parent_off_2 = tracer_parent_off->StartSpan("span 2"); @@ -498,7 +492,8 @@ TEST(Tracer, WithActiveSpan) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); auto spans = span_data.get()->GetSpans(); ASSERT_EQ(0, spans.size()); @@ -533,7 +528,8 @@ TEST(Tracer, ExpectParent) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer = initTracer(std::move(exporter)); + auto tracer_provider = initTracerProvider(std::move(exporter)); + auto tracer = tracer_provider->GetTracer("test", "1.0"); auto spans = span_data.get()->GetSpans(); ASSERT_EQ(0, spans.size()); From f286918e2455829c7517bcd8bf1cd7fadcf78ff7 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 12 Feb 2021 16:37:25 -0500 Subject: [PATCH 3/8] Make Span's keep a reference to the generating Tracer. - Remove passing of Processor/Resource to span - Spans now lookup processor from associated Tracer This should allow us to add InstrumenationLibrary to `Tracer` and expose lookups for `Resource`/`InstrumentationLibrary` from `Span`. --- sdk/include/opentelemetry/sdk/trace/tracer.h | 7 +++++-- sdk/src/trace/span.cc | 15 +++++++-------- sdk/src/trace/span.h | 7 ++----- sdk/src/trace/tracer.cc | 8 +++----- sdk/src/trace/tracer_provider.cc | 2 +- sdk/test/trace/tracer_test.cc | 1 - 6 files changed, 18 insertions(+), 22 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/tracer.h b/sdk/include/opentelemetry/sdk/trace/tracer.h index 4d57b4bdce..5b5aa36b88 100644 --- a/sdk/include/opentelemetry/sdk/trace/tracer.h +++ b/sdk/include/opentelemetry/sdk/trace/tracer.h @@ -26,7 +26,7 @@ class Tracer final : public trace_api::Tracer, public std::enable_shared_from_th * Initialize a new tracer. * @param parent The `TraceProvider` which owns resource + pipeline for traces. */ - explicit Tracer(const TracerProvider& provider) noexcept; + explicit Tracer(TracerProvider* provider) noexcept; nostd::shared_ptr StartSpan( nostd::string_view name, @@ -38,8 +38,11 @@ class Tracer final : public trace_api::Tracer, public std::enable_shared_from_th void CloseWithMicroseconds(uint64_t timeout) noexcept override; + /** Returns the provider that created this Tracer. */ + const TracerProvider& GetProvider() const { return *provider_; } + private: - const TracerProvider& provider_; + TracerProvider* provider_; }; } // namespace trace } // namespace sdk diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index bcadef775d..bcf88892be 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -3,6 +3,7 @@ #include "opentelemetry/context/runtime_context.h" #include "opentelemetry/trace/trace_flags.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" #include "opentelemetry/version.h" OPENTELEMETRY_BEGIN_NAMESPACE @@ -58,19 +59,17 @@ trace_api::SpanId GenerateRandomSpanId() } Span::Span(std::shared_ptr &&tracer, - std::shared_ptr processor, nostd::string_view name, const opentelemetry::common::KeyValueIterable &attributes, const trace_api::SpanContextKeyValueIterable &links, const trace_api::StartSpanOptions &options, - const trace_api::SpanContext &parent_span_context, - const opentelemetry::sdk::resource::Resource &resource) noexcept + const trace_api::SpanContext &parent_span_context) noexcept : tracer_{std::move(tracer)}, - processor_{processor}, - recordable_{processor_->MakeRecordable()}, + recordable_{nullptr}, start_steady_time{options.start_steady_time}, has_ended_{false} { + recordable_ = tracer_->GetProvider().GetProcessor()->MakeRecordable(); if (recordable_ == nullptr) { return; @@ -109,8 +108,8 @@ Span::Span(std::shared_ptr &&tracer, recordable_->SetSpanKind(options.kind); recordable_->SetStartTime(NowOr(options.start_system_time)); start_steady_time = NowOr(options.start_steady_time); - // recordable_->SetResource(resource_); TODO - processor_->OnStart(*recordable_, parent_span_context); + // recordable_->SetResourceRef(tracer_->GetProvider().GetResource()); TODO + tracer_->GetProvider().GetProcessor()->OnStart(*recordable_, parent_span_context); } Span::~Span() @@ -197,7 +196,7 @@ void Span::End(const trace_api::EndSpanOptions &options) noexcept recordable_->SetDuration(std::chrono::steady_clock::time_point(end_steady_time) - std::chrono::steady_clock::time_point(start_steady_time)); - processor_->OnEnd(std::move(recordable_)); + tracer_->GetProvider().GetProcessor()->OnEnd(std::move(recordable_)); recordable_.reset(); } diff --git a/sdk/src/trace/span.h b/sdk/src/trace/span.h index b44dcdb3bc..80f7ebfc0b 100644 --- a/sdk/src/trace/span.h +++ b/sdk/src/trace/span.h @@ -16,13 +16,11 @@ class Span final : public trace_api::Span { public: explicit Span(std::shared_ptr &&tracer, - std::shared_ptr processor, nostd::string_view name, const opentelemetry::common::KeyValueIterable &attributes, const trace_api::SpanContextKeyValueIterable &links, const trace_api::StartSpanOptions &options, - const trace_api::SpanContext &parent_span_context, - const opentelemetry::sdk::resource::Resource &resource) noexcept; + const trace_api::SpanContext &parent_span_context) noexcept; ~Span() override; @@ -49,8 +47,7 @@ class Span final : public trace_api::Span trace_api::SpanContext GetContext() const noexcept override { return *span_context_.get(); } private: - std::shared_ptr tracer_; - std::shared_ptr processor_; + std::shared_ptr tracer_; mutable std::mutex mu_; std::unique_ptr recordable_; opentelemetry::core::SteadyTimestamp start_steady_time; diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc index 8183e81424..e7eaf5bcf7 100644 --- a/sdk/src/trace/tracer.cc +++ b/sdk/src/trace/tracer.cc @@ -11,7 +11,7 @@ namespace sdk { namespace trace { -Tracer::Tracer(const opentelemetry::sdk::trace::TracerProvider& provider) noexcept +Tracer::Tracer(opentelemetry::sdk::trace::TracerProvider* provider) noexcept : provider_ { provider } {} @@ -43,9 +43,8 @@ nostd::shared_ptr Tracer::StartSpan( const trace_api::StartSpanOptions &options) noexcept { trace_api::SpanContext parent = GetCurrentSpanContext(options.parent); - auto sampling_result = - provider_.GetSampler()->ShouldSample(parent, parent.trace_id(), name, options.kind, attributes, links); + provider_->GetSampler()->ShouldSample(parent, parent.trace_id(), name, options.kind, attributes, links); if (sampling_result.decision == Decision::DROP) { // Don't allocate a no-op span for every DROP decision, but use a static @@ -58,8 +57,7 @@ nostd::shared_ptr Tracer::StartSpan( else { auto span = nostd::shared_ptr{ - new (std::nothrow) Span{this->shared_from_this(), provider_.GetProcessor(), name, attributes, - links, options, parent, provider_.GetResource()}}; + new (std::nothrow) Span{this->shared_from_this(), name, attributes, links, options, parent}}; // if the attributes is not nullptr, add attributes to the span. if (sampling_result.attributes) diff --git a/sdk/src/trace/tracer_provider.cc b/sdk/src/trace/tracer_provider.cc index 6803272947..b875f6150c 100644 --- a/sdk/src/trace/tracer_provider.cc +++ b/sdk/src/trace/tracer_provider.cc @@ -11,7 +11,7 @@ TracerProvider::TracerProvider(std::shared_ptr processor, : processor_{processor}, sampler_(sampler), resource_(resource), - tracer_(new Tracer(*this)) + tracer_(new Tracer(this)) {} opentelemetry::nostd::shared_ptr TracerProvider::GetTracer( diff --git a/sdk/test/trace/tracer_test.cc b/sdk/test/trace/tracer_test.cc index 2844928bac..cda024b790 100644 --- a/sdk/test/trace/tracer_test.cc +++ b/sdk/test/trace/tracer_test.cc @@ -73,7 +73,6 @@ TEST(Tracer, ToInMemorySpanExporter) std::shared_ptr span_data = exporter->GetData(); auto tracer_provider = initTracerProvider(std::move(exporter)); auto tracer = tracer_provider->GetTracer("test", "1.0"); - auto span_first = tracer->StartSpan("span 1"); auto scope_first = tracer->WithActiveSpan(span_first); auto span_second = tracer->StartSpan("span 2"); From 9d9246c10c36e5c6157907598a41d7aa5626eaa3 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 12 Feb 2021 16:42:31 -0500 Subject: [PATCH 4/8] Ensure Resource references are passed into Recordables. Add TODO for instrumentation library. --- sdk/src/trace/span.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index bcf88892be..d32d083277 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -108,7 +108,8 @@ Span::Span(std::shared_ptr &&tracer, recordable_->SetSpanKind(options.kind); recordable_->SetStartTime(NowOr(options.start_system_time)); start_steady_time = NowOr(options.start_steady_time); - // recordable_->SetResourceRef(tracer_->GetProvider().GetResource()); TODO + recordable_->SetResourceRef(&tracer_->GetProvider().GetResource()); + // TODO: recordable->SetInstrumentationLibraryRef(&tracer_->GetInstrumentationLibrary()); tracer_->GetProvider().GetProcessor()->OnStart(*recordable_, parent_span_context); } From ea996eba4d97feb07c9a8a20d6e59f52030fac70 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 12 Feb 2021 17:38:47 -0500 Subject: [PATCH 5/8] Add helper method to otlp Recordable to create Resource proto. --- .../opentelemetry/exporters/otlp/recordable.h | 4 + exporters/otlp/src/recordable.cc | 116 +++++++++++++++++- exporters/otlp/test/recordable_test.cc | 16 +++ 3 files changed, 132 insertions(+), 4 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h index 8b3092ed91..1dad072c9a 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h @@ -1,5 +1,6 @@ #pragma once +#include "opentelemetry/proto/resource/v1/resource.pb.h" #include "opentelemetry/proto/trace/v1/trace.pb.h" #include "opentelemetry/sdk/trace/recordable.h" #include "opentelemetry/version.h" @@ -13,6 +14,8 @@ class Recordable final : public sdk::trace::Recordable { public: const proto::trace::v1::Span &span() const noexcept { return span_; } + /** Dynamically converts the resource of this span into a proto. */ + proto::resource::v1::Resource resource() const noexcept; void SetIds(trace::TraceId trace_id, trace::SpanId span_id, @@ -42,6 +45,7 @@ class Recordable final : public sdk::trace::Recordable private: proto::trace::v1::Span span_; + const opentelemetry::sdk::resource::Resource* resource_; }; } // namespace otlp } // namespace exporter diff --git a/exporters/otlp/src/recordable.cc b/exporters/otlp/src/recordable.cc index 635cf17807..159949bf26 100644 --- a/exporters/otlp/src/recordable.cc +++ b/exporters/otlp/src/recordable.cc @@ -7,6 +7,7 @@ namespace otlp { const int kAttributeValueSize = 14; +const int kOwnedAttributeValueSize = 14; void Recordable::SetIds(trace::TraceId trace_id, trace::SpanId span_id, @@ -117,6 +118,117 @@ void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, } } +/** Maps from C++ attribute into OTLP proto attribute. */ +void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, + nostd::string_view key, + const sdk::common::OwnedAttributeValue &value) { + // Assert size of variant to ensure that this method gets updated if the variant + // definition changes + static_assert( + nostd::variant_size::value == kOwnedAttributeValueSize, + "AttributeValue contains unknown type"); + + attribute->set_key(key.data(), key.size()); + + if (nostd::holds_alternative(value)) + { + attribute->mutable_value()->set_bool_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + attribute->mutable_value()->set_int_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + attribute->mutable_value()->set_int_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + attribute->mutable_value()->set_int_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + attribute->mutable_value()->set_int_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + attribute->mutable_value()->set_double_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + attribute->mutable_value()->set_string_value(nostd::get(value)); + } +#ifdef HAVE_SPAN_BYTE + else if (nostd::holds_alternative(value)) + { + attribute->mutable_value()->set_int_value(nostd::get(value)); + } +#endif + else if (nostd::holds_alternative>(value)) + { + for (const auto &val : nostd::get>(value)) + { + attribute->mutable_value()->mutable_array_value()->add_values()->set_bool_value(val); + } + } + else if (nostd::holds_alternative>(value)) + { + for (const auto &val : nostd::get>(value)) + { + attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val); + } + } + else if (nostd::holds_alternative>(value)) + { + for (const auto &val : nostd::get>(value)) + { + attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val); + } + } + else if (nostd::holds_alternative>(value)) + { + for (const auto &val : nostd::get>(value)) + { + attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val); + } + } + else if (nostd::holds_alternative>(value)) + { + for (const auto &val : nostd::get>(value)) + { + attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val); + } + } + else if (nostd::holds_alternative>(value)) + { + for (const auto &val : nostd::get>(value)) + { + attribute->mutable_value()->mutable_array_value()->add_values()->set_double_value(val); + } + } + else if (nostd::holds_alternative>(value)) + { + for (const auto &val : nostd::get>(value)) + { + attribute->mutable_value()->mutable_array_value()->add_values()->set_string_value(val); + } + } +} + +proto::resource::v1::Resource Recordable::resource() const noexcept { + proto::resource::v1::Resource proto; + if (resource_) { + for (const auto& kv : resource_->GetAttributes()) { + PopulateAttribute(proto.add_attributes(), kv.first, kv.second); + } + } + return proto; +} + +void Recordable::SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept { + resource_ = resource; + }; + void Recordable::SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept { @@ -218,10 +330,6 @@ void Recordable::SetDuration(std::chrono::nanoseconds duration) noexcept span_.set_end_time_unix_nano(unix_end_time); } -void Recordable::SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept { - // TODO - Do something. -} - } // namespace otlp } // namespace exporter OPENTELEMETRY_END_NAMESPACE diff --git a/exporters/otlp/test/recordable_test.cc b/exporters/otlp/test/recordable_test.cc index b570027d0f..a80f1f1bf2 100644 --- a/exporters/otlp/test/recordable_test.cc +++ b/exporters/otlp/test/recordable_test.cc @@ -200,6 +200,22 @@ TEST(Recordable, SetArrayAtrribute) } } +TEST(Recordable, SetResourceRef) +{ + Recordable rec; + sdk::resource::Resource default_resource = sdk::resource::Resource::GetDefault(); + sdk::resource::Resource empty_resource = sdk::resource::Resource::GetEmpty(); + rec.SetResourceRef(&default_resource); + auto proto = rec.resource(); + EXPECT_EQ(3, proto.attributes_size()); + // Note: We don't test ALL attribtues, only a few to avoid flaky tests and to + // make sure we ARE getting attributes passed through. + EXPECT_EQ("telemetry.sdk.name", proto.attributes(1).key()); + EXPECT_EQ("opentelemetry", proto.attributes(1).value().string_value()); + rec.SetResourceRef(&empty_resource); + EXPECT_EQ(0, rec.resource().attributes_size()); +} + /** * AttributeValue can contain different int types, such as int, int64_t, * unsigned int, and uint64_t. To avoid writing test cases for each, we can From 474a0a56e662ade0760c82f1f7fc458763a5950f Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 12 Feb 2021 18:04:40 -0500 Subject: [PATCH 6/8] Wire `Resource` into OTLP exporter. - OTLP exporter now correctly constructs a `Resource` for spans. - Expand OTLP tests to do a modicum of content validation. --- .../opentelemetry/exporters/otlp/recordable.h | 2 +- exporters/otlp/src/otlp_exporter.cc | 8 +++- exporters/otlp/test/otlp_exporter_test.cc | 44 +++++++++++++------ 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h index 1dad072c9a..e3d768db4a 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h @@ -45,7 +45,7 @@ class Recordable final : public sdk::trace::Recordable private: proto::trace::v1::Span span_; - const opentelemetry::sdk::resource::Resource* resource_; + const opentelemetry::sdk::resource::Resource* resource_ { nullptr }; }; } // namespace otlp } // namespace exporter diff --git a/exporters/otlp/src/otlp_exporter.cc b/exporters/otlp/src/otlp_exporter.cc index 63db30436c..de623081b3 100644 --- a/exporters/otlp/src/otlp_exporter.cc +++ b/exporters/otlp/src/otlp_exporter.cc @@ -22,10 +22,16 @@ void PopulateRequest(const nostd::span> { auto resource_span = request->add_resource_spans(); auto instrumentation_lib = resource_span->add_instrumentation_library_spans(); - + bool has_resource = false; for (auto &recordable : spans) { auto rec = std::unique_ptr(static_cast(recordable.release())); + // We assume all the spans are for the same resource. + if (!has_resource) { + // *resource_span->mutable_resource() = std::move(rec->resource()); + *resource_span->mutable_resource() = rec->resource(); + has_resource = true; + } *instrumentation_lib->add_spans() = std::move(rec->span()); } } diff --git a/exporters/otlp/test/otlp_exporter_test.cc b/exporters/otlp/test/otlp_exporter_test.cc index a221365179..74974ae2b8 100644 --- a/exporters/otlp/test/otlp_exporter_test.cc +++ b/exporters/otlp/test/otlp_exporter_test.cc @@ -65,22 +65,40 @@ TEST_F(OtlpExporterTestPeer, ExportIntegrationTest) std::unique_ptr stub_interface( mock_stub); - auto exporter = GetExporter(stub_interface); + proto::collector::trace::v1::ExportTraceServiceRequest request; - auto processor = std::shared_ptr( - new sdk::trace::SimpleSpanProcessor(std::move(exporter))); - auto provider = - nostd::shared_ptr(new sdk::trace::TracerProvider(processor)); - auto tracer = provider->GetTracer("test"); + // Create a scope where all traces are flushed within it + { + auto exporter = GetExporter(stub_interface); - EXPECT_CALL(*mock_stub, Export(_, _, _)) - .Times(AtLeast(1)) - .WillRepeatedly(Return(grpc::Status::OK)); + auto processor = std::shared_ptr( + new sdk::trace::SimpleSpanProcessor(std::move(exporter))); + auto provider = + nostd::shared_ptr(new sdk::trace::TracerProvider(processor)); + auto tracer = provider->GetTracer("test"); - auto parent_span = tracer->StartSpan("Test parent span"); - auto child_span = tracer->StartSpan("Test child span"); - child_span->End(); - parent_span->End(); + // TODO: Capture the exported proto and inspect it. + EXPECT_CALL(*mock_stub, Export(_, _, _)) + .Times(AtLeast(1)) + .WillRepeatedly( + DoAll( + SaveArg<1>(&request), + Return(grpc::Status::OK) + )); + + auto parent_span = tracer->StartSpan("Test parent span"); + auto child_span = tracer->StartSpan("Test child span"); + child_span->End(); + parent_span->End(); + } + // After flushing, let's check the (last) request has the last span. + EXPECT_EQ(1, request.resource_spans_size()); + EXPECT_EQ(1, request.resource_spans(0).instrumentation_library_spans_size()); + EXPECT_EQ(1, request.resource_spans(0).instrumentation_library_spans(0).spans_size()); + // Expect the default resource to have been used. + EXPECT_EQ(3, request.resource_spans(0).resource().attributes_size()); + // And that the span name is the last span ended. + EXPECT_EQ("Test parent span", request.resource_spans(0).instrumentation_library_spans(0).spans(0).name()); } // Test exporter configuration options From 8528f64e42f26c4b88540e4c79e3b019a06caed7 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 12 Feb 2021 18:11:18 -0500 Subject: [PATCH 7/8] Run style formatter. --- .../opentelemetry/exporters/etw/etw_data.h | 4 +- .../opentelemetry/exporters/otlp/recordable.h | 5 ++- exporters/otlp/src/otlp_exporter.cc | 7 ++-- exporters/otlp/src/recordable.cc | 24 +++++++---- exporters/otlp/test/otlp_exporter_test.cc | 9 ++-- exporters/otlp/test/recordable_test.cc | 2 +- .../ext/zpages/threadsafe_span_data.h | 4 +- .../zpages/tracez_data_aggregator_test.cc | 5 ++- ext/test/zpages/tracez_processor_test.cc | 3 +- .../opentelemetry/sdk/trace/recordable.h | 7 ++-- .../opentelemetry/sdk/trace/span_data.h | 4 +- sdk/include/opentelemetry/sdk/trace/tracer.h | 6 +-- sdk/src/trace/span.cc | 2 +- sdk/src/trace/tracer.cc | 13 +++--- sdk/src/trace/tracer_provider.cc | 5 +-- sdk/test/trace/sampler_benchmark.cc | 2 +- sdk/test/trace/tracer_test.cc | 41 ++++++++++--------- 17 files changed, 77 insertions(+), 66 deletions(-) diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h index 4ed6c8b932..81f92aaef1 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h @@ -465,7 +465,9 @@ class ETWSpanData final : public sdk::trace::Recordable parent_span_id_ = parent_span_id; } - void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept override { + void SetResourceRef( + const opentelemetry::sdk::resource::Resource *const resource) noexcept override + { // TODO } diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h index e3d768db4a..1e1c73561f 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h @@ -21,7 +21,8 @@ class Recordable final : public sdk::trace::Recordable trace::SpanId span_id, trace::SpanId parent_span_id) noexcept override; - void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept override; + void SetResourceRef( + const opentelemetry::sdk::resource::Resource *const resource) noexcept override; void SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept override; @@ -45,7 +46,7 @@ class Recordable final : public sdk::trace::Recordable private: proto::trace::v1::Span span_; - const opentelemetry::sdk::resource::Resource* resource_ { nullptr }; + const opentelemetry::sdk::resource::Resource *resource_{nullptr}; }; } // namespace otlp } // namespace exporter diff --git a/exporters/otlp/src/otlp_exporter.cc b/exporters/otlp/src/otlp_exporter.cc index de623081b3..63430d4d5e 100644 --- a/exporters/otlp/src/otlp_exporter.cc +++ b/exporters/otlp/src/otlp_exporter.cc @@ -22,15 +22,16 @@ void PopulateRequest(const nostd::span> { auto resource_span = request->add_resource_spans(); auto instrumentation_lib = resource_span->add_instrumentation_library_spans(); - bool has_resource = false; + bool has_resource = false; for (auto &recordable : spans) { auto rec = std::unique_ptr(static_cast(recordable.release())); // We assume all the spans are for the same resource. - if (!has_resource) { + if (!has_resource) + { // *resource_span->mutable_resource() = std::move(rec->resource()); *resource_span->mutable_resource() = rec->resource(); - has_resource = true; + has_resource = true; } *instrumentation_lib->add_spans() = std::move(rec->span()); } diff --git a/exporters/otlp/src/recordable.cc b/exporters/otlp/src/recordable.cc index 159949bf26..b31dbc92b2 100644 --- a/exporters/otlp/src/recordable.cc +++ b/exporters/otlp/src/recordable.cc @@ -6,7 +6,7 @@ namespace exporter namespace otlp { -const int kAttributeValueSize = 14; +const int kAttributeValueSize = 14; const int kOwnedAttributeValueSize = 14; void Recordable::SetIds(trace::TraceId trace_id, @@ -121,7 +121,8 @@ void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, /** Maps from C++ attribute into OTLP proto attribute. */ void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, nostd::string_view key, - const sdk::common::OwnedAttributeValue &value) { + const sdk::common::OwnedAttributeValue &value) +{ // Assert size of variant to ensure that this method gets updated if the variant // definition changes static_assert( @@ -184,7 +185,7 @@ void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, { attribute->mutable_value()->mutable_array_value()->add_values()->set_int_value(val); } - } + } else if (nostd::holds_alternative>(value)) { for (const auto &val : nostd::get>(value)) @@ -215,19 +216,24 @@ void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, } } -proto::resource::v1::Resource Recordable::resource() const noexcept { +proto::resource::v1::Resource Recordable::resource() const noexcept +{ proto::resource::v1::Resource proto; - if (resource_) { - for (const auto& kv : resource_->GetAttributes()) { + if (resource_) + { + for (const auto &kv : resource_->GetAttributes()) + { PopulateAttribute(proto.add_attributes(), kv.first, kv.second); } } return proto; } -void Recordable::SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept { - resource_ = resource; - }; +void Recordable::SetResourceRef( + const opentelemetry::sdk::resource::Resource *const resource) noexcept +{ + resource_ = resource; +}; void Recordable::SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept diff --git a/exporters/otlp/test/otlp_exporter_test.cc b/exporters/otlp/test/otlp_exporter_test.cc index 74974ae2b8..1c953b2599 100644 --- a/exporters/otlp/test/otlp_exporter_test.cc +++ b/exporters/otlp/test/otlp_exporter_test.cc @@ -80,11 +80,7 @@ TEST_F(OtlpExporterTestPeer, ExportIntegrationTest) // TODO: Capture the exported proto and inspect it. EXPECT_CALL(*mock_stub, Export(_, _, _)) .Times(AtLeast(1)) - .WillRepeatedly( - DoAll( - SaveArg<1>(&request), - Return(grpc::Status::OK) - )); + .WillRepeatedly(DoAll(SaveArg<1>(&request), Return(grpc::Status::OK))); auto parent_span = tracer->StartSpan("Test parent span"); auto child_span = tracer->StartSpan("Test child span"); @@ -98,7 +94,8 @@ TEST_F(OtlpExporterTestPeer, ExportIntegrationTest) // Expect the default resource to have been used. EXPECT_EQ(3, request.resource_spans(0).resource().attributes_size()); // And that the span name is the last span ended. - EXPECT_EQ("Test parent span", request.resource_spans(0).instrumentation_library_spans(0).spans(0).name()); + EXPECT_EQ("Test parent span", + request.resource_spans(0).instrumentation_library_spans(0).spans(0).name()); } // Test exporter configuration options diff --git a/exporters/otlp/test/recordable_test.cc b/exporters/otlp/test/recordable_test.cc index a80f1f1bf2..1211ecf1a2 100644 --- a/exporters/otlp/test/recordable_test.cc +++ b/exporters/otlp/test/recordable_test.cc @@ -204,7 +204,7 @@ TEST(Recordable, SetResourceRef) { Recordable rec; sdk::resource::Resource default_resource = sdk::resource::Resource::GetDefault(); - sdk::resource::Resource empty_resource = sdk::resource::Resource::GetEmpty(); + sdk::resource::Resource empty_resource = sdk::resource::Resource::GetEmpty(); rec.SetResourceRef(&default_resource); auto proto = rec.resource(); EXPECT_EQ(3, proto.attributes_size()); diff --git a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h index 4fc2435788..7f53b806ca 100644 --- a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h +++ b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h @@ -127,7 +127,9 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable parent_span_id_ = parent_span_id; } - void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept override { + void SetResourceRef( + const opentelemetry::sdk::resource::Resource *const resource) noexcept override + { // TODO } diff --git a/ext/test/zpages/tracez_data_aggregator_test.cc b/ext/test/zpages/tracez_data_aggregator_test.cc index 0ff1892420..ed2a5cc18e 100644 --- a/ext/test/zpages/tracez_data_aggregator_test.cc +++ b/ext/test/zpages/tracez_data_aggregator_test.cc @@ -37,8 +37,9 @@ class TracezDataAggregatorTest : public ::testing::Test { std::shared_ptr processor(new TracezSpanProcessor()); auto resource = opentelemetry::sdk::resource::Resource::Create({}); - tracer_provider = std::shared_ptr(new TracerProvider(processor, std::move(resource))); - tracer = tracer_provider->GetTracer("test", "1.0"); + tracer_provider = + std::shared_ptr(new TracerProvider(processor, std::move(resource))); + tracer = tracer_provider->GetTracer("test", "1.0"); tracez_data_aggregator = std::unique_ptr( new TracezDataAggregator(processor, milliseconds(10))); } diff --git a/ext/test/zpages/tracez_processor_test.cc b/ext/test/zpages/tracez_processor_test.cc index 7d9eebd059..9ffe9c5ba0 100644 --- a/ext/test/zpages/tracez_processor_test.cc +++ b/ext/test/zpages/tracez_processor_test.cc @@ -178,7 +178,8 @@ class TracezProcessor : public ::testing::Test { processor = std::shared_ptr(new TracezSpanProcessor()); auto resource = opentelemetry::sdk::resource::Resource::Create({}); - tracer_provider = std::shared_ptr(new TracerProvider(processor, std::move(resource))); + tracer_provider = + std::shared_ptr(new TracerProvider(processor, std::move(resource))); tracer = tracer_provider->GetTracer("test", "1.0"); auto spans = processor->GetSpanSnapshot(); running = spans.running; diff --git a/sdk/include/opentelemetry/sdk/trace/recordable.h b/sdk/include/opentelemetry/sdk/trace/recordable.h index 779873c928..38e928f9f9 100644 --- a/sdk/include/opentelemetry/sdk/trace/recordable.h +++ b/sdk/include/opentelemetry/sdk/trace/recordable.h @@ -44,14 +44,15 @@ class Recordable /** * Sets the resource associated with a span. - * + * *

The resource is guaranteed to have a lifetime longer than this recordable. It is * owned by the `TracerProvider` which (indirectly) owns the `Exporter` that generates a * `Recordable`. - * + * * @param resource pointer to the Resource for this span. */ - virtual void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept = 0; + virtual void SetResourceRef( + const opentelemetry::sdk::resource::Resource *const resource) noexcept = 0; /** * Set an attribute of a span. diff --git a/sdk/include/opentelemetry/sdk/trace/span_data.h b/sdk/include/opentelemetry/sdk/trace/span_data.h index 2080c9d9a8..655b3c6716 100644 --- a/sdk/include/opentelemetry/sdk/trace/span_data.h +++ b/sdk/include/opentelemetry/sdk/trace/span_data.h @@ -221,7 +221,9 @@ class SpanData final : public Recordable void SetDuration(std::chrono::nanoseconds duration) noexcept override { duration_ = duration; } - void SetResourceRef(const opentelemetry::sdk::resource::Resource*const resource) noexcept override { + void SetResourceRef( + const opentelemetry::sdk::resource::Resource *const resource) noexcept override + { // TODO } diff --git a/sdk/include/opentelemetry/sdk/trace/tracer.h b/sdk/include/opentelemetry/sdk/trace/tracer.h index 5b5aa36b88..6563eb6309 100644 --- a/sdk/include/opentelemetry/sdk/trace/tracer.h +++ b/sdk/include/opentelemetry/sdk/trace/tracer.h @@ -26,7 +26,7 @@ class Tracer final : public trace_api::Tracer, public std::enable_shared_from_th * Initialize a new tracer. * @param parent The `TraceProvider` which owns resource + pipeline for traces. */ - explicit Tracer(TracerProvider* provider) noexcept; + explicit Tracer(TracerProvider *provider) noexcept; nostd::shared_ptr StartSpan( nostd::string_view name, @@ -39,10 +39,10 @@ class Tracer final : public trace_api::Tracer, public std::enable_shared_from_th void CloseWithMicroseconds(uint64_t timeout) noexcept override; /** Returns the provider that created this Tracer. */ - const TracerProvider& GetProvider() const { return *provider_; } + const TracerProvider &GetProvider() const { return *provider_; } private: - TracerProvider* provider_; + TracerProvider *provider_; }; } // namespace trace } // namespace sdk diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index d32d083277..9d987c84fe 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -2,8 +2,8 @@ #include "src/common/random.h" #include "opentelemetry/context/runtime_context.h" -#include "opentelemetry/trace/trace_flags.h" #include "opentelemetry/sdk/trace/tracer_provider.h" +#include "opentelemetry/trace/trace_flags.h" #include "opentelemetry/version.h" OPENTELEMETRY_BEGIN_NAMESPACE diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc index e7eaf5bcf7..e912a745c9 100644 --- a/sdk/src/trace/tracer.cc +++ b/sdk/src/trace/tracer.cc @@ -1,8 +1,8 @@ #include "opentelemetry/sdk/trace/tracer.h" -#include "opentelemetry/sdk/trace/tracer_provider.h" #include "opentelemetry/context/runtime_context.h" #include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/sdk/common/atomic_shared_ptr.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" #include "opentelemetry/version.h" #include "src/trace/span.h" @@ -11,8 +11,7 @@ namespace sdk { namespace trace { -Tracer::Tracer(opentelemetry::sdk::trace::TracerProvider* provider) noexcept - : provider_ { provider } +Tracer::Tracer(opentelemetry::sdk::trace::TracerProvider *provider) noexcept : provider_{provider} {} trace_api::SpanContext GetCurrentSpanContext(const trace_api::SpanContext &explicit_parent) @@ -43,8 +42,8 @@ nostd::shared_ptr Tracer::StartSpan( const trace_api::StartSpanOptions &options) noexcept { trace_api::SpanContext parent = GetCurrentSpanContext(options.parent); - auto sampling_result = - provider_->GetSampler()->ShouldSample(parent, parent.trace_id(), name, options.kind, attributes, links); + auto sampling_result = provider_->GetSampler()->ShouldSample(parent, parent.trace_id(), name, + options.kind, attributes, links); if (sampling_result.decision == Decision::DROP) { // Don't allocate a no-op span for every DROP decision, but use a static @@ -56,8 +55,8 @@ nostd::shared_ptr Tracer::StartSpan( } else { - auto span = nostd::shared_ptr{ - new (std::nothrow) Span{this->shared_from_this(), name, attributes, links, options, parent}}; + auto span = nostd::shared_ptr{new (std::nothrow) Span{ + this->shared_from_this(), name, attributes, links, options, parent}}; // if the attributes is not nullptr, add attributes to the span. if (sampling_result.attributes) diff --git a/sdk/src/trace/tracer_provider.cc b/sdk/src/trace/tracer_provider.cc index b875f6150c..ee57dc2eb8 100644 --- a/sdk/src/trace/tracer_provider.cc +++ b/sdk/src/trace/tracer_provider.cc @@ -8,10 +8,7 @@ namespace trace TracerProvider::TracerProvider(std::shared_ptr processor, opentelemetry::sdk::resource::Resource &&resource, std::shared_ptr sampler) noexcept - : processor_{processor}, - sampler_(sampler), - resource_(resource), - tracer_(new Tracer(this)) + : processor_{processor}, sampler_(sampler), resource_(resource), tracer_(new Tracer(this)) {} opentelemetry::nostd::shared_ptr TracerProvider::GetTracer( diff --git a/sdk/test/trace/sampler_benchmark.cc b/sdk/test/trace/sampler_benchmark.cc index 3a01803859..659df7c702 100644 --- a/sdk/test/trace/sampler_benchmark.cc +++ b/sdk/test/trace/sampler_benchmark.cc @@ -122,7 +122,7 @@ void BenchmarkSpanCreation(std::shared_ptr sampler, benchmark::State &s auto processor = std::make_shared(std::move(exporter)); auto resource = opentelemetry::sdk::resource::Resource::Create({}); auto tracer_provider = - std::shared_ptr(new TracerProvider(processor, std::move(resource), sampler)); + std::shared_ptr(new TracerProvider(processor, std::move(resource), sampler)); auto tracer = tracer_provider->GetTracer("test", "1.0"); while (state.KeepRunning()) diff --git a/sdk/test/trace/tracer_test.cc b/sdk/test/trace/tracer_test.cc index cda024b790..f725ed0622 100644 --- a/sdk/test/trace/tracer_test.cc +++ b/sdk/test/trace/tracer_test.cc @@ -1,5 +1,4 @@ #include "opentelemetry/sdk/trace/tracer.h" -#include "opentelemetry/sdk/trace/tracer_provider.h" #include "opentelemetry/exporters/memory/in_memory_span_exporter.h" #include "opentelemetry/sdk/resource/resource.h" #include "opentelemetry/sdk/trace/samplers/always_off.h" @@ -7,6 +6,7 @@ #include "opentelemetry/sdk/trace/samplers/parent.h" #include "opentelemetry/sdk/trace/simple_processor.h" #include "opentelemetry/sdk/trace/span_data.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" #include @@ -48,22 +48,21 @@ class MockSampler final : public Sampler namespace { -std::shared_ptr initTracerProvider( - std::unique_ptr &&exporter) +std::shared_ptr initTracerProvider(std::unique_ptr &&exporter) { auto processor = std::make_shared(std::move(exporter)); auto resource = Resource::Create({}); return std::shared_ptr(new TracerProvider(processor, std::move(resource))); } -std::shared_ptr initTracerProvider( - std::unique_ptr &&exporter, - std::shared_ptr sampler) +std::shared_ptr initTracerProvider(std::unique_ptr &&exporter, + std::shared_ptr sampler) { auto processor = std::make_shared(std::move(exporter)); auto resource = Resource::Create({}); - return std::shared_ptr(new TracerProvider(processor, std::move(resource), sampler)); + return std::shared_ptr( + new TracerProvider(processor, std::move(resource), sampler)); } } // namespace @@ -73,9 +72,9 @@ TEST(Tracer, ToInMemorySpanExporter) std::shared_ptr span_data = exporter->GetData(); auto tracer_provider = initTracerProvider(std::move(exporter)); auto tracer = tracer_provider->GetTracer("test", "1.0"); - auto span_first = tracer->StartSpan("span 1"); - auto scope_first = tracer->WithActiveSpan(span_first); - auto span_second = tracer->StartSpan("span 2"); + auto span_first = tracer->StartSpan("span 1"); + auto scope_first = tracer->WithActiveSpan(span_first); + auto span_second = tracer->StartSpan("span 2"); ASSERT_EQ(0, span_data->GetSpans().size()); @@ -123,8 +122,9 @@ TEST(Tracer, StartSpanSampleOff) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer_provider = initTracerProvider(std::move(exporter), std::make_shared()); - auto tracer_off = tracer_provider->GetTracer("test", "1.0"); + auto tracer_provider = + initTracerProvider(std::move(exporter), std::make_shared()); + auto tracer_off = tracer_provider->GetTracer("test", "1.0"); // This span will not be recorded. tracer_off->StartSpan("span 2")->End(); @@ -284,7 +284,6 @@ TEST(Tracer, StartSpanWithAttributesCopy) ASSERT_EQ("c", strings[2]); } - TEST(Tracer, SpanSetAttribute) { std::unique_ptr exporter(new InMemorySpanExporter()); @@ -432,8 +431,9 @@ TEST(Tracer, TestAlwaysOffSampler) { std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data = exporter->GetData(); - auto tracer_provider = initTracerProvider(std::move(exporter), std::make_shared()); - auto tracer_off = tracer_provider->GetTracer("test", "1.0"); + auto tracer_provider = + initTracerProvider(std::move(exporter), std::make_shared()); + auto tracer_off = tracer_provider->GetTracer("test", "1.0"); auto span_off_1 = tracer_off->StartSpan("span 1"); auto span_off_2 = tracer_off->StartSpan("span 2"); @@ -452,8 +452,9 @@ TEST(Tracer, TestParentBasedSampler) // so this sampler will work as an AlwaysOnSampler. std::unique_ptr exporter(new InMemorySpanExporter()); std::shared_ptr span_data_parent_on = exporter->GetData(); - auto tracer_provider = initTracerProvider(std::move(exporter), std::make_shared()); - auto tracer_parent_on = tracer_provider->GetTracer("test", "1.0"); + auto tracer_provider = + initTracerProvider(std::move(exporter), std::make_shared()); + auto tracer_parent_on = tracer_provider->GetTracer("test", "1.0"); auto span_parent_on_1 = tracer_parent_on->StartSpan("span 1"); auto span_parent_on_2 = tracer_parent_on->StartSpan("span 2"); @@ -472,9 +473,9 @@ TEST(Tracer, TestParentBasedSampler) // so this sampler will work as an AlwaysOnSampler. std::unique_ptr exporter2(new InMemorySpanExporter()); std::shared_ptr span_data_parent_off = exporter2->GetData(); - auto tracer_provider_parent_off = - initTracerProvider(std::move(exporter2), - std::make_shared(std::make_shared())); + auto tracer_provider_parent_off = initTracerProvider( + std::move(exporter2), + std::make_shared(std::make_shared())); auto tracer_parent_off = tracer_provider_parent_off->GetTracer("test", "1.0"); auto span_parent_off_1 = tracer_parent_off->StartSpan("span 1"); From 34a7bf0bbb6661aaadfafec9748c5e820474715b Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Wed, 17 Feb 2021 14:36:02 -0500 Subject: [PATCH 8/8] Review comments. SetResourceRef now takes a ref. --- .../etw/include/opentelemetry/exporters/etw/etw_data.h | 2 +- .../include/opentelemetry/exporters/otlp/recordable.h | 4 ++-- exporters/otlp/src/recordable.cc | 9 +++------ exporters/otlp/test/recordable_test.cc | 4 ++-- .../opentelemetry/ext/zpages/threadsafe_span_data.h | 2 +- sdk/include/opentelemetry/sdk/trace/recordable.h | 5 +++-- sdk/include/opentelemetry/sdk/trace/span_data.h | 2 +- sdk/src/trace/span.cc | 2 +- 8 files changed, 14 insertions(+), 16 deletions(-) diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h index 81f92aaef1..5f1719e7fc 100644 --- a/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h +++ b/exporters/etw/include/opentelemetry/exporters/etw/etw_data.h @@ -466,7 +466,7 @@ class ETWSpanData final : public sdk::trace::Recordable } void SetResourceRef( - const opentelemetry::sdk::resource::Resource *const resource) noexcept override + const opentelemetry::sdk::resource::Resource &resource) noexcept override { // TODO } diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h index 1e1c73561f..6dd65e5660 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h @@ -22,7 +22,7 @@ class Recordable final : public sdk::trace::Recordable trace::SpanId parent_span_id) noexcept override; void SetResourceRef( - const opentelemetry::sdk::resource::Resource *const resource) noexcept override; + const opentelemetry::sdk::resource::Resource &resource) noexcept override; void SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept override; @@ -46,7 +46,7 @@ class Recordable final : public sdk::trace::Recordable private: proto::trace::v1::Span span_; - const opentelemetry::sdk::resource::Resource *resource_{nullptr}; + opentelemetry::sdk::resource::Resource &resource_{opentelemetry::sdk::resource::Resource::GetEmpty()}; }; } // namespace otlp } // namespace exporter diff --git a/exporters/otlp/src/recordable.cc b/exporters/otlp/src/recordable.cc index b31dbc92b2..c93c5d3ae6 100644 --- a/exporters/otlp/src/recordable.cc +++ b/exporters/otlp/src/recordable.cc @@ -219,18 +219,15 @@ void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, proto::resource::v1::Resource Recordable::resource() const noexcept { proto::resource::v1::Resource proto; - if (resource_) + for (const auto &kv : resource_.GetAttributes()) { - for (const auto &kv : resource_->GetAttributes()) - { - PopulateAttribute(proto.add_attributes(), kv.first, kv.second); - } + PopulateAttribute(proto.add_attributes(), kv.first, kv.second); } return proto; } void Recordable::SetResourceRef( - const opentelemetry::sdk::resource::Resource *const resource) noexcept + const opentelemetry::sdk::resource::Resource &resource) noexcept { resource_ = resource; }; diff --git a/exporters/otlp/test/recordable_test.cc b/exporters/otlp/test/recordable_test.cc index 1211ecf1a2..9890bb8400 100644 --- a/exporters/otlp/test/recordable_test.cc +++ b/exporters/otlp/test/recordable_test.cc @@ -205,14 +205,14 @@ TEST(Recordable, SetResourceRef) Recordable rec; sdk::resource::Resource default_resource = sdk::resource::Resource::GetDefault(); sdk::resource::Resource empty_resource = sdk::resource::Resource::GetEmpty(); - rec.SetResourceRef(&default_resource); + rec.SetResourceRef(default_resource); auto proto = rec.resource(); EXPECT_EQ(3, proto.attributes_size()); // Note: We don't test ALL attribtues, only a few to avoid flaky tests and to // make sure we ARE getting attributes passed through. EXPECT_EQ("telemetry.sdk.name", proto.attributes(1).key()); EXPECT_EQ("opentelemetry", proto.attributes(1).value().string_value()); - rec.SetResourceRef(&empty_resource); + rec.SetResourceRef(empty_resource); EXPECT_EQ(0, rec.resource().attributes_size()); } diff --git a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h index 7f53b806ca..15fbdc1aa6 100644 --- a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h +++ b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h @@ -128,7 +128,7 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable } void SetResourceRef( - const opentelemetry::sdk::resource::Resource *const resource) noexcept override + const opentelemetry::sdk::resource::Resource &resource) noexcept override { // TODO } diff --git a/sdk/include/opentelemetry/sdk/trace/recordable.h b/sdk/include/opentelemetry/sdk/trace/recordable.h index 38e928f9f9..bd7a3a6b5e 100644 --- a/sdk/include/opentelemetry/sdk/trace/recordable.h +++ b/sdk/include/opentelemetry/sdk/trace/recordable.h @@ -49,10 +49,11 @@ class Recordable * owned by the `TracerProvider` which (indirectly) owns the `Exporter` that generates a * `Recordable`. * - * @param resource pointer to the Resource for this span. + * @param resource the Resource for this span. + * Note: this reference will remain stable for the life of the Recordable. */ virtual void SetResourceRef( - const opentelemetry::sdk::resource::Resource *const resource) noexcept = 0; + const opentelemetry::sdk::resource::Resource &resource) noexcept = 0; /** * Set an attribute of a span. diff --git a/sdk/include/opentelemetry/sdk/trace/span_data.h b/sdk/include/opentelemetry/sdk/trace/span_data.h index 655b3c6716..5fa88a0d2e 100644 --- a/sdk/include/opentelemetry/sdk/trace/span_data.h +++ b/sdk/include/opentelemetry/sdk/trace/span_data.h @@ -222,7 +222,7 @@ class SpanData final : public Recordable void SetDuration(std::chrono::nanoseconds duration) noexcept override { duration_ = duration; } void SetResourceRef( - const opentelemetry::sdk::resource::Resource *const resource) noexcept override + const opentelemetry::sdk::resource::Resource &resource) noexcept override { // TODO } diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index 9d987c84fe..30ec4c959c 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -108,7 +108,7 @@ Span::Span(std::shared_ptr &&tracer, recordable_->SetSpanKind(options.kind); recordable_->SetStartTime(NowOr(options.start_system_time)); start_steady_time = NowOr(options.start_steady_time); - recordable_->SetResourceRef(&tracer_->GetProvider().GetResource()); + recordable_->SetResourceRef(tracer_->GetProvider().GetResource()); // TODO: recordable->SetInstrumentationLibraryRef(&tracer_->GetInstrumentationLibrary()); tracer_->GetProvider().GetProcessor()->OnStart(*recordable_, parent_span_context); }