Associate valid SpanId/SpanContext with Spans irrespective of the Sam…

…pling decision. (#879)
open-telemetry · Jul 1, 2021 · 2383d99 · 2383d99
1 parent 8556485
commit 2383d99
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 25 deletions.
diff --git a/api/include/opentelemetry/trace/noop.h b/api/include/opentelemetry/trace/noop.h
@@ -29,7 +29,12 @@ class NoopSpan final : public Span
 {
 public:
   explicit NoopSpan(const std::shared_ptr<Tracer> &tracer) noexcept
-      : tracer_{tracer}, span_context_{SpanContext::GetInvalid()}
+      : tracer_{tracer}, span_context_{new SpanContext(false, false)}
+  {}
+
+  explicit NoopSpan(const std::shared_ptr<Tracer> &tracer,
+                    nostd::unique_ptr<SpanContext> span_context) noexcept
+      : tracer_{tracer}, span_context_{std::move(span_context)}
   {}
 
   void SetAttribute(nostd::string_view /*key*/,
@@ -55,11 +60,11 @@ class NoopSpan final : public Span
 
   bool IsRecording() const noexcept override { return false; }
 
-  SpanContext GetContext() const noexcept override { return span_context_; }
+  SpanContext GetContext() const noexcept override { return *span_context_.get(); }
 
 private:
   std::shared_ptr<Tracer> tracer_;
-  SpanContext span_context_;
+  nostd::unique_ptr<SpanContext> span_context_;
 };
 
 /**

diff --git a/api/test/trace/noop_test.cc b/api/test/trace/noop_test.cc
@@ -10,14 +10,13 @@
 
 #include <gtest/gtest.h>
 
-using opentelemetry::common::SystemTimestamp;
-using opentelemetry::trace::NoopTracer;
-using opentelemetry::trace::SpanContext;
-using opentelemetry::trace::Tracer;
+namespace trace_api = opentelemetry::trace;
+namespace nonstd    = opentelemetry::nostd;
+namespace common    = opentelemetry::common;
 
 TEST(NoopTest, UseNoopTracers)
 {
-  std::shared_ptr<Tracer> tracer{new NoopTracer{}};
+  std::shared_ptr<trace_api::Tracer> tracer{new trace_api::NoopTracer{}};
   auto s1 = tracer->StartSpan("abc");
 
   std::map<std::string, std::string> attributes1;
@@ -41,20 +40,42 @@ TEST(NoopTest, UseNoopTracers)
 
   s1->UpdateName("test_name");
 
-  SystemTimestamp t1;
+  common::SystemTimestamp t1;
   s1->AddEvent("test_time_stamp", t1);
 
   s1->GetContext();
 }
 
 TEST(NoopTest, StartSpan)
 {
-  std::shared_ptr<Tracer> tracer{new NoopTracer{}};
+  std::shared_ptr<trace_api::Tracer> tracer{new trace_api::NoopTracer{}};
 
-  std::map<std::string, std::string> attrs                                      = {{"a", "3"}};
-  std::vector<std::pair<SpanContext, std::map<std::string, std::string>>> links = {
-      {SpanContext(false, false), attrs}};
+  std::map<std::string, std::string> attrs = {{"a", "3"}};
+  std::vector<std::pair<trace_api::SpanContext, std::map<std::string, std::string>>> links = {
+      {trace_api::SpanContext(false, false), attrs}};
   auto s1 = tracer->StartSpan("abc", attrs, links);
 
-  auto s2 = tracer->StartSpan("efg", {{"a", 3}}, {{SpanContext(false, false), {{"b", 4}}}});
+  auto s2 =
+      tracer->StartSpan("efg", {{"a", 3}}, {{trace_api::SpanContext(false, false), {{"b", 4}}}});
+}
+
+TEST(NoopTest, CreateSpanValidSpanContext)
+{
+  // Create valid spancontext for NoopSpan
+
+  constexpr uint8_t buf_span[]  = {1, 2, 3, 4, 5, 6, 7, 8};
+  constexpr uint8_t buf_trace[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
+  auto trace_id                 = trace_api::TraceId{buf_trace};
+  auto span_id                  = trace_api::SpanId{buf_span};
+  auto span_context             = nonstd::unique_ptr<trace_api::SpanContext>(
+      new trace_api::SpanContext{trace_id, span_id, trace_api::TraceFlags{true}, false});
+  std::shared_ptr<trace_api::Tracer> tracer{new trace_api::NoopTracer{}};
+  auto s1 =
+      nonstd::shared_ptr<trace_api::Span>(new trace_api::NoopSpan(tracer, std::move(span_context)));
+  auto stored_span_context = s1->GetContext();
+  EXPECT_EQ(stored_span_context.span_id(), span_id);
+  EXPECT_EQ(stored_span_context.trace_id(), trace_id);
+
+  s1->AddEvent("even1");  // noop
+  s1->End();              // noop
 }
diff --git a/sdk/src/trace/tracer.cc b/sdk/src/trace/tracer.cc
@@ -46,25 +46,27 @@ nostd::shared_ptr<trace_api::Span> Tracer::StartSpan(
 
   auto sampling_result = context_->GetSampler().ShouldSample(parent_context, trace_id, name,
                                                              options.kind, attributes, links);
+  auto trace_flags     = sampling_result.decision == Decision::DROP
+                         ? trace_api::TraceFlags{}
+                         : trace_api::TraceFlags{trace_api::TraceFlags::kIsSampled};
+
+  auto span_context = std::unique_ptr<trace_api::SpanContext>(new trace_api::SpanContext(
+      trace_id, span_id, trace_flags, false,
+      sampling_result.trace_state ? sampling_result.trace_state
+                                  : is_parent_span_valid ? parent_context.trace_state()
+                                                         : trace_api::TraceState::GetDefault()));
 
   if (sampling_result.decision == Decision::DROP)
   {
-    // Don't allocate a no-op span for every DROP decision, but use a static
-    // singleton for this case.
-    static nostd::shared_ptr<trace_api::Span> noop_span(
-        new trace_api::NoopSpan{this->shared_from_this()});
+    // create no-op span with valid span-context.
 
+    auto noop_span = nostd::shared_ptr<trace_api::Span>{
+        new (std::nothrow) trace_api::NoopSpan(this->shared_from_this(), std::move(span_context))};
     return noop_span;
   }
   else
   {
 
-    auto span_context = std::unique_ptr<trace_api::SpanContext>(new trace_api::SpanContext(
-        trace_id, span_id, trace_api::TraceFlags{trace_api::TraceFlags::kIsSampled}, false,
-        sampling_result.trace_state ? sampling_result.trace_state
-                                    : is_parent_span_valid ? parent_context.trace_state()
-                                                           : trace_api::TraceState::GetDefault()));
-
     auto span = nostd::shared_ptr<trace_api::Span>{
         new (std::nothrow) Span{this->shared_from_this(), name, attributes, links, options,
                                 parent_context, std::move(span_context)}};

diff --git a/sdk/test/trace/tracer_test.cc b/sdk/test/trace/tracer_test.cc
@@ -165,8 +165,14 @@ TEST(Tracer, StartSpanSampleOff)
   auto tracer_off = initTracer(std::move(exporter), new AlwaysOffSampler());
 
   // This span will not be recorded.
-  tracer_off->StartSpan("span 2")->End();
+  auto span = tracer_off->StartSpan("span 2");
 
+  // Always generate a valid span-context (span-id)
+  auto context = span->GetContext();
+  EXPECT_TRUE(context.IsValid());
+  EXPECT_FALSE(context.IsSampled());
+
+  span->End();
   // The span doesn't write any span data because the sampling decision is alway
   // DROP.
   ASSERT_EQ(0, span_data->GetSpans().size());