From 4e2635e4e71ebabaf2f2581e5c1b7cf05320b216 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Mon, 24 Sep 2018 20:49:39 +0100 Subject: [PATCH 1/4] tracing: Configure tracer from v2 proto configuration Signed-off-by: Gary Brown --- .../extensions/tracers/dynamic_ot/config.cc | 20 +++++-- source/extensions/tracers/dynamic_ot/config.h | 2 +- source/extensions/tracers/lightstep/config.cc | 24 +++++--- source/extensions/tracers/lightstep/config.h | 2 +- .../lightstep/lightstep_tracer_impl.cc | 6 +- .../tracers/lightstep/lightstep_tracer_impl.h | 5 +- source/extensions/tracers/zipkin/config.cc | 20 +++++-- source/extensions/tracers/zipkin/config.h | 2 +- .../tracers/zipkin/zipkin_core_constants.h | 1 - .../tracers/zipkin/zipkin_tracer_impl.cc | 22 +++---- .../tracers/zipkin/zipkin_tracer_impl.h | 3 +- source/server/configuration_impl.cc | 6 +- source/server/configuration_impl.h | 4 +- .../tracers/dynamic_ot/config_test.cc | 22 +++---- .../tracers/lightstep/config_test.cc | 18 +++--- .../lightstep/lightstep_tracer_impl_test.cc | 56 ++++++++---------- test/extensions/tracers/zipkin/config_test.cc | 18 +++--- .../tracers/zipkin/zipkin_tracer_impl_test.cc | 59 ++++++++----------- 18 files changed, 154 insertions(+), 136 deletions(-) diff --git a/source/extensions/tracers/dynamic_ot/config.cc b/source/extensions/tracers/dynamic_ot/config.cc index b82f3f8e8b8c1..4dfa86e477f4b 100644 --- a/source/extensions/tracers/dynamic_ot/config.cc +++ b/source/extensions/tracers/dynamic_ot/config.cc @@ -13,11 +13,21 @@ namespace Extensions { namespace Tracers { namespace DynamicOt { -Tracing::HttpTracerPtr -DynamicOpenTracingTracerFactory::createHttpTracer(const Json::Object& json_config, - Server::Instance& server) { - const std::string library = json_config.getString("library"); - const std::string config = json_config.getObject("config")->asJsonString(); +Tracing::HttpTracerPtr DynamicOpenTracingTracerFactory::createHttpTracer( + const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) { + ProtobufTypes::MessagePtr config_ptr = + ProtobufTypes::MessagePtr{new envoy::config::trace::v2::DynamicOtConfig()}; + + if (configuration.http().has_config()) { + MessageUtil::jsonConvert(configuration.http().config(), *config_ptr); + } + + const auto& dynaot_config = + dynamic_cast(*config_ptr); + + const std::string library = dynaot_config.library(); + Json::ObjectSharedPtr json_config = MessageUtil::getJsonObjectFromMessage(dynaot_config.config()); + const std::string config = json_config->asJsonString(); Tracing::DriverPtr dynamic_driver{ std::make_unique(server.stats(), library, config)}; return std::make_unique(std::move(dynamic_driver), server.localInfo()); diff --git a/source/extensions/tracers/dynamic_ot/config.h b/source/extensions/tracers/dynamic_ot/config.h index 4db3ed14f7213..5c44bc1b77ce0 100644 --- a/source/extensions/tracers/dynamic_ot/config.h +++ b/source/extensions/tracers/dynamic_ot/config.h @@ -17,7 +17,7 @@ namespace DynamicOt { class DynamicOpenTracingTracerFactory : public Server::Configuration::TracerFactory { public: // TracerFactory - Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, + Tracing::HttpTracerPtr createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) override; std::string name() override; }; diff --git a/source/extensions/tracers/lightstep/config.cc b/source/extensions/tracers/lightstep/config.cc index e1b1c5a947b35..a832306ecb732 100644 --- a/source/extensions/tracers/lightstep/config.cc +++ b/source/extensions/tracers/lightstep/config.cc @@ -15,19 +15,29 @@ namespace Extensions { namespace Tracers { namespace Lightstep { -Tracing::HttpTracerPtr LightstepTracerFactory::createHttpTracer(const Json::Object& json_config, - Server::Instance& server) { +Tracing::HttpTracerPtr +LightstepTracerFactory::createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, + Server::Instance& server) { + ProtobufTypes::MessagePtr config_ptr = + ProtobufTypes::MessagePtr{new envoy::config::trace::v2::LightstepConfig()}; + + if (configuration.http().has_config()) { + MessageUtil::jsonConvert(configuration.http().config(), *config_ptr); + } + + const auto& lightstep_config = + dynamic_cast(*config_ptr); std::unique_ptr opts(new lightstep::LightStepTracerOptions()); - const auto access_token_file = - server.api().fileReadToEnd(json_config.getString("access_token_file")); + const auto access_token_file = server.api().fileReadToEnd(lightstep_config.access_token_file()); const auto access_token_sv = StringUtil::rtrim(access_token_file); opts->access_token.assign(access_token_sv.data(), access_token_sv.size()); opts->component_name = server.localInfo().clusterName(); - Tracing::DriverPtr lightstep_driver{new LightStepDriver{ - json_config, server.clusterManager(), server.stats(), server.threadLocal(), server.runtime(), - std::move(opts), Common::Ot::OpenTracingDriver::PropagationMode::TracerNative}}; + Tracing::DriverPtr lightstep_driver{ + new LightStepDriver{lightstep_config, server.clusterManager(), server.stats(), + server.threadLocal(), server.runtime(), std::move(opts), + Common::Ot::OpenTracingDriver::PropagationMode::TracerNative}}; return std::make_unique(std::move(lightstep_driver), server.localInfo()); } diff --git a/source/extensions/tracers/lightstep/config.h b/source/extensions/tracers/lightstep/config.h index 4d29498581bfd..5adad24c6a568 100644 --- a/source/extensions/tracers/lightstep/config.h +++ b/source/extensions/tracers/lightstep/config.h @@ -17,7 +17,7 @@ namespace Lightstep { class LightstepTracerFactory : public Server::Configuration::TracerFactory { public: // TracerFactory - Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, + Tracing::HttpTracerPtr createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) override; std::string name() override; diff --git a/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc b/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc index bc2781b548c5f..68319613bc6f3 100644 --- a/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc +++ b/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc @@ -129,7 +129,7 @@ void LightStepDriver::TlsLightStepTracer::enableTimer() { flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval)); } -LightStepDriver::LightStepDriver(const Json::Object& config, +LightStepDriver::LightStepDriver(const envoy::config::trace::v2::LightstepConfig& lightstep_config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, std::unique_ptr&& options, @@ -138,10 +138,10 @@ LightStepDriver::LightStepDriver(const Json::Object& config, tracer_stats_{LIGHTSTEP_TRACER_STATS(POOL_COUNTER_PREFIX(stats, "tracing.lightstep."))}, tls_{tls.allocateSlot()}, runtime_{runtime}, options_{std::move(options)}, propagation_mode_{propagation_mode} { - Upstream::ThreadLocalCluster* cluster = cm_.get(config.getString("collector_cluster")); + Upstream::ThreadLocalCluster* cluster = cm_.get(lightstep_config.collector_cluster()); if (!cluster) { throw EnvoyException(fmt::format("{} collector cluster is not defined on cluster manager level", - config.getString("collector_cluster"))); + lightstep_config.collector_cluster())); } cluster_ = cluster->info(); diff --git a/source/extensions/tracers/lightstep/lightstep_tracer_impl.h b/source/extensions/tracers/lightstep/lightstep_tracer_impl.h index 14952970d9253..1417678b5016d 100644 --- a/source/extensions/tracers/lightstep/lightstep_tracer_impl.h +++ b/source/extensions/tracers/lightstep/lightstep_tracer_impl.h @@ -50,8 +50,9 @@ class LightStepLogger : Logger::Loggable { */ class LightStepDriver : public Common::Ot::OpenTracingDriver { public: - LightStepDriver(const Json::Object& config, Upstream::ClusterManager& cluster_manager, - Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, + LightStepDriver(const envoy::config::trace::v2::LightstepConfig& lightstep_config, + Upstream::ClusterManager& cluster_manager, Stats::Store& stats, + ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, std::unique_ptr&& options, PropagationMode propagation_mode); diff --git a/source/extensions/tracers/zipkin/config.cc b/source/extensions/tracers/zipkin/config.cc index 51a92001032fe..84dc356680069 100644 --- a/source/extensions/tracers/zipkin/config.cc +++ b/source/extensions/tracers/zipkin/config.cc @@ -13,14 +13,24 @@ namespace Extensions { namespace Tracers { namespace Zipkin { -Tracing::HttpTracerPtr ZipkinTracerFactory::createHttpTracer(const Json::Object& json_config, - Server::Instance& server) { +Tracing::HttpTracerPtr +ZipkinTracerFactory::createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, + Server::Instance& server) { Envoy::Runtime::RandomGenerator& rand = server.random(); + ProtobufTypes::MessagePtr config_ptr = + ProtobufTypes::MessagePtr{new envoy::config::trace::v2::ZipkinConfig()}; - Tracing::DriverPtr zipkin_driver( - new Zipkin::Driver(json_config, server.clusterManager(), server.stats(), server.threadLocal(), - server.runtime(), server.localInfo(), rand, server.timeSystem())); + if (configuration.http().has_config()) { + MessageUtil::jsonConvert(configuration.http().config(), *config_ptr); + } + + const auto& zipkin_config = + dynamic_cast(*config_ptr); + + Tracing::DriverPtr zipkin_driver(new Zipkin::Driver( + zipkin_config, server.clusterManager(), server.stats(), server.threadLocal(), + server.runtime(), server.localInfo(), rand, server.timeSystem())); return Tracing::HttpTracerPtr( new Tracing::HttpTracerImpl(std::move(zipkin_driver), server.localInfo())); diff --git a/source/extensions/tracers/zipkin/config.h b/source/extensions/tracers/zipkin/config.h index 3ed3b108f8567..beccd4b8f3129 100644 --- a/source/extensions/tracers/zipkin/config.h +++ b/source/extensions/tracers/zipkin/config.h @@ -15,7 +15,7 @@ namespace Zipkin { class ZipkinTracerFactory : public Server::Configuration::TracerFactory { public: // TracerFactory - Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, + Tracing::HttpTracerPtr createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) override; std::string name() override; }; diff --git a/source/extensions/tracers/zipkin/zipkin_core_constants.h b/source/extensions/tracers/zipkin/zipkin_core_constants.h index 7ae4803aab1a0..7e90506201c88 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_constants.h +++ b/source/extensions/tracers/zipkin/zipkin_core_constants.h @@ -40,7 +40,6 @@ class ZipkinCoreConstantValues { const std::string NOT_SAMPLED = "0"; const std::string DEFAULT_COLLECTOR_ENDPOINT = "/api/v1/spans"; - const bool DEFAULT_TRACE_ID_128BIT = false; const bool DEFAULT_SHARED_SPAN_CONTEXT = true; }; diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index 0ce4c240fb380..c5adb06b82a58 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -53,8 +53,9 @@ Tracing::SpanPtr ZipkinSpan::spawnChild(const Tracing::Config& config, const std Driver::TlsTracer::TlsTracer(TracerPtr&& tracer, Driver& driver) : tracer_(std::move(tracer)), driver_(driver) {} -Driver::Driver(const Json::Object& config, Upstream::ClusterManager& cluster_manager, - Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, +Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, + Upstream::ClusterManager& cluster_manager, Stats::Store& stats, + ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info, Runtime::RandomGenerator& random_generator, TimeSource& time_source) : cm_(cluster_manager), tracer_stats_{ZIPKIN_TRACER_STATS( @@ -62,21 +63,22 @@ Driver::Driver(const Json::Object& config, Upstream::ClusterManager& cluster_man tls_(tls.allocateSlot()), runtime_(runtime), local_info_(local_info), time_source_(time_source) { - Upstream::ThreadLocalCluster* cluster = cm_.get(config.getString("collector_cluster")); + Upstream::ThreadLocalCluster* cluster = cm_.get(zipkin_config.collector_cluster()); if (!cluster) { throw EnvoyException(fmt::format("{} collector cluster is not defined on cluster manager level", - config.getString("collector_cluster"))); + zipkin_config.collector_cluster())); } cluster_ = cluster->info(); - const std::string collector_endpoint = - config.getString("collector_endpoint", ZipkinCoreConstants::get().DEFAULT_COLLECTOR_ENDPOINT); + std::string collector_endpoint = ZipkinCoreConstants::get().DEFAULT_COLLECTOR_ENDPOINT; + if (zipkin_config.collector_endpoint().size() > 0) { + collector_endpoint = zipkin_config.collector_endpoint(); + } - const bool trace_id_128bit = - config.getBoolean("trace_id_128bit", ZipkinCoreConstants::get().DEFAULT_TRACE_ID_128BIT); + const bool trace_id_128bit = zipkin_config.trace_id_128bit(); - const bool shared_span_context = config.getBoolean( - "shared_span_context", ZipkinCoreConstants::get().DEFAULT_SHARED_SPAN_CONTEXT); + const bool shared_span_context = PROTOBUF_GET_WRAPPED_OR_DEFAULT( + zipkin_config, shared_span_context, ZipkinCoreConstants::get().DEFAULT_SHARED_SPAN_CONTEXT); tls_->set([this, collector_endpoint, &random_generator, trace_id_128bit, shared_span_context]( Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h index cb3754affc1d5..aef3d537ed38f 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h @@ -89,7 +89,8 @@ class Driver : public Tracing::Driver { * Constructor. It adds itself and a newly-created Zipkin::Tracer object to a thread-local store. * Also, it associates the given random-number generator to the Zipkin::Tracer object it creates. */ - Driver(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats, + Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, + Upstream::ClusterManager& cluster_manager, Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, const LocalInfo::LocalInfo& localinfo, Runtime::RandomGenerator& random_generator, TimeSource& time_source); diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 8aab546468fb7..abdf41d6274d5 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -103,13 +103,9 @@ void MainImpl::initializeTracers(const envoy::config::trace::v2::Tracing& config std::string type = configuration.http().name(); ENVOY_LOG(info, " loading tracing driver: {}", type); - // TODO(htuch): Make this dynamically pluggable one day. - Json::ObjectSharedPtr driver_config = - MessageUtil::getJsonObjectFromMessage(configuration.http().config()); - // Now see if there is a factory that will accept the config. auto& factory = Config::Utility::getAndCheckFactory(type); - http_tracer_ = factory.createHttpTracer(*driver_config, server); + http_tracer_ = factory.createHttpTracer(configuration, server); } void MainImpl::initializeStatsSinks(const envoy::config::bootstrap::v2::Bootstrap& bootstrap, diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 615b8af4c5702..2ba0a3ff83caa 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -43,8 +43,8 @@ class TracerFactory { * @param json_config supplies the general json configuration for the HttpTracer * @param server supplies the server instance */ - virtual Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, - Instance& server) PURE; + virtual Tracing::HttpTracerPtr + createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Instance& server) PURE; /** * Returns the identifying name for a particular implementation of tracer produced by the diff --git a/test/extensions/tracers/dynamic_ot/config_test.cc b/test/extensions/tracers/dynamic_ot/config_test.cc index d2c2077bcd7d0..4ca95026c901d 100644 --- a/test/extensions/tracers/dynamic_ot/config_test.cc +++ b/test/extensions/tracers/dynamic_ot/config_test.cc @@ -23,19 +23,21 @@ TEST(DynamicOtTracerConfigTest, DynamicOpentracingHttpTracer) { ON_CALL(*server.cluster_manager_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); - const std::string valid_config = fmt::sprintf(R"EOF( - { - "library": "%s/external/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so", - "config": { - "output_file" : "fake_file" - } - } + const std::string yaml_string = fmt::sprintf(R"EOF( + http: + name: envoy.dynamic.ot + config: + library: %s/external/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so + config: + output_file: fake_file )EOF", - TestEnvironment::runfilesDirectory()); - const Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); + TestEnvironment::runfilesDirectory()); + envoy::config::trace::v2::Tracing configuration; + MessageUtil::loadFromYaml(yaml_string, configuration); + DynamicOpenTracingTracerFactory factory; - const Tracing::HttpTracerPtr tracer = factory.createHttpTracer(*valid_json, server); + const Tracing::HttpTracerPtr tracer = factory.createHttpTracer(configuration, server); EXPECT_NE(nullptr, tracer); } diff --git a/test/extensions/tracers/lightstep/config_test.cc b/test/extensions/tracers/lightstep/config_test.cc index 3eb338d87dd29..b1650ae6bcd15 100644 --- a/test/extensions/tracers/lightstep/config_test.cc +++ b/test/extensions/tracers/lightstep/config_test.cc @@ -21,16 +21,18 @@ TEST(LightstepTracerConfigTest, LightstepHttpTracer) { ON_CALL(*server.cluster_manager_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); - std::string valid_config = R"EOF( - { - "collector_cluster": "fake_cluster", - "access_token_file": "fake_file" - } - )EOF"; - Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); + const std::string yaml_string = R"EOF( + http: + name: envoy.lightstep + config: + collector_cluster: fake_cluster + access_token_file: fake_file + )EOF"; + envoy::config::trace::v2::Tracing configuration; + MessageUtil::loadFromYaml(yaml_string, configuration); LightstepTracerFactory factory; - Tracing::HttpTracerPtr lightstep_tracer = factory.createHttpTracer(*valid_json, server); + Tracing::HttpTracerPtr lightstep_tracer = factory.createHttpTracer(configuration, server); EXPECT_NE(nullptr, lightstep_tracer); } diff --git a/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc b/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc index 2650ddc7df397..6f680cb63e097 100644 --- a/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc +++ b/test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc @@ -42,7 +42,7 @@ namespace Lightstep { class LightStepDriverTest : public Test { public: - void setup(Json::Object& config, bool init_timer, + void setup(envoy::config::trace::v2::LightstepConfig& lightstep_config, bool init_timer, Common::Ot::OpenTracingDriver::PropagationMode propagation_mode = Common::Ot::OpenTracingDriver::PropagationMode::TracerNative) { std::unique_ptr opts( @@ -58,8 +58,8 @@ class LightStepDriverTest : public Test { EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); } - driver_.reset(new LightStepDriver{config, cm_, stats_, tls_, runtime_, std::move(opts), - propagation_mode}); + driver_.reset(new LightStepDriver{lightstep_config, cm_, stats_, tls_, runtime_, + std::move(opts), propagation_mode}); } void setupValidDriver(Common::Ot::OpenTracingDriver::PropagationMode propagation_mode = @@ -68,12 +68,13 @@ class LightStepDriverTest : public Test { ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); - std::string valid_config = R"EOF( - {"collector_cluster": "fake_cluster"} + const std::string yaml_string = R"EOF( + collector_cluster: fake_cluster )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(valid_config); + envoy::config::trace::v2::LightstepConfig lightstep_config; + MessageUtil::loadFromYaml(yaml_string, lightstep_config); - setup(*loader, true, propagation_mode); + setup(lightstep_config, true, propagation_mode); } const std::string operation_name_{"test"}; @@ -106,31 +107,22 @@ TEST_F(LightStepDriverTest, LightStepLogger) { TEST_F(LightStepDriverTest, InitializeDriver) { { - std::string invalid_config = R"EOF( - {"fake" : "fake"} - )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(invalid_config); - - EXPECT_THROW(setup(*loader, false), EnvoyException); - } - - { - std::string empty_config = "{}"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(empty_config); + envoy::config::trace::v2::LightstepConfig lightstep_config; - EXPECT_THROW(setup(*loader, false), EnvoyException); + EXPECT_THROW(setup(lightstep_config, false), EnvoyException); } { // Valid config but not valid cluster. EXPECT_CALL(cm_, get("fake_cluster")).WillOnce(Return(nullptr)); - std::string valid_config = R"EOF( - {"collector_cluster": "fake_cluster"} + const std::string yaml_string = R"EOF( + collector_cluster: fake_cluster )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(valid_config); + envoy::config::trace::v2::LightstepConfig lightstep_config; + MessageUtil::loadFromYaml(yaml_string, lightstep_config); - EXPECT_THROW(setup(*loader, false), EnvoyException); + EXPECT_THROW(setup(lightstep_config, false), EnvoyException); } { @@ -138,12 +130,13 @@ TEST_F(LightStepDriverTest, InitializeDriver) { EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()).WillByDefault(Return(0)); - std::string valid_config = R"EOF( - {"collector_cluster": "fake_cluster"} + const std::string yaml_string = R"EOF( + collector_cluster: fake_cluster )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(valid_config); + envoy::config::trace::v2::LightstepConfig lightstep_config; + MessageUtil::loadFromYaml(yaml_string, lightstep_config); - EXPECT_THROW(setup(*loader, false), EnvoyException); + EXPECT_THROW(setup(lightstep_config, false), EnvoyException); } { @@ -151,12 +144,13 @@ TEST_F(LightStepDriverTest, InitializeDriver) { ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); - std::string valid_config = R"EOF( - {"collector_cluster": "fake_cluster"} + const std::string yaml_string = R"EOF( + collector_cluster: fake_cluster )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(valid_config); + envoy::config::trace::v2::LightstepConfig lightstep_config; + MessageUtil::loadFromYaml(yaml_string, lightstep_config); - setup(*loader, true); + setup(lightstep_config, true); } } diff --git a/test/extensions/tracers/zipkin/config_test.cc b/test/extensions/tracers/zipkin/config_test.cc index e5326187b683f..3373dc7893861 100644 --- a/test/extensions/tracers/zipkin/config_test.cc +++ b/test/extensions/tracers/zipkin/config_test.cc @@ -17,15 +17,19 @@ TEST(ZipkinTracerConfigTest, ZipkinHttpTracer) { EXPECT_CALL(server.cluster_manager_, get("fake_cluster")) .WillRepeatedly(Return(&server.cluster_manager_.thread_local_cluster_)); - std::string valid_config = R"EOF( - { - "collector_cluster": "fake_cluster", - "collector_endpoint": "/api/v1/spans" - } + const std::string yaml_string = R"EOF( + http: + name: envoy.zipkin + config: + collector_cluster: fake_cluster + collector_endpoint: /api/v1/spans )EOF"; - Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); + + envoy::config::trace::v2::Tracing configuration; + MessageUtil::loadFromYaml(yaml_string, configuration); + ZipkinTracerFactory factory; - Tracing::HttpTracerPtr zipkin_tracer = factory.createHttpTracer(*valid_json, server); + Tracing::HttpTracerPtr zipkin_tracer = factory.createHttpTracer(configuration, server); EXPECT_NE(nullptr, zipkin_tracer); } diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index 05ad40b746e84..f89d5be4ffd62 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -40,7 +40,7 @@ class ZipkinDriverTest : public Test { public: ZipkinDriverTest() : time_source_(test_time_.timeSystem()) {} - void setup(Json::Object& config, bool init_timer) { + void setup(envoy::config::trace::v2::ZipkinConfig& zipkin_config, bool init_timer) { ON_CALL(cm_, httpAsyncClientForCluster("fake_cluster")) .WillByDefault(ReturnRef(cm_.async_client_)); @@ -50,21 +50,20 @@ class ZipkinDriverTest : public Test { } driver_.reset( - new Driver(config, cm_, stats_, tls_, runtime_, local_info_, random_, time_source_)); + new Driver(zipkin_config, cm_, stats_, tls_, runtime_, local_info_, random_, time_source_)); } void setupValidDriver() { EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); - std::string valid_config = R"EOF( - { - "collector_cluster": "fake_cluster", - "collector_endpoint": "/api/v1/spans" - } + const std::string yaml_string = R"EOF( + collector_cluster: fake_cluster + collector_endpoint: /api/v1/spans )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(valid_config); + envoy::config::trace::v2::ZipkinConfig zipkin_config; + MessageUtil::loadFromYaml(yaml_string, zipkin_config); - setup(*loader, true); + setup(zipkin_config, true); } // TODO(#4160): Currently time_system_ is initialized from DangerousDeprecatedTestTime, which uses @@ -95,34 +94,23 @@ class ZipkinDriverTest : public Test { TEST_F(ZipkinDriverTest, InitializeDriver) { { - std::string invalid_config = R"EOF( - {"fake" : "fake"} - )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(invalid_config); - - EXPECT_THROW(setup(*loader, false), EnvoyException); - } + // Empty config + envoy::config::trace::v2::ZipkinConfig zipkin_config; - { - std::string empty_config = "{}"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(empty_config); - - EXPECT_THROW(setup(*loader, false), EnvoyException); + EXPECT_THROW(setup(zipkin_config, false), EnvoyException); } { // Valid config but not valid cluster. EXPECT_CALL(cm_, get("fake_cluster")).WillOnce(Return(nullptr)); - - std::string valid_config = R"EOF( - { - "collector_cluster": "fake_cluster", - "collector_endpoint": "/api/v1/spans" - } + const std::string yaml_string = R"EOF( + collector_cluster: fake_cluster + collector_endpoint: /api/v1/spans )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(valid_config); + envoy::config::trace::v2::ZipkinConfig zipkin_config; + MessageUtil::loadFromYaml(yaml_string, zipkin_config); - EXPECT_THROW(setup(*loader, false), EnvoyException); + EXPECT_THROW(setup(zipkin_config, false), EnvoyException); } { @@ -130,15 +118,14 @@ TEST_F(ZipkinDriverTest, InitializeDriver) { EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()).WillByDefault(Return(0)); - std::string valid_config = R"EOF( - { - "collector_cluster": "fake_cluster", - "collector_endpoint": "/api/v1/spans" - } + const std::string yaml_string = R"EOF( + collector_cluster: fake_cluster + collector_endpoint: /api/v1/spans )EOF"; - Json::ObjectSharedPtr loader = Json::Factory::loadFromString(valid_config); + envoy::config::trace::v2::ZipkinConfig zipkin_config; + MessageUtil::loadFromYaml(yaml_string, zipkin_config); - setup(*loader, true); + setup(zipkin_config, true); } } From 2a8658e10a98fb0858417b427c057cbbf69db0fb Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Fri, 28 Sep 2018 11:49:03 +0100 Subject: [PATCH 2/4] Factor out creation of empty proto message Signed-off-by: Gary Brown --- source/extensions/tracers/dynamic_ot/config.cc | 3 +-- source/extensions/tracers/dynamic_ot/config.h | 5 +++++ source/extensions/tracers/lightstep/config.cc | 3 +-- source/extensions/tracers/lightstep/config.h | 4 ++++ source/extensions/tracers/zipkin/config.cc | 3 +-- source/extensions/tracers/zipkin/config.h | 5 +++++ source/server/configuration_impl.h | 7 +++++++ 7 files changed, 24 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/dynamic_ot/config.cc b/source/extensions/tracers/dynamic_ot/config.cc index 4dfa86e477f4b..f9158a568ff77 100644 --- a/source/extensions/tracers/dynamic_ot/config.cc +++ b/source/extensions/tracers/dynamic_ot/config.cc @@ -15,8 +15,7 @@ namespace DynamicOt { Tracing::HttpTracerPtr DynamicOpenTracingTracerFactory::createHttpTracer( const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) { - ProtobufTypes::MessagePtr config_ptr = - ProtobufTypes::MessagePtr{new envoy::config::trace::v2::DynamicOtConfig()}; + ProtobufTypes::MessagePtr config_ptr = createEmptyConfigProto(); if (configuration.http().has_config()) { MessageUtil::jsonConvert(configuration.http().config(), *config_ptr); diff --git a/source/extensions/tracers/dynamic_ot/config.h b/source/extensions/tracers/dynamic_ot/config.h index 5c44bc1b77ce0..186ac204aab7c 100644 --- a/source/extensions/tracers/dynamic_ot/config.h +++ b/source/extensions/tracers/dynamic_ot/config.h @@ -19,6 +19,11 @@ class DynamicOpenTracingTracerFactory : public Server::Configuration::TracerFact // TracerFactory Tracing::HttpTracerPtr createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) override; + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new envoy::config::trace::v2::DynamicOtConfig()}; + } + std::string name() override; }; diff --git a/source/extensions/tracers/lightstep/config.cc b/source/extensions/tracers/lightstep/config.cc index a832306ecb732..e7ef9af908cb9 100644 --- a/source/extensions/tracers/lightstep/config.cc +++ b/source/extensions/tracers/lightstep/config.cc @@ -18,8 +18,7 @@ namespace Lightstep { Tracing::HttpTracerPtr LightstepTracerFactory::createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) { - ProtobufTypes::MessagePtr config_ptr = - ProtobufTypes::MessagePtr{new envoy::config::trace::v2::LightstepConfig()}; + ProtobufTypes::MessagePtr config_ptr = createEmptyConfigProto(); if (configuration.http().has_config()) { MessageUtil::jsonConvert(configuration.http().config(), *config_ptr); diff --git a/source/extensions/tracers/lightstep/config.h b/source/extensions/tracers/lightstep/config.h index 5adad24c6a568..d9d27a59fe29f 100644 --- a/source/extensions/tracers/lightstep/config.h +++ b/source/extensions/tracers/lightstep/config.h @@ -20,6 +20,10 @@ class LightstepTracerFactory : public Server::Configuration::TracerFactory { Tracing::HttpTracerPtr createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) override; + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new envoy::config::trace::v2::LightstepConfig()}; + } + std::string name() override; }; diff --git a/source/extensions/tracers/zipkin/config.cc b/source/extensions/tracers/zipkin/config.cc index 84dc356680069..52eccaceee159 100644 --- a/source/extensions/tracers/zipkin/config.cc +++ b/source/extensions/tracers/zipkin/config.cc @@ -18,8 +18,7 @@ ZipkinTracerFactory::createHttpTracer(const envoy::config::trace::v2::Tracing& c Server::Instance& server) { Envoy::Runtime::RandomGenerator& rand = server.random(); - ProtobufTypes::MessagePtr config_ptr = - ProtobufTypes::MessagePtr{new envoy::config::trace::v2::ZipkinConfig()}; + ProtobufTypes::MessagePtr config_ptr = createEmptyConfigProto(); if (configuration.http().has_config()) { MessageUtil::jsonConvert(configuration.http().config(), *config_ptr); diff --git a/source/extensions/tracers/zipkin/config.h b/source/extensions/tracers/zipkin/config.h index beccd4b8f3129..55893148636e1 100644 --- a/source/extensions/tracers/zipkin/config.h +++ b/source/extensions/tracers/zipkin/config.h @@ -17,6 +17,11 @@ class ZipkinTracerFactory : public Server::Configuration::TracerFactory { // TracerFactory Tracing::HttpTracerPtr createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Server::Instance& server) override; + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new envoy::config::trace::v2::ZipkinConfig()}; + } + std::string name() override; }; diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 2ba0a3ff83caa..f73afe718ac1c 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -46,6 +46,13 @@ class TracerFactory { virtual Tracing::HttpTracerPtr createHttpTracer(const envoy::config::trace::v2::Tracing& configuration, Instance& server) PURE; + /** + * @return ProtobufTypes::MessagePtr create empty config proto message for v2. The tracing + * config, which arrives in an opaque google.protobuf.Struct message, will be converted to + * JSON and then parsed into this empty proto. + */ + virtual ProtobufTypes::MessagePtr createEmptyConfigProto() PURE; + /** * Returns the identifying name for a particular implementation of tracer produced by the * factory. From 2c1f8259fc10957a8d5555af58a3a35571ba0c24 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Sat, 29 Sep 2018 11:31:09 +0100 Subject: [PATCH 3/4] Fix additional comments Signed-off-by: Gary Brown --- source/extensions/tracers/dynamic_ot/config.cc | 3 +-- source/extensions/tracers/dynamic_ot/config.h | 2 +- source/extensions/tracers/lightstep/config.h | 2 +- source/extensions/tracers/zipkin/config.h | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/source/extensions/tracers/dynamic_ot/config.cc b/source/extensions/tracers/dynamic_ot/config.cc index f9158a568ff77..a8ebd85495278 100644 --- a/source/extensions/tracers/dynamic_ot/config.cc +++ b/source/extensions/tracers/dynamic_ot/config.cc @@ -25,8 +25,7 @@ Tracing::HttpTracerPtr DynamicOpenTracingTracerFactory::createHttpTracer( dynamic_cast(*config_ptr); const std::string library = dynaot_config.library(); - Json::ObjectSharedPtr json_config = MessageUtil::getJsonObjectFromMessage(dynaot_config.config()); - const std::string config = json_config->asJsonString(); + const std::string config = MessageUtil::getJsonStringFromMessage(dynaot_config.config()); Tracing::DriverPtr dynamic_driver{ std::make_unique(server.stats(), library, config)}; return std::make_unique(std::move(dynamic_driver), server.localInfo()); diff --git a/source/extensions/tracers/dynamic_ot/config.h b/source/extensions/tracers/dynamic_ot/config.h index 186ac204aab7c..67501acf068f3 100644 --- a/source/extensions/tracers/dynamic_ot/config.h +++ b/source/extensions/tracers/dynamic_ot/config.h @@ -21,7 +21,7 @@ class DynamicOpenTracingTracerFactory : public Server::Configuration::TracerFact Server::Instance& server) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{new envoy::config::trace::v2::DynamicOtConfig()}; + return ProtobufTypes::MessagePtr{std::make_unique()}; } std::string name() override; diff --git a/source/extensions/tracers/lightstep/config.h b/source/extensions/tracers/lightstep/config.h index d9d27a59fe29f..ecbba0c3f83a2 100644 --- a/source/extensions/tracers/lightstep/config.h +++ b/source/extensions/tracers/lightstep/config.h @@ -21,7 +21,7 @@ class LightstepTracerFactory : public Server::Configuration::TracerFactory { Server::Instance& server) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{new envoy::config::trace::v2::LightstepConfig()}; + return ProtobufTypes::MessagePtr{std::make_unique()}; } std::string name() override; diff --git a/source/extensions/tracers/zipkin/config.h b/source/extensions/tracers/zipkin/config.h index 55893148636e1..267d0c712d2dd 100644 --- a/source/extensions/tracers/zipkin/config.h +++ b/source/extensions/tracers/zipkin/config.h @@ -19,7 +19,7 @@ class ZipkinTracerFactory : public Server::Configuration::TracerFactory { Server::Instance& server) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{new envoy::config::trace::v2::ZipkinConfig()}; + return ProtobufTypes::MessagePtr{std::make_unique()}; } std::string name() override; From d1848235a004ee6e69bdbcad7167ddbbc2ae504b Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Wed, 3 Oct 2018 11:55:56 +0100 Subject: [PATCH 4/4] Changes based on further comments Signed-off-by: Gary Brown --- source/extensions/tracers/dynamic_ot/config.h | 2 +- source/extensions/tracers/lightstep/config.cc | 8 ++++---- source/extensions/tracers/lightstep/config.h | 2 +- source/extensions/tracers/zipkin/config.cc | 4 ++-- source/extensions/tracers/zipkin/config.h | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/source/extensions/tracers/dynamic_ot/config.h b/source/extensions/tracers/dynamic_ot/config.h index 67501acf068f3..7614799883239 100644 --- a/source/extensions/tracers/dynamic_ot/config.h +++ b/source/extensions/tracers/dynamic_ot/config.h @@ -21,7 +21,7 @@ class DynamicOpenTracingTracerFactory : public Server::Configuration::TracerFact Server::Instance& server) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{std::make_unique()}; + return std::make_unique(); } std::string name() override; diff --git a/source/extensions/tracers/lightstep/config.cc b/source/extensions/tracers/lightstep/config.cc index e7ef9af908cb9..8fd61fac90e99 100644 --- a/source/extensions/tracers/lightstep/config.cc +++ b/source/extensions/tracers/lightstep/config.cc @@ -33,10 +33,10 @@ LightstepTracerFactory::createHttpTracer(const envoy::config::trace::v2::Tracing opts->access_token.assign(access_token_sv.data(), access_token_sv.size()); opts->component_name = server.localInfo().clusterName(); - Tracing::DriverPtr lightstep_driver{ - new LightStepDriver{lightstep_config, server.clusterManager(), server.stats(), - server.threadLocal(), server.runtime(), std::move(opts), - Common::Ot::OpenTracingDriver::PropagationMode::TracerNative}}; + Tracing::DriverPtr lightstep_driver{std::make_unique( + lightstep_config, server.clusterManager(), server.stats(), server.threadLocal(), + server.runtime(), std::move(opts), + Common::Ot::OpenTracingDriver::PropagationMode::TracerNative)}; return std::make_unique(std::move(lightstep_driver), server.localInfo()); } diff --git a/source/extensions/tracers/lightstep/config.h b/source/extensions/tracers/lightstep/config.h index ecbba0c3f83a2..94b88674ac3d5 100644 --- a/source/extensions/tracers/lightstep/config.h +++ b/source/extensions/tracers/lightstep/config.h @@ -21,7 +21,7 @@ class LightstepTracerFactory : public Server::Configuration::TracerFactory { Server::Instance& server) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{std::make_unique()}; + return std::make_unique(); } std::string name() override; diff --git a/source/extensions/tracers/zipkin/config.cc b/source/extensions/tracers/zipkin/config.cc index 52eccaceee159..ba7d6e3e4b4f9 100644 --- a/source/extensions/tracers/zipkin/config.cc +++ b/source/extensions/tracers/zipkin/config.cc @@ -27,9 +27,9 @@ ZipkinTracerFactory::createHttpTracer(const envoy::config::trace::v2::Tracing& c const auto& zipkin_config = dynamic_cast(*config_ptr); - Tracing::DriverPtr zipkin_driver(new Zipkin::Driver( + Tracing::DriverPtr zipkin_driver{std::make_unique( zipkin_config, server.clusterManager(), server.stats(), server.threadLocal(), - server.runtime(), server.localInfo(), rand, server.timeSystem())); + server.runtime(), server.localInfo(), rand, server.timeSystem())}; return Tracing::HttpTracerPtr( new Tracing::HttpTracerImpl(std::move(zipkin_driver), server.localInfo())); diff --git a/source/extensions/tracers/zipkin/config.h b/source/extensions/tracers/zipkin/config.h index 267d0c712d2dd..424bbe9aeaef3 100644 --- a/source/extensions/tracers/zipkin/config.h +++ b/source/extensions/tracers/zipkin/config.h @@ -19,7 +19,7 @@ class ZipkinTracerFactory : public Server::Configuration::TracerFactory { Server::Instance& server) override; ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return ProtobufTypes::MessagePtr{std::make_unique()}; + return std::make_unique(); } std::string name() override;