From 91d5884c968aa3caefbf89b04ae112be0ba6e44a Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Wed, 26 Aug 2020 22:44:47 +0000 Subject: [PATCH 1/5] Set SpanContext in Span, propagate ids --- .../opentelemetry/trace/span_context.h | 22 +++++++++++++------ sdk/src/trace/span.cc | 19 ++++++++++------ sdk/src/trace/tracer.cc | 13 +++++++---- sdk/test/trace/tracer_test.cc | 2 +- 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/api/include/opentelemetry/trace/span_context.h b/api/include/opentelemetry/trace/span_context.h index 6228bf215c..9fea890ac7 100644 --- a/api/include/opentelemetry/trace/span_context.h +++ b/api/include/opentelemetry/trace/span_context.h @@ -78,7 +78,7 @@ class SpanContext final SpanContext(const SpanContext &ctx) : trace_id_(ctx.trace_id()), span_id_(ctx.span_id()), trace_flags_(ctx.trace_flags()) {} - // + // SpanContext &operator=(const SpanContext &ctx) // { // SpanContext *spn_ctx = @@ -86,7 +86,15 @@ class SpanContext final // ctx.HasRemoteParent()); // this = spn_ctx; // return *this; - // }; + // } + SpanContext &operator=(const SpanContext &ctx) + { + trace_id_ = TraceId(ctx.trace_id_.Id()); + span_id_ = SpanId(ctx.span_id_.Id()); + trace_flags_ = TraceFlags(ctx.trace_flags_.flags());; + remote_parent_ = ctx.remote_parent_; + return *this; + }; // // SpanContext &operator=(SpanContext &&ctx) // { @@ -95,7 +103,7 @@ class SpanContext final // ctx.HasRemoteParent()); // this = spn_ctx; // return *this; - // }; + // } bool operator==(const SpanContext &that) const noexcept { @@ -110,10 +118,10 @@ class SpanContext final bool IsSampled() const noexcept { return trace_flags_.IsSampled(); } private: - const trace_api::TraceId trace_id_; - const trace_api::SpanId span_id_; - const trace_api::TraceFlags trace_flags_; - const bool remote_parent_ = false; + trace_api::TraceId trace_id_; + trace_api::SpanId span_id_; + trace_api::TraceFlags trace_flags_; + bool remote_parent_ = false; }; } // namespace trace OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index c6c3151a7f..7c5d4e1ecd 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -2,6 +2,7 @@ #include "src/common/random.h" #include "opentelemetry/context/runtime_context.h" +#include "opentelemetry/trace/trace_flags.h" #include "opentelemetry/version.h" OPENTELEMETRY_BEGIN_NAMESPACE @@ -77,22 +78,26 @@ Span::Span(std::shared_ptr &&tracer, } recordable_->SetName(name); + trace_api::TraceId trace_id = GenerateRandomTraceId(); if (parent_span_context.IsValid()) { + trace_id = parent_span_context.trace_id(); recordable_->SetIds(parent_span_context.trace_id(), GenerateRandomSpanId(), parent_span_context.span_id()); } else { - recordable_->SetIds(GenerateRandomTraceId(), GenerateRandomSpanId(), trace_api::SpanId()); + recordable_->SetIds(trace_id, GenerateRandomSpanId(), trace_api::SpanId()); } - // TODO: Create and populate SpanContext for this span when SpanContext is fully implemented - attributes.ForEachKeyValue([&](nostd::string_view key, - opentelemetry::common::AttributeValue value) noexcept { - recordable_->SetAttribute(key, value); - return true; - }); + span_context_ = std::unique_ptr( + new trace_api::SpanContext(trace_id, GenerateRandomSpanId(), trace_api::TraceFlags(), false)); + + attributes.ForEachKeyValue( + [&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept { + recordable_->SetAttribute(key, value); + return true; + }); recordable_->SetStartTime(NowOr(options.start_system_time)); start_steady_time = NowOr(options.start_steady_time); diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc index 3923a47161..81c244f5a8 100644 --- a/sdk/src/trace/tracer.cc +++ b/sdk/src/trace/tracer.cc @@ -47,13 +47,18 @@ nostd::shared_ptr Tracer::StartSpan( } else { - // TODO: Get parent span context from current context. For now we assume all spans - // have no parent, and pass an invalid parent span context to the Span constructor. - const trace_api::SpanContext kInvalidParentSpanContext(false, false); + context::ContextValue curr_span_context = context::RuntimeContext::GetValue(SpanKey); + trace_api::SpanContext parent_span_context; + + if (nostd::holds_alternative>(curr_span_context)) + { + auto curr_span = nostd::get>(curr_span_context); + parent_span_context = curr_span->GetContext(); + } auto span = nostd::shared_ptr{ new (std::nothrow) Span{this->shared_from_this(), processor_.load(), name, attributes, - options, kInvalidParentSpanContext}}; + options, parent_span_context}}; span->SetToken( nostd::unique_ptr(new context::Token(context::RuntimeContext::Attach( diff --git a/sdk/test/trace/tracer_test.cc b/sdk/test/trace/tracer_test.cc index efdc6b6f8b..12e95b5f57 100644 --- a/sdk/test/trace/tracer_test.cc +++ b/sdk/test/trace/tracer_test.cc @@ -113,7 +113,7 @@ TEST(Tracer, ToMockSpanExporter) ASSERT_EQ("span 2", spans_received->at(0)->GetName()); EXPECT_TRUE(spans_received->at(0)->GetTraceId().IsValid()); EXPECT_TRUE(spans_received->at(0)->GetSpanId().IsValid()); - EXPECT_FALSE(spans_received->at(0)->GetParentSpanId().IsValid()); + EXPECT_TRUE(spans_received->at(0)->GetParentSpanId().IsValid()); span_first->End(); ASSERT_EQ(2, spans_received->size()); From c50cd7332bcf6fde1c8b053ac014e714f635d7b7 Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Fri, 28 Aug 2020 01:02:21 +0000 Subject: [PATCH 2/5] Clean up comments --- api/include/opentelemetry/trace/span_context.h | 17 ----------------- sdk/src/trace/span.cc | 7 ++++--- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/api/include/opentelemetry/trace/span_context.h b/api/include/opentelemetry/trace/span_context.h index 9fea890ac7..cf500b0bc7 100644 --- a/api/include/opentelemetry/trace/span_context.h +++ b/api/include/opentelemetry/trace/span_context.h @@ -79,14 +79,6 @@ class SpanContext final : trace_id_(ctx.trace_id()), span_id_(ctx.span_id()), trace_flags_(ctx.trace_flags()) {} - // SpanContext &operator=(const SpanContext &ctx) - // { - // SpanContext *spn_ctx = - // new SpanContext(ctx.trace_id(), ctx.span_id(), ctx.trace_flags(), - // ctx.HasRemoteParent()); - // this = spn_ctx; - // return *this; - // } SpanContext &operator=(const SpanContext &ctx) { trace_id_ = TraceId(ctx.trace_id_.Id()); @@ -95,15 +87,6 @@ class SpanContext final remote_parent_ = ctx.remote_parent_; return *this; }; - // - // SpanContext &operator=(SpanContext &&ctx) - // { - // SpanContext *spn_ctx = - // new SpanContext(ctx.trace_id(), ctx.span_id(), ctx.trace_flags(), - // ctx.HasRemoteParent()); - // this = spn_ctx; - // return *this; - // } bool operator==(const SpanContext &that) const noexcept { diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index 7c5d4e1ecd..911c9ad384 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -78,15 +78,16 @@ Span::Span(std::shared_ptr &&tracer, } recordable_->SetName(name); - trace_api::TraceId trace_id = GenerateRandomTraceId(); + trace_api::TraceId trace_id; + if (parent_span_context.IsValid()) { trace_id = parent_span_context.trace_id(); - recordable_->SetIds(parent_span_context.trace_id(), GenerateRandomSpanId(), - parent_span_context.span_id()); + recordable_->SetIds(trace_id, GenerateRandomSpanId(), parent_span_context.span_id()); } else { + trace_id = GenerateRandomTraceId(); recordable_->SetIds(trace_id, GenerateRandomSpanId(), trace_api::SpanId()); } From 2ce1198ae40a0c6ebfab0842d4a2d79ba1847bbf Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Fri, 28 Aug 2020 01:13:42 +0000 Subject: [PATCH 3/5] Add helper function --- .../opentelemetry/trace/span_context.h | 35 ++++++++++++------- sdk/src/trace/tracer.cc | 25 +++++++------ 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/api/include/opentelemetry/trace/span_context.h b/api/include/opentelemetry/trace/span_context.h index cf500b0bc7..6228bf215c 100644 --- a/api/include/opentelemetry/trace/span_context.h +++ b/api/include/opentelemetry/trace/span_context.h @@ -78,15 +78,24 @@ class SpanContext final SpanContext(const SpanContext &ctx) : trace_id_(ctx.trace_id()), span_id_(ctx.span_id()), trace_flags_(ctx.trace_flags()) {} - - SpanContext &operator=(const SpanContext &ctx) - { - trace_id_ = TraceId(ctx.trace_id_.Id()); - span_id_ = SpanId(ctx.span_id_.Id()); - trace_flags_ = TraceFlags(ctx.trace_flags_.flags());; - remote_parent_ = ctx.remote_parent_; - return *this; - }; + // + // SpanContext &operator=(const SpanContext &ctx) + // { + // SpanContext *spn_ctx = + // new SpanContext(ctx.trace_id(), ctx.span_id(), ctx.trace_flags(), + // ctx.HasRemoteParent()); + // this = spn_ctx; + // return *this; + // }; + // + // SpanContext &operator=(SpanContext &&ctx) + // { + // SpanContext *spn_ctx = + // new SpanContext(ctx.trace_id(), ctx.span_id(), ctx.trace_flags(), + // ctx.HasRemoteParent()); + // this = spn_ctx; + // return *this; + // }; bool operator==(const SpanContext &that) const noexcept { @@ -101,10 +110,10 @@ class SpanContext final bool IsSampled() const noexcept { return trace_flags_.IsSampled(); } private: - trace_api::TraceId trace_id_; - trace_api::SpanId span_id_; - trace_api::TraceFlags trace_flags_; - bool remote_parent_ = false; + const trace_api::TraceId trace_id_; + const trace_api::SpanId span_id_; + const trace_api::TraceFlags trace_flags_; + const bool remote_parent_ = false; }; } // namespace trace OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc index 81c244f5a8..f7b2d38033 100644 --- a/sdk/src/trace/tracer.cc +++ b/sdk/src/trace/tracer.cc @@ -30,6 +30,20 @@ std::shared_ptr Tracer::GetSampler() const noexcept return sampler_; } +// Helper function to extract the current span context from the runtime context. +// Returns an invalid span context if the runtime context doesn't contain a span. +trace_api::SpanContext GetCurrentSpanContext() +{ + context::ContextValue curr_span_context = context::RuntimeContext::GetValue(SpanKey); + + if (nostd::holds_alternative>(curr_span_context)) + { + auto curr_span = nostd::get>(curr_span_context); + return curr_span->GetContext(); + } + return trace_api::SpanContext(); +} + nostd::shared_ptr Tracer::StartSpan( nostd::string_view name, const trace_api::KeyValueIterable &attributes, @@ -47,18 +61,9 @@ nostd::shared_ptr Tracer::StartSpan( } else { - context::ContextValue curr_span_context = context::RuntimeContext::GetValue(SpanKey); - trace_api::SpanContext parent_span_context; - - if (nostd::holds_alternative>(curr_span_context)) - { - auto curr_span = nostd::get>(curr_span_context); - parent_span_context = curr_span->GetContext(); - } - auto span = nostd::shared_ptr{ new (std::nothrow) Span{this->shared_from_this(), processor_.load(), name, attributes, - options, parent_span_context}}; + options, GetCurrentSpanContext()}}; span->SetToken( nostd::unique_ptr(new context::Token(context::RuntimeContext::Attach( From ba12078edc8a02bde437e6327274913b1b672111 Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Fri, 28 Aug 2020 01:43:44 +0000 Subject: [PATCH 4/5] Fix formatting --- sdk/src/trace/span.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index 911c9ad384..639c844e86 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -94,11 +94,11 @@ Span::Span(std::shared_ptr &&tracer, span_context_ = std::unique_ptr( new trace_api::SpanContext(trace_id, GenerateRandomSpanId(), trace_api::TraceFlags(), false)); - attributes.ForEachKeyValue( - [&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept { - recordable_->SetAttribute(key, value); - return true; - }); + attributes.ForEachKeyValue([&](nostd::string_view key, + opentelemetry::common::AttributeValue value) noexcept { + recordable_->SetAttribute(key, value); + return true; + }); recordable_->SetStartTime(NowOr(options.start_system_time)); start_steady_time = NowOr(options.start_steady_time); From fba8043f493d315d77ef1b2726f1d1c0d03b77e6 Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Fri, 28 Aug 2020 17:26:26 +0000 Subject: [PATCH 5/5] Address review comments --- sdk/src/trace/span.cc | 7 ++++--- sdk/test/trace/tracer_test.cc | 5 +++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index 639c844e86..c27c071b62 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -79,20 +79,21 @@ Span::Span(std::shared_ptr &&tracer, recordable_->SetName(name); trace_api::TraceId trace_id; + trace_api::SpanId span_id = GenerateRandomSpanId(); if (parent_span_context.IsValid()) { trace_id = parent_span_context.trace_id(); - recordable_->SetIds(trace_id, GenerateRandomSpanId(), parent_span_context.span_id()); + recordable_->SetIds(trace_id, span_id, parent_span_context.span_id()); } else { trace_id = GenerateRandomTraceId(); - recordable_->SetIds(trace_id, GenerateRandomSpanId(), trace_api::SpanId()); + recordable_->SetIds(trace_id, span_id, trace_api::SpanId()); } span_context_ = std::unique_ptr( - new trace_api::SpanContext(trace_id, GenerateRandomSpanId(), trace_api::TraceFlags(), false)); + new trace_api::SpanContext(trace_id, span_id, trace_api::TraceFlags(), false)); attributes.ForEachKeyValue([&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept { diff --git a/sdk/test/trace/tracer_test.cc b/sdk/test/trace/tracer_test.cc index 12e95b5f57..891cce22d4 100644 --- a/sdk/test/trace/tracer_test.cc +++ b/sdk/test/trace/tracer_test.cc @@ -118,6 +118,11 @@ TEST(Tracer, ToMockSpanExporter) span_first->End(); ASSERT_EQ(2, spans_received->size()); + // Verify trace and parent span id propagation + EXPECT_EQ(span_second->GetContext().trace_id(), span_first->GetContext().trace_id()); + EXPECT_EQ(spans_received->at(0)->GetTraceId(), spans_received->at(1)->GetTraceId()); + EXPECT_EQ(spans_received->at(0)->GetParentSpanId(), spans_received->at(1)->GetSpanId()); + ASSERT_EQ("span 1", spans_received->at(1)->GetName()); EXPECT_TRUE(spans_received->at(1)->GetTraceId().IsValid()); EXPECT_TRUE(spans_received->at(1)->GetSpanId().IsValid());