Cleanup: Get Span from Context, Set Span to Context (open-telemetry#723)

Troels51 · May 7, 2021 · 5a97750 · 5a97750
1 parent 32f10c7
commit 5a97750
Show file tree

Hide file tree

Showing 8 changed files with 23 additions and 40 deletions.
diff --git a/api/include/opentelemetry/trace/propagation/b3_propagator.h b/api/include/opentelemetry/trace/propagation/b3_propagator.h
@@ -47,7 +47,7 @@ class B3PropagatorExtractor : public opentelemetry::context::propagation::TextMa
   {
     SpanContext span_context = ExtractImpl(carrier);
     nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
-    return context.SetValue(kSpanKey, sp);
+    return SetSpan(context, sp);
   }
 
   static TraceId TraceIdFromHex(nostd::string_view trace_id)
@@ -128,7 +128,7 @@ class B3Propagator : public B3PropagatorExtractor
   void Inject(opentelemetry::context::propagation::TextMapCarrier &carrier,
               const context::Context &context) noexcept override
   {
-    SpanContext span_context = detail::GetCurrentSpan(context);
+    SpanContext span_context = GetSpan(context)->GetContext();
     if (!span_context.IsValid())
     {
       return;
@@ -155,7 +155,7 @@ class B3PropagatorMultiHeader : public B3PropagatorExtractor
   void Inject(opentelemetry::context::propagation::TextMapCarrier &carrier,
               const context::Context &context) noexcept override
   {
-    SpanContext span_context = detail::GetCurrentSpan(context);
+    SpanContext span_context = GetSpan(context)->GetContext();
     if (!span_context.IsValid())
     {
       return;

diff --git a/api/include/opentelemetry/trace/propagation/detail/context.h b/api/include/opentelemetry/trace/propagation/detail/context.h
@@ -1,26 +1,29 @@
 #pragma once
 #include "opentelemetry/context/context.h"
+#include "opentelemetry/trace/default_span.h"
 
 OPENTELEMETRY_BEGIN_NAMESPACE
 namespace trace
 {
 namespace propagation
 {
-namespace detail
-{
 
-inline trace::SpanContext GetCurrentSpan(const context::Context &context)
+inline nostd::shared_ptr<trace::Span> GetSpan(const context::Context &context)
 {
   context::ContextValue span = context.GetValue(trace::kSpanKey);
   if (nostd::holds_alternative<nostd::shared_ptr<trace::Span>>(span))
   {
-    return nostd::get<nostd::shared_ptr<trace::Span>>(span).get()->GetContext();
+    return nostd::get<nostd::shared_ptr<trace::Span>>(span);
   }
+  return nostd::shared_ptr<trace::Span>(new DefaultSpan(SpanContext::GetInvalid()));
+}
+
+inline context::Context SetSpan(context::Context &context, nostd::shared_ptr<trace::Span> span)
+{
 
-  return trace::SpanContext::GetInvalid();
+  return context.SetValue(kSpanKey, span);
 }
 
-}  // namespace detail
 }  // namespace propagation
 }  // namespace trace
 OPENTELEMETRY_END_NAMESPACE
diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h
@@ -45,7 +45,7 @@ class HttpTraceContext : public opentelemetry::context::propagation::TextMapProp
   void Inject(opentelemetry::context::propagation::TextMapCarrier &carrier,
               const context::Context &context) noexcept override
   {
-    SpanContext span_context = detail::GetCurrentSpan(context);
+    SpanContext span_context = GetSpan(context)->GetContext();
     if (!span_context.IsValid())
     {
       return;
@@ -58,7 +58,7 @@ class HttpTraceContext : public opentelemetry::context::propagation::TextMapProp
   {
     SpanContext span_context = ExtractImpl(carrier);
     nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
-    return context.SetValue(kSpanKey, sp);
+    return SetSpan(context, sp);
   }
 
   static TraceId TraceIdFromHex(nostd::string_view trace_id)

diff --git a/api/include/opentelemetry/trace/propagation/jaeger.h b/api/include/opentelemetry/trace/propagation/jaeger.h
@@ -34,7 +34,7 @@ class JaegerPropagator : public context::propagation::TextMapPropagator
   void Inject(context::propagation::TextMapCarrier &carrier,
               const context::Context &context) noexcept override
   {
-    SpanContext span_context = detail::GetCurrentSpan(context);
+    SpanContext span_context = GetSpan(context)->GetContext();
     if (!span_context.IsValid())
     {
       return;
@@ -64,7 +64,7 @@ class JaegerPropagator : public context::propagation::TextMapPropagator
   {
     SpanContext span_context = ExtractImpl(carrier);
     nostd::shared_ptr<Span> sp{new DefaultSpan(span_context)};
-    return context.SetValue(kSpanKey, sp);
+    return SetSpan(context, sp);
   }
 
 private:

diff --git a/api/include/opentelemetry/trace/tracer.h b/api/include/opentelemetry/trace/tracer.h
@@ -140,7 +140,7 @@ class Tracer
    * @param span the span that should be set as the new active span.
    * @return a Scope that controls how long the span will be active.
    */
-  nostd::unique_ptr<Scope> WithActiveSpan(nostd::shared_ptr<Span> &span) noexcept
+  static nostd::unique_ptr<Scope> WithActiveSpan(nostd::shared_ptr<Span> &span) noexcept
   {
     return nostd::unique_ptr<Scope>(new Scope{span});
   }
@@ -150,7 +150,7 @@ class Tracer
    * @return the currently active span, or an invalid default span if no span
    * is active.
    */
-  nostd::shared_ptr<Span> GetCurrentSpan() noexcept
+  static nostd::shared_ptr<Span> GetCurrentSpan() noexcept
   {
     context::ContextValue active_span = context::RuntimeContext::GetValue(kSpanKey);
     if (nostd::holds_alternative<nostd::shared_ptr<Span>>(active_span))

diff --git a/api/test/trace/propagation/http_text_format_test.cc b/api/test/trace/propagation/http_text_format_test.cc
@@ -147,7 +147,7 @@ TEST(TextMapPropagatorTest, InvalidIdentitiesAreNotExtracted)
     context::Context ctx1 = context::Context{};
     context::Context ctx2 = format.Extract(carrier, ctx1);
 
-    auto span = trace::propagation::detail::GetCurrentSpan(ctx2);
+    auto span = trace::propagation::GetSpan(ctx2)->GetContext();
     EXPECT_FALSE(span.IsValid());
   }
 }

diff --git a/api/test/trace/propagation/jaeger_propagation_test.cc b/api/test/trace/propagation/jaeger_propagation_test.cc
@@ -95,7 +95,7 @@ TEST(JaegerPropagatorTest, ExtractValidSpans)
     context::Context ctx1 = context::Context{};
     context::Context ctx2 = format.Extract(carrier, ctx1);
 
-    auto span = trace::propagation::detail::GetCurrentSpan(ctx2);
+    auto span = trace::propagation::GetSpan(ctx2)->GetContext();
     EXPECT_TRUE(span.IsValid());
 
     EXPECT_EQ(Hex(span.trace_id()), test_trace.expected_trace_id);
@@ -126,7 +126,7 @@ TEST(JaegerPropagatorTest, ExctractInvalidSpans)
     context::Context ctx1 = context::Context{};
     context::Context ctx2 = format.Extract(carrier, ctx1);
 
-    auto span = trace::propagation::detail::GetCurrentSpan(ctx2);
+    auto span = trace::propagation::GetSpan(ctx2)->GetContext();
     EXPECT_FALSE(span.IsValid());
   }
 }

diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc
@@ -16,34 +16,14 @@ Tracer::Tracer(std::shared_ptr<sdk::trace::TracerContext> context,
     : context_{context}, instrumentation_library_{std::move(instrumentation_library)}
 {}
 
-trace_api::SpanContext GetCurrentSpanContext(const trace_api::SpanContext &explicit_parent)
-{
-  // 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(trace_api::kSpanKey);
-
-  if (nostd::holds_alternative<nostd::shared_ptr<trace_api::Span>>(curr_span_context))
-  {
-    auto curr_span = nostd::get<nostd::shared_ptr<trace_api::Span>>(curr_span_context);
-    return curr_span->GetContext();
-  }
-
-  // Otherwise return an invalid SpanContext.
-  return trace_api::SpanContext::GetInvalid();
-}
-
 nostd::shared_ptr<trace_api::Span> Tracer::StartSpan(
     nostd::string_view name,
     const opentelemetry::common::KeyValueIterable &attributes,
     const trace_api::SpanContextKeyValueIterable &links,
     const trace_api::StartSpanOptions &options) noexcept
 {
-  trace_api::SpanContext parent = GetCurrentSpanContext(options.parent);
+  trace_api::SpanContext parent =
+      options.parent.IsValid() ? options.parent : GetCurrentSpan()->GetContext();
 
   auto sampling_result = context_->GetSampler().ShouldSample(parent, parent.trace_id(), name,
                                                              options.kind, attributes, links);