From 567467203e4ca4f1ffa730cb942ba559f583ebc0 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Fri, 27 Apr 2018 23:48:54 -0700 Subject: [PATCH 1/2] Create a child span for mixer check call. --- src/envoy/http/mixer/control.cc | 6 ++++-- src/envoy/http/mixer/control.h | 2 ++ src/envoy/utils/grpc_transport.cc | 24 +++++++++++++++--------- src/envoy/utils/grpc_transport.h | 3 +++ src/envoy/utils/mixer_control.cc | 6 ++++-- 5 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/envoy/http/mixer/control.cc b/src/envoy/http/mixer/control.cc index fb8fd8bedb2..718dc3cf268 100644 --- a/src/envoy/http/mixer/control.cc +++ b/src/envoy/http/mixer/control.cc @@ -32,7 +32,8 @@ Control::Control(const Config& config, Upstream::ClusterManager& cm, config_.config_pb().transport().stats_update_interval(), [this](::istio::mixerclient::Statistics* stat) -> bool { return GetStats(stat); - }) { + }), + random_(random) { ::istio::control::http::Controller::Options options(config_.config_pb()); Utils::CreateEnvironment(dispatcher, random, *check_client_factory_, @@ -43,7 +44,8 @@ Control::Control(const Config& config, Upstream::ClusterManager& cm, Utils::CheckTransport::Func Control::GetCheckTransport( const HeaderMap* headers) { - return Utils::CheckTransport::GetFunc(*check_client_factory_, headers); + return Utils::CheckTransport::GetFunc(*check_client_factory_, random_, + headers); } // Call controller to get statistics. diff --git a/src/envoy/http/mixer/control.h b/src/envoy/http/mixer/control.h index aacf783c512..d70962b45ff 100644 --- a/src/envoy/http/mixer/control.h +++ b/src/envoy/http/mixer/control.h @@ -56,6 +56,8 @@ class Control final : public ThreadLocal::ThreadLocalObject { Grpc::AsyncClientFactoryPtr report_client_factory_; // The stats object. Utils::MixerStatsObject stats_obj_; + // Random number generator for remote check span id. + Runtime::RandomGenerator& random_; }; } // namespace Mixer diff --git a/src/envoy/utils/grpc_transport.cc b/src/envoy/utils/grpc_transport.cc index fe7923a2a48..abdb9c0f9cb 100644 --- a/src/envoy/utils/grpc_transport.cc +++ b/src/envoy/utils/grpc_transport.cc @@ -50,10 +50,11 @@ template GrpcTransport::GrpcTransport( Grpc::AsyncClientPtr async_client, const RequestType &request, const Http::HeaderMap *headers, ResponseType *response, - istio::mixerclient::DoneFunc on_done) + Runtime::RandomGenerator &random, istio::mixerclient::DoneFunc on_done) : async_client_(std::move(async_client)), headers_(headers), response_(response), + random_(random), on_done_(on_done), request_(async_client_->send( descriptor(), request, *this, Tracing::NullSpan::instance(), @@ -69,10 +70,11 @@ void GrpcTransport::onCreateInitialMetadata( CopyHeaderEntry(headers_->RequestId(), kRequestId, metadata); CopyHeaderEntry(headers_->XB3TraceId(), kB3TraceId, metadata); - CopyHeaderEntry(headers_->XB3SpanId(), kB3SpanId, metadata); - CopyHeaderEntry(headers_->XB3ParentSpanId(), kB3ParentSpanId, metadata); CopyHeaderEntry(headers_->XB3Sampled(), kB3Sampled, metadata); CopyHeaderEntry(headers_->XB3Flags(), kB3Flags, metadata); + // Create a child span for mixer call. + CopyHeaderEntry(headers_->XB3SpanId(), kB3ParentSpanId, metadata); + metadata.addReferenceKey(kB3SpanId, Hex::uint64ToHex(random_.random())); // This one is NOT inline, need to do linar search. CopyHeaderEntry(headers_->get(kOtSpanContext), kOtSpanContext, metadata); @@ -107,12 +109,14 @@ void GrpcTransport::Cancel() { template typename GrpcTransport::Func GrpcTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers) { - return [&factory, headers](const RequestType &request, ResponseType *response, - istio::mixerclient::DoneFunc on_done) + Grpc::AsyncClientFactory &factory, Runtime::RandomGenerator &random, + const Http::HeaderMap *headers) { + return [&factory, &random, headers](const RequestType &request, + ResponseType *response, + istio::mixerclient::DoneFunc on_done) -> istio::mixerclient::CancelFunc { auto transport = new GrpcTransport( - factory.create(), request, headers, response, on_done); + factory.create(), request, headers, response, random, on_done); return [transport]() { transport->Cancel(); }; }; } @@ -137,9 +141,11 @@ const google::protobuf::MethodDescriptor &ReportTransport::descriptor() { // explicitly instantiate CheckTransport and ReportTransport template CheckTransport::Func CheckTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers); + Grpc::AsyncClientFactory &factory, Runtime::RandomGenerator &random, + const Http::HeaderMap *headers); template ReportTransport::Func ReportTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers); + Grpc::AsyncClientFactory &factory, Runtime::RandomGenerator &random, + const Http::HeaderMap *headers); } // namespace Utils } // namespace Envoy diff --git a/src/envoy/utils/grpc_transport.h b/src/envoy/utils/grpc_transport.h index 2cb60834bec..adf4f48dd13 100644 --- a/src/envoy/utils/grpc_transport.h +++ b/src/envoy/utils/grpc_transport.h @@ -39,10 +39,12 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, istio::mixerclient::DoneFunc on_done)>; static Func GetFunc(Grpc::AsyncClientFactory& factory, + Runtime::RandomGenerator& random, const Http::HeaderMap* headers = nullptr); GrpcTransport(Grpc::AsyncClientPtr async_client, const RequestType& request, const Http::HeaderMap* headers, ResponseType* response, + Runtime::RandomGenerator& random, istio::mixerclient::DoneFunc on_done); // Grpc::AsyncRequestCallbacks @@ -62,6 +64,7 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, Grpc::AsyncClientPtr async_client_; const Http::HeaderMap* headers_; ResponseType* response_; + Runtime::RandomGenerator& random_; ::istio::mixerclient::DoneFunc on_done_; Grpc::AsyncRequest* request_{}; }; diff --git a/src/envoy/utils/mixer_control.cc b/src/envoy/utils/mixer_control.cc index 01ccaac74b5..aff9c9819b5 100644 --- a/src/envoy/utils/mixer_control.cc +++ b/src/envoy/utils/mixer_control.cc @@ -61,8 +61,10 @@ void CreateEnvironment(Event::Dispatcher &dispatcher, Grpc::AsyncClientFactory &check_client_factory, Grpc::AsyncClientFactory &report_client_factory, ::istio::mixerclient::Environment *env) { - env->check_transport = CheckTransport::GetFunc(check_client_factory, nullptr); - env->report_transport = ReportTransport::GetFunc(report_client_factory); + env->check_transport = + CheckTransport::GetFunc(check_client_factory, random, nullptr); + env->report_transport = + ReportTransport::GetFunc(report_client_factory, random); env->timer_create_func = [&dispatcher](std::function timer_cb) -> std::unique_ptr<::istio::mixerclient::Timer> { From b87c5fd2cb428e1e3dee5fb718bb83ff4061f0cd Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Tue, 1 May 2018 11:51:29 -0700 Subject: [PATCH 2/2] Using envoy tracing library instead of propagating header manually. --- src/envoy/http/mixer/control.cc | 8 ++-- src/envoy/http/mixer/control.h | 4 +- src/envoy/http/mixer/filter.cc | 3 +- src/envoy/utils/grpc_transport.cc | 61 +++++-------------------------- src/envoy/utils/grpc_transport.h | 11 ++---- src/envoy/utils/mixer_control.cc | 8 ++-- 6 files changed, 23 insertions(+), 72 deletions(-) diff --git a/src/envoy/http/mixer/control.cc b/src/envoy/http/mixer/control.cc index 718dc3cf268..0ce9e327415 100644 --- a/src/envoy/http/mixer/control.cc +++ b/src/envoy/http/mixer/control.cc @@ -32,8 +32,7 @@ Control::Control(const Config& config, Upstream::ClusterManager& cm, config_.config_pb().transport().stats_update_interval(), [this](::istio::mixerclient::Statistics* stat) -> bool { return GetStats(stat); - }), - random_(random) { + }) { ::istio::control::http::Controller::Options options(config_.config_pb()); Utils::CreateEnvironment(dispatcher, random, *check_client_factory_, @@ -43,9 +42,8 @@ Control::Control(const Config& config, Upstream::ClusterManager& cm, } Utils::CheckTransport::Func Control::GetCheckTransport( - const HeaderMap* headers) { - return Utils::CheckTransport::GetFunc(*check_client_factory_, random_, - headers); + Tracing::Span& parent_span) { + return Utils::CheckTransport::GetFunc(*check_client_factory_, parent_span); } // Call controller to get statistics. diff --git a/src/envoy/http/mixer/control.h b/src/envoy/http/mixer/control.h index d70962b45ff..07740a4df9e 100644 --- a/src/envoy/http/mixer/control.h +++ b/src/envoy/http/mixer/control.h @@ -41,7 +41,7 @@ class Control final : public ThreadLocal::ThreadLocalObject { ::istio::control::http::Controller* controller() { return controller_.get(); } // Create a per-request Check transport function. - Utils::CheckTransport::Func GetCheckTransport(const HeaderMap* headers); + Utils::CheckTransport::Func GetCheckTransport(Tracing::Span& parent_span); private: // Call controller to get statistics. @@ -56,8 +56,6 @@ class Control final : public ThreadLocal::ThreadLocalObject { Grpc::AsyncClientFactoryPtr report_client_factory_; // The stats object. Utils::MixerStatsObject stats_obj_; - // Random number generator for remote check span id. - Runtime::RandomGenerator& random_; }; } // namespace Mixer diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc index a2c67664143..e6e57388438 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -133,7 +133,8 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) { HeaderUpdate header_update(&headers); headers_ = &headers; cancel_check_ = handler_->Check( - &check_data, &header_update, control_.GetCheckTransport(&headers), + &check_data, &header_update, + control_.GetCheckTransport(decoder_callbacks_->activeSpan()), [this](const Status& status) { completeCheck(status); }); initiating_call_ = false; diff --git a/src/envoy/utils/grpc_transport.cc b/src/envoy/utils/grpc_transport.cc index abdb9c0f9cb..8c2829f13ae 100644 --- a/src/envoy/utils/grpc_transport.cc +++ b/src/envoy/utils/grpc_transport.cc @@ -25,61 +25,23 @@ namespace { // gRPC request timeout const std::chrono::milliseconds kGrpcRequestTimeoutMs(5000); -// HTTP trace headers that should pass to gRPC metadata from origin request. -// x-request-id is added for easy debugging. -const Http::LowerCaseString kRequestId("x-request-id"); -const Http::LowerCaseString kB3TraceId("x-b3-traceid"); -const Http::LowerCaseString kB3SpanId("x-b3-spanid"); -const Http::LowerCaseString kB3ParentSpanId("x-b3-parentspanid"); -const Http::LowerCaseString kB3Sampled("x-b3-sampled"); -const Http::LowerCaseString kB3Flags("x-b3-flags"); -const Http::LowerCaseString kOtSpanContext("x-ot-span-context"); - -inline void CopyHeaderEntry(const Http::HeaderEntry *entry, - const Http::LowerCaseString &key, - Http::HeaderMap &headers) { - if (entry) { - std::string val(entry->value().c_str(), entry->value().size()); - headers.addReferenceKey(key, val); - } -} - } // namespace template GrpcTransport::GrpcTransport( Grpc::AsyncClientPtr async_client, const RequestType &request, - const Http::HeaderMap *headers, ResponseType *response, - Runtime::RandomGenerator &random, istio::mixerclient::DoneFunc on_done) + ResponseType *response, Tracing::Span &parent_span, + istio::mixerclient::DoneFunc on_done) : async_client_(std::move(async_client)), - headers_(headers), response_(response), - random_(random), on_done_(on_done), request_(async_client_->send( - descriptor(), request, *this, Tracing::NullSpan::instance(), + descriptor(), request, *this, parent_span, absl::optional(kGrpcRequestTimeoutMs))) { ENVOY_LOG(debug, "Sending {} request: {}", descriptor().name(), request.DebugString()); } -template -void GrpcTransport::onCreateInitialMetadata( - Http::HeaderMap &metadata) { - if (!headers_) return; - - CopyHeaderEntry(headers_->RequestId(), kRequestId, metadata); - CopyHeaderEntry(headers_->XB3TraceId(), kB3TraceId, metadata); - CopyHeaderEntry(headers_->XB3Sampled(), kB3Sampled, metadata); - CopyHeaderEntry(headers_->XB3Flags(), kB3Flags, metadata); - // Create a child span for mixer call. - CopyHeaderEntry(headers_->XB3SpanId(), kB3ParentSpanId, metadata); - metadata.addReferenceKey(kB3SpanId, Hex::uint64ToHex(random_.random())); - - // This one is NOT inline, need to do linar search. - CopyHeaderEntry(headers_->get(kOtSpanContext), kOtSpanContext, metadata); -} - template void GrpcTransport::onSuccess( std::unique_ptr &&response, Tracing::Span &) { @@ -109,14 +71,13 @@ void GrpcTransport::Cancel() { template typename GrpcTransport::Func GrpcTransport::GetFunc( - Grpc::AsyncClientFactory &factory, Runtime::RandomGenerator &random, - const Http::HeaderMap *headers) { - return [&factory, &random, headers](const RequestType &request, - ResponseType *response, - istio::mixerclient::DoneFunc on_done) + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span) { + return [&factory, &parent_span](const RequestType &request, + ResponseType *response, + istio::mixerclient::DoneFunc on_done) -> istio::mixerclient::CancelFunc { auto transport = new GrpcTransport( - factory.create(), request, headers, response, random, on_done); + factory.create(), request, response, parent_span, on_done); return [transport]() { transport->Cancel(); }; }; } @@ -141,11 +102,9 @@ const google::protobuf::MethodDescriptor &ReportTransport::descriptor() { // explicitly instantiate CheckTransport and ReportTransport template CheckTransport::Func CheckTransport::GetFunc( - Grpc::AsyncClientFactory &factory, Runtime::RandomGenerator &random, - const Http::HeaderMap *headers); + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span); template ReportTransport::Func ReportTransport::GetFunc( - Grpc::AsyncClientFactory &factory, Runtime::RandomGenerator &random, - const Http::HeaderMap *headers); + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span); } // namespace Utils } // namespace Envoy diff --git a/src/envoy/utils/grpc_transport.h b/src/envoy/utils/grpc_transport.h index adf4f48dd13..bda56b6621f 100644 --- a/src/envoy/utils/grpc_transport.h +++ b/src/envoy/utils/grpc_transport.h @@ -39,16 +39,13 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, istio::mixerclient::DoneFunc on_done)>; static Func GetFunc(Grpc::AsyncClientFactory& factory, - Runtime::RandomGenerator& random, - const Http::HeaderMap* headers = nullptr); + Tracing::Span& parent_span); GrpcTransport(Grpc::AsyncClientPtr async_client, const RequestType& request, - const Http::HeaderMap* headers, ResponseType* response, - Runtime::RandomGenerator& random, + ResponseType* response, Tracing::Span& parent_span, istio::mixerclient::DoneFunc on_done); - // Grpc::AsyncRequestCallbacks - void onCreateInitialMetadata(Http::HeaderMap& metadata) override; + void onCreateInitialMetadata(Http::HeaderMap&) override {} void onSuccess(std::unique_ptr&& response, Tracing::Span& span) override; @@ -62,9 +59,7 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, static const google::protobuf::MethodDescriptor& descriptor(); Grpc::AsyncClientPtr async_client_; - const Http::HeaderMap* headers_; ResponseType* response_; - Runtime::RandomGenerator& random_; ::istio::mixerclient::DoneFunc on_done_; Grpc::AsyncRequest* request_{}; }; diff --git a/src/envoy/utils/mixer_control.cc b/src/envoy/utils/mixer_control.cc index aff9c9819b5..082dde704b5 100644 --- a/src/envoy/utils/mixer_control.cc +++ b/src/envoy/utils/mixer_control.cc @@ -61,10 +61,10 @@ void CreateEnvironment(Event::Dispatcher &dispatcher, Grpc::AsyncClientFactory &check_client_factory, Grpc::AsyncClientFactory &report_client_factory, ::istio::mixerclient::Environment *env) { - env->check_transport = - CheckTransport::GetFunc(check_client_factory, random, nullptr); - env->report_transport = - ReportTransport::GetFunc(report_client_factory, random); + env->check_transport = CheckTransport::GetFunc(check_client_factory, + Tracing::NullSpan::instance()); + env->report_transport = ReportTransport::GetFunc( + report_client_factory, Tracing::NullSpan::instance()); env->timer_create_func = [&dispatcher](std::function timer_cb) -> std::unique_ptr<::istio::mixerclient::Timer> {