skywalking: use upstream host address as addressUsedAtClient in propagation header

changelogs/1.23.0.yaml

@@ -33,6 +33,9 @@ minor_behavior_changes:
   change: |
     export symbols of LuaJit by default on Linux. This is useful in cases where you have a lua script
     that loads shared object libraries, such as those installed via luarocks.
+- area: skywalking
+  change: |
+    use upstream host address as ``addressUsedAtClient`` in propagation header.
 
 bug_fixes:
 

envoy/tracing/trace_driver.h

@@ -50,8 +50,10 @@ class Span {
    * Mutate the provided headers with the context necessary to propagate this
    * (implementation-specific) trace.
    * @param request_headers the headers to which propagation context will be added
+   * @param upstream connecting host description
    */
-  virtual void injectContext(TraceContext& trace_conext) PURE;
+  virtual void injectContext(TraceContext& trace_conext,
+                             const Upstream::HostDescriptionConstSharedPtr& upstream) PURE;
 
   /**
    * Create and start a child Span, with this Span as its parent in the trace.

source/common/grpc/async_client_impl.cc

@@ -254,7 +254,7 @@ void AsyncRequestImpl::cancel() {
 }
 
 void AsyncRequestImpl::onCreateInitialMetadata(Http::RequestHeaderMap& metadata) {
-  current_span_->injectContext(metadata);
+  current_span_->injectContext(metadata, nullptr);
   callbacks_.onCreateInitialMetadata(metadata);
 }
 

source/common/grpc/google_async_client_impl.cc

@@ -455,7 +455,7 @@ void GoogleAsyncRequestImpl::cancel() {
 }
 
 void GoogleAsyncRequestImpl::onCreateInitialMetadata(Http::RequestHeaderMap& metadata) {
-  current_span_->injectContext(metadata);
+  current_span_->injectContext(metadata, nullptr);
   callbacks_.onCreateInitialMetadata(metadata);
 }
 

source/common/http/async_client_impl.cc

@@ -270,7 +270,7 @@ AsyncRequestImpl::AsyncRequestImpl(RequestMessagePtr&& request, AsyncClientImpl&
 }
 
 void AsyncRequestImpl::initialize() {
-  child_span_->injectContext(request_->headers());
+  child_span_->injectContext(request_->headers(), nullptr);
   sendHeaders(request_->headers(), request_->body().length() == 0);
   if (request_->body().length() != 0) {
     // It's possible this will be a no-op due to a local response synchronously generated in

source/common/router/upstream_request.cc

@@ -488,12 +488,12 @@ void UpstreamRequest::onPoolReady(
   }
 
   if (span_ != nullptr) {
-    span_->injectContext(*parent_.downstreamHeaders());
+    span_->injectContext(*parent_.downstreamHeaders(), host);
   } else {
     // No independent child span for current upstream request then inject the parent span's tracing
     // context into the request headers.
     // The injectContext() of the parent span may be called repeatedly when the request is retried.
-    parent_.callbacks()->activeSpan().injectContext(*parent_.downstreamHeaders());
+    parent_.callbacks()->activeSpan().injectContext(*parent_.downstreamHeaders(), host);
   }
 
   upstreamTiming().onFirstUpstreamTxByteSent(parent_.callbacks()->dispatcher().timeSource());

source/common/tracing/null_span_impl.h

@@ -22,7 +22,8 @@ class NullSpan : public Span {
   void setTag(absl::string_view, absl::string_view) override {}
   void log(SystemTime, const std::string&) override {}
   void finishSpan() override {}
-  void injectContext(Tracing::TraceContext&) override {}
+  void injectContext(Tracing::TraceContext&,
+                     const Upstream::HostDescriptionConstSharedPtr&) override {}
   void setBaggage(absl::string_view, absl::string_view) override {}
   std::string getBaggage(absl::string_view) override { return EMPTY_STRING; }
   std::string getTraceIdAsHex() const override { return EMPTY_STRING; }

source/extensions/tracers/common/ot/opentracing_driver_impl.cc

@@ -107,7 +107,8 @@ std::string OpenTracingSpan::getBaggage(absl::string_view key) {
   return span_->BaggageItem({key.data(), key.length()});
 }
 
-void OpenTracingSpan::injectContext(Tracing::TraceContext& trace_context) {
+void OpenTracingSpan::injectContext(Tracing::TraceContext& trace_context,
+                                    const Upstream::HostDescriptionConstSharedPtr&) {
   if (driver_.propagationMode() == OpenTracingDriver::PropagationMode::SingleHeader) {
     // Inject the span context using Envoy's single-header format.
     std::ostringstream oss;

source/extensions/tracers/common/ot/opentracing_driver_impl.h

@@ -37,7 +37,8 @@ class OpenTracingSpan : public Tracing::Span, Logger::Loggable<Logger::Id::traci
   void setOperation(absl::string_view operation) override;
   void setTag(absl::string_view name, const absl::string_view) override;
   void log(SystemTime timestamp, const std::string& event) override;
-  void injectContext(Tracing::TraceContext& trace_context) override;
+  void injectContext(Tracing::TraceContext& trace_context,
+                     const Upstream::HostDescriptionConstSharedPtr&) override;
   Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name,
                               SystemTime start_time) override;
   void setSampled(bool) override;

source/extensions/tracers/opencensus/opencensus_tracer_impl.cc

@@ -69,7 +69,8 @@ class Span : public Tracing::Span {
   void setTag(absl::string_view name, absl::string_view value) override;
   void log(SystemTime timestamp, const std::string& event) override;
   void finishSpan() override;
-  void injectContext(Tracing::TraceContext& trace_context) override;
+  void injectContext(Tracing::TraceContext& trace_context,
+                     const Upstream::HostDescriptionConstSharedPtr&) override;
   Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name,
                               SystemTime start_time) override;
   void setSampled(bool sampled) override;
@@ -201,7 +202,8 @@ void Span::log(SystemTime /*timestamp*/, const std::string& event) {
 
 void Span::finishSpan() { span_.End(); }
 
-void Span::injectContext(Tracing::TraceContext& trace_context) {
+void Span::injectContext(Tracing::TraceContext& trace_context,
+                         const Upstream::HostDescriptionConstSharedPtr&) {
   using OpenCensusConfig = envoy::config::trace::v3::OpenCensusConfig;
   const auto& ctx = span_.context();
   for (const auto& outgoing : oc_config_.outgoing_trace_context()) {

source/extensions/tracers/skywalking/tracer.cc

@@ -46,13 +46,16 @@ void Span::finishSpan() {
   parent_tracer_.sendSegment(tracing_context_);
 }
 
-void Span::injectContext(Tracing::TraceContext& trace_context) {
+void Span::injectContext(Tracing::TraceContext& trace_context,
+                         const Upstream::HostDescriptionConstSharedPtr& upstream) {
+  const auto remote_address = upstream != nullptr ? upstream->address()->asString()
+                                                  : std::string(trace_context.authority());
   // TODO(wbpcode): Due to https://github.com/SkyAPM/cpp2sky/issues/83 in cpp2sky, it is necessary
   // to ensure that there is '\0' at the end of the string_view parameter to ensure that the
   // corresponding trace header is generated correctly. For this reason, we cannot directly use host
   // as argument. We need create a copy of std::string based on host and std::string will
   // automatically add '\0' to the end of the string content.
-  auto sw8_header = tracing_context_->createSW8HeaderValue(std::string(trace_context.authority()));
+  auto sw8_header = tracing_context_->createSW8HeaderValue(remote_address);
   if (sw8_header.has_value()) {
     trace_context.setByReferenceKey(skywalkingPropagationHeaderKey(), sw8_header.value());
   }

source/extensions/tracers/skywalking/tracer.h

@@ -68,7 +68,8 @@ class Span : public Tracing::Span {
   void setTag(absl::string_view name, absl::string_view value) override;
   void log(SystemTime timestamp, const std::string& event) override;
   void finishSpan() override;
-  void injectContext(Tracing::TraceContext& trace_context) override;
+  void injectContext(Tracing::TraceContext& trace_context,
+                     const Upstream::HostDescriptionConstSharedPtr& upstream) override;
   Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name,
                               SystemTime start_time) override;
   void setSampled(bool do_sample) override;

source/extensions/tracers/xray/tracer.cc

@@ -101,7 +101,8 @@ void Span::finishSpan() {
   broker_.send(json);
 } // namespace XRay
 
-void Span::injectContext(Tracing::TraceContext& trace_context) {
+void Span::injectContext(Tracing::TraceContext& trace_context,
+                         const Upstream::HostDescriptionConstSharedPtr&) {
   const std::string xray_header_value =
       fmt::format("Root={};Parent={};Sampled={}", traceId(), id(), sampled() ? "1" : "0");
   trace_context.setByReferenceKey(XRayTraceHeader, xray_header_value);

source/extensions/tracers/xray/tracer.h

@@ -165,7 +165,8 @@ class Span : public Tracing::Span, Logger::Loggable<Logger::Id::config> {
   /**
    * Adds X-Ray trace header to the set of outgoing headers.
    */
-  void injectContext(Tracing::TraceContext& trace_context) override;
+  void injectContext(Tracing::TraceContext& trace_context,
+                     const Upstream::HostDescriptionConstSharedPtr&) override;
 
   /**
    * Gets the start time of this Span.

source/extensions/tracers/zipkin/zipkin_tracer_impl.cc

@@ -39,7 +39,8 @@ void ZipkinSpan::log(SystemTime timestamp, const std::string& event) {
 void ZipkinSpan::setBaggage(absl::string_view, absl::string_view) {}
 std::string ZipkinSpan::getBaggage(absl::string_view) { return EMPTY_STRING; }
 
-void ZipkinSpan::injectContext(Tracing::TraceContext& trace_context) {
+void ZipkinSpan::injectContext(Tracing::TraceContext& trace_context,
+                               const Upstream::HostDescriptionConstSharedPtr&) {
   // Set the trace-id and span-id headers properly, based on the newly-created span structure.
   trace_context.setByReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID,
                                   span_.traceIdAsHexString());

source/extensions/tracers/zipkin/zipkin_tracer_impl.h

@@ -72,7 +72,8 @@ class ZipkinSpan : public Tracing::Span {
 
   void log(SystemTime timestamp, const std::string& event) override;
 
-  void injectContext(Tracing::TraceContext& trace_context) override;
+  void injectContext(Tracing::TraceContext& trace_context,
+                     const Upstream::HostDescriptionConstSharedPtr&) override;
   Tracing::SpanPtr spawnChild(const Tracing::Config&, const std::string& name,
                               SystemTime start_time) override;
 

test/common/grpc/async_client_impl_test.cc

@@ -184,7 +184,7 @@ TEST_F(EnvoyAsyncClientImplTest, RequestHttpStartFail) {
   EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14")));
   EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(*child_span, finishSpan());
-  EXPECT_CALL(*child_span, injectContext(_)).Times(0);
+  EXPECT_CALL(*child_span, injectContext(_, _)).Times(0);
 
   auto* grpc_request = grpc_client_->send(*method_descriptor_, request_msg, grpc_callbacks,
                                           active_span, Http::AsyncClient::RequestOptions());
@@ -249,7 +249,7 @@ TEST_F(EnvoyAsyncClientImplTest, RequestHttpSendHeadersFail) {
               setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy)));
   EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), Eq("test_cluster")));
   EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().UpstreamAddress), Eq("test_cluster")));
-  EXPECT_CALL(*child_span, injectContext(_));
+  EXPECT_CALL(*child_span, injectContext(_, _));
   EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("13")));
   EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(*child_span, finishSpan());

test/common/grpc/google_async_client_impl_test.cc

@@ -169,7 +169,7 @@ TEST_F(EnvoyGoogleAsyncClientImplTest, RequestHttpStartFail) {
   EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14")));
   EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(*child_span, finishSpan());
-  EXPECT_CALL(*child_span, injectContext(_));
+  EXPECT_CALL(*child_span, injectContext(_, _));
 
   auto* grpc_request = grpc_client_->send(*method_descriptor_, request_msg, grpc_callbacks,
                                           active_span, Http::AsyncClient::RequestOptions());

test/common/grpc/grpc_client_integration_test_harness.h

@@ -385,7 +385,7 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest {
         .Times(AtMost(1));
     EXPECT_CALL(*request->child_span_,
                 setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy)));
-    EXPECT_CALL(*request->child_span_, injectContext(_));
+    EXPECT_CALL(*request->child_span_, injectContext(_, _));
 
     request->grpc_request_ = grpc_client_->send(*method_descriptor_, request_msg, *request,
                                                 active_span, Http::AsyncClient::RequestOptions());

test/common/http/async_client_impl_test.cc

@@ -227,7 +227,7 @@ TEST_F(AsyncClientImplTracingTest, Basic) {
 
   AsyncClient::RequestOptions options = AsyncClient::RequestOptions().setParentSpan(parent_span_);
   EXPECT_CALL(*child_span, setSampled(true));
-  EXPECT_CALL(*child_span, injectContext(_));
+  EXPECT_CALL(*child_span, injectContext(_, _));
 
   auto* request = client_.send(std::move(message_), callbacks_, options);
   EXPECT_NE(request, nullptr);
@@ -281,7 +281,7 @@ TEST_F(AsyncClientImplTracingTest, BasicNamedChildSpan) {
                                             .setChildSpanName(child_span_name_)
                                             .setSampled(false);
   EXPECT_CALL(*child_span, setSampled(false));
-  EXPECT_CALL(*child_span, injectContext(_));
+  EXPECT_CALL(*child_span, injectContext(_, _));
 
   auto* request = client_.send(std::move(message_), callbacks_, options);
   EXPECT_NE(request, nullptr);
@@ -333,7 +333,7 @@ TEST_F(AsyncClientImplTracingTest, BasicNamedChildSpanKeepParentSampling) {
                                             .setChildSpanName(child_span_name_)
                                             .setSampled(absl::nullopt);
   EXPECT_CALL(*child_span, setSampled(_)).Times(0);
-  EXPECT_CALL(*child_span, injectContext(_));
+  EXPECT_CALL(*child_span, injectContext(_, _));
 
   auto* request = client_.send(std::move(message_), callbacks_, options);
   EXPECT_NE(request, nullptr);
@@ -1248,7 +1248,7 @@ TEST_F(AsyncClientImplTracingTest, CancelRequest) {
 
   AsyncClient::RequestOptions options = AsyncClient::RequestOptions().setParentSpan(parent_span_);
   EXPECT_CALL(*child_span, setSampled(true));
-  EXPECT_CALL(*child_span, injectContext(_));
+  EXPECT_CALL(*child_span, injectContext(_, _));
   EXPECT_CALL(callbacks_, onBeforeFinalizeUpstreamSpan(_, _))
       .WillOnce(Invoke([](Tracing::Span& span, const Http::ResponseHeaderMap* response_headers) {
         span.setTag("onBeforeFinalizeUpstreamSpan", "called");
@@ -1336,7 +1336,7 @@ TEST_F(AsyncClientImplTracingTest, DestroyWithActiveRequest) {
 
   AsyncClient::RequestOptions options = AsyncClient::RequestOptions().setParentSpan(parent_span_);
   EXPECT_CALL(*child_span, setSampled(true));
-  EXPECT_CALL(*child_span, injectContext(_));
+  EXPECT_CALL(*child_span, injectContext(_, _));
 
   auto* request = client_.send(std::move(message_), callbacks_, options);
   EXPECT_NE(request, nullptr);
@@ -1537,7 +1537,7 @@ TEST_F(AsyncClientImplTracingTest, RequestTimeout) {
                                             .setParentSpan(parent_span_)
                                             .setTimeout(std::chrono::milliseconds(40));
   EXPECT_CALL(*child_span, setSampled(true));
-  EXPECT_CALL(*child_span, injectContext(_));
+  EXPECT_CALL(*child_span, injectContext(_, _));
 
   auto* request = client_.send(std::move(message_), callbacks_, options);
   EXPECT_NE(request, nullptr);

test/common/router/router_2_test.cc

@@ -309,7 +309,7 @@ TEST_F(RouterTestChildSpan, BasicFlow) {
                      const Http::ConnectionPool::Instance::StreamOptions&)
                      -> Http::ConnectionPool::Cancellable* {
             response_decoder = &decoder;
-            EXPECT_CALL(*child_span, injectContext(_));
+            EXPECT_CALL(*child_span, injectContext(_, _));
             callbacks.onPoolReady(encoder, cm_.thread_local_cluster_.conn_pool_.host_,
                                   upstream_stream_info_, Http::Protocol::Http10);
             return nullptr;
@@ -358,7 +358,7 @@ TEST_F(RouterTestChildSpan, ResetFlow) {
                      const Http::ConnectionPool::Instance::StreamOptions&)
                      -> Http::ConnectionPool::Cancellable* {
             response_decoder = &decoder;
-            EXPECT_CALL(*child_span, injectContext(_));
+            EXPECT_CALL(*child_span, injectContext(_, _));
             callbacks.onPoolReady(encoder, cm_.thread_local_cluster_.conn_pool_.host_,
                                   upstream_stream_info_, Http::Protocol::Http10);
             return nullptr;
@@ -411,7 +411,7 @@ TEST_F(RouterTestChildSpan, CancelFlow) {
       .WillOnce(Invoke([&](Http::StreamDecoder&, Http::ConnectionPool::Callbacks& callbacks,
                            const Http::ConnectionPool::Instance::StreamOptions&)
                            -> Http::ConnectionPool::Cancellable* {
-        EXPECT_CALL(*child_span, injectContext(_));
+        EXPECT_CALL(*child_span, injectContext(_, _));
         callbacks.onPoolReady(encoder, cm_.thread_local_cluster_.conn_pool_.host_,
                               upstream_stream_info_, Http::Protocol::Http10);
         return nullptr;
@@ -458,7 +458,7 @@ TEST_F(RouterTestChildSpan, ResetRetryFlow) {
                      const Http::ConnectionPool::Instance::StreamOptions&)
                      -> Http::ConnectionPool::Cancellable* {
             response_decoder = &decoder;
-            EXPECT_CALL(*child_span_1, injectContext(_));
+            EXPECT_CALL(*child_span_1, injectContext(_, _));
             callbacks.onPoolReady(encoder1, cm_.thread_local_cluster_.conn_pool_.host_,
                                   upstream_stream_info_, Http::Protocol::Http10);
             return nullptr;
@@ -503,7 +503,7 @@ TEST_F(RouterTestChildSpan, ResetRetryFlow) {
                      const Http::ConnectionPool::Instance::StreamOptions&)
                      -> Http::ConnectionPool::Cancellable* {
             response_decoder = &decoder;
-            EXPECT_CALL(*child_span_2, injectContext(_));
+            EXPECT_CALL(*child_span_2, injectContext(_, _));
             EXPECT_CALL(*router_.retry_state_, onHostAttempted(_));
             callbacks.onPoolReady(encoder2, cm_.thread_local_cluster_.conn_pool_.host_,
                                   upstream_stream_info_, Http::Protocol::Http10);
@@ -556,7 +556,7 @@ TEST_F(RouterTestNoChildSpan, BasicFlow) {
                      const Http::ConnectionPool::Instance::StreamOptions&)
                      -> Http::ConnectionPool::Cancellable* {
             response_decoder = &decoder;
-            EXPECT_CALL(callbacks_.active_span_, injectContext(_));
+            EXPECT_CALL(callbacks_.active_span_, injectContext(_, _));
             callbacks.onPoolReady(encoder, cm_.thread_local_cluster_.conn_pool_.host_,
                                   upstream_stream_info_, Http::Protocol::Http10);
             return nullptr;

test/common/router/router_test.cc

@@ -412,6 +412,7 @@ TEST_F(RouterTest, Http1Upstream) {
   Http::TestRequestHeaderMapImpl headers;
   HttpTestUtility::addDefaultHeaders(headers);
   EXPECT_CALL(callbacks_.route_->route_entry_, finalizeRequestHeaders(_, _, true));
+
   router_.decodeHeaders(headers, true);
   EXPECT_EQ("10", headers.get_("x-envoy-expected-rq-timeout-ms"));
 
@@ -431,6 +432,7 @@ TEST_F(RouterTest, Http2Upstream) {
 
   Http::TestRequestHeaderMapImpl headers;
   HttpTestUtility::addDefaultHeaders(headers);
+
   router_.decodeHeaders(headers, true);
 
   // When the router filter gets reset we should cancel the pool request.

test/common/tracing/http_tracer_impl_test.cc

@@ -740,6 +740,8 @@ TEST(HttpNullTracerTest, BasicFunctionality) {
   Http::TestRequestHeaderMapImpl request_headers;
   Http::TestResponseHeaderMapImpl response_headers;
   Http::TestResponseTrailerMapImpl response_trailers;
+  Upstream::HostDescriptionConstSharedPtr host{
+      new testing::NiceMock<Upstream::MockHostDescription>()};
 
   SpanPtr span_ptr =
       null_tracer.startSpan(config, request_headers, stream_info, {Reason::Sampling, true});
@@ -750,7 +752,7 @@ TEST(HttpNullTracerTest, BasicFunctionality) {
   span_ptr->setBaggage("key", "value");
   ASSERT_EQ("", span_ptr->getBaggage("baggage_key"));
   ASSERT_EQ(span_ptr->getTraceIdAsHex(), "");
-  span_ptr->injectContext(request_headers);
+  span_ptr->injectContext(request_headers, host);
   span_ptr->log(SystemTime(), "fake_event");
 
   EXPECT_NE(nullptr, span_ptr->spawnChild(config, "foo", SystemTime()));