tracing: Delegate envoy tracing decision to tracer to determine if sh…

…ould override sampling Signed-off-by: Gary Brown <gary@brownuk.com>
envoyproxy · Mar 26, 2018 · 4160ed8 · 4160ed8
1 parent 833769c
commit 4160ed8
Show file tree

Hide file tree

Showing 17 changed files with 290 additions and 128 deletions.
diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h
@@ -13,6 +13,19 @@ namespace Tracing {
 
 enum class OperationName { Ingress, Egress };
 
+enum class Reason {
+  NotTraceableRequestId,
+  HealthCheck,
+  Sampling,
+  ServiceForced,
+  ClientForced,
+};
+
+struct Decision {
+  Reason reason;
+  bool is_tracing;
+};
+
 /**
  * Tracing configuration, it carries additional data needed to populate the span.
  */
@@ -88,7 +101,8 @@ class Driver {
    * Start driver specific span.
    */
   virtual SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
-                            const std::string& operation_name, SystemTime start_time) PURE;
+                            const std::string& operation_name, SystemTime start_time,
+                            const Tracing::Decision& tracing_decision) PURE;
 };
 
 typedef std::unique_ptr<Driver> DriverPtr;
@@ -102,7 +116,8 @@ class HttpTracer {
   virtual ~HttpTracer() {}
 
   virtual SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
-                            const RequestInfo::RequestInfo& request_info) PURE;
+                            const RequestInfo::RequestInfo& request_info,
+                            const Tracing::Decision& tracing_decision) PURE;
 };
 
 typedef std::unique_ptr<HttpTracer> HttpTracerPtr;

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -607,12 +607,13 @@ void ConnectionManagerImpl::ActiveStream::traceRequest() {
   ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason,
                                             connection_manager_.config_.tracingStats());
 
-  if (!tracing_decision.is_tracing) {
+  active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_,
+                                                       tracing_decision);
+
+  if (!active_span_) {
     return;
   }
 
-  active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_);
-
   // TODO: Need to investigate the following code based on the cached route, as may
   // be broken in the case a filter changes the route.
 

diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.h b/source/common/tracing/dynamic_opentracing_driver_impl.h
@@ -32,6 +32,9 @@ class DynamicOpenTracingDriver : public OpenTracingDriver {
     return OpenTracingDriver::PropagationMode::TracerNative;
   }
 
+protected:
+  bool useTagForSamplingDecision() override { return true; }
+
 private:
   opentracing::DynamicTracingLibraryHandle library_handle_;
   std::shared_ptr<opentracing::Tracer> tracer_;

diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc
@@ -168,16 +168,17 @@ HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& l
     : driver_(std::move(driver)), local_info_(local_info) {}
 
 SpanPtr HttpTracerImpl::startSpan(const Config& config, Http::HeaderMap& request_headers,
-                                  const RequestInfo::RequestInfo& request_info) {
+                                  const RequestInfo::RequestInfo& request_info,
+                                  const Tracing::Decision& tracing_decision) {
   std::string span_name = HttpTracerUtility::toString(config.operationName());
 
   if (config.operationName() == OperationName::Egress) {
     span_name.append(" ");
     span_name.append(request_headers.Host()->value().c_str());
   }
 
-  SpanPtr active_span =
-      driver_->startSpan(config, request_headers, span_name, request_info.startTime());
+  SpanPtr active_span = driver_->startSpan(config, request_headers, span_name,
+                                           request_info.startTime(), tracing_decision);
   if (active_span) {
     active_span->setTag(Tracing::Tags::get().COMPONENT, Tracing::Tags::get().PROXY);
     active_span->setTag(Tracing::Tags::get().NODE_ID, local_info_.nodeName());

diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h
@@ -14,19 +14,6 @@
 namespace Envoy {
 namespace Tracing {
 
-enum class Reason {
-  NotTraceableRequestId,
-  HealthCheck,
-  Sampling,
-  ServiceForced,
-  ClientForced,
-};
-
-struct Decision {
-  Reason reason;
-  bool is_tracing;
-};
-
 /**
  * Tracing tag names.
  */
@@ -143,7 +130,8 @@ class NullSpan : public Span {
 class HttpNullTracer : public HttpTracer {
 public:
   // Tracing::HttpTracer
-  SpanPtr startSpan(const Config&, Http::HeaderMap&, const RequestInfo::RequestInfo&) override {
+  SpanPtr startSpan(const Config&, Http::HeaderMap&, const RequestInfo::RequestInfo&,
+                    const Tracing::Decision&) override {
     return SpanPtr{new NullSpan()};
   }
 };
@@ -154,7 +142,8 @@ class HttpTracerImpl : public HttpTracer {
 
   // Tracing::HttpTracer
   SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
-                    const RequestInfo::RequestInfo& request_info) override;
+                    const RequestInfo::RequestInfo& request_info,
+                    const Tracing::Decision& tracing_decision) override;
 
 private:
   DriverPtr driver_;

diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h
@@ -61,6 +61,9 @@ class LightStepDriver : public OpenTracingDriver {
   opentracing::Tracer& tracer() override;
   PropagationMode propagationMode() const override { return propagation_mode_; }
 
+protected:
+  bool useTagForSamplingDecision() override { return false; }
+
 private:
   class LightStepTransporter : public lightstep::AsyncTransporter, Http::AsyncClient::Callbacks {
   public:

diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc
@@ -128,7 +128,13 @@ OpenTracingDriver::OpenTracingDriver(Stats::Store& stats)
     : tracer_stats_{OPENTRACING_TRACER_STATS(POOL_COUNTER_PREFIX(stats, "tracing.opentracing."))} {}
 
 SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_headers,
-                                     const std::string& operation_name, SystemTime start_time) {
+                                     const std::string& operation_name, SystemTime start_time,
+                                     const Tracing::Decision& tracing_decision) {
+  // If tracing decision is no, and sampling decision is not communicated via tags, then
+  // return a null span to indicate that tracing is not being performed.
+  if (!tracing_decision.is_tracing && !useTagForSamplingDecision()) {
+    return nullptr;
+  }
   const PropagationMode propagation_mode = this->propagationMode();
   const opentracing::Tracer& tracer = this->tracer();
   std::unique_ptr<opentracing::Span> active_span;
@@ -164,8 +170,13 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea
       tracerStats().span_context_extraction_error_.inc();
     }
   }
-  active_span = tracer.StartSpan(operation_name, {opentracing::ChildOf(parent_span_ctx.get()),
-                                                  opentracing::StartTimestamp(start_time)});
+  opentracing::StartSpanOptions options;
+  opentracing::ChildOf(parent_span_ctx.get()).Apply(options);
+  opentracing::StartTimestamp(start_time).Apply(options);
+  if (!tracing_decision.is_tracing) {
+    opentracing::SetTag(OpenTracingTags::get().SAMPLING_PRIORITY, 0).Apply(options);
+  }
+  active_span = tracer.StartSpanWithOptions(operation_name, options);
   RELEASE_ASSERT(active_span != nullptr);
   return SpanPtr{new OpenTracingSpan{*this, std::move(active_span)}};
 }

diff --git a/source/common/tracing/opentracing_driver_impl.h b/source/common/tracing/opentracing_driver_impl.h
@@ -5,12 +5,20 @@
 #include "envoy/tracing/http_tracer.h"
 
 #include "common/common/logger.h"
+#include "common/singleton/const_singleton.h"
 
 #include "opentracing/tracer.h"
 
 namespace Envoy {
 namespace Tracing {
 
+class OpenTracingTagValues {
+public:
+  const std::string SAMPLING_PRIORITY = "sampling.priority";
+};
+
+typedef ConstSingleton<OpenTracingTagValues> OpenTracingTags;
+
 #define OPENTRACING_TRACER_STATS(COUNTER)                                                          \
   COUNTER(span_context_extraction_error)                                                           \
   COUNTER(span_context_injection_error)
@@ -49,7 +57,8 @@ class OpenTracingDriver : public Driver, protected Logger::Loggable<Logger::Id::
 
   // Tracer::TracingDriver
   SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
-                    const std::string& operation_name, SystemTime start_time) override;
+                    const std::string& operation_name, SystemTime start_time,
+                    const Tracing::Decision& tracing_decision) override;
 
   virtual opentracing::Tracer& tracer() PURE;
 
@@ -64,6 +73,9 @@ class OpenTracingDriver : public Driver, protected Logger::Loggable<Logger::Id::
 
   OpenTracingTracerStats& tracerStats() { return tracer_stats_; }
 
+protected:
+  virtual bool useTagForSamplingDecision() PURE;
+
 private:
   OpenTracingTracerStats tracer_stats_;
 };

diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.cc b/source/common/tracing/zipkin/zipkin_tracer_impl.cc
@@ -76,7 +76,8 @@ Driver::Driver(const Json::Object& config, Upstream::ClusterManager& cluster_man
 }
 
 Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMap& request_headers,
-                                   const std::string&, SystemTime start_time) {
+                                   const std::string&, SystemTime start_time,
+                                   const Tracing::Decision& tracing_decision) {
   Tracer& tracer = *tls_->getTyped<TlsTracer>().tracer_;
   SpanPtr new_zipkin_span;
   bool sampled(true);
@@ -86,6 +87,8 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa
     // zipkin tracers may still use that value, although should be 0 or 1.
     absl::string_view xb3_sampled = request_headers.XB3Sampled()->value().getStringView();
     sampled = xb3_sampled == ZipkinCoreConstants::get().SAMPLED || xb3_sampled == "true";
+  } else {
+    sampled = tracing_decision.is_tracing;
   }
 
   if (request_headers.XB3TraceId() && request_headers.XB3SpanId()) {

diff --git a/source/common/tracing/zipkin/zipkin_tracer_impl.h b/source/common/tracing/zipkin/zipkin_tracer_impl.h
@@ -99,7 +99,8 @@ class Driver : public Tracing::Driver {
    * ("ingress" or "egress") passed by the caller.
    */
   Tracing::SpanPtr startSpan(const Tracing::Config&, Http::HeaderMap& request_headers,
-                             const std::string&, SystemTime start_time) override;
+                             const std::string&, SystemTime start_time,
+                             const Tracing::Decision& tracing_decision) override;
 
   // Getters to return the ZipkinDriver's key members.
   Upstream::ClusterManager& clusterManager() { return cm_; }

diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc
@@ -517,13 +517,14 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) {
   setup(false, "");
 
   NiceMock<Tracing::MockSpan>* span = new NiceMock<Tracing::MockSpan>();
-  EXPECT_CALL(tracer_, startSpan_(_, _, _))
-      .WillOnce(Invoke([&](const Tracing::Config& config, const HeaderMap&,
-                           const RequestInfo::RequestInfo&) -> Tracing::Span* {
-        EXPECT_EQ(Tracing::OperationName::Ingress, config.operationName());
+  EXPECT_CALL(tracer_, startSpan_(_, _, _, _))
+      .WillOnce(
+          Invoke([&](const Tracing::Config& config, const HeaderMap&,
+                     const RequestInfo::RequestInfo&, const Tracing::Decision&) -> Tracing::Span* {
+            EXPECT_EQ(Tracing::OperationName::Ingress, config.operationName());
 
-        return span;
-      }));
+            return span;
+          }));
   // No decorator.
   EXPECT_CALL(*route_config_provider_.route_config_->route_, decorator())
       .WillRepeatedly(Return(nullptr));
@@ -583,13 +584,14 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat
   setup(false, "");
 
   NiceMock<Tracing::MockSpan>* span = new NiceMock<Tracing::MockSpan>();
-  EXPECT_CALL(tracer_, startSpan_(_, _, _))
-      .WillOnce(Invoke([&](const Tracing::Config& config, const HeaderMap&,
-                           const RequestInfo::RequestInfo&) -> Tracing::Span* {
-        EXPECT_EQ(Tracing::OperationName::Ingress, config.operationName());
+  EXPECT_CALL(tracer_, startSpan_(_, _, _, _))
+      .WillOnce(
+          Invoke([&](const Tracing::Config& config, const HeaderMap&,
+                     const RequestInfo::RequestInfo&, const Tracing::Decision&) -> Tracing::Span* {
+            EXPECT_EQ(Tracing::OperationName::Ingress, config.operationName());
 
-        return span;
-      }));
+            return span;
+          }));
   route_config_provider_.route_config_->route_->decorator_.operation_ = "testOp";
   EXPECT_CALL(*route_config_provider_.route_config_->route_, decorator()).Times(4);
   EXPECT_CALL(route_config_provider_.route_config_->route_->decorator_, apply(_))
@@ -644,13 +646,14 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat
   setup(false, "");
 
   NiceMock<Tracing::MockSpan>* span = new NiceMock<Tracing::MockSpan>();
-  EXPECT_CALL(tracer_, startSpan_(_, _, _))
-      .WillOnce(Invoke([&](const Tracing::Config& config, const HeaderMap&,
-                           const RequestInfo::RequestInfo&) -> Tracing::Span* {
-        EXPECT_EQ(Tracing::OperationName::Ingress, config.operationName());
+  EXPECT_CALL(tracer_, startSpan_(_, _, _, _))
+      .WillOnce(
+          Invoke([&](const Tracing::Config& config, const HeaderMap&,
+                     const RequestInfo::RequestInfo&, const Tracing::Decision&) -> Tracing::Span* {
+            EXPECT_EQ(Tracing::OperationName::Ingress, config.operationName());
 
-        return span;
-      }));
+            return span;
+          }));
   route_config_provider_.route_config_->route_->decorator_.operation_ = "initOp";
   EXPECT_CALL(*route_config_provider_.route_config_->route_, decorator()).Times(4);
   EXPECT_CALL(route_config_provider_.route_config_->route_->decorator_, apply(_))
@@ -710,13 +713,14 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato
       {Tracing::OperationName::Egress, {LowerCaseString(":method")}}));
 
   NiceMock<Tracing::MockSpan>* span = new NiceMock<Tracing::MockSpan>();
-  EXPECT_CALL(tracer_, startSpan_(_, _, _))
-      .WillOnce(Invoke([&](const Tracing::Config& config, const HeaderMap&,
-                           const RequestInfo::RequestInfo&) -> Tracing::Span* {
-        EXPECT_EQ(Tracing::OperationName::Egress, config.operationName());
+  EXPECT_CALL(tracer_, startSpan_(_, _, _, _))
+      .WillOnce(
+          Invoke([&](const Tracing::Config& config, const HeaderMap&,
+                     const RequestInfo::RequestInfo&, const Tracing::Decision&) -> Tracing::Span* {
+            EXPECT_EQ(Tracing::OperationName::Egress, config.operationName());
 
-        return span;
-      }));
+            return span;
+          }));
   route_config_provider_.route_config_->route_->decorator_.operation_ = "testOp";
   EXPECT_CALL(*route_config_provider_.route_config_->route_, decorator()).Times(4);
   EXPECT_CALL(route_config_provider_.route_config_->route_->decorator_, apply(_))
@@ -776,13 +780,14 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato
       {Tracing::OperationName::Egress, {LowerCaseString(":method")}}));
 
   NiceMock<Tracing::MockSpan>* span = new NiceMock<Tracing::MockSpan>();
-  EXPECT_CALL(tracer_, startSpan_(_, _, _))
-      .WillOnce(Invoke([&](const Tracing::Config& config, const HeaderMap&,
-                           const RequestInfo::RequestInfo&) -> Tracing::Span* {
-        EXPECT_EQ(Tracing::OperationName::Egress, config.operationName());
+  EXPECT_CALL(tracer_, startSpan_(_, _, _, _))
+      .WillOnce(
+          Invoke([&](const Tracing::Config& config, const HeaderMap&,
+                     const RequestInfo::RequestInfo&, const Tracing::Decision&) -> Tracing::Span* {
+            EXPECT_EQ(Tracing::OperationName::Egress, config.operationName());
 
-        return span;
-      }));
+            return span;
+          }));
   route_config_provider_.route_config_->route_->decorator_.operation_ = "initOp";
   EXPECT_CALL(*route_config_provider_.route_config_->route_, decorator()).Times(4);
   EXPECT_CALL(route_config_provider_.route_config_->route_->decorator_, apply(_))
@@ -959,7 +964,7 @@ TEST_F(HttpConnectionManagerImplTest, DoNotStartSpanIfTracingIsNotEnabled) {
   // Disable tracing.
   tracing_config_.reset();
 
-  EXPECT_CALL(tracer_, startSpan_(_, _, _)).Times(0);
+  EXPECT_CALL(tracer_, startSpan_(_, _, _, _)).Times(0);
   ON_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
       .WillByDefault(Return(true));
 
@@ -997,7 +1002,7 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) {
 
   NiceMock<Tracing::MockSpan>* span = new NiceMock<Tracing::MockSpan>();
 
-  EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span));
+  EXPECT_CALL(tracer_, startSpan_(_, _, _, _)).WillOnce(Return(span));
   EXPECT_CALL(*span, finishSpan()).Times(0);
 
   EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))

diff --git a/test/common/tracing/dynamic_opentracing_driver_impl_test.cc b/test/common/tracing/dynamic_opentracing_driver_impl_test.cc
@@ -70,7 +70,8 @@ TEST_F(DynamicOpenTracingDriverTest, InitializeDriver) {
 TEST_F(DynamicOpenTracingDriverTest, FlushSpans) {
   setupValidDriver();
 
-  SpanPtr first_span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_);
+  SpanPtr first_span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_,
+                                          {Reason::Sampling, true});
   first_span->finishSpan();
   driver_->tracer().Close();