disallow entry span to create propagation context (#55)

* disallow entry span to create propagation context * review
SkyAPM · Feb 3, 2021 · 7dbdfbe · 7dbdfbe
1 parent 591e0dc
commit 7dbdfbe
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 39 deletions.
diff --git a/cpp2sky/segment_context.h b/cpp2sky/segment_context.h
@@ -16,6 +16,7 @@
 
 #include <list>
 #include <memory>
+#include <optional>
 #include <string_view>
 
 #include "cpp2sky/config.pb.h"
@@ -238,15 +239,12 @@ class SegmentContext {
    * @param target_address Target address to send request. For more detail:
    * https://github.com/apache/skywalking-data-collect-protocol/blob/master/language-agent/Tracing.proto#L97-L101
    */
-  virtual std::string createSW8HeaderValue(
-      CurrentSegmentSpanPtr parent, const std::string& target_address) = 0;
-  virtual std::string createSW8HeaderValue(CurrentSegmentSpanPtr parent,
-                                           std::string&& target_address) = 0;
+  virtual std::optional<std::string> createSW8HeaderValue(
+      CurrentSegmentSpanPtr parent, const std::string_view target_address) = 0;
   // If you don't specify parent span, stored to current segment, it will be
   // selected newest span as parent span.
-  virtual std::string createSW8HeaderValue(
-      const std::string& target_address) = 0;
-  virtual std::string createSW8HeaderValue(std::string&& target_address) = 0;
+  virtual std::optional<std::string> createSW8HeaderValue(
+      const std::string_view target_address) = 0;
 
   /**
    * Generate Apache SkyWalking native segment object.

diff --git a/example/sample_client.cc b/example/sample_client.cc
@@ -48,9 +48,14 @@ int main() {
   current_span->startSpan("/ping");
 
   httplib::Client cli("remote", 8082);
-  httplib::Headers headers = {
-      {kPropagationHeader.data(),
-       current_segment->createSW8HeaderValue(current_span, "remote:8082")}};
+
+  auto context =
+      current_segment->createSW8HeaderValue(current_span, "remote:8082");
+
+  httplib::Headers headers;
+  if (context.has_value()) {
+    headers = {{kPropagationHeader.data(), *context}};
+  }
   auto res = cli.Get("/ping", headers);
 
   current_span->endSpan();

diff --git a/source/segment_context_impl.cc b/source/segment_context_impl.cc
@@ -198,26 +198,25 @@ CurrentSegmentSpanPtr SegmentContextImpl::createCurrentSegmentRootSpan() {
   return createCurrentSegmentSpan(nullptr);
 }
 
-std::string SegmentContextImpl::createSW8HeaderValue(
-    CurrentSegmentSpanPtr parent_span, const std::string& target_address) {
-  if (parent_span == nullptr) {
+std::optional<std::string> SegmentContextImpl::createSW8HeaderValue(
+    CurrentSegmentSpanPtr parent_span, const std::string_view target_address) {
+  CurrentSegmentSpanPtr target_span = parent_span;
+  if (target_span == nullptr) {
     if (spans_.empty()) {
       throw TracerException(
           "Can't create propagation header because current segment has no "
           "valid span.");
     }
-    return encodeSpan(spans_.back(), target_address);
+    target_span = spans_.back();
   }
-  return encodeSpan(parent_span, target_address);
-}
-
-std::string SegmentContextImpl::createSW8HeaderValue(
-    CurrentSegmentSpanPtr parent_span, std::string&& target_address) {
-  return createSW8HeaderValue(parent_span, target_address);
+  if (target_span->spanType() != SpanType::Exit) {
+    return std::nullopt;
+  }
+  return encodeSpan(target_span, target_address);
 }
 
 std::string SegmentContextImpl::encodeSpan(CurrentSegmentSpanPtr parent_span,
-                                           const std::string& target_address) {
+                                           const std::string_view target_address) {
   assert(parent_span);
   std::string header_value;
 
@@ -232,7 +231,7 @@ std::string SegmentContextImpl::encodeSpan(CurrentSegmentSpanPtr parent_span,
   header_value += Base64::encode(service_) + "-";
   header_value += Base64::encode(service_instance_) + "-";
   header_value += Base64::encode(endpoint) + "-";
-  header_value += Base64::encode(target_address);
+  header_value += Base64::encode(target_address.data());
 
   return header_value;
 }

diff --git a/source/segment_context_impl.h b/source/segment_context_impl.h
@@ -148,24 +148,21 @@ class SegmentContextImpl : public SegmentContext {
       CurrentSegmentSpanPtr parent_span) override;
 
   CurrentSegmentSpanPtr createCurrentSegmentRootSpan() override;
-  std::string createSW8HeaderValue(const std::string& target_address) override {
+  std::optional<std::string> createSW8HeaderValue(
+      const std::string_view target_address) override {
     return createSW8HeaderValue(nullptr, target_address);
   }
-  std::string createSW8HeaderValue(std::string&& target_address) override {
-    return createSW8HeaderValue(nullptr, target_address);
-  }
-  std::string createSW8HeaderValue(CurrentSegmentSpanPtr parent_span,
-                                   const std::string& target_address) override;
-  std::string createSW8HeaderValue(CurrentSegmentSpanPtr parent,
-                                   std::string&& target_address) override;
+  std::optional<std::string> createSW8HeaderValue(
+      CurrentSegmentSpanPtr parent_span,
+      const std::string_view target_address) override;
   SegmentObject createSegmentObject() override;
   void setSkipAnalysis() override { should_skip_analysis_ = true; }
   bool skipAnalysis() override { return should_skip_analysis_; }
   bool readyToSend() override;
 
  private:
   std::string encodeSpan(CurrentSegmentSpanPtr parent_span,
-                         const std::string& target_address);
+                         const std::string_view target_address);
 
   SpanContextPtr parent_span_context_;
   SpanContextExtensionPtr parent_ext_span_context_;

diff --git a/test/e2e/provider.cc b/test/e2e/provider.cc
@@ -38,9 +38,12 @@ void requestPong(Tracer* tracer, SegmentContext* scp,
   current_span->setPeer(target_address);
 
   httplib::Client cli("consumer", 8080);
-  httplib::Headers headers = {
-      {kPropagationHeader.data(),
-       scp->createSW8HeaderValue(current_span, target_address)}};
+  auto context = scp->createSW8HeaderValue(current_span, target_address);
+
+  httplib::Headers headers;
+  if (context.has_value()) {
+    headers = {{kPropagationHeader.data(), *context}};
+  }
   auto res = cli.Get("/pong", headers);
 
   current_span->endSpan();
@@ -54,9 +57,13 @@ void requestUsers(Tracer* tracer, SegmentContext* scp,
   current_span->setPeer(target_address);
 
   httplib::Client cli("interm", 8082);
-  httplib::Headers headers = {
-      {kPropagationHeader.data(),
-       scp->createSW8HeaderValue(current_span, target_address)}};
+  auto context = scp->createSW8HeaderValue(current_span, target_address);
+
+  httplib::Headers headers;
+  if (context.has_value()) {
+    headers = {{kPropagationHeader.data(), *context}};
+  }
+
   auto res = cli.Get("/users", headers);
 
   current_span->endSpan();

diff --git a/test/segment_context_test.cc b/test/segment_context_test.cc
@@ -299,10 +299,21 @@ TEST_F(SegmentContextTest, SW8CreateTest) {
   span->endSpan();
 
   std::string target_address("10.0.0.1:443");
+
+  // Entry span should be rejected as propagation context
+  EXPECT_FALSE(sc.createSW8HeaderValue(span, target_address).has_value());
+
+  auto span2 = sc.createCurrentSegmentSpan(span);
+
+  EXPECT_EQ(sc.spans().size(), 2);
+  EXPECT_EQ(span2->spanId(), 1);
+  span2->startSpan("sample2");
+  span2->endSpan();
+
   std::string expect_sw8(
-      "1-MQ==-dXVpZA==-0-bWVzaA==-c2VydmljZV8w-c2FtcGxlMQ==-MTAuMC4wLjE6NDQz");
+      "1-MQ==-dXVpZA==-1-bWVzaA==-c2VydmljZV8w-c2FtcGxlMQ==-MTAuMC4wLjE6NDQz");
 
-  EXPECT_EQ(expect_sw8, sc.createSW8HeaderValue(span, target_address));
+  EXPECT_EQ(expect_sw8, *sc.createSW8HeaderValue(span2, target_address));
 }
 
 TEST_F(SegmentContextTest, ReadyToSendTest) {