From 10cc6875630ad58c9da9c9d39c5e52dba45117b8 Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Wed, 30 Sep 2020 20:03:42 -0700 Subject: [PATCH 1/6] Add parent context --- api/include/opentelemetry/trace/span.h | 7 +++- .../opentelemetry/trace/span_context.h | 37 ++++++++++--------- sdk/src/trace/tracer.cc | 8 ++-- 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/api/include/opentelemetry/trace/span.h b/api/include/opentelemetry/trace/span.h index 922f38fb92..49f7fb3418 100644 --- a/api/include/opentelemetry/trace/span.h +++ b/api/include/opentelemetry/trace/span.h @@ -43,8 +43,13 @@ struct StartSpanOptions core::SystemTimestamp start_system_time; core::SteadyTimestamp start_steady_time; + // Explicitely set the parent of a Span. + // + // This defaults to an invalid span context. In this case, the Span is + // automatically parented to the currently active span. + SpanContext parent; + // TODO: - // Span(Context?) parent; // SpanContext remote_parent; // Links SpanKind kind = SpanKind::kInternal; diff --git a/api/include/opentelemetry/trace/span_context.h b/api/include/opentelemetry/trace/span_context.h index 6228bf215c..e3915287ac 100644 --- a/api/include/opentelemetry/trace/span_context.h +++ b/api/include/opentelemetry/trace/span_context.h @@ -78,24 +78,25 @@ 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 = - // 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; - // }; + + /* + SpanContext &operator=(const SpanContext &ctx) + { + trace_id_ = ctx.trace_id(); + span_id_ = ctx.span_id(); + trace_flags_ = ctx.trace_flags(); + remote_parent_ = ctx.HasRemoteParent(); + return *this; + }; + + SpanContext &operator=(SpanContext &&ctx) + { + trace_id_ = std::move(ctx.trace_id()); + span_id_ = std::move(ctx.span_id()); + trace_flags_ = std::move(ctx.trace_flags()); + remote_parent_ = ctx.HasRemoteParent(); + return *this; + }; */ bool operator==(const SpanContext &that) const noexcept { diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc index 70a7b42109..9359a19b80 100644 --- a/sdk/src/trace/tracer.cc +++ b/sdk/src/trace/tracer.cc @@ -49,9 +49,11 @@ nostd::shared_ptr Tracer::StartSpan( const trace_api::KeyValueIterable &attributes, const trace_api::StartSpanOptions &options) noexcept { - // TODO: replace nullptr with parent context in span context + trace_api::SpanContext parent( + options.parent.IsValid() ? options.parent : GetCurrentSpanContext()); + auto sampling_result = - sampler_->ShouldSample(nullptr, trace_api::TraceId(), name, options.kind, attributes); + sampler_->ShouldSample(&parent, parent.trace_id(), name, options.kind, attributes); if (sampling_result.decision == Decision::DROP) { // Don't allocate a no-op span for every DROP decision, but use a static @@ -65,7 +67,7 @@ nostd::shared_ptr Tracer::StartSpan( { auto span = nostd::shared_ptr{ new (std::nothrow) Span{this->shared_from_this(), processor_.load(), name, attributes, - options, GetCurrentSpanContext()}}; + options, parent}}; // if the attributes is not nullptr, add attributes to the span. if (sampling_result.attributes) From 3ba1346a1b92a4f36d95d6afad8c980d139c8b69 Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Thu, 1 Oct 2020 17:26:17 -0700 Subject: [PATCH 2/6] Explicit span parenting --- api/include/opentelemetry/trace/noop.h | 4 ++- api/include/opentelemetry/trace/span.h | 2 +- .../opentelemetry/trace/span_context.h | 35 ++++--------------- api/test/trace/span_context_test.cc | 2 +- examples/plugin/plugin/tracer.cc | 2 +- sdk/src/trace/tracer.cc | 27 ++++++++------ sdk/test/trace/tracer_test.cc | 35 +++++++++++++++++++ 7 files changed, 63 insertions(+), 44 deletions(-) diff --git a/api/include/opentelemetry/trace/noop.h b/api/include/opentelemetry/trace/noop.h index 533225e851..b4d950fd60 100644 --- a/api/include/opentelemetry/trace/noop.h +++ b/api/include/opentelemetry/trace/noop.h @@ -24,7 +24,9 @@ namespace trace class NoopSpan final : public Span { public: - explicit NoopSpan(const std::shared_ptr &tracer) noexcept : tracer_{tracer} {} + explicit NoopSpan(const std::shared_ptr &tracer) noexcept + : tracer_{tracer}, span_context_{SpanContext::GetInvalid()} + {} void SetAttribute(nostd::string_view /*key*/, const common::AttributeValue & /*value*/) noexcept override diff --git a/api/include/opentelemetry/trace/span.h b/api/include/opentelemetry/trace/span.h index 49f7fb3418..016cd557e5 100644 --- a/api/include/opentelemetry/trace/span.h +++ b/api/include/opentelemetry/trace/span.h @@ -47,7 +47,7 @@ struct StartSpanOptions // // This defaults to an invalid span context. In this case, the Span is // automatically parented to the currently active span. - SpanContext parent; + SpanContext parent = SpanContext::GetInvalid(); // TODO: // SpanContext remote_parent; diff --git a/api/include/opentelemetry/trace/span_context.h b/api/include/opentelemetry/trace/span_context.h index e3915287ac..179696d5dd 100644 --- a/api/include/opentelemetry/trace/span_context.h +++ b/api/include/opentelemetry/trace/span_context.h @@ -34,8 +34,8 @@ class SpanContext final { public: // An invalid SpanContext. - SpanContext() noexcept - : trace_flags_(trace::TraceFlags((uint8_t) false)), remote_parent_(false){}; + // SpanContext() noexcept + // : trace_flags_(trace::TraceFlags((uint8_t) false)), remote_parent_(false){}; /* A temporary constructor for an invalid SpanContext. * Trace id and span id are set to invalid (all zeros). @@ -71,32 +71,9 @@ class SpanContext final remote_parent_(has_remote_parent) {} - SpanContext(SpanContext &&ctx) - : trace_id_(ctx.trace_id()), span_id_(ctx.span_id()), trace_flags_(ctx.trace_flags()) - {} - 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_ = ctx.trace_id(); - span_id_ = ctx.span_id(); - trace_flags_ = ctx.trace_flags(); - remote_parent_ = ctx.HasRemoteParent(); - return *this; - }; - - SpanContext &operator=(SpanContext &&ctx) - { - trace_id_ = std::move(ctx.trace_id()); - span_id_ = std::move(ctx.span_id()); - trace_flags_ = std::move(ctx.trace_flags()); - remote_parent_ = ctx.HasRemoteParent(); - return *this; - }; */ bool operator==(const SpanContext &that) const noexcept { @@ -111,10 +88,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/api/test/trace/span_context_test.cc b/api/test/trace/span_context_test.cc index b364348ccd..1a4b7c62a2 100644 --- a/api/test/trace/span_context_test.cc +++ b/api/test/trace/span_context_test.cc @@ -42,7 +42,7 @@ TEST(SpanContextTest, TraceFlags) // Test that SpanContext is invalid TEST(SpanContextTest, Invalid) { - SpanContext s1(false, false); + SpanContext s1 = SpanContext::GetInvalid(); EXPECT_FALSE(s1.IsValid()); // Test that trace id and span id are invalid diff --git a/examples/plugin/plugin/tracer.cc b/examples/plugin/plugin/tracer.cc index e8f324c9e7..31b9367c7c 100644 --- a/examples/plugin/plugin/tracer.cc +++ b/examples/plugin/plugin/tracer.cc @@ -20,7 +20,7 @@ class Span final : public trace::Span nostd::string_view name, const opentelemetry::trace::KeyValueIterable & /*attributes*/, const trace::StartSpanOptions & /*options*/) noexcept - : tracer_{std::move(tracer)}, name_{name} + : tracer_{std::move(tracer)}, name_{name}, span_context_{trace::SpanContext::GetInvalid()} { std::cout << "StartSpan: " << name << "\n"; } diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc index 9359a19b80..079158f87d 100644 --- a/sdk/src/trace/tracer.cc +++ b/sdk/src/trace/tracer.cc @@ -30,18 +30,25 @@ 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() +trace_api::SpanContext GetCurrentSpanContext(const trace_api::SpanContext &explicit_parent) { - context::ContextValue curr_span_context = context::RuntimeContext::GetValue(SpanKey); + // Use the explicit parent, if it's valid. + if (explicit_parent.IsValid()) + { + return explicit_parent; + } + + // Use the currently active span, if there's one. + auto 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(); + + // Otherwise return an invalid SpanContext. + return trace_api::SpanContext::GetInvalid(); } nostd::shared_ptr Tracer::StartSpan( @@ -49,11 +56,10 @@ nostd::shared_ptr Tracer::StartSpan( const trace_api::KeyValueIterable &attributes, const trace_api::StartSpanOptions &options) noexcept { - trace_api::SpanContext parent( - options.parent.IsValid() ? options.parent : GetCurrentSpanContext()); + trace_api::SpanContext parent = GetCurrentSpanContext(options.parent); auto sampling_result = - sampler_->ShouldSample(&parent, parent.trace_id(), name, options.kind, attributes); + sampler_->ShouldSample(nullptr, parent.trace_id(), name, options.kind, attributes); if (sampling_result.decision == Decision::DROP) { // Don't allocate a no-op span for every DROP decision, but use a static @@ -65,9 +71,8 @@ nostd::shared_ptr Tracer::StartSpan( } else { - auto span = nostd::shared_ptr{ - new (std::nothrow) Span{this->shared_from_this(), processor_.load(), name, attributes, - options, parent}}; + auto span = nostd::shared_ptr{new (std::nothrow) Span{ + this->shared_from_this(), processor_.load(), name, attributes, options, parent}}; // if the attributes is not nullptr, add attributes to the span. if (sampling_result.attributes) diff --git a/sdk/test/trace/tracer_test.cc b/sdk/test/trace/tracer_test.cc index 98a18ea688..1e70af316c 100644 --- a/sdk/test/trace/tracer_test.cc +++ b/sdk/test/trace/tracer_test.cc @@ -442,3 +442,38 @@ TEST(Tracer, WithActiveSpan) ASSERT_EQ(1, spans.size()); EXPECT_EQ("span 1", spans.at(0)->GetName()); } + +TEST(Tracer, ExpectParent) +{ + std::unique_ptr exporter(new InMemorySpanExporter()); + std::shared_ptr span_data = exporter->GetData(); + auto tracer = initTracer(std::move(exporter)); + auto spans = span_data.get()->GetSpans(); + + ASSERT_EQ(0, spans.size()); + + auto span_first = tracer->StartSpan("span 1"); + + trace_api::StartSpanOptions options; + options.parent = span_first->GetContext(); + auto span_second = tracer->StartSpan("span 2", options); + + options.parent = span_second->GetContext(); + auto span_third = tracer->StartSpan("span 3", options); + + span_third->End(); + span_second->End(); + span_first->End(); + + spans = span_data->GetSpans(); + ASSERT_EQ(3, spans.size()); + auto spandata_first = std::move(spans.at(2)); + auto spandata_second = std::move(spans.at(1)); + auto spandata_third = std::move(spans.at(0)); + EXPECT_EQ("span 1", spandata_first->GetName()); + EXPECT_EQ("span 2", spandata_second->GetName()); + EXPECT_EQ("span 3", spandata_third->GetName()); + + EXPECT_EQ(spandata_first->GetSpanId(), spandata_second->GetParentSpanId()); + EXPECT_EQ(spandata_second->GetSpanId(), spandata_third->GetParentSpanId()); +} From 276963f07dc57ee574679da7a12fc3527afa54b4 Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Thu, 1 Oct 2020 17:33:53 -0700 Subject: [PATCH 3/6] Cleanup --- api/include/opentelemetry/trace/span_context.h | 10 +++------- sdk/src/trace/tracer.cc | 1 + 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/api/include/opentelemetry/trace/span_context.h b/api/include/opentelemetry/trace/span_context.h index 179696d5dd..6add265eed 100644 --- a/api/include/opentelemetry/trace/span_context.h +++ b/api/include/opentelemetry/trace/span_context.h @@ -33,10 +33,6 @@ namespace trace_api = opentelemetry::trace; class SpanContext final { public: - // An invalid SpanContext. - // SpanContext() noexcept - // : trace_flags_(trace::TraceFlags((uint8_t) false)), remote_parent_(false){}; - /* A temporary constructor for an invalid SpanContext. * Trace id and span id are set to invalid (all zeros). * @@ -71,9 +67,9 @@ class SpanContext final remote_parent_(has_remote_parent) {} - SpanContext(const SpanContext &ctx) - : trace_id_(ctx.trace_id()), span_id_(ctx.span_id()), trace_flags_(ctx.trace_flags()) - {} + SpanContext(const SpanContext &ctx) = default; + + SpanContext &operator=(const SpanContext &ctx) = default; bool operator==(const SpanContext &that) const noexcept { diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc index 079158f87d..ee3d74ce1e 100644 --- a/sdk/src/trace/tracer.cc +++ b/sdk/src/trace/tracer.cc @@ -58,6 +58,7 @@ nostd::shared_ptr Tracer::StartSpan( { trace_api::SpanContext parent = GetCurrentSpanContext(options.parent); + // TODO: replace nullptr with parent context in span context auto sampling_result = sampler_->ShouldSample(nullptr, parent.trace_id(), name, options.kind, attributes); if (sampling_result.decision == Decision::DROP) From d63c34487bd9532f0c1f8dd596e48149bce6dedd Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Thu, 1 Oct 2020 17:39:50 -0700 Subject: [PATCH 4/6] Fix compilation --- .../opentelemetry/trace/propagation/http_trace_context.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h index 259bbcbc3b..07467b7ef6 100644 --- a/api/include/opentelemetry/trace/propagation/http_trace_context.h +++ b/api/include/opentelemetry/trace/propagation/http_trace_context.h @@ -92,7 +92,7 @@ class HttpTraceContext : public HTTPTextFormat { return nostd::get>(span).get()->GetContext(); } - return SpanContext(); + return SpanContext::GetInvalid(); } static TraceId GenerateTraceIdFromString(nostd::string_view trace_id) From 6e54d16ab42d9fda2680a247949294dacb0e2b52 Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Thu, 1 Oct 2020 17:45:33 -0700 Subject: [PATCH 5/6] Fix compilation --- api/include/opentelemetry/trace/default_span.h | 2 -- api/test/trace/propagation/http_text_format_test.cc | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/api/include/opentelemetry/trace/default_span.h b/api/include/opentelemetry/trace/default_span.h index 9dbe682bc7..bfe46ae2ca 100644 --- a/api/include/opentelemetry/trace/default_span.h +++ b/api/include/opentelemetry/trace/default_span.h @@ -41,8 +41,6 @@ class DefaultSpan : public Span nostd::string_view ToString() { return "DefaultSpan"; } - DefaultSpan() = default; - DefaultSpan(SpanContext span_context) : span_context_(span_context) {} // movable and copiable diff --git a/api/test/trace/propagation/http_text_format_test.cc b/api/test/trace/propagation/http_text_format_test.cc index c6262a1606..8d496713bc 100644 --- a/api/test/trace/propagation/http_text_format_test.cc +++ b/api/test/trace/propagation/http_text_format_test.cc @@ -67,7 +67,7 @@ TEST(HTTPTextFormatTest, NoSendEmptyTraceState) const std::map carrier = { {"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"}}; context::Context ctx1 = - context::Context("current-span", nostd::shared_ptr(new trace::DefaultSpan())); + context::Context("current-span", nostd::shared_ptr(new trace::DefaultSpan::GetInvalid())); context::Context ctx2 = format.Extract(Getter, carrier, ctx1); std::map c2 = {}; format.Inject(Setter, c2, ctx2); From f44f603a868da8faacd6dbb14f88a56811979716 Mon Sep 17 00:00:00 2001 From: Johannes Tax Date: Thu, 1 Oct 2020 21:21:25 -0700 Subject: [PATCH 6/6] Fix build issues --- api/test/trace/propagation/http_text_format_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/test/trace/propagation/http_text_format_test.cc b/api/test/trace/propagation/http_text_format_test.cc index 8d496713bc..bbdba64d36 100644 --- a/api/test/trace/propagation/http_text_format_test.cc +++ b/api/test/trace/propagation/http_text_format_test.cc @@ -66,8 +66,9 @@ TEST(HTTPTextFormatTest, NoSendEmptyTraceState) // If the trace state is empty, do not set the header. const std::map carrier = { {"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-0102030405060708-01"}}; - context::Context ctx1 = - context::Context("current-span", nostd::shared_ptr(new trace::DefaultSpan::GetInvalid())); + context::Context ctx1 = context::Context{ + "current-span", + nostd::shared_ptr(new trace::DefaultSpan(trace::SpanContext::GetInvalid()))}; context::Context ctx2 = format.Extract(Getter, carrier, ctx1); std::map c2 = {}; format.Inject(Setter, c2, ctx2);