tracing: add SpanFinalizer and test and use NullSpan

include/envoy/tracing/http_tracer.h

@@ -31,13 +31,27 @@ class Config {
   virtual const std::vector<Http::LowerCaseString>& requestHeadersForTags() const PURE;
 };
 
-/*
- * Basic abstraction for span.
- */
 class Span;
 
 typedef std::unique_ptr<Span> SpanPtr;
 
+/*
+ * Interface to perform context-specific tasks when completing a Span.
+ */
+class SpanFinalizer {
+public:
+  virtual ~SpanFinalizer() {}
+
+  /**
+   * Finalize the Span.
+   * @param span the Span to be finalized
+   */
+  virtual void finalize(Span& span) PURE;
+};
+
+/*
+ * Basic abstraction for span.
+ */
 class Span {
 public:
   virtual ~Span() {}
@@ -52,8 +66,9 @@ class Span {
   /**
    * Capture the final duration for this Span and carry out any work necessary to complete it.
    * Once this method is called, the Span may be safely discarded.
+   * @param finalizer callback for context-specific work to complete the Span
    */
-  virtual void finishSpan() PURE;
+  virtual void finishSpan(SpanFinalizer& finalizer) PURE;
 
   /**
    * Mutate the provided headers with the context necessary to propagate this

source/common/http/conn_manager_impl.cc

@@ -336,13 +336,11 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() {
     log_handler->log(request_headers_.get(), response_headers_.get(), request_info_);
   }
 
-  if (active_span_) {
-    if (request_info_.healthCheck()) {
-      connection_manager_.config_.tracingStats().health_check_.inc();
-    } else {
-      Tracing::HttpTracerUtility::finalizeSpan(*active_span_, *request_headers_, request_info_,
-                                               *this);
-    }
+  if (request_info_.healthCheck()) {
+    connection_manager_.config_.tracingStats().health_check_.inc();
+  } else {
+    Tracing::HttpConnManFinalizerImpl finalizer(request_headers_.get(), request_info_, *this);
+    active_span_->finishSpan(finalizer);
   }
 
   ASSERT(state_.filter_call_state_ == 0);

source/common/http/conn_manager_impl.h

@@ -445,7 +445,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
 
     ConnectionManagerImpl& connection_manager_;
     Router::ConfigConstSharedPtr snapped_route_config_;
-    Tracing::SpanPtr active_span_;
+    Tracing::SpanPtr active_span_{new Tracing::NullSpan()};
     const uint64_t stream_id_;
     StreamEncoder* response_encoder_{};
     HeaderMapPtr response_headers_;

source/common/tracing/http_tracer_impl.cc

@@ -116,44 +116,46 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques
   NOT_REACHED;
 }
 
-void HttpTracerUtility::finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers,
-                                     const Http::AccessLog::RequestInfo& request_info,
-                                     const Config& config) {
+HttpConnManFinalizerImpl::HttpConnManFinalizerImpl(Http::HeaderMap* request_headers,
+                                                   Http::AccessLog::RequestInfo& request_info,
+                                                   Config& tracing_config)
+    : request_headers_(request_headers), request_info_(request_info),
+      tracing_config_(tracing_config) {}
+
+void HttpConnManFinalizerImpl::finalize(Span& span) {
   // Pre response data.
-  active_span.setTag("guid:x-request-id",
-                     std::string(request_headers.RequestId()->value().c_str()));
-  active_span.setTag("request_line", buildRequestLine(request_headers, request_info));
-  active_span.setTag("request_size", std::to_string(request_info.bytesReceived()));
-  active_span.setTag("host_header", valueOrDefault(request_headers.Host(), "-"));
-  active_span.setTag("downstream_cluster",
-                     valueOrDefault(request_headers.EnvoyDownstreamServiceCluster(), "-"));
-  active_span.setTag("user_agent", valueOrDefault(request_headers.UserAgent(), "-"));
-
-  if (request_headers.ClientTraceId()) {
-    active_span.setTag("guid:x-client-trace-id",
-                       std::string(request_headers.ClientTraceId()->value().c_str()));
-  }
+  if (request_headers_) {
+    span.setTag("guid:x-request-id", std::string(request_headers_->RequestId()->value().c_str()));
+    span.setTag("request_line", buildRequestLine(*request_headers_, request_info_));
+    span.setTag("host_header", valueOrDefault(request_headers_->Host(), "-"));
+    span.setTag("downstream_cluster",
+                valueOrDefault(request_headers_->EnvoyDownstreamServiceCluster(), "-"));
+    span.setTag("user_agent", valueOrDefault(request_headers_->UserAgent(), "-"));
+
+    if (request_headers_->ClientTraceId()) {
+      span.setTag("guid:x-client-trace-id",
+                  std::string(request_headers_->ClientTraceId()->value().c_str()));
+    }
 
-  // Build tags based on the custom headers.
-  for (const Http::LowerCaseString& header : config.requestHeadersForTags()) {
-    const Http::HeaderEntry* entry = request_headers.get(header);
-    if (entry) {
-      active_span.setTag(header.get(), entry->value().c_str());
+    // Build tags based on the custom headers.
+    for (const Http::LowerCaseString& header : tracing_config_.requestHeadersForTags()) {
+      const Http::HeaderEntry* entry = request_headers_->get(header);
+      if (entry) {
+        span.setTag(header.get(), entry->value().c_str());
+      }
     }
   }
+  span.setTag("request_size", std::to_string(request_info_.bytesReceived()));
 
   // Post response data.
-  active_span.setTag("response_code", buildResponseCode(request_info));
-  active_span.setTag("response_size", std::to_string(request_info.bytesSent()));
-  active_span.setTag("response_flags",
-                     Http::AccessLog::ResponseFlagUtils::toShortString(request_info));
-
-  if (request_info.responseCode().valid() &&
-      Http::CodeUtility::is5xx(request_info.responseCode().value())) {
-    active_span.setTag("error", "true");
-  }
+  span.setTag("response_code", buildResponseCode(request_info_));
+  span.setTag("response_size", std::to_string(request_info_.bytesSent()));
+  span.setTag("response_flags", Http::AccessLog::ResponseFlagUtils::toShortString(request_info_));
 
-  active_span.finishSpan();
+  if (request_info_.responseCode().valid() &&
+      Http::CodeUtility::is5xx(request_info_.responseCode().value())) {
+    span.setTag("error", "true");
+  }
 }
 
 HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& local_info)

source/common/tracing/http_tracer_impl.h

@@ -62,11 +62,42 @@ class HttpTracerUtility {
   static const std::string EGRESS_OPERATION;
 };
 
+class NullSpan : public Span {
+public:
+  // Tracing::Span
+  void setTag(const std::string&, const std::string&) override {}
+  void finishSpan(SpanFinalizer&) override {}
+  void injectContext(Http::HeaderMap&) override {}
+  SpanPtr spawnChild(const std::string&, SystemTime) override { return SpanPtr{new NullSpan()}; }
+};
+
+class NullFinalizer : public SpanFinalizer {
+public:
+  // Tracing::SpanFinalizer
+  void finalize(Span&) override {}
+};
+
+/**
+ * Finalizer for Spans covering standard request ingress.
+ */
+class HttpConnManFinalizerImpl : public SpanFinalizer {
+public:
+  HttpConnManFinalizerImpl(Http::HeaderMap* request_headers,
+                           Http::AccessLog::RequestInfo& request_info, Config& tracing_config);
+
+  void finalize(Span& span) override;
+
+private:
+  Http::HeaderMap* request_headers_;
+  Http::AccessLog::RequestInfo& request_info_;
+  Config& tracing_config_;
+};
+
 class HttpNullTracer : public HttpTracer {
 public:
   // Tracing::HttpTracer
   SpanPtr startSpan(const Config&, Http::HeaderMap&, const Http::AccessLog::RequestInfo&) override {
-    return nullptr;
+    return SpanPtr{new NullSpan()};
   }
 };
 
@@ -83,14 +114,5 @@ class HttpTracerImpl : public HttpTracer {
   const LocalInfo::LocalInfo& local_info_;
 };
 
-class NullSpan : public Tracing::Span {
-public:
-  // Tracing::Span
-  void setTag(const std::string&, const std::string&) override {}
-  void finishSpan() override {}
-  void injectContext(Http::HeaderMap&) override {}
-  SpanPtr spawnChild(const std::string&, SystemTime) override { return SpanPtr{new NullSpan()}; }
-};
-
 } // Tracing
 } // Envoy

source/common/tracing/lightstep_tracer_impl.cc

@@ -18,7 +18,10 @@ namespace Tracing {
 LightStepSpan::LightStepSpan(lightstep::Span& span, lightstep::Tracer& tracer)
     : span_(span), tracer_(tracer) {}
 
-void LightStepSpan::finishSpan() { span_.Finish(); }
+void LightStepSpan::finishSpan(SpanFinalizer& finalizer) {
+  finalizer.finalize(*this);
+  span_.Finish();
+}
 
 void LightStepSpan::setTag(const std::string& name, const std::string& value) {
   span_.SetTag(name, value);

source/common/tracing/lightstep_tracer_impl.h

@@ -32,7 +32,7 @@ class LightStepSpan : public Span {
   LightStepSpan(lightstep::Span& span, lightstep::Tracer& tracer);
 
   // Tracing::Span
-  void finishSpan() override;
+  void finishSpan(SpanFinalizer& finalizer) override;
   void setTag(const std::string& name, const std::string& value) override;
   void injectContext(Http::HeaderMap& request_headers) override;
   SpanPtr spawnChild(const std::string& name, SystemTime start_time) override;

source/common/tracing/zipkin/zipkin_tracer_impl.cc

@@ -14,7 +14,10 @@ namespace Zipkin {
 
 ZipkinSpan::ZipkinSpan(Zipkin::Span& span, Zipkin::Tracer& tracer) : span_(span), tracer_(tracer) {}
 
-void ZipkinSpan::finishSpan() { span_.finish(); }
+void ZipkinSpan::finishSpan(Tracing::SpanFinalizer& finalizer) {
+  finalizer.finalize(*this);
+  span_.finish();
+}
 
 void ZipkinSpan::setTag(const std::string& name, const std::string& value) {
   if (this->hasCSAnnotation()) {

source/common/tracing/zipkin/zipkin_tracer_impl.h

@@ -40,8 +40,10 @@ class ZipkinSpan : public Tracing::Span {
   /**
    * Calls Zipkin::Span::finishSpan() to perform all actions needed to finalize the span.
    * This function is called by Tracing::HttpTracerUtility::finalizeSpan().
+   *
+   * @param finalizer SpanFinalizer to be called to complete this Span
    */
-  void finishSpan() override;
+  void finishSpan(Tracing::SpanFinalizer& finalizer) override;
 
   /**
    * This function adds a Zipkin "string" binary annotation to this span.

test/common/http/conn_manager_impl_test.cc

@@ -293,7 +293,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) {
 
         return span;
       }));
-  EXPECT_CALL(*span, finishSpan());
+  EXPECT_CALL(*span, finishSpan(_))
+      .WillOnce(
+          Invoke([span](Tracing::SpanFinalizer& finalizer) -> void { finalizer.finalize(*span); }));
   EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber());
   // Verify tag is set based on the request headers.
   EXPECT_CALL(*span, setTag(":method", "GET"));
@@ -422,7 +424,7 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) {
   NiceMock<Tracing::MockSpan>* span = new NiceMock<Tracing::MockSpan>();
 
   EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span));
-  EXPECT_CALL(*span, finishSpan()).Times(0);
+  EXPECT_CALL(*span, finishSpan(_)).Times(0);
 
   EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
       .WillOnce(Return(true));

test/common/tracing/http_tracer_impl_test.cc

@@ -234,7 +234,7 @@ TEST(HttpTracerUtilityTest, IsTracing) {
   }
 }
 
-TEST(HttpTracerUtilityTest, OriginalAndLongPath) {
+TEST(HttpConnManFinalizerImpl, OriginalAndLongPath) {
   const std::string path(300, 'a');
   const std::string expected_path(128, 'a');
   std::unique_ptr<NiceMock<MockSpan>> span(new NiceMock<MockSpan>());
@@ -253,10 +253,31 @@ TEST(HttpTracerUtilityTest, OriginalAndLongPath) {
   EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber());
   EXPECT_CALL(*span, setTag("request_line", "GET " + expected_path + " HTTP/2"));
   NiceMock<MockConfig> config;
-  HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config);
+
+  HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config);
+  finalizer.finalize(*span);
 }
 
-TEST(HttpTracerUtilityTest, SpanOptionalHeaders) {
+TEST(HttpConnManFinalizerImpl, NullRequestHeaders) {
+  std::unique_ptr<NiceMock<MockSpan>> span(new NiceMock<MockSpan>());
+  NiceMock<Http::AccessLog::MockRequestInfo> request_info;
+
+  EXPECT_CALL(request_info, bytesReceived()).WillOnce(Return(10));
+  EXPECT_CALL(request_info, bytesSent()).WillOnce(Return(11));
+  Optional<uint32_t> response_code;
+  EXPECT_CALL(request_info, responseCode()).WillRepeatedly(ReturnRef(response_code));
+
+  EXPECT_CALL(*span, setTag("response_code", "0"));
+  EXPECT_CALL(*span, setTag("response_size", "11"));
+  EXPECT_CALL(*span, setTag("response_flags", "-"));
+  EXPECT_CALL(*span, setTag("request_size", "10"));
+  NiceMock<MockConfig> config;
+
+  HttpConnManFinalizerImpl finalizer(nullptr, request_info, config);
+  finalizer.finalize(*span);
+}
+
+TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) {
   std::unique_ptr<NiceMock<MockSpan>> span(new NiceMock<MockSpan>());
 
   Http::TestHeaderMapImpl request_headers{
@@ -284,12 +305,13 @@ TEST(HttpTracerUtilityTest, SpanOptionalHeaders) {
   EXPECT_CALL(*span, setTag("response_size", "100"));
   EXPECT_CALL(*span, setTag("response_flags", "-"));
 
-  EXPECT_CALL(*span, finishSpan());
   NiceMock<MockConfig> config;
-  HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config);
+
+  HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config);
+  finalizer.finalize(*span);
 }
 
-TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) {
+TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) {
   std::unique_ptr<NiceMock<MockSpan>> span(new NiceMock<MockSpan>());
   Http::TestHeaderMapImpl request_headers{
       {"x-request-id", "id"}, {":path", "/test"}, {":method", "GET"}};
@@ -337,8 +359,8 @@ TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) {
   EXPECT_CALL(*span, setTag("response_size", "100"));
   EXPECT_CALL(*span, setTag("response_flags", "UT"));
 
-  EXPECT_CALL(*span, finishSpan());
-  HttpTracerUtility::finalizeSpan(*span, request_headers, request_info, config);
+  HttpConnManFinalizerImpl finalizer(&request_headers, request_info, config);
+  finalizer.finalize(*span);
 }
 
 TEST(HttpTracerUtilityTest, operationTypeToString) {
@@ -352,7 +374,8 @@ TEST(HttpNullTracerTest, BasicFunctionality) {
   Http::AccessLog::MockRequestInfo request_info;
   Http::TestHeaderMapImpl request_headers;
 
-  EXPECT_EQ(nullptr, null_tracer.startSpan(config, request_headers, request_info));
+  SpanPtr span_ptr = null_tracer.startSpan(config, request_headers, request_info);
+  EXPECT_TRUE(dynamic_cast<NullSpan*>(span_ptr.get()) != nullptr);
 }
 
 class HttpTracerImplTest : public Test {

test/common/tracing/lightstep_tracer_impl_test.cc

@@ -166,10 +166,15 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) {
       .WillOnce(Return(5000U));
 
   SpanPtr first_span = driver_->startSpan(request_headers_, operation_name_, start_time_);
-  first_span->finishSpan();
+  Tracing::MockFinalizer finalizer;
+
+  EXPECT_CALL(finalizer, finalize(_));
+  first_span->finishSpan(finalizer);
 
   SpanPtr second_span = driver_->startSpan(request_headers_, operation_name_, start_time_);
-  second_span->finishSpan();
+
+  EXPECT_CALL(finalizer, finalize(_));
+  second_span->finishSpan(finalizer);
 
   Http::MessagePtr msg(new Http::ResponseMessageImpl(
       Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}));
@@ -203,7 +208,10 @@ TEST_F(LightStepDriverTest, FlushSpansTimer) {
       .WillOnce(Return(5));
 
   SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_);
-  span->finishSpan();
+  Tracing::MockFinalizer finalizer;
+
+  EXPECT_CALL(finalizer, finalize(_));
+  span->finishSpan(finalizer);
 
   // Timer should be re-enabled.
   EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000)));
@@ -244,7 +252,10 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) {
       .WillOnce(Return(5000U));
 
   SpanPtr span = driver_->startSpan(request_headers_, operation_name_, start_time_);
-  span->finishSpan();
+  Tracing::MockFinalizer finalizer;
+
+  EXPECT_CALL(finalizer, finalize(_));
+  span->finishSpan(finalizer);
 
   Http::MessagePtr msg(new Http::ResponseMessageImpl(
       Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}}));