From 3dfbfbc857e60e2e312fb79071d21f69baa1d799 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 21 Dec 2017 12:31:07 -0800 Subject: [PATCH 01/32] Support dynamically loading tracing libraries. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 4 +- source/common/config/well_known_names.h | 2 + source/common/json/config_schemas.cc | 21 ++++++++++ source/common/tracing/BUILD | 14 +++++++ .../dynamic_opentracing_driver_impl.cc | 38 +++++++++++++++++++ .../tracing/dynamic_opentracing_driver_impl.h | 37 ++++++++++++++++++ source/exe/BUILD | 1 + source/server/config/http/BUILD | 12 ++++++ .../http/dynamic_opentracing_http_tracer.cc | 38 +++++++++++++++++++ .../http/dynamic_opentracing_http_tracer.h | 29 ++++++++++++++ source/server/configuration_impl.cc | 10 ++--- source/server/configuration_impl.h | 5 +++ 12 files changed, 204 insertions(+), 7 deletions(-) create mode 100644 source/common/tracing/dynamic_opentracing_driver_impl.cc create mode 100644 source/common/tracing/dynamic_opentracing_driver_impl.h create mode 100644 source/server/config/http/dynamic_opentracing_http_tracer.cc create mode 100644 source/server/config/http/dynamic_opentracing_http_tracer.h diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index be74628c1e0e..07d1f8721bd2 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -30,8 +30,8 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/gcovr/gcovr", ), io_opentracing_cpp = dict( - commit = "e57161e2a4bd1f9d3a8d3edf23185f033bb45f17", - remote = "https://github.com/opentracing/opentracing-cpp", # v1.2.0 + commit = "c41edee9322b6563022de843162d5c24c1824b77", + remote = "https://github.com/rnburn/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( # This picks up a commit after v0.6.0 (d4501f84de2d149da2a7a56c545a1c40f214db3f) that fixes diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index 578fbabc5e02..98766f90bab7 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -137,6 +137,8 @@ class HttpTracerNameValues { const std::string LIGHTSTEP = "envoy.lightstep"; // Zipkin tracer const std::string ZIPKIN = "envoy.zipkin"; + // Dynamic tracer + const std::string DYNAMIC = "envoy.dynamic"; }; typedef ConstSingleton HttpTracerNames; diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 80ffe54593a8..194255a24e6b 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -1221,6 +1221,26 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF( "required" : ["type", "config"], "additionalProperties" : false }, + "dynamic_driver" : { + "type" : "object", + "properties" : { + "type" : { + "type" : "string", + "enum" : ["dynamic"] + }, + "config" : { + "type" : "object", + "properties" : { + "library" : {"type" : "string"}, + "config_file" : {"type" : "string"} + }, + "required": ["library", "config_file"], + "additionalProperties" : false + } + }, + "required" : ["type", "config"], + "additionalProperties" : false + }, "zipkin_driver" : { "type" : "object", "properties" : { @@ -1297,6 +1317,7 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF( "type" : "object", "oneOf" : [ {"$ref" : "#/definitions/lightstep_driver"}, + {"$ref" : "#/definitions/dynamic_driver"}, {"$ref" : "#/definitions/zipkin_driver"} ] } diff --git a/source/common/tracing/BUILD b/source/common/tracing/BUILD index 823a42552c01..59b02981253d 100644 --- a/source/common/tracing/BUILD +++ b/source/common/tracing/BUILD @@ -53,6 +53,20 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "dynamic_opentracing_tracer_lib", + srcs = [ + "dynamic_opentracing_driver_impl.cc", + ], + hdrs = [ + "dynamic_opentracing_driver_impl.h", + ], + deps = [ + ":http_tracer_lib", + ":opentracing_driver_lib", + ], +) + envoy_cc_library( name = "lightstep_tracer_lib", srcs = [ diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.cc b/source/common/tracing/dynamic_opentracing_driver_impl.cc new file mode 100644 index 000000000000..02f71e22fbf7 --- /dev/null +++ b/source/common/tracing/dynamic_opentracing_driver_impl.cc @@ -0,0 +1,38 @@ +#include "common/tracing/dynamic_opentracing_driver_impl.h" + +#include "common/common/assert.h" + +namespace Envoy { +namespace Tracing { + +DynamicOpenTracingDriver::DynamicOpenTracingDriver(Stats::Store& stats, const std::string& library, + const std::string& tracer_config) + : OpenTracingDriver{stats} { + std::string error_message; + opentracing::expected library_handle_maybe = + opentracing::dynamically_load_tracing_library(library.c_str(), error_message); + if (!library_handle_maybe) { + if (error_message.empty()) { + throw EnvoyException{fmt::format("{}", library_handle_maybe.error().message())}; + } else { + throw EnvoyException{ + fmt::format("{}: {}", library_handle_maybe.error().message(), error_message)}; + } + } + library_handle_ = std::move(*library_handle_maybe); + + opentracing::expected> tracer_maybe = + library_handle_.tracer_factory().MakeTracer(tracer_config.c_str(), error_message); + if (!tracer_maybe) { + if (error_message.empty()) { + throw EnvoyException{fmt::format("{}", tracer_maybe.error().message())}; + } else { + throw EnvoyException{fmt::format("{}: {}", tracer_maybe.error().message(), error_message)}; + } + } + tracer_ = std::move(*tracer_maybe); + RELEASE_ASSERT(tracer_ != nullptr); +} + +} // namespace Tracing +} // namespace Envoy diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.h b/source/common/tracing/dynamic_opentracing_driver_impl.h new file mode 100644 index 000000000000..ce313bd0eb06 --- /dev/null +++ b/source/common/tracing/dynamic_opentracing_driver_impl.h @@ -0,0 +1,37 @@ +#pragma once + +#include "envoy/runtime/runtime.h" +#include "envoy/thread_local/thread_local.h" +#include "envoy/tracing/http_tracer.h" +#include "envoy/upstream/cluster_manager.h" + +#include "common/tracing/opentracing_driver_impl.h" + +#include "opentracing/dynamic_load.h" + +namespace Envoy { +namespace Tracing { + +/** + * This driver provides support for dynamically loading tracing libraries into Envoy that provide an + * implementation of the OpenTracing API (see https://github.com/opentracing/opentracing-cpp). + */ +class DynamicOpenTracingDriver : public OpenTracingDriver { +public: + DynamicOpenTracingDriver(Stats::Store& stats, const std::string& library, + const std::string& tracer_config); + + // Tracer::OpenTracingDriver + const opentracing::Tracer& tracer() const override { return *tracer_; } + + PropagationMode propagationMode() const override { + return OpenTracingDriver::PropagationMode::TracerNative; + } + +private: + opentracing::DynamicTracingLibraryHandle library_handle_; + std::shared_ptr tracer_; +}; + +} // namespace Tracing +} // namespace Envoy diff --git a/source/exe/BUILD b/source/exe/BUILD index 47d827527d90..8be80b8c885d 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -74,6 +74,7 @@ envoy_cc_library( "//source/server:hot_restart_lib", "//source/server:options_lib", "//source/server/config/http:lightstep_lib", + "//source/server/config/http:dynamic_opentracing_lib", "//source/server/config/http:zipkin_lib", ] + select({ "//bazel:disable_signal_trace": [], diff --git a/source/server/config/http/BUILD b/source/server/config/http/BUILD index 842bba28b0e2..7edc85398c19 100644 --- a/source/server/config/http/BUILD +++ b/source/server/config/http/BUILD @@ -148,6 +148,18 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "dynamic_opentracing_lib", + srcs = ["dynamic_opentracing_http_tracer.cc"], + hdrs = ["dynamic_opentracing_http_tracer.h"], + deps = [ + "//source/common/config:well_known_names", + "//source/common/tracing:dynamic_opentracing_tracer_lib", + "//source/common/tracing:http_tracer_lib", + "//source/server:configuration_lib", + ], +) + envoy_cc_library( name = "ratelimit_lib", srcs = ["ratelimit.cc"], diff --git a/source/server/config/http/dynamic_opentracing_http_tracer.cc b/source/server/config/http/dynamic_opentracing_http_tracer.cc new file mode 100644 index 000000000000..187029bd0fa4 --- /dev/null +++ b/source/server/config/http/dynamic_opentracing_http_tracer.cc @@ -0,0 +1,38 @@ +#include "server/config/http/dynamic_opentracing_http_tracer.h" + +#include + +#include "envoy/registry/registry.h" + +#include "common/common/utility.h" +#include "common/config/well_known_names.h" +#include "common/tracing/dynamic_opentracing_driver_impl.h" +#include "common/tracing/http_tracer_impl.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +Tracing::HttpTracerPtr DynamicOpenTracingHttpTracerFactory::createHttpTracer( + const Json::Object& json_config, Server::Instance& server, + Upstream::ClusterManager& /*cluster_manager*/) { + std::string library = json_config.getString("library"); + std::string config = server.api().fileReadToEnd(json_config.getString("config_file")); + Tracing::DriverPtr dynamic_driver{ + new Tracing::DynamicOpenTracingDriver{server.stats(), library, config}}; + return Tracing::HttpTracerPtr( + new Tracing::HttpTracerImpl(std::move(dynamic_driver), server.localInfo())); +} + +std::string DynamicOpenTracingHttpTracerFactory::name() { + return Config::HttpTracerNames::get().DYNAMIC; +} + +/** + * Static registration for the dynamic opentracing http tracer. @see RegisterFactory. + */ +static Registry::RegisterFactory register_; + +} // namespace Configuration +} // namespace Server +} // namespace Envoy diff --git a/source/server/config/http/dynamic_opentracing_http_tracer.h b/source/server/config/http/dynamic_opentracing_http_tracer.h new file mode 100644 index 000000000000..79d304b454fc --- /dev/null +++ b/source/server/config/http/dynamic_opentracing_http_tracer.h @@ -0,0 +1,29 @@ +#pragma once + +#include + +#include "envoy/server/instance.h" + +#include "server/configuration_impl.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +/** + * Config registration for the dynamic opentracing tracer. @see HttpTracerFactory. + */ +class DynamicOpenTracingHttpTracerFactory : public HttpTracerFactory { +public: + // HttpTracerFactory + Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, Server::Instance& server, + Upstream::ClusterManager& cluster_manager) override; + + std::string name() override; + + bool requiresClusterName() override { return false; } +}; + +} // namespace Configuration +} // namespace Server +} // namespace Envoy diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 9f982cb04e87..7fb35bf3d96c 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -88,11 +88,6 @@ void MainImpl::initializeTracers(const envoy::api::v2::Tracing& configuration, I return; } - if (server.localInfo().clusterName().empty()) { - throw EnvoyException("cluster name must be defined if tracing is enabled. See " - "--service-cluster option."); - } - // Initialize tracing driver. std::string type = configuration.http().name(); ENVOY_LOG(info, " loading tracing driver: {}", type); @@ -103,6 +98,11 @@ void MainImpl::initializeTracers(const envoy::api::v2::Tracing& configuration, I // Now see if there is a factory that will accept the config. auto& factory = Config::Utility::getAndCheckFactory(type); + if (factory.requiresClusterName() && server.localInfo().clusterName().empty()) { + throw EnvoyException(fmt::format("cluster name must be defined for the tracing driver {}. See " + "--service-cluster option.", + type)); + } http_tracer_ = factory.createHttpTracer(*driver_config, server, *cluster_manager_); } diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 3dafe282a6b3..43108730eb90 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -55,6 +55,11 @@ class HttpTracerFactory { * factory. */ virtual std::string name() PURE; + + /** + * Returns true if the tracing driver requires cluster name to be defined. + */ + virtual bool requiresClusterName() { return true; } }; /** From 4aa5578c5346a10592c25c011737a1d16085b4b6 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 3 Jan 2018 09:44:41 +0000 Subject: [PATCH 02/32] Add v2 configuration support for dynamically loaded tracers. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 07d1f8721bd2..4ba6877f5b6c 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -70,8 +70,8 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"], ), envoy_api = dict( - commit = "3deffbd132b40fa544137902fdf83bec7442a87f", - remote = "https://github.com/envoyproxy/data-plane-api", + commit = "379ef19568b81c3fafa15421e9d4a9bbbb75e560", + remote = "https://github.com/rnburn/data-plane-api", ), grpc_httpjson_transcoding = dict( commit = "e4f58aa07b9002befa493a0a82e10f2e98b51fc6", From 3473a7de91f4ef8fe72e0fda27224effe788ba57 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 9 Jan 2018 10:48:45 +0000 Subject: [PATCH 03/32] Don't change v1 configuration. Signed-off-by: Ryan Burn --- source/common/json/config_schemas.cc | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 194255a24e6b..80ffe54593a8 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -1221,26 +1221,6 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF( "required" : ["type", "config"], "additionalProperties" : false }, - "dynamic_driver" : { - "type" : "object", - "properties" : { - "type" : { - "type" : "string", - "enum" : ["dynamic"] - }, - "config" : { - "type" : "object", - "properties" : { - "library" : {"type" : "string"}, - "config_file" : {"type" : "string"} - }, - "required": ["library", "config_file"], - "additionalProperties" : false - } - }, - "required" : ["type", "config"], - "additionalProperties" : false - }, "zipkin_driver" : { "type" : "object", "properties" : { @@ -1317,7 +1297,6 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF( "type" : "object", "oneOf" : [ {"$ref" : "#/definitions/lightstep_driver"}, - {"$ref" : "#/definitions/dynamic_driver"}, {"$ref" : "#/definitions/zipkin_driver"} ] } From 581004258696944bb3730d6f9852b5cdef744b6a Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 25 Jan 2018 14:34:40 +0000 Subject: [PATCH 04/32] s/DYNAMIC/DYNAMIC_OT/ Signed-off-by: Ryan Burn --- source/common/config/well_known_names.h | 2 +- source/server/config/http/dynamic_opentracing_http_tracer.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index ff0951842c6c..b36ad0a88b85 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -151,7 +151,7 @@ class HttpTracerNameValues { // Zipkin tracer const std::string ZIPKIN = "envoy.zipkin"; // Dynamic tracer - const std::string DYNAMIC = "envoy.dynamic"; + const std::string DYNAMIC_OT = "envoy.dynamic.ot"; }; typedef ConstSingleton HttpTracerNames; diff --git a/source/server/config/http/dynamic_opentracing_http_tracer.cc b/source/server/config/http/dynamic_opentracing_http_tracer.cc index 187029bd0fa4..7dc228647c18 100644 --- a/source/server/config/http/dynamic_opentracing_http_tracer.cc +++ b/source/server/config/http/dynamic_opentracing_http_tracer.cc @@ -25,7 +25,7 @@ Tracing::HttpTracerPtr DynamicOpenTracingHttpTracerFactory::createHttpTracer( } std::string DynamicOpenTracingHttpTracerFactory::name() { - return Config::HttpTracerNames::get().DYNAMIC; + return Config::HttpTracerNames::get().DYNAMIC_OT; } /** From a8a1dad28a4d42a63c0481388f971270fe9f5bee Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 25 Jan 2018 14:43:56 +0000 Subject: [PATCH 05/32] Update opentracing-cpp dependency. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- source/common/tracing/dynamic_opentracing_driver_impl.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 453f78ade707..b9b00a9cd796 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -34,7 +34,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "c41edee9322b6563022de843162d5c24c1824b77", + commit = "5daa522328999d88ec7a055fdfed4dd8f4416d4c", remote = "https://github.com/rnburn/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.cc b/source/common/tracing/dynamic_opentracing_driver_impl.cc index 02f71e22fbf7..d88b2c572805 100644 --- a/source/common/tracing/dynamic_opentracing_driver_impl.cc +++ b/source/common/tracing/dynamic_opentracing_driver_impl.cc @@ -10,7 +10,7 @@ DynamicOpenTracingDriver::DynamicOpenTracingDriver(Stats::Store& stats, const st : OpenTracingDriver{stats} { std::string error_message; opentracing::expected library_handle_maybe = - opentracing::dynamically_load_tracing_library(library.c_str(), error_message); + opentracing::DynamicallyLoadTracingLibrary(library.c_str(), error_message); if (!library_handle_maybe) { if (error_message.empty()) { throw EnvoyException{fmt::format("{}", library_handle_maybe.error().message())}; From d0d9a124b26974da9ec9fa38c5e7f90d192af76c Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 25 Jan 2018 15:10:00 +0000 Subject: [PATCH 06/32] Switch to use inline tracer configuration. Signed-off-by: Ryan Burn --- source/server/config/http/dynamic_opentracing_http_tracer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/config/http/dynamic_opentracing_http_tracer.cc b/source/server/config/http/dynamic_opentracing_http_tracer.cc index 7dc228647c18..a3f48ccd7d43 100644 --- a/source/server/config/http/dynamic_opentracing_http_tracer.cc +++ b/source/server/config/http/dynamic_opentracing_http_tracer.cc @@ -17,7 +17,7 @@ Tracing::HttpTracerPtr DynamicOpenTracingHttpTracerFactory::createHttpTracer( const Json::Object& json_config, Server::Instance& server, Upstream::ClusterManager& /*cluster_manager*/) { std::string library = json_config.getString("library"); - std::string config = server.api().fileReadToEnd(json_config.getString("config_file")); + std::string config = json_config.getObject("config")->asJsonString(); Tracing::DriverPtr dynamic_driver{ new Tracing::DynamicOpenTracingDriver{server.stats(), library, config}}; return Tracing::HttpTracerPtr( From 4b538a358041ea801f4af1fe93d7e936cd6804a1 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 31 Jan 2018 14:40:35 +0100 Subject: [PATCH 07/32] Add dynamic tracer tests. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- source/common/tracing/BUILD | 2 +- source/server/config/http/BUILD | 2 +- test/common/tracing/BUILD | 11 ++++ .../dynamic_opentracing_driver_impl_test.cc | 35 +++++++++++ test/server/config/http/BUILD | 14 +++++ .../http/dynamic_opentracing_config_test.cc | 63 +++++++++++++++++++ 7 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 test/common/tracing/dynamic_opentracing_driver_impl_test.cc create mode 100644 test/server/config/http/dynamic_opentracing_config_test.cc diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index b9b00a9cd796..f6731b6cd02b 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -34,7 +34,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "5daa522328999d88ec7a055fdfed4dd8f4416d4c", + commit = "9c5c8c66461520511d5672556cac874a80848342", remote = "https://github.com/rnburn/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( diff --git a/source/common/tracing/BUILD b/source/common/tracing/BUILD index 59b02981253d..8d7c4431322e 100644 --- a/source/common/tracing/BUILD +++ b/source/common/tracing/BUILD @@ -54,7 +54,7 @@ envoy_cc_library( ) envoy_cc_library( - name = "dynamic_opentracing_tracer_lib", + name = "dynamic_opentracing_driver_lib", srcs = [ "dynamic_opentracing_driver_impl.cc", ], diff --git a/source/server/config/http/BUILD b/source/server/config/http/BUILD index 7edc85398c19..98d7602cc7d5 100644 --- a/source/server/config/http/BUILD +++ b/source/server/config/http/BUILD @@ -154,7 +154,7 @@ envoy_cc_library( hdrs = ["dynamic_opentracing_http_tracer.h"], deps = [ "//source/common/config:well_known_names", - "//source/common/tracing:dynamic_opentracing_tracer_lib", + "//source/common/tracing:dynamic_opentracing_driver_lib", "//source/common/tracing:http_tracer_lib", "//source/server:configuration_lib", ], diff --git a/test/common/tracing/BUILD b/test/common/tracing/BUILD index ec7031afa005..5433b38b75d1 100644 --- a/test/common/tracing/BUILD +++ b/test/common/tracing/BUILD @@ -55,3 +55,14 @@ envoy_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_cc_test( + name = "dynamic_opentracing_driver_impl_test", + srcs = [ + "dynamic_opentracing_driver_impl_test.cc", + ], + deps = [ + "//source/common/tracing:dynamic_opentracing_driver_lib", + "//test/mocks/stats:stats_mocks", + ], +) diff --git a/test/common/tracing/dynamic_opentracing_driver_impl_test.cc b/test/common/tracing/dynamic_opentracing_driver_impl_test.cc new file mode 100644 index 000000000000..a80b8a3782d0 --- /dev/null +++ b/test/common/tracing/dynamic_opentracing_driver_impl_test.cc @@ -0,0 +1,35 @@ +#include "common/tracing/dynamic_opentracing_driver_impl.h" + +#include "test/mocks/stats/mocks.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::Test; + +namespace Envoy { +namespace Tracing { + +class DynamicOpenTracingDriverTest : public Test { +public: + void setup(const std::string& library, const std::string& tracer_config) { + driver_.reset(new DynamicOpenTracingDriver{stats_, library, tracer_config}); + } + + std::unique_ptr driver_; + Stats::IsolatedStoreImpl stats_; +}; + +TEST_F(DynamicOpenTracingDriverTest, InitializeDriver) { + { + std::string invalid_library = "abc123"; + std::string invalid_config = R"EOF( + {"fake" : "fake"} + )EOF"; + + EXPECT_THROW(setup(invalid_library, invalid_config), EnvoyException); + } +} + +} // namespace Tracing +} // namespace Envoy diff --git a/test/server/config/http/BUILD b/test/server/config/http/BUILD index a0a83afa22f8..66fac86433e1 100644 --- a/test/server/config/http/BUILD +++ b/test/server/config/http/BUILD @@ -43,3 +43,17 @@ envoy_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_cc_test( + name = "dynamic_opentracing_config_test", + srcs = ["dynamic_opentracing_config_test.cc"], + data = [ + "@io_opentracing_cpp//mocktracer", + ], + deps = [ + "//source/server/config/http:dynamic_opentracing_lib", + "//test/mocks/server:server_mocks", + "//test/test_common:environment_lib", + "//test/test_common:utility_lib", + ], +) diff --git a/test/server/config/http/dynamic_opentracing_config_test.cc b/test/server/config/http/dynamic_opentracing_config_test.cc new file mode 100644 index 000000000000..347b3bfcea06 --- /dev/null +++ b/test/server/config/http/dynamic_opentracing_config_test.cc @@ -0,0 +1,63 @@ +#include + +#include "server/config/http/dynamic_opentracing_http_tracer.h" + +#include "test/mocks/server/mocks.h" +#include "test/test_common/environment.h" + +#include "fmt/printf.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { + +using testing::NiceMock; +using testing::Return; +using testing::_; + +namespace Server { +namespace Configuration { + +/** + * With `cc_test`, if `data` points to a library, then bazel puts in in a platform-architecture + * dependent location, so this function uses `find` to discover the path. + * + * See https://stackoverflow.com/q/48461242/4447365. + */ +const char* mocktracer_library_path() { + static const std::string path = [] { + const std::string result = + TestEnvironment::runfilesDirectory() + "/libopentracing_mocktracer.so"; + TestEnvironment::exec({"find", TestEnvironment::runfilesDirectory(), + "-name *libmock* -exec ln -s {}", result, "\\;"}); + return result; + }(); + return path.c_str(); +} + +TEST(HttpTracerConfigTest, DynamicOpentracingHttpTracer) { + NiceMock cm; + EXPECT_CALL(cm, get("fake_cluster")).WillRepeatedly(Return(&cm.thread_local_cluster_)); + ON_CALL(*cm.thread_local_cluster_.cluster_.info_, features()) + .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); + + std::string valid_config = fmt::sprintf(R"EOF( + { + "library": "%s", + "config": { + "output_file" : "fake_file" + } + } + )EOF", + mocktracer_library_path()); + Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); + NiceMock server; + DynamicOpenTracingHttpTracerFactory factory; + + Tracing::HttpTracerPtr tracer = factory.createHttpTracer(*valid_json, server, cm); + EXPECT_NE(nullptr, tracer); +} + +} // namespace Configuration +} // namespace Server +} // namespace Envoy From c0b059521dce5bb5239aa010f4f93aaee95cba38 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Fri, 2 Feb 2018 13:32:32 +0100 Subject: [PATCH 08/32] Fix mocktracer dependencies. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- test/server/config/http/BUILD | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index f6731b6cd02b..264bdef984ae 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -34,7 +34,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "9c5c8c66461520511d5672556cac874a80848342", + commit = "2502ce81c00a7d91ee777aa1ecb794aad6ffb91f", remote = "https://github.com/rnburn/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( diff --git a/test/server/config/http/BUILD b/test/server/config/http/BUILD index 66fac86433e1..d64b3a7e4362 100644 --- a/test/server/config/http/BUILD +++ b/test/server/config/http/BUILD @@ -48,7 +48,7 @@ envoy_cc_test( name = "dynamic_opentracing_config_test", srcs = ["dynamic_opentracing_config_test.cc"], data = [ - "@io_opentracing_cpp//mocktracer", + "@io_opentracing_cpp//mocktracer:libmocktracer_plugin.so", ], deps = [ "//source/server/config/http:dynamic_opentracing_lib", From 538582e05ca89a869bee5603e9a0d637c91db934 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Sat, 3 Feb 2018 12:52:53 +0100 Subject: [PATCH 09/32] Suppress false positive from undefined behavior sanitizer. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 188ac804a98e..a0ab11660cd3 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -34,7 +34,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "2502ce81c00a7d91ee777aa1ecb794aad6ffb91f", + commit = "a53744b3966565465099964b5208b2660f2e89b5", remote = "https://github.com/rnburn/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( From 0a46919e84ac4e145d488bb8fe48b1f1cd889e95 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 5 Feb 2018 12:12:13 +0100 Subject: [PATCH 10/32] Remove code to find mocktracer library. Signed-off-by: Ryan Burn --- .../http/dynamic_opentracing_config_test.cc | 21 ++----------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/test/server/config/http/dynamic_opentracing_config_test.cc b/test/server/config/http/dynamic_opentracing_config_test.cc index 347b3bfcea06..217073563122 100644 --- a/test/server/config/http/dynamic_opentracing_config_test.cc +++ b/test/server/config/http/dynamic_opentracing_config_test.cc @@ -18,23 +18,6 @@ using testing::_; namespace Server { namespace Configuration { -/** - * With `cc_test`, if `data` points to a library, then bazel puts in in a platform-architecture - * dependent location, so this function uses `find` to discover the path. - * - * See https://stackoverflow.com/q/48461242/4447365. - */ -const char* mocktracer_library_path() { - static const std::string path = [] { - const std::string result = - TestEnvironment::runfilesDirectory() + "/libopentracing_mocktracer.so"; - TestEnvironment::exec({"find", TestEnvironment::runfilesDirectory(), - "-name *libmock* -exec ln -s {}", result, "\\;"}); - return result; - }(); - return path.c_str(); -} - TEST(HttpTracerConfigTest, DynamicOpentracingHttpTracer) { NiceMock cm; EXPECT_CALL(cm, get("fake_cluster")).WillRepeatedly(Return(&cm.thread_local_cluster_)); @@ -43,13 +26,13 @@ TEST(HttpTracerConfigTest, DynamicOpentracingHttpTracer) { std::string valid_config = fmt::sprintf(R"EOF( { - "library": "%s", + "library": "%s/external/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so", "config": { "output_file" : "fake_file" } } )EOF", - mocktracer_library_path()); + TestEnvironment::runfilesDirectory()); Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); NiceMock server; DynamicOpenTracingHttpTracerFactory factory; From 41555a1f0d6b3b36cdbb848308f8ec53babcde9b Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 5 Feb 2018 16:33:35 +0100 Subject: [PATCH 11/32] Add test coverage that creates spans with the mocktracer. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- .../tracing/dynamic_opentracing_driver_impl.h | 2 +- .../common/tracing/lightstep_tracer_impl.cc | 2 +- source/common/tracing/lightstep_tracer_impl.h | 2 +- .../common/tracing/opentracing_driver_impl.h | 2 +- test/common/tracing/BUILD | 7 ++++ .../dynamic_opentracing_driver_impl_test.cc | 42 +++++++++++++++++++ 7 files changed, 54 insertions(+), 5 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 63f22b57cc4f..20aafe8bb4b0 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -34,7 +34,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "a53744b3966565465099964b5208b2660f2e89b5", + commit = "dc1e79eb92e17fb78e2c39c4a62971bdd1cebb74", remote = "https://github.com/rnburn/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.h b/source/common/tracing/dynamic_opentracing_driver_impl.h index ce313bd0eb06..690a8e8cbeee 100644 --- a/source/common/tracing/dynamic_opentracing_driver_impl.h +++ b/source/common/tracing/dynamic_opentracing_driver_impl.h @@ -22,7 +22,7 @@ class DynamicOpenTracingDriver : public OpenTracingDriver { const std::string& tracer_config); // Tracer::OpenTracingDriver - const opentracing::Tracer& tracer() const override { return *tracer_; } + opentracing::Tracer& tracer() override { return *tracer_; } PropagationMode propagationMode() const override { return OpenTracingDriver::PropagationMode::TracerNative; diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 49f719e4d643..5e15e4f8a1c3 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -160,7 +160,7 @@ LightStepDriver::LightStepDriver(const Json::Object& config, }); } -const opentracing::Tracer& LightStepDriver::tracer() const { +opentracing::Tracer& LightStepDriver::tracer() { return tls_->getTyped().tracer(); } diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index 46dddbeddcdc..5a3f7dafaa61 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -90,7 +90,7 @@ class LightStepDriver : public OpenTracingDriver { TlsLightStepTracer(const std::shared_ptr& tracer, LightStepDriver& driver, Event::Dispatcher& dispatcher); - const opentracing::Tracer& tracer() const; + opentracing::Tracer& tracer() const; private: void enableTimer(); diff --git a/source/common/tracing/opentracing_driver_impl.h b/source/common/tracing/opentracing_driver_impl.h index f843eb868b7c..e0f262a09231 100644 --- a/source/common/tracing/opentracing_driver_impl.h +++ b/source/common/tracing/opentracing_driver_impl.h @@ -51,7 +51,7 @@ class OpenTracingDriver : public Driver, protected Logger::Loggable driver_; Stats::IsolatedStoreImpl stats_; + + const std::string operation_name_{"test"}; + Http::TestHeaderMapImpl request_headers_{ + {":path", "/"}, {":method", "GET"}, {"x-request-id", "foo"}}; + SystemTime start_time_; + NiceMock config_; }; TEST_F(DynamicOpenTracingDriverTest, InitializeDriver) { @@ -29,6 +52,25 @@ TEST_F(DynamicOpenTracingDriverTest, InitializeDriver) { EXPECT_THROW(setup(invalid_library, invalid_config), EnvoyException); } + + { + std::string empty_config = "{}"; + + EXPECT_THROW(setup(library_path_, empty_config), EnvoyException); + } +} + +TEST_F(DynamicOpenTracingDriverTest, FlushSpans) { + setupValidDriver(); + + SpanPtr first_span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + first_span->finishSpan(); + driver_->tracer().Close(); + + const Json::ObjectSharedPtr spans_json = + TestEnvironment::jsonLoadFromString(TestEnvironment::readFileToStringForTest(spans_file_)); + EXPECT_NE(spans_json, nullptr); + EXPECT_EQ(spans_json->asObjectArray().size(), 1); } } // namespace Tracing From af4344b8b253ceb09295e37e7f896df0b4f581b2 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 5 Feb 2018 17:09:52 +0100 Subject: [PATCH 12/32] Fix prototypes. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.cc | 2 +- source/common/tracing/lightstep_tracer_impl.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 5e15e4f8a1c3..91c567da599d 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -112,7 +112,7 @@ LightStepDriver::TlsLightStepTracer::TlsLightStepTracer( enableTimer(); } -const opentracing::Tracer& LightStepDriver::TlsLightStepTracer::tracer() const { return *tracer_; } +opentracing::Tracer& LightStepDriver::TlsLightStepTracer::tracer() { return *tracer_; } void LightStepDriver::TlsLightStepTracer::enableTimer() { const uint64_t flush_interval = diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index 5a3f7dafaa61..2b8dcfa30453 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -50,7 +50,7 @@ class LightStepDriver : public OpenTracingDriver { LightstepTracerStats& tracerStats() { return tracer_stats_; } // Tracer::OpenTracingDriver - const opentracing::Tracer& tracer() const override; + opentracing::Tracer& tracer() override; PropagationMode propagationMode() const override { return propagation_mode_; } private: @@ -90,7 +90,7 @@ class LightStepDriver : public OpenTracingDriver { TlsLightStepTracer(const std::shared_ptr& tracer, LightStepDriver& driver, Event::Dispatcher& dispatcher); - opentracing::Tracer& tracer() const; + opentracing::Tracer& tracer(); private: void enableTimer(); From caa2205e495c980b60996a6f3a239ee2457a010d Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 5 Feb 2018 17:38:18 +0100 Subject: [PATCH 13/32] Const correctness. Signed-off-by: Ryan Burn --- .../server/config/http/dynamic_opentracing_http_tracer.cc | 4 ++-- .../server/config/http/dynamic_opentracing_config_test.cc | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/server/config/http/dynamic_opentracing_http_tracer.cc b/source/server/config/http/dynamic_opentracing_http_tracer.cc index a3f48ccd7d43..5f5cab4af112 100644 --- a/source/server/config/http/dynamic_opentracing_http_tracer.cc +++ b/source/server/config/http/dynamic_opentracing_http_tracer.cc @@ -16,8 +16,8 @@ namespace Configuration { Tracing::HttpTracerPtr DynamicOpenTracingHttpTracerFactory::createHttpTracer( const Json::Object& json_config, Server::Instance& server, Upstream::ClusterManager& /*cluster_manager*/) { - std::string library = json_config.getString("library"); - std::string config = json_config.getObject("config")->asJsonString(); + const std::string library = json_config.getString("library"); + const std::string config = json_config.getObject("config")->asJsonString(); Tracing::DriverPtr dynamic_driver{ new Tracing::DynamicOpenTracingDriver{server.stats(), library, config}}; return Tracing::HttpTracerPtr( diff --git a/test/server/config/http/dynamic_opentracing_config_test.cc b/test/server/config/http/dynamic_opentracing_config_test.cc index 217073563122..062426b25073 100644 --- a/test/server/config/http/dynamic_opentracing_config_test.cc +++ b/test/server/config/http/dynamic_opentracing_config_test.cc @@ -24,7 +24,7 @@ TEST(HttpTracerConfigTest, DynamicOpentracingHttpTracer) { ON_CALL(*cm.thread_local_cluster_.cluster_.info_, features()) .WillByDefault(Return(Upstream::ClusterInfo::Features::HTTP2)); - std::string valid_config = fmt::sprintf(R"EOF( + const std::string valid_config = fmt::sprintf(R"EOF( { "library": "%s/external/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so", "config": { @@ -32,12 +32,12 @@ TEST(HttpTracerConfigTest, DynamicOpentracingHttpTracer) { } } )EOF", - TestEnvironment::runfilesDirectory()); - Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); + TestEnvironment::runfilesDirectory()); + const Json::ObjectSharedPtr valid_json = Json::Factory::loadFromString(valid_config); NiceMock server; DynamicOpenTracingHttpTracerFactory factory; - Tracing::HttpTracerPtr tracer = factory.createHttpTracer(*valid_json, server, cm); + const Tracing::HttpTracerPtr tracer = factory.createHttpTracer(*valid_json, server, cm); EXPECT_NE(nullptr, tracer); } From e3bb7f5c00335652e7fba0fb6dfd42f04470a1e6 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 7 Feb 2018 12:05:35 -0500 Subject: [PATCH 14/32] Modify mocktracer's json format. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 20aafe8bb4b0..b56671000b20 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1,6 +1,6 @@ REPOSITORY_LOCATIONS = dict( com_google_absl = dict( - commit = "787891a3882795cee0364e8a0f0dda315578d155", + commit = "152218a11036efbb36ad65ac67cc0dcd8d8dc2c0", remote = "https://github.com/abseil/abseil-cpp", ), com_github_bombela_backward = dict( From 61264a9dab29ad5e759bdef51ea3a5de3569fac5 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 7 Feb 2018 14:52:22 -0500 Subject: [PATCH 15/32] Fix commit hashes. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index b56671000b20..22b9045b12ad 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1,6 +1,6 @@ REPOSITORY_LOCATIONS = dict( com_google_absl = dict( - commit = "152218a11036efbb36ad65ac67cc0dcd8d8dc2c0", + commit = "787891a3882795cee0364e8a0f0dda315578d155", remote = "https://github.com/abseil/abseil-cpp", ), com_github_bombela_backward = dict( @@ -34,7 +34,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "dc1e79eb92e17fb78e2c39c4a62971bdd1cebb74", + commit = "152218a11036efbb36ad65ac67cc0dcd8d8dc2c0", remote = "https://github.com/rnburn/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( From 3ede32eef55b6e9c2f0846a4df6e0a686a2fcc6b Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 13 Feb 2018 13:57:31 -0500 Subject: [PATCH 16/32] Use std::make_unique. Signed-off-by: Ryan Burn --- source/server/config/http/dynamic_opentracing_http_tracer.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/source/server/config/http/dynamic_opentracing_http_tracer.cc b/source/server/config/http/dynamic_opentracing_http_tracer.cc index 5f5cab4af112..9f64515cd86b 100644 --- a/source/server/config/http/dynamic_opentracing_http_tracer.cc +++ b/source/server/config/http/dynamic_opentracing_http_tracer.cc @@ -19,9 +19,8 @@ Tracing::HttpTracerPtr DynamicOpenTracingHttpTracerFactory::createHttpTracer( const std::string library = json_config.getString("library"); const std::string config = json_config.getObject("config")->asJsonString(); Tracing::DriverPtr dynamic_driver{ - new Tracing::DynamicOpenTracingDriver{server.stats(), library, config}}; - return Tracing::HttpTracerPtr( - new Tracing::HttpTracerImpl(std::move(dynamic_driver), server.localInfo())); + std::make_unique(server.stats(), library, config)}; + return std::make_unique(std::move(dynamic_driver), server.localInfo()); } std::string DynamicOpenTracingHttpTracerFactory::name() { From 3474e3980b44dcfa9a976342c6ea51f8926b7fe6 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 15 Feb 2018 12:26:09 -0500 Subject: [PATCH 17/32] Update opentracing-cpp. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 22b9045b12ad..32be764d70c3 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -34,7 +34,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "152218a11036efbb36ad65ac67cc0dcd8d8dc2c0", + commit = "1368b7de0343388a0e217643481ec4d475ed22e5", remote = "https://github.com/rnburn/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( From 3f80927f6f28978225e68f621dcaade65e625b2c Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 21 Feb 2018 13:39:16 -0500 Subject: [PATCH 18/32] Make requiresClusterName const. Signed-off-by: Ryan Burn --- source/server/config/http/dynamic_opentracing_http_tracer.h | 2 +- source/server/configuration_impl.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/server/config/http/dynamic_opentracing_http_tracer.h b/source/server/config/http/dynamic_opentracing_http_tracer.h index 79d304b454fc..e3db4cb1bd07 100644 --- a/source/server/config/http/dynamic_opentracing_http_tracer.h +++ b/source/server/config/http/dynamic_opentracing_http_tracer.h @@ -21,7 +21,7 @@ class DynamicOpenTracingHttpTracerFactory : public HttpTracerFactory { std::string name() override; - bool requiresClusterName() override { return false; } + bool requiresClusterName() const override { return false; } }; } // namespace Configuration diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 934a89ca2943..0cdcb664415c 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -58,7 +58,7 @@ class HttpTracerFactory { /** * Returns true if the tracing driver requires cluster name to be defined. */ - virtual bool requiresClusterName() { return true; } + virtual bool requiresClusterName() const { return true; } }; /** From b0d18904eda8229a71adfe24d514ecd3a328e5f0 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 22 Feb 2018 12:15:40 -0500 Subject: [PATCH 19/32] Add missing test coverage. Signed-off-by: Ryan Burn --- test/server/BUILD | 5 +++++ test/server/configuration_impl_test.cc | 28 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/test/server/BUILD b/test/server/BUILD index 5d570330215a..1b38aa6b8569 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -20,16 +20,21 @@ envoy_cc_test( envoy_cc_test( name = "configuration_impl_test", srcs = ["configuration_impl_test.cc"], + data = [ + "@io_opentracing_cpp//mocktracer:libmocktracer_plugin.so", + ], deps = [ "//source/common/config:well_known_names", "//source/common/event:dispatcher_lib", "//source/common/upstream:cluster_manager_lib", "//source/server:configuration_lib", + "//source/server/config/http:dynamic_opentracing_lib", "//source/server/config/network:raw_buffer_socket_lib", "//source/server/config/stats:statsd_lib", "//test/mocks:common_lib", "//test/mocks/network:network_mocks", "//test/mocks/server:server_mocks", + "//test/test_common:environment_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index f03ef6c98e86..78be848aaa43 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -10,8 +10,10 @@ #include "test/mocks/common.h" #include "test/mocks/network/mocks.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/environment.h" #include "test/test_common/utility.h" +#include "fmt/printf.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -158,6 +160,32 @@ TEST_F(ConfigurationImplTest, ServiceClusterNotSetWhenLSTracing) { EXPECT_THROW(config.initialize(bootstrap, server_, cluster_manager_factory_), EnvoyException); } +TEST_F(ConfigurationImplTest, ServiceClusterNotSetWhenOtDynamicTracing) { + std::string yaml = fmt::sprintf(R"EOF( + admin: + access_log_path: /dev/null + address: + socket_address: { address: 1.2.3.4, port_value: 5678 } + + tracing: + http: + name: envoy.dynamic.ot + config: + library: %s/external/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so + config: { + "output_file" : "fake_file" + } + )EOF", + TestEnvironment::runfilesDirectory()); + + envoy::config::bootstrap::v2::Bootstrap bootstrap; + MessageUtil::loadFromYaml(yaml, bootstrap); + + server_.local_info_.node_.set_cluster(""); + MainImpl config; + config.initialize(bootstrap, server_, cluster_manager_factory_); +} + TEST_F(ConfigurationImplTest, NullTracerSetWhenTracingConfigurationAbsent) { std::string json = R"EOF( { From e758593e5a74b31072c2a3c2bf4f8d5100ef4bcc Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 22 Feb 2018 13:01:38 -0500 Subject: [PATCH 20/32] Add release notes. Signed-off-by: Ryan Burn --- RAW_RELEASE_NOTES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/RAW_RELEASE_NOTES.md b/RAW_RELEASE_NOTES.md index 6c1a48824742..6dd714cc7013 100644 --- a/RAW_RELEASE_NOTES.md +++ b/RAW_RELEASE_NOTES.md @@ -58,3 +58,4 @@ final version. * Added the ability to pass a URL encoded Pem encoded peer certificate in the x-forwarded-client-cert header. * Added support for abstract unix domain sockets on linux. The abstract namespace can be used by prepending '@' to a socket path. +* Added support for dynamically loading a tracer. From ebefe2fd673486d70b7f9ed1a3cb2fb487147f97 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 22 Feb 2018 14:20:22 -0500 Subject: [PATCH 21/32] Fix coverage. Signed-off-by: Ryan Burn --- .../tracing/dynamic_opentracing_driver_impl.cc | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.cc b/source/common/tracing/dynamic_opentracing_driver_impl.cc index d88b2c572805..7d82653fdc6e 100644 --- a/source/common/tracing/dynamic_opentracing_driver_impl.cc +++ b/source/common/tracing/dynamic_opentracing_driver_impl.cc @@ -12,23 +12,15 @@ DynamicOpenTracingDriver::DynamicOpenTracingDriver(Stats::Store& stats, const st opentracing::expected library_handle_maybe = opentracing::DynamicallyLoadTracingLibrary(library.c_str(), error_message); if (!library_handle_maybe) { - if (error_message.empty()) { - throw EnvoyException{fmt::format("{}", library_handle_maybe.error().message())}; - } else { - throw EnvoyException{ - fmt::format("{}: {}", library_handle_maybe.error().message(), error_message)}; - } + throw EnvoyException{ + fmt::format("{}: {}", library_handle_maybe.error().message(), error_message)}; } library_handle_ = std::move(*library_handle_maybe); opentracing::expected> tracer_maybe = library_handle_.tracer_factory().MakeTracer(tracer_config.c_str(), error_message); if (!tracer_maybe) { - if (error_message.empty()) { - throw EnvoyException{fmt::format("{}", tracer_maybe.error().message())}; - } else { - throw EnvoyException{fmt::format("{}: {}", tracer_maybe.error().message(), error_message)}; - } + throw EnvoyException{fmt::format("{}: {}", tracer_maybe.error().message(), error_message)}; } tracer_ = std::move(*tracer_maybe); RELEASE_ASSERT(tracer_ != nullptr); From d9e9a185ae1c06653cefdcb43a086e6c4dfbcb92 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 22 Feb 2018 15:42:30 -0500 Subject: [PATCH 22/32] Fix formatting of error messages. Signed-off-by: Ryan Burn --- .../tracing/dynamic_opentracing_driver_impl.cc | 14 +++++++++++--- .../tracing/dynamic_opentracing_driver_impl.h | 3 +++ .../dynamic_opentracing_driver_impl_test.cc | 7 +++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.cc b/source/common/tracing/dynamic_opentracing_driver_impl.cc index 7d82653fdc6e..132f8628cc6a 100644 --- a/source/common/tracing/dynamic_opentracing_driver_impl.cc +++ b/source/common/tracing/dynamic_opentracing_driver_impl.cc @@ -12,19 +12,27 @@ DynamicOpenTracingDriver::DynamicOpenTracingDriver(Stats::Store& stats, const st opentracing::expected library_handle_maybe = opentracing::DynamicallyLoadTracingLibrary(library.c_str(), error_message); if (!library_handle_maybe) { - throw EnvoyException{ - fmt::format("{}: {}", library_handle_maybe.error().message(), error_message)}; + throw EnvoyException{formatErrorMessage(library_handle_maybe.error(), error_message)}; } library_handle_ = std::move(*library_handle_maybe); opentracing::expected> tracer_maybe = library_handle_.tracer_factory().MakeTracer(tracer_config.c_str(), error_message); if (!tracer_maybe) { - throw EnvoyException{fmt::format("{}: {}", tracer_maybe.error().message(), error_message)}; + throw EnvoyException{formatErrorMessage(tracer_maybe.error(), error_message)}; } tracer_ = std::move(*tracer_maybe); RELEASE_ASSERT(tracer_ != nullptr); } +std::string DynamicOpenTracingDriver::formatErrorMessage(std::error_code error_code, + const std::string& error_message) { + if (error_message.empty()) { + return fmt::format("{}", error_code.message()); + } else { + return fmt::format("{}: {}", error_code.message(), error_message); + } +} + } // namespace Tracing } // namespace Envoy diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.h b/source/common/tracing/dynamic_opentracing_driver_impl.h index 690a8e8cbeee..1f2e31997404 100644 --- a/source/common/tracing/dynamic_opentracing_driver_impl.h +++ b/source/common/tracing/dynamic_opentracing_driver_impl.h @@ -21,6 +21,9 @@ class DynamicOpenTracingDriver : public OpenTracingDriver { DynamicOpenTracingDriver(Stats::Store& stats, const std::string& library, const std::string& tracer_config); + static std::string formatErrorMessage(std::error_code error_code, + const std::string& error_message); + // Tracer::OpenTracingDriver opentracing::Tracer& tracer() override { return *tracer_; } diff --git a/test/common/tracing/dynamic_opentracing_driver_impl_test.cc b/test/common/tracing/dynamic_opentracing_driver_impl_test.cc index e757141cb30c..8a2a19331126 100644 --- a/test/common/tracing/dynamic_opentracing_driver_impl_test.cc +++ b/test/common/tracing/dynamic_opentracing_driver_impl_test.cc @@ -43,6 +43,13 @@ class DynamicOpenTracingDriverTest : public Test { NiceMock config_; }; +TEST_F(DynamicOpenTracingDriverTest, formatErrorMessage) { + const std::error_code error_code = std::make_error_code(std::errc::permission_denied); + EXPECT_EQ(error_code.message(), DynamicOpenTracingDriver::formatErrorMessage(error_code, "")); + EXPECT_EQ(error_code.message() + ": abc", + DynamicOpenTracingDriver::formatErrorMessage(error_code, "abc")); +} + TEST_F(DynamicOpenTracingDriverTest, InitializeDriver) { { std::string invalid_library = "abc123"; From 899ce3febd1c7f87c753c85b0ba5228c1df2c679 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 26 Feb 2018 15:48:06 -0500 Subject: [PATCH 23/32] Point back to opentracing-cpp's main repository. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 32be764d70c3..12bb59197554 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -34,8 +34,8 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "1368b7de0343388a0e217643481ec4d475ed22e5", - remote = "https://github.com/rnburn/opentracing-cpp", + commit = "6d813faa3a214869de2ea6164a34616398414378", + remote = "https://github.com/opentracing/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( commit = "6a198acd328f976984699f7272bbec7c8b220f65", From ace09fb5c14826fed857f4652f9efac3ee98a232 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 26 Feb 2018 15:58:14 -0500 Subject: [PATCH 24/32] Fix format. Signed-off-by: Ryan Burn --- source/exe/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/exe/BUILD b/source/exe/BUILD index a4b4f2762d9c..d1663a4e16ff 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -80,8 +80,8 @@ envoy_cc_library( deps = [ ":envoy_main_common_lib", ":extra_protocol_proxies_lib", - "//source/server/config/http:lightstep_lib", "//source/server/config/http:dynamic_opentracing_lib", + "//source/server/config/http:lightstep_lib", "//source/server/config/http:zipkin_lib", ], ) From 8dd4318a9ca1b5f36a6ac2eb742b5000de9c5eeb Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 26 Feb 2018 16:10:07 -0500 Subject: [PATCH 25/32] Add TODO note for example. Signed-off-by: Ryan Burn --- source/common/tracing/dynamic_opentracing_driver_impl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/tracing/dynamic_opentracing_driver_impl.h b/source/common/tracing/dynamic_opentracing_driver_impl.h index 1f2e31997404..b6f51e5a1b14 100644 --- a/source/common/tracing/dynamic_opentracing_driver_impl.h +++ b/source/common/tracing/dynamic_opentracing_driver_impl.h @@ -15,6 +15,7 @@ namespace Tracing { /** * This driver provides support for dynamically loading tracing libraries into Envoy that provide an * implementation of the OpenTracing API (see https://github.com/opentracing/opentracing-cpp). + * TODO(rnburn): Add an example showing how to use a tracer library with this driver. */ class DynamicOpenTracingDriver : public OpenTracingDriver { public: From 21c9d670dda25c1b489397bff1bd31685cd2891b Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 26 Feb 2018 18:27:00 -0500 Subject: [PATCH 26/32] Add test coverage for setTag. Signed-off-by: Ryan Burn --- test/common/tracing/BUILD | 14 ++++ .../tracing/opentracing_driver_impl_test.cc | 66 +++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 test/common/tracing/opentracing_driver_impl_test.cc diff --git a/test/common/tracing/BUILD b/test/common/tracing/BUILD index a8204b32273b..81661d78e626 100644 --- a/test/common/tracing/BUILD +++ b/test/common/tracing/BUILD @@ -32,6 +32,20 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "opentracing_driver_impl_test", + srcs = [ + "opentracing_driver_impl_test.cc", + ], + deps = [ + "//source/common/tracing:dynamic_opentracing_driver_lib", + "//test/mocks/http:http_mocks", + "//test/mocks/stats:stats_mocks", + "//test/mocks/tracing:tracing_mocks", + "@io_opentracing_cpp//mocktracer", + ], +) + envoy_cc_test( name = "lightstep_tracer_impl_test", srcs = [ diff --git a/test/common/tracing/opentracing_driver_impl_test.cc b/test/common/tracing/opentracing_driver_impl_test.cc new file mode 100644 index 000000000000..592c235fdb1d --- /dev/null +++ b/test/common/tracing/opentracing_driver_impl_test.cc @@ -0,0 +1,66 @@ +#include "common/tracing/opentracing_driver_impl.h" + +#include "test/mocks/http/mocks.h" +#include "test/mocks/stats/mocks.h" +#include "test/mocks/tracing/mocks.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "opentracing/mocktracer/in_memory_recorder.h" +#include "opentracing/mocktracer/tracer.h" + +using testing::Test; + +namespace Envoy { +namespace Tracing { + +class TestDriver : public OpenTracingDriver { +public: + explicit TestDriver(Stats::Store& stats) : OpenTracingDriver{stats} { + opentracing::mocktracer::MockTracerOptions options; + auto recorder = new opentracing::mocktracer::InMemoryRecorder{}; + recorder_ = recorder; + options.recorder.reset(recorder); + tracer_.reset(new opentracing::mocktracer::MockTracer{std::move(options)}); + } + + opentracing::Tracer& tracer() override { return *tracer_; } + + const opentracing::mocktracer::InMemoryRecorder& recorder() const { return *recorder_; } + +private: + const opentracing::mocktracer::InMemoryRecorder* recorder_; + std::shared_ptr tracer_; +}; + +class OpenTracingDriverTest : public Test { +public: + void setupValidDriver() { driver_.reset(new TestDriver{stats_}); } + + const std::string operation_name_{"test"}; + Http::TestHeaderMapImpl request_headers_{ + {":path", "/"}, {":method", "GET"}, {"x-request-id", "foo"}}; + const Http::TestHeaderMapImpl response_headers_{{":status", "500"}}; + SystemTime start_time_; + + std::unique_ptr driver_; + Stats::IsolatedStoreImpl stats_; + + NiceMock config_; +}; + +TEST_F(OpenTracingDriverTest, FlushSpanWithTag) { + setupValidDriver(); + + SpanPtr first_span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + first_span->setTag("abc", "123"); + first_span->finishSpan(); + + const std::unordered_map expected_tags = { + {"abc", std::string{"123"}}}; + + EXPECT_EQ(1, driver_->recorder().spans().size()); + EXPECT_EQ(expected_tags, driver_->recorder().top().tags); +} +} // namespace Tracing +} // namespace Envoy From 4567d6f0bbe2b291cce6e0011592ca752b35b19f Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 26 Feb 2018 18:59:53 -0500 Subject: [PATCH 27/32] Add test coverage for LightStepLogger. Signed-off-by: Ryan Burn --- .../common/tracing/lightstep_tracer_impl.cc | 20 ------------------ source/common/tracing/lightstep_tracer_impl.h | 21 +++++++++++++++++++ .../tracing/lightstep_tracer_impl_test.cc | 9 ++++++++ 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 91c567da599d..7395a382316c 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -15,26 +15,6 @@ namespace Envoy { namespace Tracing { -namespace { -class LightStepLogger : Logger::Loggable { -public: - void operator()(lightstep::LogLevel level, opentracing::string_view message) const { - const fmt::StringRef fmt_message{message.data(), message.size()}; - switch (level) { - case lightstep::LogLevel::debug: - ENVOY_LOG(debug, "{}", fmt_message); - break; - case lightstep::LogLevel::info: - ENVOY_LOG(info, "{}", fmt_message); - break; - default: - ENVOY_LOG(warn, "{}", fmt_message); - break; - } - } -}; -} // namespace - LightStepDriver::LightStepTransporter::LightStepTransporter(LightStepDriver& driver) : driver_(driver) {} diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index 2b8dcfa30453..ab891c8fe200 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -31,6 +31,27 @@ struct LightstepTracerStats { LIGHTSTEP_TRACER_STATS(GENERATE_COUNTER_STRUCT) }; +/** + * LightStepLogger is used to translate logs generated from LightStep's tracer to Envoy logs. + */ +class LightStepLogger : Logger::Loggable { +public: + void operator()(lightstep::LogLevel level, opentracing::string_view message) const { + const fmt::StringRef fmt_message{message.data(), message.size()}; + switch (level) { + case lightstep::LogLevel::debug: + ENVOY_LOG(debug, "{}", fmt_message); + break; + case lightstep::LogLevel::info: + ENVOY_LOG(info, "{}", fmt_message); + break; + default: + ENVOY_LOG(warn, "{}", fmt_message); + break; + } + } +}; + /** * LightStep (http://lightstep.com/) provides tracing capabilities, aggregation, visualization of * application trace data. diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index e878cdbf6a83..d87e0397bb12 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -92,6 +92,15 @@ class LightStepDriverTest : public Test { NiceMock config_; }; +TEST_F(LightStepDriverTest, LightStepLogger) { + LightStepLogger logger; + + // Verify calls to logger don't crash + logger(lightstep::LogLevel::debug, "abc"); + logger(lightstep::LogLevel::info, "abc"); + logger(lightstep::LogLevel::error, "abc"); +} + TEST_F(LightStepDriverTest, InitializeDriver) { { std::string invalid_config = R"EOF( From 1b491d9699f8e8c0a392cac7796c788c54aa687b Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 26 Feb 2018 19:10:56 -0500 Subject: [PATCH 28/32] Move method to cc file. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.cc | 16 ++++++++++++++++ source/common/tracing/lightstep_tracer_impl.h | 15 +-------------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 7395a382316c..33a6ea8ad991 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -15,6 +15,22 @@ namespace Envoy { namespace Tracing { +void LightStepLogger::operator()(lightstep::LogLevel level, + opentracing::string_view message) const { + const fmt::StringRef fmt_message{message.data(), message.size()}; + switch (level) { + case lightstep::LogLevel::debug: + ENVOY_LOG(debug, "{}", fmt_message); + break; + case lightstep::LogLevel::info: + ENVOY_LOG(info, "{}", fmt_message); + break; + default: + ENVOY_LOG(warn, "{}", fmt_message); + break; + } +} + LightStepDriver::LightStepTransporter::LightStepTransporter(LightStepDriver& driver) : driver_(driver) {} diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index ab891c8fe200..ec4c5cf07d2c 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -36,20 +36,7 @@ struct LightstepTracerStats { */ class LightStepLogger : Logger::Loggable { public: - void operator()(lightstep::LogLevel level, opentracing::string_view message) const { - const fmt::StringRef fmt_message{message.data(), message.size()}; - switch (level) { - case lightstep::LogLevel::debug: - ENVOY_LOG(debug, "{}", fmt_message); - break; - case lightstep::LogLevel::info: - ENVOY_LOG(info, "{}", fmt_message); - break; - default: - ENVOY_LOG(warn, "{}", fmt_message); - break; - } - } + void operator()(lightstep::LogLevel level, opentracing::string_view message) const; }; /** From 8f7fa81138153942410fb8f6c0c2ab719befb15f Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 26 Feb 2018 19:12:27 -0500 Subject: [PATCH 29/32] Fix typo. Signed-off-by: Ryan Burn --- test/common/tracing/lightstep_tracer_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index d87e0397bb12..94bd02be5164 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -95,7 +95,7 @@ class LightStepDriverTest : public Test { TEST_F(LightStepDriverTest, LightStepLogger) { LightStepLogger logger; - // Verify calls to logger don't crash + // Verify calls to logger don't crash. logger(lightstep::LogLevel::debug, "abc"); logger(lightstep::LogLevel::info, "abc"); logger(lightstep::LogLevel::error, "abc"); From 5f2d43074471c37e21b432df4eb11de9c37e4917 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 27 Feb 2018 12:05:05 -0500 Subject: [PATCH 30/32] Fix tests for lightstep_tracer_impl. Signed-off-by: Ryan Burn --- .../common/tracing/lightstep_tracer_impl.cc | 8 +++- .../tracing/lightstep_tracer_impl_test.cc | 44 ++++++++++++++++++- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 33a6ea8ad991..6d3b4069dec0 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -64,8 +64,6 @@ void LightStepDriver::LightStepTransporter::onSuccess(Http::MessagePtr&& respons active_request_ = nullptr; Grpc::Common::validateResponse(*response); - Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), - lightstep::CollectorMethodName(), true); // http://www.grpc.io/docs/guides/wire.html // First 5 bytes contain the message header. response->body()->drain(5); @@ -73,11 +71,17 @@ void LightStepDriver::LightStepTransporter::onSuccess(Http::MessagePtr&& respons if (!active_response_->ParseFromZeroCopyStream(&stream)) { throw EnvoyException("Failed to parse LightStep collector response"); } + Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), + lightstep::CollectorMethodName(), true); active_callback_->OnSuccess(); } catch (const Grpc::Exception& ex) { Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), lightstep::CollectorMethodName(), false); active_callback_->OnFailure(std::make_error_code(std::errc::network_down)); + } catch (const EnvoyException& ex) { + Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), + lightstep::CollectorMethodName(), false); + active_callback_->OnFailure(std::make_error_code(std::errc::bad_message)); } } diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index 94bd02be5164..67c36149d19a 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -245,12 +245,54 @@ TEST_F(LightStepDriverTest, FlushOneFailure) { first_span->finishSpan(); + callback->onFailure(Http::AsyncClient::FailureReason::Reset); + + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("grpc.lightstep.collector.CollectorService.Report.failure") + .value()); + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("grpc.lightstep.collector.CollectorService.Report.total") + .value()); + EXPECT_EQ(1U, stats_.counter("tracing.lightstep.spans_sent").value()); +} + +TEST_F(LightStepDriverTest, FlushOneInvalidResponse) { + setupValidDriver(); + + Http::MockAsyncClientRequest request(&cm_.async_client_); + Http::AsyncClient::Callbacks* callback; + const Optional timeout(std::chrono::seconds(5)); + + EXPECT_CALL(cm_.async_client_, send_(_, _, timeout)) + .WillOnce( + Invoke([&](Http::MessagePtr& message, Http::AsyncClient::Callbacks& callbacks, + const Optional&) -> Http::AsyncClient::Request* { + callback = &callbacks; + + EXPECT_STREQ("/lightstep.collector.CollectorService/Report", + message->headers().Path()->value().c_str()); + EXPECT_STREQ("fake_cluster", message->headers().Host()->value().c_str()); + EXPECT_STREQ("application/grpc", message->headers().ContentType()->value().c_str()); + + return &request; + })); + + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) + .WillOnce(Return(1)); + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) + .WillOnce(Return(5000U)); + + SpanPtr first_span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + + first_span->finishSpan(); + Http::MessagePtr msg(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); msg->trailers(Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{"grpc-status", "0"}}}); + msg->body() = std::make_unique("invalidresponse"); - callback->onFailure(Http::AsyncClient::FailureReason::Reset); + callback->onSuccess(std::move(msg)); EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ .counter("grpc.lightstep.collector.CollectorService.Report.failure") From f1a158c44e892d944e623c925d1d53f2556229f6 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 27 Feb 2018 12:43:26 -0500 Subject: [PATCH 31/32] Fix build. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 53e07fa2e994..8a279ea7654a 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -39,7 +39,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "6d813faa3a214869de2ea6164a34616398414378", + commit = "3ef3c24a4b0ac37681f6c389545e9b61d823cfe0", remote = "https://github.com/opentracing/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( From 6878e86863b368e17de40d4d9c3e9cf7fcddd579 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 27 Feb 2018 16:40:53 -0500 Subject: [PATCH 32/32] Add missing test coverage. Signed-off-by: Ryan Burn --- bazel/repository_locations.bzl | 2 +- .../tracing/opentracing_driver_impl_test.cc | 55 ++++++++++++++++++- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 8a279ea7654a..7a2f16a057e0 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -39,7 +39,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/grpc/grpc.git", ), io_opentracing_cpp = dict( - commit = "3ef3c24a4b0ac37681f6c389545e9b61d823cfe0", + commit = "f3c1f42601d13504c68e2bc81c60604f0de055dd", remote = "https://github.com/opentracing/opentracing-cpp", ), com_lightstep_tracer_cpp = dict( diff --git a/test/common/tracing/opentracing_driver_impl_test.cc b/test/common/tracing/opentracing_driver_impl_test.cc index 592c235fdb1d..925b5c0a92d8 100644 --- a/test/common/tracing/opentracing_driver_impl_test.cc +++ b/test/common/tracing/opentracing_driver_impl_test.cc @@ -16,26 +16,39 @@ namespace Tracing { class TestDriver : public OpenTracingDriver { public: - explicit TestDriver(Stats::Store& stats) : OpenTracingDriver{stats} { + TestDriver(OpenTracingDriver::PropagationMode propagation_mode, + const opentracing::mocktracer::PropagationOptions& propagation_options, + Stats::Store& stats) + : OpenTracingDriver{stats}, propagation_mode_{propagation_mode} { opentracing::mocktracer::MockTracerOptions options; auto recorder = new opentracing::mocktracer::InMemoryRecorder{}; recorder_ = recorder; options.recorder.reset(recorder); + options.propagation_options = propagation_options; tracer_.reset(new opentracing::mocktracer::MockTracer{std::move(options)}); } + const opentracing::mocktracer::InMemoryRecorder& recorder() const { return *recorder_; } + + // OpenTracingDriver opentracing::Tracer& tracer() override { return *tracer_; } - const opentracing::mocktracer::InMemoryRecorder& recorder() const { return *recorder_; } + PropagationMode propagationMode() const override { return propagation_mode_; } private: + const OpenTracingDriver::PropagationMode propagation_mode_; const opentracing::mocktracer::InMemoryRecorder* recorder_; std::shared_ptr tracer_; }; class OpenTracingDriverTest : public Test { public: - void setupValidDriver() { driver_.reset(new TestDriver{stats_}); } + void + setupValidDriver(OpenTracingDriver::PropagationMode propagation_mode = + OpenTracingDriver::PropagationMode::SingleHeader, + const opentracing::mocktracer::PropagationOptions& propagation_options = {}) { + driver_.reset(new TestDriver{propagation_mode, propagation_options, stats_}); + } const std::string operation_name_{"test"}; Http::TestHeaderMapImpl request_headers_{ @@ -62,5 +75,41 @@ TEST_F(OpenTracingDriverTest, FlushSpanWithTag) { EXPECT_EQ(1, driver_->recorder().spans().size()); EXPECT_EQ(expected_tags, driver_->recorder().top().tags); } + +TEST_F(OpenTracingDriverTest, InjectFailure) { + for (OpenTracingDriver::PropagationMode propagation_mode : + {OpenTracingDriver::PropagationMode::SingleHeader, + OpenTracingDriver::PropagationMode::TracerNative}) { + opentracing::mocktracer::PropagationOptions propagation_options; + propagation_options.inject_error_code = std::make_error_code(std::errc::bad_message); + setupValidDriver(propagation_mode, propagation_options); + + SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + + const auto span_context_injection_error_count = + stats_.counter("tracing.opentracing.span_context_injection_error").value(); + EXPECT_EQ(nullptr, request_headers_.OtSpanContext()); + span->injectContext(request_headers_); + + EXPECT_EQ(span_context_injection_error_count + 1, + stats_.counter("tracing.opentracing.span_context_injection_error").value()); + } +} + +TEST_F(OpenTracingDriverTest, ExtractWithUnindexedHeader) { + opentracing::mocktracer::PropagationOptions propagation_options; + propagation_options.propagation_key = "unindexed-header"; + setupValidDriver(OpenTracingDriver::PropagationMode::TracerNative, propagation_options); + + SpanPtr first_span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + first_span->injectContext(request_headers_); + + SpanPtr second_span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + second_span->finishSpan(); + first_span->finishSpan(); + + auto spans = driver_->recorder().spans(); + EXPECT_EQ(spans.at(1).span_context.span_id, spans.at(0).references.at(0).span_id); +} } // namespace Tracing } // namespace Envoy