Zipkin: Allow SetTag even when the CS annotation is missing (#1414)

ci/bazel/get_workspace_status

@@ -0,0 +1 @@
+/source/bazel/get_workspace_status

ci/tools/bazel.rc

@@ -0,0 +1 @@
+/source/tools/bazel.rc

source/common/tracing/zipkin/zipkin_tracer_impl.cc

@@ -20,9 +20,7 @@ void ZipkinSpan::finishSpan(Tracing::SpanFinalizer& finalizer) {
 }
 
 void ZipkinSpan::setTag(const std::string& name, const std::string& value) {
-  if (this->hasCSAnnotation()) {
-    span_.setTag(name, value);
-  }
+  span_.setTag(name, value);
 }
 
 void ZipkinSpan::injectContext(Http::HeaderMap& request_headers) {
@@ -48,15 +46,6 @@ Tracing::SpanPtr ZipkinSpan::spawnChild(const std::string& name, SystemTime star
   return Tracing::SpanPtr{new ZipkinSpan(*tracer_.startSpan(name, start_time, context), tracer_)};
 }
 
-bool ZipkinSpan::hasCSAnnotation() {
-  auto annotations = span_.annotations();
-  if (annotations.size() > 0) {
-    // We currently expect only one annotation to be in the span when this function is called.
-    return annotations[0].value() == ZipkinCoreConstants::get().CLIENT_SEND;
-  }
-  return false;
-}
-
 Driver::TlsTracer::TlsTracer(TracerPtr&& tracer, Driver& driver)
     : tracer_(std::move(tracer)), driver_(driver) {}
 

source/common/tracing/zipkin/zipkin_tracer_impl.h

@@ -50,23 +50,13 @@ class ZipkinSpan : public Tracing::Span {
    * In Zipkin, binary annotations of the type "string" allow arbitrary key-value pairs
    * to be associated with a span.
    *
-   * This function will only add the binary annotation to the span IF
-   * it contains the CS (Client Send) basic annotation. If this span contains the CS basic
-   * annotation then this Envoy instance initiated the span. Only the Envoy that initiates a Zipkin
-   * span should add binary annotations to it; otherwise, duplicate binary annotations
-   * would be created.
-   *
    * Note that Tracing::HttpTracerUtility::finalizeSpan() makes several calls to this function,
    * associating several key-value pairs with this span.
    */
   void setTag(const std::string& name, const std::string& value) override;
 
   void injectContext(Http::HeaderMap& request_headers) override;
   Tracing::SpanPtr spawnChild(const std::string& name, SystemTime start_time) override;
-  /**
-   * @return true if this span has a CS (Client Send) basic annotation, or false otherwise.
-   */
-  bool hasCSAnnotation();
 
   /**
    * @return a reference to the Zipkin::Span object.

test/common/tracing/zipkin/zipkin_tracer_impl_test.cc

@@ -297,7 +297,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanTest) {
   EXPECT_EQ("value", zipkin_zipkin_span.binaryAnnotations()[0].value());
 
   // ====
-  // Test innocuous setTag() with annotated span
+  // Test setTag() with SR annotated span
   // ====
 
   const std::string trace_id = Hex::uint64ToHex(Util::generateRandom64());
@@ -312,18 +312,27 @@ TEST_F(ZipkinDriverTest, ZipkinSpanTest) {
   Tracing::SpanPtr span2 = driver_->startSpan(request_headers_, operation_name_, start_time_);
 
   ZipkinSpanPtr zipkin_span2(dynamic_cast<ZipkinSpan*>(span2.release()));
-  zipkin_span2->setTag("key", "value");
+  zipkin_span2->setTag("key2", "value2");
 
   Span& zipkin_zipkin_span2 = zipkin_span2->span();
-  EXPECT_EQ(0ULL, zipkin_zipkin_span2.binaryAnnotations().size());
+  EXPECT_EQ(1ULL, zipkin_zipkin_span2.binaryAnnotations().size());
+  EXPECT_EQ("key2", zipkin_zipkin_span2.binaryAnnotations()[0].key());
+  EXPECT_EQ("value2", zipkin_zipkin_span2.binaryAnnotations()[0].value());
 
   // ====
-  // Test innocuous setTag() with empty annotations vector
+  // Test setTag() with empty annotations vector
   // ====
+  Tracing::SpanPtr span3 = driver_->startSpan(request_headers_, operation_name_, start_time_);
+  ZipkinSpanPtr zipkin_span3(dynamic_cast<ZipkinSpan*>(span3.release()));
+  Span& zipkin_zipkin_span3 = zipkin_span3->span();
+
   std::vector<Annotation> annotations;
-  zipkin_zipkin_span2.setAnnotations(annotations);
-  zipkin_span2->setTag("key", "value");
-  EXPECT_EQ(0ULL, zipkin_zipkin_span2.binaryAnnotations().size());
+  zipkin_zipkin_span3.setAnnotations(annotations);
+
+  zipkin_span3->setTag("key3", "value3");
+  EXPECT_EQ(1ULL, zipkin_zipkin_span3.binaryAnnotations().size());
+  EXPECT_EQ("key3", zipkin_zipkin_span3.binaryAnnotations()[0].key());
+  EXPECT_EQ("value3", zipkin_zipkin_span3.binaryAnnotations()[0].value());
 }
 } // namespace Zipkin
 } // namespace Envoy