Skip to content

skywalking: inject peer IP address into span's peer field #20454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Shikugawa wants to merge 3 commits into from
+46 −2
Closed

skywalking: inject peer IP address into span's peer field #20454

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7067267
skywalking: inject peer IP address into span's peer field
Shikugawa Mar 22, 2022
6eff106
fix
Shikugawa Mar 28, 2022
94303d4
fix
Shikugawa Mar 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions envoy/tracing/trace_driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,13 @@ class Span {
* @return trace ID as a hex string
*/
virtual std::string getTraceIdAsHex() const PURE;

/**
* To set some fields in Span exclude tags, which needs additional info related with current
* stream.
* @param stream_info stream info
*/
virtual void setStreamInfoIntoSpan(const StreamInfo::StreamInfo& stream_info) PURE;
Comment thread
Shikugawa marked this conversation as resolved.
Outdated
};

/**
Expand Down
1 change: 1 addition & 0 deletions source/common/tracing/http_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ void HttpTracerUtility::finalizeDownstreamSpan(Span& span,
span.setTag(Tracing::Tags::get().ResponseSize, std::to_string(stream_info.bytesSent()));

setCommonTags(span, response_headers, response_trailers, stream_info, tracing_config);
span.setStreamInfoIntoSpan(stream_info);

span.finishSpan();
}
Expand Down
2 changes: 2 additions & 0 deletions source/common/tracing/null_span_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class NullSpan : public Span {
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; }
void setStreamInfoIntoSpan(const StreamInfo::StreamInfo&) override {}

SpanPtr spawnChild(const Config&, const std::string&, SystemTime) override {
return SpanPtr{new NullSpan()};
}
Expand Down
1 change: 1 addition & 0 deletions source/extensions/tracers/common/ot/opentracing_driver_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class OpenTracingSpan : public Tracing::Span, Logger::Loggable<Logger::Id::traci

// TODO: This method is unimplemented for OpenTracing.
std::string getTraceIdAsHex() const override { return EMPTY_STRING; };
void setStreamInfoIntoSpan(const StreamInfo::StreamInfo&) override{};

private:
OpenTracingDriver& driver_;
Expand Down
1 change: 1 addition & 0 deletions source/extensions/tracers/opencensus/opencensus_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class Span : public Tracing::Span {
std::string getBaggage(absl::string_view) override { return EMPTY_STRING; };

std::string getTraceIdAsHex() const override;
void setStreamInfoIntoSpan(const StreamInfo::StreamInfo&) override{};

private:
::opencensus::trace::Span span_;
Expand Down
6 changes: 6 additions & 0 deletions source/extensions/tracers/skywalking/tracer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ void Span::setSampled(bool do_sample) {

void Span::log(SystemTime, const std::string& event) { span_entity_->addLog(EMPTY_STRING, event); }

void Span::setStreamInfoIntoSpan(const StreamInfo::StreamInfo& stream_info) {
if (stream_info.upstreamInfo() && stream_info.upstreamInfo()->upstreamHost()) {
span_entity_->setPeer(stream_info.upstreamInfo()->upstreamHost()->address()->asString());
}
}
Comment on lines +42 to +46

@wbpcode wbpcode Mar 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setStreamInfo will only be called when we finalize downstream span (Entry Span for Skywalking). I am not sure the peer address of Entry Span is the upstream host address ?

The peer address of Entry Span should be the downstream address, right? And the peer address of Exit Span should be the upstream address.
cc @wu-sheng

@wu-sheng wu-sheng Mar 29, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The peer address of Entry Span should be the downstream address, right? And the peer address of Exit Span should be the upstream address.

The peer of exit span is the key. The entry span's is optional actually.

@wbpcode wbpcode Mar 29, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, the peer address of Exit Span should be the upstream address, is that right?

@wu-sheng wu-sheng Mar 29, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is correct.

@wbpcode wbpcode Mar 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wu-sheng Get it, thanks.

@wbpcode wbpcode Mar 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shikugawa based on the above discussions, may be we can complete the peer address injection of Exit Span directly in the #20367.

We can inject the downstream address of the Entry Span here if you think it's necessary. But it should be unnecessary, because Envoy will set peer address tag for the downstream span.

    const auto& remote_address = stream_info.downstreamAddressProvider().directRemoteAddress();

    if (remote_address->type() == Network::Address::Type::Ip) {
      const auto remote_ip = remote_address->ip();
      span.setTag(Tracing::Tags::get().PeerAddress, remote_ip->addressAsString());
    } else {
      span.setTag(Tracing::Tags::get().PeerAddress, remote_address->logicalName());
    }

@Shikugawa Shikugawa Mar 30, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is good to set peer address in exit span in injectContext due to the incompatibility of the name of the function. I think that it will be better as follows:

void setStreamInfo(const StreamInfo::StreamInfo& stream) {
  if (span_entity_->spanType() == Entry) {
    span_entity_->setPeer(tream_info.downstreamAddressProvider().directRemoteAddress()->addressAsString());
  } else {
    span_entity_->setPeer(tream_info.upstreamInfo()->upstreamHost()->address()->asString());
  }
}

And calls setStreamInfo before finishSpan is called.

@wbpcode wbpcode Mar 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reply. My hope is that this can be done with minimal code modification, minimal performance overhead, and avoiding duplicate field settings.

If we had done this directly in #20367, the most critical exit span peer injection might have been done with just one extra line of code. And there is no need to repeatedly obtain the upstream host from stream info, and the performance will be slightly better.

The method name doesn't really matter. Because peer of exit span is a unique concept of skywalking, it is unknown to the caller of the virtual method. So we only need to do this work in any suitable method implementation.

However, this is only my personal opinion after all, it is only for reference. As long as the implementation is clean and correct, we can find another relevant maintainer to review the PR, and his review opinion shall prevail. 😸

@Shikugawa Shikugawa Mar 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these operations have nothing to do with Header Injection, they complicate the code on the SkyWalking side. In addition, the performance impact of such operations is negligible. So I think I should define setStreamInfo(). Do you have any quick ideas? cc. @mattklein123 @lizan

@wu-sheng wu-sheng Mar 31, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these operations have nothing to do with Header Injection

The entry span's operation does. The parent endpoint in the header is the entry span's operation name(path).


void Span::finishSpan() {
span_entity_->endSpan();
parent_tracer_.sendSegment(tracing_context_);
Expand Down
1 change: 1 addition & 0 deletions source/extensions/tracers/skywalking/tracer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class Span : public Tracing::Span {
std::string getBaggage(absl::string_view) override { return EMPTY_STRING; }
void setBaggage(absl::string_view, absl::string_view) override {}
std::string getTraceIdAsHex() const override { return EMPTY_STRING; }
void setStreamInfoIntoSpan(const StreamInfo::StreamInfo& stream_info) override;

const TracingContextPtr tracingContext() { return tracing_context_; }
const TracingSpanPtr spanEntity() { return span_entity_; }
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/tracers/xray/tracer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@ class Span : public Tracing::Span, Logger::Loggable<Logger::Id::config> {
// TODO: This method is unimplemented for X-Ray.
std::string getTraceIdAsHex() const override { return EMPTY_STRING; };

void setStreamInfoIntoSpan(const StreamInfo::StreamInfo&) override{};

/**
* Creates a child span.
* In X-Ray terms this creates a sub-segment and sets its parent ID to the current span's ID.
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/tracers/zipkin/zipkin_tracer_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class ZipkinSpan : public Tracing::Span {
// TODO: This method is unimplemented for Zipkin.
std::string getTraceIdAsHex() const override { return EMPTY_STRING; };

void setStreamInfoIntoSpan(const StreamInfo::StreamInfo&) override{};

/**
* @return a reference to the Zipkin::Span object.
*/
Expand Down
14 changes: 14 additions & 0 deletions test/common/tracing/http_tracer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ TEST_F(HttpConnManFinalizerImplTest, OriginalAndLongPath) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("GET")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip)));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand Down Expand Up @@ -185,6 +186,7 @@ TEST_F(HttpConnManFinalizerImplTest, NoGeneratedId) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("GET")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip)));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand Down Expand Up @@ -215,6 +217,7 @@ TEST_F(HttpConnManFinalizerImplTest, Connect) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("CONNECT")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip)));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand Down Expand Up @@ -258,6 +261,7 @@ tag: d
kind: { host: {} }
metadata_key: { key: m.host, path: [ {key: not-found } ] })EOF",
false, ""}});
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, nullptr, nullptr, nullptr, stream_info, config);
}
Expand Down Expand Up @@ -299,6 +303,8 @@ TEST_F(HttpConnManFinalizerImplTest, StreamInfoLogs) {
EXPECT_CALL(span, log(log_timestamp, Tracing::Logs::get().LastDownstreamTxByteSent));

EXPECT_CALL(config, verbose).WillOnce(Return(true));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, nullptr, nullptr, nullptr, stream_info, config);
}

Expand All @@ -320,6 +326,7 @@ TEST_F(HttpConnManFinalizerImplTest, UpstreamClusterTagSet) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseSize), Eq("11")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseFlags), Eq("-")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().RequestSize), Eq("10")));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, nullptr, nullptr, nullptr, stream_info, config);
}
Expand Down Expand Up @@ -361,6 +368,7 @@ TEST_F(HttpConnManFinalizerImplTest, SpanOptionalHeaders) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamAddress), _)).Times(0);
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), _)).Times(0);
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamClusterName), _)).Times(0);
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand All @@ -380,6 +388,7 @@ TEST_F(HttpConnManFinalizerImplTest, UnixDomainSocketPeerAddressTag) {
EXPECT_CALL(span, setTag(_, _)).Times(AnyNumber());
EXPECT_CALL(span,
setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(remote_address->logicalName())));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand Down Expand Up @@ -498,6 +507,7 @@ tag: dd-10,
kind: { host: {} }
metadata_key: { key: m.host, path: [ { key: not-found } ] })EOF",
false, ""}});
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, nullptr, nullptr, stream_info,
config);
Expand Down Expand Up @@ -551,6 +561,7 @@ TEST_F(HttpConnManFinalizerImplTest, SpanPopulatedFailureResponse) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamAddress), _)).Times(0);
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), _)).Times(0);
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamClusterName), _)).Times(0);
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand Down Expand Up @@ -600,6 +611,7 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcOkStatus) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("0")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("")));
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip)));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand Down Expand Up @@ -657,6 +669,7 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcErrorTag) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied")));
}
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip)));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand Down Expand Up @@ -713,6 +726,7 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcTrailersOnly) {
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied")));
}
EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip)));
EXPECT_CALL(span, setStreamInfoIntoSpan(_));

HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,
&response_trailers, stream_info, config);
Expand Down
4 changes: 3 additions & 1 deletion test/extensions/tracers/opencensus/tracer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ TEST(OpenCensusTracerTest, Span) {
{":path", "/"}, {":method", "GET"}, {"x-request-id", "foo"}};
const std::string operation_name{"my_operation_1"};
SystemTime start_time;
NiceMock<StreamInfo::MockStreamInfo> stream_info;

{
Tracing::SpanPtr span = driver->startSpan(config, request_headers, operation_name, start_time,
Expand All @@ -120,7 +121,8 @@ TEST(OpenCensusTracerTest, Span) {
// injectContext is tested in another unit test.
Tracing::SpanPtr child = span->spawnChild(config, "child_span", start_time);
child->finishSpan();
span->setSampled(false); // Abandon tracer.
span->setSampled(false); // Abandon tracer.
span->setStreamInfoIntoSpan(stream_info); // Nothing to do
span->finishSpan();

// Baggage methods are a noop in opencensus and won't affect events.
Expand Down
3 changes: 3 additions & 0 deletions test/extensions/tracers/skywalking/tracer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class TracerTest : public testing::Test {
SkyWalkingTracerStats tracing_stats_{
SKYWALKING_TRACER_STATS(POOL_COUNTER_PREFIX(mock_scope_, "tracing.skywalking."))};
TracerPtr tracer_;
NiceMock<StreamInfo::MockStreamInfo> stream_info_;
};

// Test that the basic functionality of Tracer is working, including creating Span, using Span to
Expand Down Expand Up @@ -145,8 +146,10 @@ TEST_F(TracerTest, TracerTestCreateNewSpanWithNoPropagationHeaders) {

EXPECT_EQ("TestChild", first_child_span->spanEntity()->operationName());

first_child_span->setStreamInfoIntoSpan(stream_info_);
first_child_span->finishSpan();
EXPECT_NE(0, first_child_span->spanEntity()->endTime());
EXPECT_EQ("10.0.0.1:443", first_child_span->spanEntity()->peer());

Http::TestRequestHeaderMapImpl first_child_headers{{":authority", "test.com"}};

Expand Down
1 change: 1 addition & 0 deletions test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanTest) {

ZipkinSpanPtr zipkin_span(dynamic_cast<ZipkinSpan*>(span.release()));
zipkin_span->setTag("key", "value");
zipkin_span->setStreamInfoIntoSpan(stream_info_); // Nothing to do

Span& zipkin_zipkin_span = zipkin_span->span();
EXPECT_EQ(1ULL, zipkin_zipkin_span.binaryAnnotations().size());
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/tracing/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class MockSpan : public Span {
MOCK_METHOD(void, setBaggage, (absl::string_view key, absl::string_view value));
MOCK_METHOD(std::string, getBaggage, (absl::string_view key));
MOCK_METHOD(std::string, getTraceIdAsHex, (), (const));

MOCK_METHOD(void, setStreamInfoIntoSpan, (const StreamInfo::StreamInfo& stream_info));
SpanPtr spawnChild(const Config& config, const std::string& name,
SystemTime start_time) override {
return SpanPtr{spawnChild_(config, name, start_time)};
Expand Down