Skip to content

Tracing: revamp extract/inject span context #444

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

Merged
RomanDzhabarov merged 13 commits into from
Feb 13, 2017
Merged

Tracing: revamp extract/inject span context #444

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
511fb33
Revert "Revert "Tracing: Allow extract span context data from request…
RomanDzhabarov Feb 7, 2017
520ef9d
Merge branch 'master' into extract_inject
RomanDzhabarov Feb 8, 2017
4d9c751
introduce Base64::encode on string
RomanDzhabarov Feb 9, 2017
4d5897e
nit.
RomanDzhabarov Feb 9, 2017
6a15e7d
move.
RomanDzhabarov Feb 10, 2017
8bab524
boom
RomanDzhabarov Feb 10, 2017
b002b41
addressed comments.
RomanDzhabarov Feb 10, 2017
5abc15f
better comments on methods.
RomanDzhabarov Feb 13, 2017
5157daf
new line.
RomanDzhabarov Feb 13, 2017
2e40027
Merge branch 'master' into encode_string
RomanDzhabarov Feb 13, 2017
b46b109
Merge branch 'encode_string' into extract_inject
RomanDzhabarov Feb 13, 2017
dbc3423
tmp, save work.
RomanDzhabarov Feb 13, 2017
9c05d79
Merge branch 'master' into extract_inject
RomanDzhabarov Feb 13, 2017
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
1 change: 1 addition & 0 deletions include/envoy/http/header_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ class HeaderEntry {
HEADER_FUNC(Host) \
HEADER_FUNC(KeepAlive) \
HEADER_FUNC(Method) \
HEADER_FUNC(OtSpanContext) \
HEADER_FUNC(Path) \
HEADER_FUNC(ProxyConnection) \
HEADER_FUNC(RequestId) \
Expand Down
8 changes: 6 additions & 2 deletions include/envoy/tracing/http_tracer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ class Driver {
public:
virtual ~Driver() {}

virtual SpanPtr startSpan(const std::string& operation_name, SystemTime start_time) PURE;
/**
* Start driver specific span.
*/
virtual SpanPtr startSpan(Http::HeaderMap& request_headers, const std::string& operation_name,
SystemTime start_time) PURE;
};

typedef std::unique_ptr<Driver> DriverPtr;
Expand All @@ -49,7 +53,7 @@ class HttpTracer {
public:
virtual ~HttpTracer() {}

virtual SpanPtr startSpan(const Config& config, const Http::HeaderMap& request_headers,
virtual SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
const Http::AccessLog::RequestInfo& request_info) PURE;
};

Expand Down
2 changes: 1 addition & 1 deletion source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() {
if (request_info_.healthCheck()) {
connection_manager_.config_.tracingStats().health_check_.inc();
} else {
Tracing::HttpTracerUtility::finalizeSpan(*active_span_, request_info_);
Tracing::HttpTracerUtility::finalizeSpan(*active_span_, *request_headers_, request_info_);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions source/common/http/headers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class Headers {
const LowerCaseString KeepAlive{"keep-alive"};
const LowerCaseString Location{"location"};
const LowerCaseString Method{":method"};
const LowerCaseString OtSpanContext{"x-ot-span-context"};
const LowerCaseString Path{":path"};
const LowerCaseString ProxyConnection{"proxy-connection"};
const LowerCaseString RequestId{"x-request-id"};
Expand Down
55 changes: 40 additions & 15 deletions source/common/tracing/http_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "http_tracer_impl.h"

#include "common/common/base64.h"
#include "common/common/macros.h"
#include "common/common/utility.h"
#include "common/grpc/common.h"
Expand All @@ -18,7 +19,7 @@ static std::string buildRequestLine(const Http::HeaderMap& request_headers,
std::string path = request_headers.EnvoyOriginalPath()
? request_headers.EnvoyOriginalPath()->value().c_str()
: request_headers.Path()->value().c_str();
static const size_t max_path_length = 256;
static const size_t max_path_length = 128;

if (path.length() > max_path_length) {
path = path.substr(0, max_path_length);
Expand Down Expand Up @@ -98,9 +99,9 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques
throw std::invalid_argument("Unknown trace_status");
}

void HttpTracerUtility::populateSpan(Span& active_span, const std::string& service_node,
const Http::HeaderMap& request_headers,
void HttpTracerUtility::finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers,
const Http::AccessLog::RequestInfo& request_info) {
// 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));
Expand All @@ -109,16 +110,13 @@ void HttpTracerUtility::populateSpan(Span& active_span, const std::string& servi
active_span.setTag("downstream_cluster",
valueOrDefault(request_headers.EnvoyDownstreamServiceCluster(), "-"));
active_span.setTag("user_agent", valueOrDefault(request_headers.UserAgent(), "-"));
active_span.setTag("node_id", service_node);

if (request_headers.ClientTraceId()) {
active_span.setTag("guid:x-client-trace-id",
std::string(request_headers.ClientTraceId()->value().c_str()));
}
}

void HttpTracerUtility::finalizeSpan(Span& active_span,
const Http::AccessLog::RequestInfo& request_info) {
// 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",
Expand All @@ -135,12 +133,13 @@ void HttpTracerUtility::finalizeSpan(Span& active_span,
HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& local_info)
: driver_(std::move(driver)), local_info_(local_info) {}

SpanPtr HttpTracerImpl::startSpan(const Config& config, const Http::HeaderMap& request_headers,
SpanPtr HttpTracerImpl::startSpan(const Config& config, Http::HeaderMap& request_headers,
const Http::AccessLog::RequestInfo& request_info) {
SpanPtr active_span = driver_->startSpan(config.operationName(), request_info.startTime());
SpanPtr active_span =
driver_->startSpan(request_headers, config.operationName(), request_info.startTime());
if (active_span) {
HttpTracerUtility::populateSpan(*active_span, local_info_.nodeName(), request_headers,
request_info);
active_span->setTag("node_id", local_info_.nodeName());
active_span->setTag("zone", local_info_.zoneName());
}

return active_span;
Expand Down Expand Up @@ -244,11 +243,37 @@ LightStepDriver::LightStepDriver(const Json::Object& config,
});
}

SpanPtr LightStepDriver::startSpan(const std::string& operation_name, SystemTime start_time) {
lightstep::Span span = tls_.getTyped<TlsLightStepTracer>(tls_slot_).tracer_.StartSpan(
operation_name, {lightstep::StartTimestamp(start_time)});
SpanPtr LightStepDriver::startSpan(Http::HeaderMap& request_headers,
const std::string& operation_name, SystemTime start_time) {
lightstep::Tracer& tracer = tls_.getTyped<TlsLightStepTracer>(tls_slot_).tracer_;
LightStepSpanPtr active_span;

if (request_headers.OtSpanContext()) {
// Extract downstream context from HTTP carrier.
std::string parent_context = Base64::decode(request_headers.OtSpanContext()->value().c_str());

@mattklein123 mattklein123 Feb 9, 2017

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.

What happens if the header is invalid or ParseFromString below fails? Do we handle that?

@RomanDzhabarov RomanDzhabarov Feb 9, 2017 •
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.

if header is invalid CarrierStruct will not be populated properly and we'll get spans joined in a single trace by x-request-id, not a big deal

@mattklein123 mattklein123 Feb 9, 2017

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.

Can you add comment that this code is safe if Base64::decode returns empty string or the data is invalid.

@RomanDzhabarov RomanDzhabarov Feb 9, 2017

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.

ParseFromString in the worst case will leave CarrierStruct empty, that'll not crash or anything

@RomanDzhabarov RomanDzhabarov Feb 10, 2017

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'll add comment.

@RomanDzhabarov RomanDzhabarov Feb 13, 2017

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.

added comment.

lightstep::envoy::CarrierStruct ctx;
ctx.ParseFromString(parent_context);

lightstep::SpanContext parent_span_ctx = tracer.Extract(
lightstep::CarrierFormat::EnvoyProtoCarrier, lightstep::envoy::ProtoReader(ctx));
lightstep::Span ls_span =
tracer.StartSpan(operation_name, {lightstep::ChildOf(parent_span_ctx),
lightstep::StartTimestamp(start_time)});
active_span.reset(new LightStepSpan(ls_span));
} else {
lightstep::Span ls_span =
tracer.StartSpan(operation_name, {lightstep::StartTimestamp(start_time)});
active_span.reset(new LightStepSpan(ls_span));
}

// Inject newly created span context into HTTP carrier.
lightstep::envoy::CarrierStruct ctx;
tracer.Inject(active_span->context(), lightstep::CarrierFormat::EnvoyProtoCarrier,
lightstep::envoy::ProtoWriter(&ctx));
Buffer::OwnedImpl buffer(ctx.SerializeAsString());

@mattklein123 mattklein123 Feb 9, 2017

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.

Instead of copying this just to call the function, can you have a version of Base64::encode that just takes memory and length directly.

@RomanDzhabarov RomanDzhabarov Feb 9, 2017

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.

let me look here, should be doable but will schedule encode change on a separate PR first than.

@RomanDzhabarov RomanDzhabarov Feb 10, 2017

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.

#457

@RomanDzhabarov RomanDzhabarov Feb 13, 2017

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.

updated, will post update on this PR soon

request_headers.insertOtSpanContext().value(Base64::encode(buffer, buffer.length()));

return SpanPtr{new LightStepSpan(span)};
return std::move(active_span);
}

void LightStepRecorder::onFailure(Http::AsyncClient::FailureReason) {
Expand Down
23 changes: 11 additions & 12 deletions source/common/tracing/http_tracer_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "common/http/header_map_impl.h"
#include "common/json/json_loader.h"

#include "lightstep/envoy.h"
#include "lightstep/tracer.h"

namespace Tracing {
Expand Down Expand Up @@ -50,25 +51,18 @@ class HttpTracerUtility {
*/
static void mutateHeaders(Http::HeaderMap& request_headers, Runtime::Loader& runtime);

/**
* Fill in span tags based on request.
*/
static void populateSpan(Span& active_span, const std::string& service_node,
const Http::HeaderMap& request_headers,
const Http::AccessLog::RequestInfo& request_info);

/**
* 1) Fill in span tags based on the response headers.
* 2) Finish active span.
*/
static void finalizeSpan(Span& active_span, const Http::AccessLog::RequestInfo& request_info);
static void finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers,
const Http::AccessLog::RequestInfo& request_info);
};

class HttpNullTracer : public HttpTracer {
public:
// Tracing::HttpTracer
SpanPtr startSpan(const Config&, const Http::HeaderMap&,
const Http::AccessLog::RequestInfo&) override {
SpanPtr startSpan(const Config&, Http::HeaderMap&, const Http::AccessLog::RequestInfo&) override {
return nullptr;
}
};
Expand All @@ -78,7 +72,7 @@ class HttpTracerImpl : public HttpTracer {
HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& local_info);

// Tracing::HttpTracer
SpanPtr startSpan(const Config& config, const Http::HeaderMap& request_headers,
SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
const Http::AccessLog::RequestInfo& request_info) override;

private:
Expand All @@ -94,10 +88,14 @@ class LightStepSpan : public Span {
void finishSpan() override;
void setTag(const std::string& name, const std::string& value) override;

lightstep::SpanContext context() { return span_.context(); }

private:
lightstep::Span span_;
};

typedef std::unique_ptr<LightStepSpan> LightStepSpanPtr;

/**
* LightStep (http://lightstep.com/) provides tracing capabilities, aggregation, visualization of
* application trace data.
Expand All @@ -111,7 +109,8 @@ class LightStepDriver : public Driver {
std::unique_ptr<lightstep::TracerOptions> options);

// Tracer::TracingDriver
SpanPtr startSpan(const std::string& operation_name, SystemTime start_time) override;
SpanPtr startSpan(Http::HeaderMap& request_headers, const std::string& operation_name,
SystemTime start_time) override;

Upstream::ClusterManager& clusterManager() { return cm_; }
Upstream::ClusterInfoPtr cluster() { return cluster_; }
Expand Down
14 changes: 10 additions & 4 deletions test/common/http/conn_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) {
decoder = &conn_manager_->newStream(encoder);

Http::HeaderMapPtr headers{
new TestHeaderMapImpl{{":authority", "host"},
new TestHeaderMapImpl{{":method", "GET"},
{":authority", "host"},
{":path", "/"},
{"x-request-id", "125a4afb-6f55-a4ba-ad80-413f09f48a28"}}};
decoder->decodeHeaders(std::move(headers), true);
Expand Down Expand Up @@ -278,7 +279,8 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLog) {
decoder = &conn_manager_->newStream(encoder);

Http::HeaderMapPtr headers{
new TestHeaderMapImpl{{":authority", "host"},
new TestHeaderMapImpl{{":method", "GET"},
{":authority", "host"},
{":path", "/"},
{"x-request-id", "125a4afb-6f55-a4ba-ad80-413f09f48a28"}}};
decoder->decodeHeaders(std::move(headers), true);
Expand Down Expand Up @@ -317,7 +319,8 @@ TEST_F(HttpConnectionManagerImplTest, DoNotStartSpanIfTracingIsNotEnabled) {
decoder = &conn_manager_->newStream(encoder);

Http::HeaderMapPtr headers{
new TestHeaderMapImpl{{":authority", "host"},
new TestHeaderMapImpl{{":method", "GET"},
{":authority", "host"},
{":path", "/"},
{"x-request-id", "125a4afb-6f55-a4ba-ad80-413f09f48a28"}}};
decoder->decodeHeaders(std::move(headers), true);
Expand All @@ -336,8 +339,10 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) {
setup(false, "");

NiceMock<Tracing::MockSpan>* span = new NiceMock<Tracing::MockSpan>();

EXPECT_CALL(tracer_, startSpan_(_, _, _)).WillOnce(Return(span));
EXPECT_CALL(*span, finishSpan()).Times(0);

EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
.WillOnce(Return(true));

Expand All @@ -361,7 +366,8 @@ TEST_F(HttpConnectionManagerImplTest, StartSpanOnlyHealthCheckRequest) {
decoder = &conn_manager_->newStream(encoder);

Http::HeaderMapPtr headers{
new TestHeaderMapImpl{{":authority", "host"},
new TestHeaderMapImpl{{":method", "GET"},
{":authority", "host"},
{":path", "/healthcheck"},
{"x-request-id", "125a4afb-6f55-94ba-ad80-413f09f48a28"}}};
decoder->decodeHeaders(std::move(headers), true);
Expand Down
Loading