From 1a42e58483cc9685109e5af652b5784513409f84 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Tue, 30 Jun 2020 19:20:30 +0000 Subject: [PATCH 1/7] add initial metadata interceptor into google gRPC channel creation Signed-off-by: Pengyuan Bian --- .../common/grpc/google_async_client_impl.cc | 2 +- source/common/grpc/google_grpc_utils.cc | 57 ++++++++++++++++++- source/common/grpc/google_grpc_utils.h | 5 +- .../opencensus/opencensus_tracer_impl.cc | 4 +- 4 files changed, 62 insertions(+), 6 deletions(-) diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index d22903da723e..e885a88560b8 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -86,7 +86,7 @@ GoogleAsyncClientImpl::GoogleAsyncClientImpl(Event::Dispatcher& dispatcher, // smart enough to do connection pooling and reuse with identical channel args, so this should // have comparable overhead to what we are doing in Grpc::AsyncClientImpl, i.e. no expensive // new connection implied. - std::shared_ptr channel = GoogleGrpcUtils::createChannel(config, api); + std::shared_ptr channel = GoogleGrpcUtils::createChannel(config, api, false); stub_ = stub_factory.createStub(channel); // Initialize client stats. // TODO(jmarantz): Capture these names in async_client_manager_impl.cc and diff --git a/source/common/grpc/google_grpc_utils.cc b/source/common/grpc/google_grpc_utils.cc index b3fe1e20a320..bc843a0493a8 100644 --- a/source/common/grpc/google_grpc_utils.cc +++ b/source/common/grpc/google_grpc_utils.cc @@ -44,6 +44,46 @@ getGoogleGrpcChannelCredentials(const envoy::config::core::v3::GrpcService& grpc return credentials_factory->getChannelCredentials(grpc_service, api); } +// initialMetadataInterceptor is used to inject the given initial metadata to gRPC channel. +class initialMetadataInterceptor : public grpc::experimental::Interceptor { +public: + initialMetadataInterceptor( + const Protobuf::RepeatedPtrField& initial_metadata) + : initial_metadata_(initial_metadata) {} + + virtual void Intercept(grpc::experimental::InterceptorBatchMethods* methods) { + if (methods->QueryInterceptionHookPoint( + grpc::experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { + auto* metadata_map = methods->GetSendInitialMetadata(); + if (metadata_map != nullptr) { + for (const auto& header_value : initial_metadata_) { + metadata_map->insert(std::make_pair(header_value.key(), header_value.value())); + } + } + } + methods->Proceed(); + } + +private: + const Protobuf::RepeatedPtrField& initial_metadata_; +}; + +class initialMetadataInterceptorFactory + : public grpc::experimental::ClientInterceptorFactoryInterface { +public: + initialMetadataInterceptorFactory( + const Protobuf::RepeatedPtrField& initial_metadata) + : initial_metadata_(initial_metadata) {} + + virtual grpc::experimental::Interceptor* + CreateClientInterceptor(grpc::experimental::ClientRpcInfo*) override { + return new initialMetadataInterceptor(initial_metadata_); + } + +private: + const Protobuf::RepeatedPtrField initial_metadata_; +}; + } // namespace struct BufferInstanceContainer { @@ -134,10 +174,23 @@ GoogleGrpcUtils::channelArgsFromConfig(const envoy::config::core::v3::GrpcServic } std::shared_ptr -GoogleGrpcUtils::createChannel(const envoy::config::core::v3::GrpcService& config, Api::Api& api) { +GoogleGrpcUtils::createChannel(const envoy::config::core::v3::GrpcService& config, Api::Api& api, + bool enable_initial_metadata_interceptor) { std::shared_ptr creds = getGoogleGrpcChannelCredentials(config, api); const grpc::ChannelArguments args = channelArgsFromConfig(config); - return CreateCustomChannel(config.google_grpc().target_uri(), creds, args); + if (!enable_initial_metadata_interceptor || config.initial_metadata().empty()) { + // Skip adding initial metadata interceptor if it is not enabled, or initial metadata is not + // configured. + return CreateCustomChannel(config.google_grpc().target_uri(), creds, args); + } + + // Create gRPC channel with initial metadata interceptor. + std::vector> + interceptor_factories; + interceptor_factories.push_back(std::unique_ptr( + new initialMetadataInterceptorFactory(config.initial_metadata()))); + return ::grpc::experimental::CreateCustomChannelWithInterceptors( + config.google_grpc().target_uri(), creds, args, std::move(interceptor_factories)); } } // namespace Grpc diff --git a/source/common/grpc/google_grpc_utils.h b/source/common/grpc/google_grpc_utils.h index 859a61ccfff9..b91afe2671a3 100644 --- a/source/common/grpc/google_grpc_utils.h +++ b/source/common/grpc/google_grpc_utils.h @@ -43,10 +43,13 @@ class GoogleGrpcUtils { * Build gRPC channel based on the given GrpcService configuration. * @param config Google gRPC config. * @param api reference to the Api object + * @param enable_initial_metadata_interceptor enables initial metadata interceptor if it is + * configured. * @return static std::shared_ptr a gRPC channel. */ static std::shared_ptr - createChannel(const envoy::config::core::v3::GrpcService& config, Api::Api& api); + createChannel(const envoy::config::core::v3::GrpcService& config, Api::Api& api, + bool enable_initial_metadata_interceptor); }; } // namespace Grpc diff --git a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc index 53e82591350f..acc8071a1b19 100644 --- a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc +++ b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc @@ -280,7 +280,7 @@ Driver::Driver(const envoy::config::trace::v3::OpenCensusConfig& oc_config, // address will be used. stackdriver_service.mutable_google_grpc()->set_target_uri(GoogleStackdriverTraceAddress); } - auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(stackdriver_service, api); + auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(stackdriver_service, api, true); opts.trace_service_stub = ::google::devtools::cloudtrace::v2::TraceService::NewStub(channel); #else throw EnvoyException("Opencensus tracer: cannot handle stackdriver google grpc service, " @@ -303,7 +303,7 @@ Driver::Driver(const envoy::config::trace::v3::OpenCensusConfig& oc_config, #ifdef ENVOY_GOOGLE_GRPC const envoy::config::core::v3::GrpcService& ocagent_service = oc_config.ocagent_grpc_service(); - auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(ocagent_service, api); + auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(ocagent_service, api, true); opts.trace_service_stub = ::opencensus::proto::agent::trace::v1::TraceService::NewStub(channel); #else From a8e6f3155c0f79d35571d472a2e301811a00ae92 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Wed, 1 Jul 2020 16:56:48 +0000 Subject: [PATCH 2/7] tidy Signed-off-by: Pengyuan Bian --- source/common/grpc/google_grpc_utils.cc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/source/common/grpc/google_grpc_utils.cc b/source/common/grpc/google_grpc_utils.cc index bc843a0493a8..3c428039e480 100644 --- a/source/common/grpc/google_grpc_utils.cc +++ b/source/common/grpc/google_grpc_utils.cc @@ -44,14 +44,14 @@ getGoogleGrpcChannelCredentials(const envoy::config::core::v3::GrpcService& grpc return credentials_factory->getChannelCredentials(grpc_service, api); } -// initialMetadataInterceptor is used to inject the given initial metadata to gRPC channel. -class initialMetadataInterceptor : public grpc::experimental::Interceptor { +// InitialMetadataInterceptor is used to inject the given initial metadata to gRPC channel. +class InitialMetadataInterceptor : public grpc::experimental::Interceptor { public: - initialMetadataInterceptor( + InitialMetadataInterceptor( const Protobuf::RepeatedPtrField& initial_metadata) : initial_metadata_(initial_metadata) {} - virtual void Intercept(grpc::experimental::InterceptorBatchMethods* methods) { + void Intercept(grpc::experimental::InterceptorBatchMethods* methods) override { if (methods->QueryInterceptionHookPoint( grpc::experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { auto* metadata_map = methods->GetSendInitialMetadata(); @@ -68,16 +68,16 @@ class initialMetadataInterceptor : public grpc::experimental::Interceptor { const Protobuf::RepeatedPtrField& initial_metadata_; }; -class initialMetadataInterceptorFactory +class InitialMetadataInterceptorFactory : public grpc::experimental::ClientInterceptorFactoryInterface { public: - initialMetadataInterceptorFactory( + InitialMetadataInterceptorFactory( const Protobuf::RepeatedPtrField& initial_metadata) : initial_metadata_(initial_metadata) {} - virtual grpc::experimental::Interceptor* + grpc::experimental::Interceptor* CreateClientInterceptor(grpc::experimental::ClientRpcInfo*) override { - return new initialMetadataInterceptor(initial_metadata_); + return new InitialMetadataInterceptor(initial_metadata_); } private: @@ -187,8 +187,8 @@ GoogleGrpcUtils::createChannel(const envoy::config::core::v3::GrpcService& confi // Create gRPC channel with initial metadata interceptor. std::vector> interceptor_factories; - interceptor_factories.push_back(std::unique_ptr( - new initialMetadataInterceptorFactory(config.initial_metadata()))); + interceptor_factories.push_back( + std::make_unique((config.initial_metadata()))); return ::grpc::experimental::CreateCustomChannelWithInterceptors( config.google_grpc().target_uri(), creds, args, std::move(interceptor_factories)); } From 3ecfc93b7db09c660b5be34b5b9f59fb127ffd77 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Sun, 12 Jul 2020 07:14:43 +0000 Subject: [PATCH 3/7] use prepare_client_context Signed-off-by: Pengyuan Bian --- bazel/repository_locations.bzl | 8 +-- .../common/grpc/google_async_client_impl.cc | 2 +- source/common/grpc/google_grpc_utils.cc | 57 +------------------ source/common/grpc/google_grpc_utils.h | 5 +- .../opencensus/opencensus_tracer_impl.cc | 12 +++- 5 files changed, 18 insertions(+), 66 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 4ea42e9748a3..6a573c7a31fe 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -388,10 +388,10 @@ DEPENDENCY_REPOSITORIES = dict( use_category = ["other"], ), io_opencensus_cpp = dict( - sha256 = "193ffb4e13bd7886757fd22b61b7f7a400634412ad8e7e1071e73f57bedd7fc6", - strip_prefix = "opencensus-cpp-04ed0211931f12b03c1a76b3907248ca4db7bc90", - # 2020-03-24 - urls = ["https://github.com/census-instrumentation/opencensus-cpp/archive/04ed0211931f12b03c1a76b3907248ca4db7bc90.tar.gz"], + sha256 = "12ff300fa804f97bd07e2ff071d969e09d5f3d7bbffeac438c725fa52a51a212", + strip_prefix = "opencensus-cpp-7877337633466358ed680f9b26967da5b310d7aa", + # 2020-06-01 + urls = ["https://github.com/census-instrumentation/opencensus-cpp/archive/7877337633466358ed680f9b26967da5b310d7aa.tar.gz"], use_category = ["observability"], cpe = "N/A", ), diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index e885a88560b8..d22903da723e 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -86,7 +86,7 @@ GoogleAsyncClientImpl::GoogleAsyncClientImpl(Event::Dispatcher& dispatcher, // smart enough to do connection pooling and reuse with identical channel args, so this should // have comparable overhead to what we are doing in Grpc::AsyncClientImpl, i.e. no expensive // new connection implied. - std::shared_ptr channel = GoogleGrpcUtils::createChannel(config, api, false); + std::shared_ptr channel = GoogleGrpcUtils::createChannel(config, api); stub_ = stub_factory.createStub(channel); // Initialize client stats. // TODO(jmarantz): Capture these names in async_client_manager_impl.cc and diff --git a/source/common/grpc/google_grpc_utils.cc b/source/common/grpc/google_grpc_utils.cc index 3c428039e480..b3fe1e20a320 100644 --- a/source/common/grpc/google_grpc_utils.cc +++ b/source/common/grpc/google_grpc_utils.cc @@ -44,46 +44,6 @@ getGoogleGrpcChannelCredentials(const envoy::config::core::v3::GrpcService& grpc return credentials_factory->getChannelCredentials(grpc_service, api); } -// InitialMetadataInterceptor is used to inject the given initial metadata to gRPC channel. -class InitialMetadataInterceptor : public grpc::experimental::Interceptor { -public: - InitialMetadataInterceptor( - const Protobuf::RepeatedPtrField& initial_metadata) - : initial_metadata_(initial_metadata) {} - - void Intercept(grpc::experimental::InterceptorBatchMethods* methods) override { - if (methods->QueryInterceptionHookPoint( - grpc::experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { - auto* metadata_map = methods->GetSendInitialMetadata(); - if (metadata_map != nullptr) { - for (const auto& header_value : initial_metadata_) { - metadata_map->insert(std::make_pair(header_value.key(), header_value.value())); - } - } - } - methods->Proceed(); - } - -private: - const Protobuf::RepeatedPtrField& initial_metadata_; -}; - -class InitialMetadataInterceptorFactory - : public grpc::experimental::ClientInterceptorFactoryInterface { -public: - InitialMetadataInterceptorFactory( - const Protobuf::RepeatedPtrField& initial_metadata) - : initial_metadata_(initial_metadata) {} - - grpc::experimental::Interceptor* - CreateClientInterceptor(grpc::experimental::ClientRpcInfo*) override { - return new InitialMetadataInterceptor(initial_metadata_); - } - -private: - const Protobuf::RepeatedPtrField initial_metadata_; -}; - } // namespace struct BufferInstanceContainer { @@ -174,23 +134,10 @@ GoogleGrpcUtils::channelArgsFromConfig(const envoy::config::core::v3::GrpcServic } std::shared_ptr -GoogleGrpcUtils::createChannel(const envoy::config::core::v3::GrpcService& config, Api::Api& api, - bool enable_initial_metadata_interceptor) { +GoogleGrpcUtils::createChannel(const envoy::config::core::v3::GrpcService& config, Api::Api& api) { std::shared_ptr creds = getGoogleGrpcChannelCredentials(config, api); const grpc::ChannelArguments args = channelArgsFromConfig(config); - if (!enable_initial_metadata_interceptor || config.initial_metadata().empty()) { - // Skip adding initial metadata interceptor if it is not enabled, or initial metadata is not - // configured. - return CreateCustomChannel(config.google_grpc().target_uri(), creds, args); - } - - // Create gRPC channel with initial metadata interceptor. - std::vector> - interceptor_factories; - interceptor_factories.push_back( - std::make_unique((config.initial_metadata()))); - return ::grpc::experimental::CreateCustomChannelWithInterceptors( - config.google_grpc().target_uri(), creds, args, std::move(interceptor_factories)); + return CreateCustomChannel(config.google_grpc().target_uri(), creds, args); } } // namespace Grpc diff --git a/source/common/grpc/google_grpc_utils.h b/source/common/grpc/google_grpc_utils.h index b91afe2671a3..859a61ccfff9 100644 --- a/source/common/grpc/google_grpc_utils.h +++ b/source/common/grpc/google_grpc_utils.h @@ -43,13 +43,10 @@ class GoogleGrpcUtils { * Build gRPC channel based on the given GrpcService configuration. * @param config Google gRPC config. * @param api reference to the Api object - * @param enable_initial_metadata_interceptor enables initial metadata interceptor if it is - * configured. * @return static std::shared_ptr a gRPC channel. */ static std::shared_ptr - createChannel(const envoy::config::core::v3::GrpcService& config, Api::Api& api, - bool enable_initial_metadata_interceptor); + createChannel(const envoy::config::core::v3::GrpcService& config, Api::Api& api); }; } // namespace Grpc diff --git a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc index acc8071a1b19..aafea7dc339d 100644 --- a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc +++ b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc @@ -280,8 +280,16 @@ Driver::Driver(const envoy::config::trace::v3::OpenCensusConfig& oc_config, // address will be used. stackdriver_service.mutable_google_grpc()->set_target_uri(GoogleStackdriverTraceAddress); } - auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(stackdriver_service, api, true); + auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(stackdriver_service, api); opts.trace_service_stub = ::google::devtools::cloudtrace::v2::TraceService::NewStub(channel); + const auto& initial_metadata = oc_config.stackdriver_grpc_service().initial_metadata(); + if (!initial_metadata.empty()) { + opts.prepare_client_context = [initial_metadata](grpc::ClientContext* ctx) { + for (const auto& metadata : initial_metadata) { + ctx->AddMetadata(metadata.key(), metadata.value()); + } + }; + } #else throw EnvoyException("Opencensus tracer: cannot handle stackdriver google grpc service, " "google grpc is not built in."); @@ -303,7 +311,7 @@ Driver::Driver(const envoy::config::trace::v3::OpenCensusConfig& oc_config, #ifdef ENVOY_GOOGLE_GRPC const envoy::config::core::v3::GrpcService& ocagent_service = oc_config.ocagent_grpc_service(); - auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(ocagent_service, api, true); + auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(ocagent_service, api); opts.trace_service_stub = ::opencensus::proto::agent::trace::v1::TraceService::NewStub(channel); #else From a901b501ada0339d0fdb497526425ce3b0be1ce0 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Sun, 12 Jul 2020 18:17:53 +0000 Subject: [PATCH 4/7] improve coverage Signed-off-by: Pengyuan Bian --- test/extensions/tracers/opencensus/config_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/extensions/tracers/opencensus/config_test.cc b/test/extensions/tracers/opencensus/config_test.cc index 0f5baa929bdc..59857b41c11a 100644 --- a/test/extensions/tracers/opencensus/config_test.cc +++ b/test/extensions/tracers/opencensus/config_test.cc @@ -322,6 +322,9 @@ TEST(OpenCensusTracerConfigTest, OpenCensusHttpTracerStackdriverGrpc) { google_grpc: target_uri: 127.0.0.1:55678 stat_prefix: test + initial_metadata: + - key: foo + value: bar )EOF"; envoy::config::trace::v3::Tracing configuration; From 4740bb2f97205bd570111a1922fa6f2893e56644 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Mon, 13 Jul 2020 20:11:46 +0000 Subject: [PATCH 5/7] nit Signed-off-by: Pengyuan Bian --- source/extensions/tracers/opencensus/opencensus_tracer_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc index aafea7dc339d..ca7e67a1f82d 100644 --- a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc +++ b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc @@ -282,7 +282,7 @@ Driver::Driver(const envoy::config::trace::v3::OpenCensusConfig& oc_config, } auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(stackdriver_service, api); opts.trace_service_stub = ::google::devtools::cloudtrace::v2::TraceService::NewStub(channel); - const auto& initial_metadata = oc_config.stackdriver_grpc_service().initial_metadata(); + const auto& initial_metadata = stackdriver_service.initial_metadata(); if (!initial_metadata.empty()) { opts.prepare_client_context = [initial_metadata](grpc::ClientContext* ctx) { for (const auto& metadata : initial_metadata) { From abb1557b8355fbabc320acc40c14fdd59feec397 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Thu, 16 Jul 2020 03:46:34 +0000 Subject: [PATCH 6/7] add todo Signed-off-by: Pengyuan Bian --- source/extensions/tracers/opencensus/opencensus_tracer_impl.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc index ca7e67a1f82d..c836bcc49ba6 100644 --- a/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc +++ b/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc @@ -281,6 +281,8 @@ Driver::Driver(const envoy::config::trace::v3::OpenCensusConfig& oc_config, stackdriver_service.mutable_google_grpc()->set_target_uri(GoogleStackdriverTraceAddress); } auto channel = Envoy::Grpc::GoogleGrpcUtils::createChannel(stackdriver_service, api); + // TODO(bianpengyuan): add tests for trace_service_stub and initial_metadata options with mock + // stubs. opts.trace_service_stub = ::google::devtools::cloudtrace::v2::TraceService::NewStub(channel); const auto& initial_metadata = stackdriver_service.initial_metadata(); if (!initial_metadata.empty()) { From 61d1e68bd82958f72ab0c3fea4321c42c7c2e68e Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Thu, 16 Jul 2020 21:27:13 +0000 Subject: [PATCH 7/7] lower coverage limit for opencensus tracer Signed-off-by: Pengyuan Bian --- test/per_file_coverage.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index 8dd9531fc065..37ffe52160db 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -44,8 +44,8 @@ declare -a KNOWN_LOW_COVERAGE=( "source/extensions/retry/host:85.7" "source/extensions/retry/host/omit_canary_hosts:92.9" "source/extensions/stat_sinks/statsd:85.2" -"source/extensions/tracers:96.5" -"source/extensions/tracers/opencensus:92.4" +"source/extensions/tracers:96.3" +"source/extensions/tracers/opencensus:91.2" "source/extensions/tracers/xray:95.3" "source/extensions/transport_sockets:94.9" "source/extensions/transport_sockets/tap:95.6"