From 773211e9c04e7ca5034cc90ff8c24c82e552eb58 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 8 Nov 2017 13:05:43 -0800 Subject: [PATCH 01/25] Support OpenTracing API. Signed-off-by: Ryan Burn --- bazel/external/BUILD | 1 + bazel/external/lightstep.BUILD | 28 ++- bazel/external/lightstep.genrule_cmd | 25 ++- bazel/external/opentracing.BUILD | 47 ++++ bazel/external/opentracing.genrule_cmd | 18 ++ bazel/repositories.bzl | 28 ++- ci/build_container/Makefile | 3 - docs/install/requirements.rst | 3 +- include/envoy/http/header_map.h | 14 +- source/common/common/logger.h | 1 + source/common/grpc/common.cc | 9 + source/common/grpc/common.h | 5 + source/common/http/header_map_impl.cc | 17 ++ source/common/http/header_map_impl.h | 1 + source/common/tracing/BUILD | 3 + .../common/tracing/lightstep_tracer_impl.cc | 203 ++++++++---------- source/common/tracing/lightstep_tracer_impl.h | 105 +++++---- .../common/tracing/opentracing_driver_impl.cc | 168 +++++++++++++++ .../common/tracing/opentracing_driver_impl.h | 45 ++++ .../config/http/lightstep_http_tracer.cc | 9 +- test/common/http/header_map_impl_test.cc | 26 +++ .../tracing/lightstep_tracer_impl_test.cc | 64 +++++- 22 files changed, 609 insertions(+), 214 deletions(-) create mode 100644 bazel/external/opentracing.BUILD create mode 100644 bazel/external/opentracing.genrule_cmd create mode 100644 source/common/tracing/opentracing_driver_impl.cc create mode 100644 source/common/tracing/opentracing_driver_impl.h diff --git a/bazel/external/BUILD b/bazel/external/BUILD index 03a47a410703..f2008d5781bc 100644 --- a/bazel/external/BUILD +++ b/bazel/external/BUILD @@ -4,6 +4,7 @@ cc_library( name = "all_external", srcs = [":empty.cc"], deps = [ + "@com_github_opentracing_opentracing_cpp//:opentracing", "@com_github_lightstep_lightstep_tracer_cpp//:lightstep", "@com_google_googletest//:gtest", ], diff --git a/bazel/external/lightstep.BUILD b/bazel/external/lightstep.BUILD index 6269c5cef7f3..3c495f2344b5 100644 --- a/bazel/external/lightstep.BUILD +++ b/bazel/external/lightstep.BUILD @@ -2,29 +2,31 @@ load("@envoy//bazel:genrule_repository.bzl", "genrule_cc_deps", "genrule_environ load(":genrule_cmd.bzl", "genrule_cmd") _HDRS = glob([ - "src/c++11/lightstep/**/*.h", - "src/c++11/mapbox_variant/**/*.hpp", + "include/lightstep/**/*.h", ]) -_PREFIX_HDRS = [hdr.replace("src/c++11/", "_prefix/include/") for hdr in _HDRS] + [ - "_prefix/include/collector.pb.h", - "_prefix/include/lightstep_carrier.pb.h", +_PREFIX_HDRS = ["_prefix/" + hdr for hdr in _HDRS] + [ + "_prefix/include/lightstep/version.h", + "_prefix/include/lightstep/config.h", ] cc_library( name = "lightstep", - srcs = [":_prefix/lib/liblightstep_core_cxx11.a"], + srcs = [":_prefix/lib/liblightstep_tracer.a"], hdrs = [":" + hdr for hdr in _PREFIX_HDRS], includes = ["_prefix/include"], visibility = ["//visibility:public"], - deps = ["@com_google_protobuf//:protobuf"], + deps = [ + "@com_google_protobuf//:protobuf", + "@com_github_opentracing_opentracing_cpp//:opentracing", + ], ) genrule_environment( name = "lightstep_compiler_flags", ) -# This intermediate rule lets cc_library outputs be fed into a genrule. +# These intermediate rules let cc_library outputs be fed into a genrule. # Normally a cc_library creates a CcSkylarkApiProvider, but no direct # file outputs. genrule_cc_deps( @@ -32,16 +34,24 @@ genrule_cc_deps( deps = ["@com_google_protobuf//:protobuf"], ) +genrule_cc_deps( + name = "opentracing_deps", + deps = ["@com_github_opentracing_opentracing_cpp//:opentracing"], +) + + genrule( name = "install", srcs = glob(["**"]) + [ ":lightstep_compiler_flags", ":protobuf_deps", + ":opentracing_deps", + "@com_github_opentracing_opentracing_cpp//:opentracing_library", "@com_google_protobuf//:well_known_protos", "@local_config_cc//:toolchain", ], outs = _PREFIX_HDRS + [ - "_prefix/lib/liblightstep_core_cxx11.a", + "_prefix/lib/liblightstep_tracer.a", ], cmd = genrule_cmd("@envoy//bazel/external:lightstep.genrule_cmd"), tools = [ diff --git a/bazel/external/lightstep.genrule_cmd b/bazel/external/lightstep.genrule_cmd index 816d9061eafb..f3cf24db3e3d 100644 --- a/bazel/external/lightstep.genrule_cmd +++ b/bazel/external/lightstep.genrule_cmd @@ -6,18 +6,25 @@ cat "$(location :lightstep_compiler_flags)" source "$(location :lightstep_compiler_flags)" -CONFIGURE="$${PWD}/$(location :configure)" +SOURCE_DIR=`dirname "$${PWD}/$(location :CMakeLists.txt)"` PROTOC="$${PWD}/$(location @com_google_protobuf//:protoc)" PROTOBUF_SRC="$${PWD}/external/com_google_protobuf/src/" PROTOBUF_LIBDIR="$${PWD}/$(BINDIR)/external/com_google_protobuf/" +OPENTRACING_LIBRARY="$${PWD}/$(location @com_github_opentracing_opentracing_cpp//:opentracing_library)" +OPENTRACING_INCLUDE_DIR="`dirname $${OPENTRACING_LIBRARY}`/../include" mkdir -p "$(@D)/build" cd "$(@D)/build" -"$${CONFIGURE}" \ - --prefix="$${PWD}/../_prefix" \ - --disable-grpc \ - --enable-shared=no \ - protobuf_CFLAGS="-I$${PROTOBUF_SRC}" \ - protobuf_LIBS="-L$${PROTOBUF_LIBDIR} -lprotobuf -lprotobuf_lite" \ - PKG_CONFIG="true" \ - PROTOC="$${PROTOC} -I$${PROTOBUF_SRC}" +CC="" +cmake \ + -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_INSTALL_PREFIX="$${PWD}/../_prefix" \ + -DBUILD_TESTING=OFF \ + -DWITH_GRPC=OFF \ + -DPROTOBUF_LIBRARY="$${PROTOBUF_LIBDIR}/libprotobuf.a" \ + -DPROTOBUF_IMPORT_DIRS="$${PROTOBUF_SRC}" \ + -DPROTOBUF_INCLUDE_DIR="$${PROTOBUF_SRC}" \ + -DPROTOBUF_PROTOC_EXECUTABLE="$${PROTOC}" \ + -DOPENTRACING_INCLUDE_DIR="$${OPENTRACING_INCLUDE_DIR}" \ + -DOPENTRACING_LIBRARY="$${OPENTRACING_LIBRARY}" \ + "$${SOURCE_DIR}" make V=1 install diff --git a/bazel/external/opentracing.BUILD b/bazel/external/opentracing.BUILD new file mode 100644 index 000000000000..e8c49d6d32b2 --- /dev/null +++ b/bazel/external/opentracing.BUILD @@ -0,0 +1,47 @@ +load("@envoy//bazel:genrule_repository.bzl", "genrule_cc_deps", "genrule_environment") +load(":genrule_cmd.bzl", "genrule_cmd") + +_HDRS = glob([ + "include/opentracing/**/*.h", + "include/opentracing/**/*.hpp", +]) + +_THIRD_PARTY_HDRS = glob([ + "3rd_party/include/opentracing/**/*.h", + "3rd_party/include/opentracing/**/*.hpp", +]) + +_PREFIX_HDRS = ["_prefix/" + hdr for hdr in _HDRS] + [ + hdr.replace("3rd_party/", "_prefix/") for hdr in _THIRD_PARTY_HDRS] + [ + "_prefix/include/opentracing/version.h", +] + +cc_library( + name = "opentracing", + srcs = [":_prefix/lib/libopentracing.a"], + hdrs = [":" + hdr for hdr in _PREFIX_HDRS], + includes = ["_prefix/include"], + visibility = ["//visibility:public"], +) + +genrule_environment( + name = "opentracing_compiler_flags", +) + +filegroup( + name = "opentracing_library", + srcs = [":_prefix/lib/libopentracing.a"], + visibility = ["//visibility:public"], +) + +genrule( + name = "install", + srcs = glob(["**"]) + [ + ":opentracing_compiler_flags", + "@local_config_cc//:toolchain", + ], + outs = _PREFIX_HDRS + [ + "_prefix/lib/libopentracing.a", + ], + cmd = genrule_cmd("@envoy//bazel/external:opentracing.genrule_cmd"), +) diff --git a/bazel/external/opentracing.genrule_cmd b/bazel/external/opentracing.genrule_cmd new file mode 100644 index 000000000000..f5d1b8788a8c --- /dev/null +++ b/bazel/external/opentracing.genrule_cmd @@ -0,0 +1,18 @@ +# This is a Bazel genrule script. It acts like Bash, but has some extra +# substitutions for locating dependencies. +# +# https://docs.bazel.build/versions/master/be/general.html#genrule + +cat "$(location :opentracing_compiler_flags)" +source "$(location :opentracing_compiler_flags)" + +SOURCE_DIR=`dirname "$${PWD}/$(location :CMakeLists.txt)"` +mkdir -p "$(@D)/build" +cd "$(@D)/build" +CC="" +cmake \ + -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_INSTALL_PREFIX="$${PWD}/../_prefix" \ + -DBUILD_TESTING=OFF \ + "$${SOURCE_DIR}" +make V=1 install diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index c8c9e349ca7f..bdea8e837917 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -223,6 +223,8 @@ def envoy_dependencies(path = "@envoy_deps//", skip_com_google_protobuf = False, com_github_fmtlib_fmt(repository) if not ("spdlog" in skip_targets or "com_github_gabime_spdlog" in existing_rule_keys): com_github_gabime_spdlog(repository) + if not ("opentracing" in skip_targets or "com_github_opentracing_opentracing_cpp" in existing_rule_keys): + com_github_opentracing_opentracing_cpp(repository) if not ("lightstep" in skip_targets or "com_github_lightstep_lightstep_tracer_cpp" in existing_rule_keys): com_github_lightstep_lightstep_tracer_cpp(repository) if not ("googletest" in skip_targets or "com_google_googletest" in existing_rule_keys): @@ -272,17 +274,31 @@ def com_github_gabime_spdlog(repository = ""): actual="@com_github_gabime_spdlog//:spdlog", ) +def com_github_opentracing_opentracing_cpp(repository = ""): + genrule_repository( + name = "com_github_opentracing_opentracing_cpp", + urls = [ + "https://github.com/opentracing/opentracing-cpp/archive/v1.1.0.tar.gz", + ], + sha256 = "621b28eb5961d0622d5b37939b13379d75038366e3628f96cda411c8b94ac042", + strip_prefix = "opentracing-cpp-1.1.0", + genrule_cmd_file = repository + "//bazel/external:opentracing.genrule_cmd", + build_file = repository + "//bazel/external:opentracing.BUILD", + ) + native.bind( + name="opentracing", + actual="@com_github_opentracing_opentracing_cpp//:opentracing", + ) + + def com_github_lightstep_lightstep_tracer_cpp(repository = ""): genrule_repository( name = "com_github_lightstep_lightstep_tracer_cpp", urls = [ - "https://github.com/lightstep/lightstep-tracer-cpp/releases/download/v0_36/lightstep-tracer-cpp-0.36.tar.gz", - ], - sha256 = "f7477e67eca65f904c0b90a6bfec46d58cccfc998a8e75bc3259b6e93157ff84", - strip_prefix = "lightstep-tracer-cpp-0.36", - patches = [ - repository + "//bazel/external:lightstep-missing-header.patch", + "https://github.com/lightstep/lightstep-tracer-cpp/archive/v0.5.0.tar.gz", ], + sha256 = "10563addb4dee74d69809e59fd2c5377a323efa78941ab00bb4ce1402b3f1a33", + strip_prefix = "lightstep-tracer-cpp-0.5.0", genrule_cmd_file = repository + "//bazel/external:lightstep.genrule_cmd", build_file = repository + "//bazel/external:lightstep.BUILD", ) diff --git a/ci/build_container/Makefile b/ci/build_container/Makefile index df388f0a2dd8..91932a4372c8 100644 --- a/ci/build_container/Makefile +++ b/ci/build_container/Makefile @@ -52,6 +52,3 @@ $(THIRDPARTY_DEPS)/%.dep: $(RECIPES)/%.sh # Special support for targets that need protobuf, and hence take a dependency on protobuf.dep. PROTOBUF_BUILD ?= $(THIRDPARTY_BUILD)/$(if $(BUILD_DISTINCT),protobuf.dep,) - -$(THIRDPARTY_DEPS)/lightstep.dep: $(RECIPES)/lightstep.sh $(THIRDPARTY_DEPS)/protobuf.dep - @+$(call build-recipe,PROTOBUF_BUILD=$(PROTOBUF_BUILD)) diff --git a/docs/install/requirements.rst b/docs/install/requirements.rst index 3d3c0ca3105d..e79f99ce7210 100644 --- a/docs/install/requirements.rst +++ b/docs/install/requirements.rst @@ -18,7 +18,8 @@ Envoy has the following requirements: * `gperftools `_ (last tested with 2.6.1) * `http-parser `_ (last tested with 2.7.1) * `libevent `_ (last tested with 2.1.8) -* `lightstep-tracer-cpp `_ (last tested with 0.36) +* `opentracing-cpp `_ (last tested with 1.1.0) +* `lightstep-tracer-cpp `_ (last tested with 0.5.0) * `luajit `_ (last tested with 2.0.5) * `nghttp2 `_ (last tested with 1.25.0) * `protobuf `_ (last tested with 3.4.0) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 53aba737dcc7..1fa454e8e8e3 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -355,7 +355,7 @@ class HeaderMap { /** * Get a header by key. * @param key supplies the header key. - * @return the header entry if it exsits otherwise nullptr. + * @return the header entry if it exists otherwise nullptr. */ virtual const HeaderEntry* get(const LowerCaseString& key) const PURE; @@ -384,6 +384,18 @@ class HeaderMap { */ virtual void iterateReverse(ConstIterateCb cb, void* context) const PURE; + enum class Lookup { Found, NotFound, NotSupported }; + + /** + * Lookup one of the O(1) headers by key. + * @param key supplies the header key. + * @param entry is set to the header entry if it exists and if key is one of the O(1) headers; + * otherwise, nullptr. + * @return Lookup::Found if lookup was successful, Lookup::NotFound if the header entry doesn't + * exist, or Lookup::NotSupported if key is not one of the O(1) headers. + */ + virtual Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const PURE; + /** * Remove all instances of a header by key. * @param key supplies the header key to remove. diff --git a/source/common/common/logger.h b/source/common/common/logger.h index 1fc7811b0ba9..720a5b47c0c2 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -38,6 +38,7 @@ namespace Logger { FUNCTION(router) \ FUNCTION(runtime) \ FUNCTION(testing) \ + FUNCTION(tracing) \ FUNCTION(upstream) enum class Id { diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 0ed1faacc97f..d844e0663aab 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -218,6 +218,15 @@ Buffer::InstancePtr Common::serializeBody(const Protobuf::Message& message) { return body; } +bool Common::deserializeBody(const std::string& body, Protobuf::Message& message) { + // http://www.grpc.io/docs/guides/wire.html + // There must be at least 5 bytes to contain the message header. + if (body.size() < 5) { + return false; + } + return message.ParseFromString(body.substr(5)); +} + Http::MessagePtr Common::prepareHeaders(const std::string& upstream_cluster, const std::string& service_full_name, const std::string& method_name) { diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index fa934c8d8abc..58febc119714 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -112,6 +112,11 @@ class Common { */ static Buffer::InstancePtr serializeBody(const Protobuf::Message& message); + /** + * Deserialize protobuf message. + */ + static bool deserializeBody(const std::string& body, Protobuf::Message& message); + /** * Prepare headers for protobuf service. */ diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index d182d615306f..f159c4c787ff 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -399,6 +399,23 @@ void HeaderMapImpl::iterateReverse(ConstIterateCb cb, void* context) const { } } +HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key, + const HeaderEntry** entry) const { + StaticLookupEntry::EntryCb cb = ConstSingleton::get().find(key.get().c_str()); + if (cb) { + StaticLookupResponse ref_lookup_response = cb(const_cast(*this)); + *entry = *ref_lookup_response.entry_; + if (*entry) { + return Lookup::Found; + } else { + return Lookup::NotFound; + } + } else { + *entry = nullptr; + return Lookup::NotSupported; + } +} + void HeaderMapImpl::remove(const LowerCaseString& key) { StaticLookupEntry::EntryCb cb = ConstSingleton::get().find(key.get().c_str()); if (cb) { diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 07e75c946f81..a90f25097179 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -64,6 +64,7 @@ class HeaderMapImpl : public HeaderMap { const HeaderEntry* get(const LowerCaseString& key) const override; void iterate(ConstIterateCb cb, void* context) const override; void iterateReverse(ConstIterateCb cb, void* context) const override; + Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override; void remove(const LowerCaseString& key) override; size_t size() const override { return headers_.size(); } diff --git a/source/common/tracing/BUILD b/source/common/tracing/BUILD index 87f62a990778..e550cd3f4af8 100644 --- a/source/common/tracing/BUILD +++ b/source/common/tracing/BUILD @@ -12,10 +12,13 @@ envoy_cc_library( name = "http_tracer_lib", srcs = [ "http_tracer_impl.cc", + "opentracing_driver_impl.cc", ], hdrs = [ "http_tracer_impl.h", + "opentracing_driver_impl.h", ], + external_deps = ["opentracing"], deps = [ "//include/envoy/local_info:local_info_interface", "//include/envoy/runtime:runtime_interface", diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index a280126351ea..0229622f1665 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -15,102 +15,102 @@ namespace Envoy { namespace Tracing { -LightStepSpan::LightStepSpan(lightstep::Span& span, lightstep::Tracer& tracer) - : span_(span), tracer_(tracer) {} +namespace { +class LightStepLogger : Logger::Loggable { +public: + void operator()(lightstep::LogLevel level, opentracing::string_view message) 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) {} + +void LightStepDriver::LightStepTransporter::Send(const Protobuf::Message& request, + Protobuf::Message& response, + lightstep::AsyncTransporter::Callback& callback) { + active_callback_ = &callback; + active_response_ = &response; + + Http::MessagePtr message = + Grpc::Common::prepareHeaders(driver_.cluster()->name(), lightstep::CollectorServiceFullName(), + lightstep::CollectorMethodName()); + message->body() = Grpc::Common::serializeBody(request); + + uint64_t timeout = + driver_.runtime().snapshot().getInteger("tracing.lightstep.request_timeout", 5000U); + driver_.clusterManager() + .httpAsyncClientForCluster(driver_.cluster()->name()) + .send(std::move(message), *this, std::chrono::milliseconds(timeout)); +} -void LightStepSpan::finishSpan() { span_.Finish(); } +void LightStepDriver::LightStepTransporter::onSuccess(Http::MessagePtr&& response) { + try { + Grpc::Common::validateResponse(*response); -void LightStepSpan::setOperation(const std::string& operation) { - span_.SetOperationName(operation); + Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), + lightstep::CollectorMethodName(), true); + if (!Grpc::Common::deserializeBody(response->bodyAsString(), *active_response_)) { + throw EnvoyException("Failed to parse LightStep collector response"); + } + 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)); + } } -void LightStepSpan::setTag(const std::string& name, const std::string& value) { - span_.SetTag(name, value); +void LightStepDriver::LightStepTransporter::onFailure(Http::AsyncClient::FailureReason) { + Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), + lightstep::CollectorMethodName(), false); + active_callback_->OnFailure(std::make_error_code(std::errc::network_down)); } -void LightStepSpan::injectContext(Http::HeaderMap& request_headers) { - lightstep::BinaryCarrier ctx; - tracer_.Inject(context(), lightstep::CarrierFormat::LightStepBinaryCarrier, - lightstep::ProtoWriter(&ctx)); - const std::string current_span_context = ctx.SerializeAsString(); - request_headers.insertOtSpanContext().value( - Base64::encode(current_span_context.c_str(), current_span_context.length())); -} +LightStepDriver::LightStepMetricsObserver::LightStepMetricsObserver(LightStepDriver& driver) + : driver_(driver) {} -SpanPtr LightStepSpan::spawnChild(const Config&, const std::string& name, SystemTime start_time) { - lightstep::Span ls_span = tracer_.StartSpan( - name, {lightstep::ChildOf(span_.context()), lightstep::StartTimestamp(start_time)}); - return SpanPtr{new LightStepSpan(ls_span, tracer_)}; +void LightStepDriver::LightStepMetricsObserver::OnSpansSent(int num_spans) { + driver_.tracerStats().spans_sent_.add(num_spans); } -LightStepRecorder::LightStepRecorder(const lightstep::TracerImpl& tracer, LightStepDriver& driver, - Event::Dispatcher& dispatcher) - : builder_(tracer), driver_(driver) { +LightStepDriver::TlsLightStepTracer::TlsLightStepTracer( + std::shared_ptr&& tracer, LightStepDriver& driver, + Event::Dispatcher& dispatcher) + : tracer_(std::move(tracer)), driver_(driver) { flush_timer_ = dispatcher.createTimer([this]() -> void { driver_.tracerStats().timer_flushed_.inc(); - flushSpans(); + tracer_->Flush(); enableTimer(); }); enableTimer(); } -void LightStepRecorder::RecordSpan(lightstep::collector::Span&& span) { - builder_.addSpan(std::move(span)); - - uint64_t min_flush_spans = - driver_.runtime().snapshot().getInteger("tracing.lightstep.min_flush_spans", 5U); - if (builder_.pendingSpans() == min_flush_spans) { - flushSpans(); - } -} - -bool LightStepRecorder::FlushWithTimeout(lightstep::Duration) { - // Note: We don't expect this to be called, since the Tracer - // reference is private to its LightStepSink. - return true; -} - -std::unique_ptr -LightStepRecorder::NewInstance(LightStepDriver& driver, Event::Dispatcher& dispatcher, - const lightstep::TracerImpl& tracer) { - return std::unique_ptr(new LightStepRecorder(tracer, driver, dispatcher)); -} +const opentracing::Tracer& LightStepDriver::TlsLightStepTracer::tracer() const { return *tracer_; } -void LightStepRecorder::enableTimer() { +void LightStepDriver::TlsLightStepTracer::enableTimer() { uint64_t flush_interval = driver_.runtime().snapshot().getInteger("tracing.lightstep.flush_interval_ms", 1000U); flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval)); } -void LightStepRecorder::flushSpans() { - if (builder_.pendingSpans() != 0) { - driver_.tracerStats().spans_sent_.add(builder_.pendingSpans()); - lightstep::collector::ReportRequest request; - std::swap(request, builder_.pending()); - - Http::MessagePtr message = Grpc::Common::prepareHeaders(driver_.cluster()->name(), - lightstep::CollectorServiceFullName(), - lightstep::CollectorMethodName()); - - message->body() = Grpc::Common::serializeBody(std::move(request)); - - uint64_t timeout = - driver_.runtime().snapshot().getInteger("tracing.lightstep.request_timeout", 5000U); - driver_.clusterManager() - .httpAsyncClientForCluster(driver_.cluster()->name()) - .send(std::move(message), *this, std::chrono::milliseconds(timeout)); - } -} - -LightStepDriver::TlsLightStepTracer::TlsLightStepTracer(lightstep::Tracer tracer, - LightStepDriver& driver) - : tracer_(new lightstep::Tracer(tracer)), driver_(driver) {} - LightStepDriver::LightStepDriver(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, - std::unique_ptr options) + std::unique_ptr&& options) : cm_(cluster_manager), tracer_stats_{LIGHTSTEP_TRACER_STATS( POOL_COUNTER_PREFIX(stats, "tracing.lightstep."))}, tls_(tls.allocateSlot()), runtime_(runtime), options_(std::move(options)) { @@ -127,57 +127,28 @@ LightStepDriver::LightStepDriver(const Json::Object& config, } tls_->set([this](Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { - lightstep::Tracer tracer(lightstep::NewUserDefinedTransportLightStepTracer( - *options_, std::bind(&LightStepRecorder::NewInstance, std::ref(*this), std::ref(dispatcher), - std::placeholders::_1))); + lightstep::LightStepTracerOptions tls_options; + tls_options.access_token = options_->access_token; + tls_options.component_name = options_->component_name; + tls_options.use_thread = false; + tls_options.use_single_key_propagation = true; + tls_options.logger_sink = LightStepLogger{}; + tls_options.max_buffered_spans = std::function{[this] { + auto result = runtime_.snapshot().getInteger("tracing.lightstep.min_flush_spans", 5U); + return result; + }}; + tls_options.metrics_observer.reset(new LightStepMetricsObserver{*this}); + tls_options.transporter.reset(new LightStepTransporter{*this}); + std::shared_ptr tracer = + lightstep::MakeLightStepTracer(std::move(tls_options)); return ThreadLocal::ThreadLocalObjectSharedPtr{ - new TlsLightStepTracer(std::move(tracer), *this)}; + new TlsLightStepTracer(std::move(tracer), *this, dispatcher)}; }); } -SpanPtr LightStepDriver::startSpan(const Config&, Http::HeaderMap& request_headers, - const std::string& operation_name, SystemTime start_time) { - lightstep::Tracer& tracer = *tls_->getTyped().tracer_; - LightStepSpanPtr active_span; - - if (request_headers.OtSpanContext()) { - // Extract downstream context from HTTP carrier. - // This code is safe even if decode returns empty string or data is malformed. - std::string parent_context = Base64::decode(request_headers.OtSpanContext()->value().c_str()); - lightstep::BinaryCarrier ctx; - ctx.ParseFromString(parent_context); - - lightstep::SpanContext parent_span_ctx = tracer.Extract( - lightstep::CarrierFormat::LightStepBinaryCarrier, lightstep::ProtoReader(ctx)); - lightstep::Span ls_span = - tracer.StartSpan(operation_name, {lightstep::ChildOf(parent_span_ctx), - lightstep::StartTimestamp(start_time)}); - active_span.reset(new LightStepSpan(ls_span, tracer)); - } else { - lightstep::Span ls_span = - tracer.StartSpan(operation_name, {lightstep::StartTimestamp(start_time)}); - active_span.reset(new LightStepSpan(ls_span, tracer)); - } - - return std::move(active_span); -} - -void LightStepRecorder::onFailure(Http::AsyncClient::FailureReason) { - Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), - lightstep::CollectorMethodName(), false); -} - -void LightStepRecorder::onSuccess(Http::MessagePtr&& msg) { - try { - Grpc::Common::validateResponse(*msg); - - Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), - lightstep::CollectorMethodName(), true); - } catch (const Grpc::Exception& ex) { - Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), - lightstep::CollectorMethodName(), false); - } +const opentracing::Tracer& LightStepDriver::tracer() const { + return tls_->getTyped().tracer(); } } // namespace Tracing diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index 903213fa6d0c..4d8a66acb3e1 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -12,9 +12,13 @@ #include "common/http/header_map_impl.h" #include "common/http/message_impl.h" #include "common/json/json_loader.h" +#include "common/protobuf/protobuf.h" +#include "common/tracing/opentracing_driver_impl.h" -#include "lightstep/carrier.h" #include "lightstep/tracer.h" +#include "lightstep/transporter.h" +#include "opentracing/noop.h" +#include "opentracing/tracer.h" namespace Envoy { namespace Tracing { @@ -27,41 +31,22 @@ struct LightstepTracerStats { LIGHTSTEP_TRACER_STATS(GENERATE_COUNTER_STRUCT) }; -class LightStepSpan : public Span { -public: - LightStepSpan(lightstep::Span& span, lightstep::Tracer& tracer); - - // Tracing::Span - void finishSpan() override; - void setOperation(const std::string& operation) override; - void setTag(const std::string& name, const std::string& value) override; - void injectContext(Http::HeaderMap& request_headers) override; - SpanPtr spawnChild(const Config& config, const std::string& name, SystemTime start_time) override; - - lightstep::SpanContext context() { return span_.context(); } - -private: - lightstep::Span span_; - lightstep::Tracer& tracer_; -}; - -typedef std::unique_ptr LightStepSpanPtr; - /** * LightStep (http://lightstep.com/) provides tracing capabilities, aggregation, visualization of * application trace data. * * LightStepSink is for flushing data to LightStep collectors. */ -class LightStepDriver : public Driver { +class LightStepDriver : public OpenTracingDriver { public: LightStepDriver(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, - std::unique_ptr options); + std::unique_ptr&& options); - // Tracer::TracingDriver - SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers, - const std::string& operation_name, SystemTime start_time) override; + // Tracer::OpenTracingDriver + const opentracing::Tracer& tracer() const override; + bool useTracerPropagation() const override { return true; } + bool useSingleHeaderPropagation() const override { return false; } Upstream::ClusterManager& clusterManager() { return cm_; } Upstream::ClusterInfoConstSharedPtr cluster() { return cluster_; } @@ -69,45 +54,55 @@ class LightStepDriver : public Driver { LightstepTracerStats& tracerStats() { return tracer_stats_; } private: - struct TlsLightStepTracer : ThreadLocal::ThreadLocalObject { - TlsLightStepTracer(lightstep::Tracer tracer, LightStepDriver& driver); + class LightStepTransporter : public lightstep::AsyncTransporter, Http::AsyncClient::Callbacks { + public: + explicit LightStepTransporter(LightStepDriver& driver); + + // lightstep::AsyncTransporter + void Send(const Protobuf::Message& request, Protobuf::Message& response, + lightstep::AsyncTransporter::Callback& callback) override; - std::unique_ptr tracer_; + // Http::AsyncClient::Callbacks + void onSuccess(Http::MessagePtr&& response) override; + void onFailure(Http::AsyncClient::FailureReason) override; + + private: + lightstep::AsyncTransporter::Callback* active_callback_ = nullptr; + Protobuf::Message* active_response_ = nullptr; LightStepDriver& driver_; }; - Upstream::ClusterManager& cm_; - Upstream::ClusterInfoConstSharedPtr cluster_; - LightstepTracerStats tracer_stats_; - ThreadLocal::SlotPtr tls_; - Runtime::Loader& runtime_; - std::unique_ptr options_; -}; + class LightStepMetricsObserver : public ::lightstep::MetricsObserver { + public: + explicit LightStepMetricsObserver(LightStepDriver& driver); -class LightStepRecorder : public lightstep::Recorder, Http::AsyncClient::Callbacks { -public: - LightStepRecorder(const lightstep::TracerImpl& tracer, LightStepDriver& driver, - Event::Dispatcher& dispatcher); + void OnSpansSent(int num_spans) override; - // lightstep::Recorder - void RecordSpan(lightstep::collector::Span&& span) override; - bool FlushWithTimeout(lightstep::Duration) override; + private: + LightStepDriver& driver_; + }; - // Http::AsyncClient::Callbacks - void onSuccess(Http::MessagePtr&&) override; - void onFailure(Http::AsyncClient::FailureReason) override; + class TlsLightStepTracer : public ThreadLocal::ThreadLocalObject { + public: + TlsLightStepTracer(std::shared_ptr&& tracer, + LightStepDriver& driver, Event::Dispatcher& dispatcher); - static std::unique_ptr NewInstance(LightStepDriver& driver, - Event::Dispatcher& dispatcher, - const lightstep::TracerImpl& tracer); + const opentracing::Tracer& tracer() const; -private: - void enableTimer(); - void flushSpans(); + private: + void enableTimer(); + + std::shared_ptr tracer_; + LightStepDriver& driver_; + Event::TimerPtr flush_timer_; + }; - lightstep::ReportBuilder builder_; - LightStepDriver& driver_; - Event::TimerPtr flush_timer_; + Upstream::ClusterManager& cm_; + Upstream::ClusterInfoConstSharedPtr cluster_; + LightstepTracerStats tracer_stats_; + ThreadLocal::SlotPtr tls_; + Runtime::Loader& runtime_; + std::unique_ptr options_; }; } // Tracing diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc new file mode 100644 index 000000000000..46339758cb00 --- /dev/null +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -0,0 +1,168 @@ +#include "common/tracing/opentracing_driver_impl.h" + +#include + +#include "common/common/assert.h" +#include "common/common/base64.h" + +namespace Envoy { +namespace Tracing { + +namespace { +class OpenTracingHTTPHeadersWriter : public opentracing::HTTPHeadersWriter { +public: + explicit OpenTracingHTTPHeadersWriter(Http::HeaderMap& request_headers) + : request_headers_(request_headers) {} + + // opentracing::HTTPHeadersWriter + opentracing::expected Set(opentracing::string_view key, + opentracing::string_view value) const override { + request_headers_.addCopy(Http::LowerCaseString{key}, value); + return {}; + } + +private: + Http::HeaderMap& request_headers_; +}; + +class OpenTracingHTTPHeadersReader : public opentracing::HTTPHeadersReader { +public: + explicit OpenTracingHTTPHeadersReader(const Http::HeaderMap& request_headers) + : request_headers_(request_headers) {} + + typedef std::function(opentracing::string_view, + opentracing::string_view)> + OpenTracingCb; + + // opentracing::HTTPHeadersReader + opentracing::expected + LookupKey(opentracing::string_view key) const override { + const Http::HeaderEntry* entry; + Http::HeaderMap::Lookup lookup_result = + request_headers_.lookup(Http::LowerCaseString{key}, &entry); + switch (lookup_result) { + case Http::HeaderMap::Lookup::Found: + return opentracing::string_view{entry->value().c_str(), entry->value().size()}; + case Http::HeaderMap::Lookup::NotFound: + return opentracing::make_unexpected(opentracing::key_not_found_error); + case Http::HeaderMap::Lookup::NotSupported: + return opentracing::make_unexpected(opentracing::lookup_key_not_supported_error); + default: + NOT_REACHED; + } + } + + opentracing::expected ForeachKey(OpenTracingCb f) const override { + request_headers_.iterate(header_map_callback, static_cast(&f)); + return {}; + } + +private: + const Http::HeaderMap& request_headers_; + + static Http::HeaderMap::Iterate header_map_callback(const Http::HeaderEntry& header, + void* context) { + OpenTracingCb* callback = static_cast(context); + opentracing::string_view key{header.key().c_str(), header.key().size()}; + opentracing::string_view value{header.value().c_str(), header.value().size()}; + if ((*callback)(key, value)) { + return Http::HeaderMap::Iterate::Continue; + } else { + return Http::HeaderMap::Iterate::Break; + } + } +}; +} // namespace + +OpenTracingSpan::OpenTracingSpan(bool use_single_header_propagation, bool use_tracer_propagation, + std::unique_ptr&& span) + : use_single_header_propagation_(use_single_header_propagation), + use_tracer_propagation_(use_tracer_propagation), span_(std::move(span)) {} + +void OpenTracingSpan::finishSpan() { span_->Finish(); } + +void OpenTracingSpan::setOperation(const std::string& operation) { + span_->SetOperationName(operation); +} + +void OpenTracingSpan::setTag(const std::string& name, const std::string& value) { + span_->SetTag(name, value); +} + +void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) { + if (use_single_header_propagation_) { + // Inject the span context using Envoy's single-header format. + std::ostringstream oss; + const opentracing::expected was_successful = + span_->tracer().Inject(span_->context(), oss); + if (!was_successful) { + ENVOY_LOG(warn, "Failed to inject span context: {}", was_successful.error().message()); + return; + } + const std::string current_span_context = oss.str(); + request_headers.insertOtSpanContext().value( + Base64::encode(current_span_context.c_str(), current_span_context.length())); + } + + if (use_tracer_propagation_) { + // Inject the context using the tracer's standard HTTP header format. + const OpenTracingHTTPHeadersWriter writer{request_headers}; + const opentracing::expected was_successful = + span_->tracer().Inject(span_->context(), writer); + if (!was_successful) { + ENVOY_LOG(warn, "Failed to inject span context: {}", was_successful.error().message()); + return; + } + } +} + +SpanPtr OpenTracingSpan::spawnChild(const Config&, const std::string& name, SystemTime start_time) { + std::unique_ptr ot_span = span_->tracer().StartSpan( + name, {opentracing::ChildOf(&span_->context()), opentracing::StartTimestamp(start_time)}); + if (ot_span == nullptr) { + return nullptr; + } + return SpanPtr{new OpenTracingSpan{use_single_header_propagation_, use_tracer_propagation_, + std::move(ot_span)}}; +} + +SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_headers, + const std::string& operation_name, SystemTime start_time) { + const bool use_single_header_propagation = this->useSingleHeaderPropagation(); + const bool use_tracer_propagation = this->useTracerPropagation(); + const opentracing::Tracer& tracer = this->tracer(); + std::unique_ptr active_span; + std::unique_ptr parent_span_ctx; + if (use_single_header_propagation && request_headers.OtSpanContext()) { + std::string parent_context = Base64::decode(request_headers.OtSpanContext()->value().c_str()); + std::istringstream iss(parent_context); + opentracing::expected> parent_span_ctx_maybe = + tracer.Extract(iss); + if (parent_span_ctx_maybe) { + parent_span_ctx = std::move(*parent_span_ctx_maybe); + } else { + ENVOY_LOG(warn, "Failed to extract span context: {}", + parent_span_ctx_maybe.error().message()); + } + } else if (use_tracer_propagation) { + const OpenTracingHTTPHeadersReader reader{request_headers}; + opentracing::expected> parent_span_ctx_maybe = + tracer.Extract(reader); + if (parent_span_ctx_maybe) { + parent_span_ctx = std::move(*parent_span_ctx_maybe); + } else { + ENVOY_LOG(warn, "Failed to extract span context: {}", + parent_span_ctx_maybe.error().message()); + } + } + active_span = tracer.StartSpan(operation_name, {opentracing::ChildOf(parent_span_ctx.get()), + opentracing::StartTimestamp(start_time)}); + if (active_span == nullptr) { + return nullptr; + } + return SpanPtr{new OpenTracingSpan{use_single_header_propagation, use_tracer_propagation, + std::move(active_span)}}; +} + +} // namespace Tracing +} // namespace Envoy diff --git a/source/common/tracing/opentracing_driver_impl.h b/source/common/tracing/opentracing_driver_impl.h new file mode 100644 index 000000000000..e8cbcbbb334a --- /dev/null +++ b/source/common/tracing/opentracing_driver_impl.h @@ -0,0 +1,45 @@ +#pragma once + +#include + +#include "envoy/tracing/http_tracer.h" + +#include "common/common/logger.h" + +#include "opentracing/tracer.h" + +namespace Envoy { +namespace Tracing { + +class OpenTracingSpan : public Span, Logger::Loggable { +public: + OpenTracingSpan(bool use_single_header_propagation, bool use_tracer_propagation, + std::unique_ptr&& span); + + // Tracing::Span + void finishSpan() override; + void setOperation(const std::string& operation) override; + void setTag(const std::string& name, const std::string& value) override; + void injectContext(Http::HeaderMap& request_headers) override; + SpanPtr spawnChild(const Config& config, const std::string& name, SystemTime start_time) override; + +private: + bool use_single_header_propagation_; + bool use_tracer_propagation_; + std::unique_ptr span_; +}; + +class OpenTracingDriver : public Driver, protected Logger::Loggable { +public: + // Tracer::TracingDriver + SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers, + const std::string& operation_name, SystemTime start_time) override; + + virtual const opentracing::Tracer& tracer() const = 0; + + virtual bool useSingleHeaderPropagation() const { return true; } + virtual bool useTracerPropagation() const { return true; } +}; + +} // namespace Tracing +} // namespace Envoy diff --git a/source/server/config/http/lightstep_http_tracer.cc b/source/server/config/http/lightstep_http_tracer.cc index fd1314e01be2..a7227f55b653 100644 --- a/source/server/config/http/lightstep_http_tracer.cc +++ b/source/server/config/http/lightstep_http_tracer.cc @@ -9,7 +9,6 @@ #include "common/tracing/http_tracer_impl.h" #include "common/tracing/lightstep_tracer_impl.h" -#include "lightstep/options.h" #include "lightstep/tracer.h" namespace Envoy { @@ -21,14 +20,10 @@ LightstepHttpTracerFactory::createHttpTracer(const Json::Object& json_config, Server::Instance& server, Upstream::ClusterManager& cluster_manager) { - Envoy::Runtime::RandomGenerator& rand = server.random(); - - std::unique_ptr opts(new lightstep::TracerOptions()); + std::unique_ptr opts(new lightstep::LightStepTracerOptions()); opts->access_token = server.api().fileReadToEnd(json_config.getString("access_token_file")); StringUtil::rtrim(opts->access_token); - - opts->tracer_attributes["lightstep.component_name"] = server.localInfo().clusterName(); - opts->guid_generator = [&rand]() { return rand.random(); }; + opts->component_name = server.localInfo().clusterName(); Tracing::DriverPtr lightstep_driver( new Tracing::LightStepDriver(json_config, cluster_manager, server.stats(), diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 1dff5cc73861..1865f0efac77 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -446,5 +446,31 @@ TEST(HeaderMapImplTest, IterateReverse) { &cb); } +TEST(HeaderMapImplTest, Lookup) { + TestHeaderMapImpl headers; + headers.addCopy("hello", "world"); + headers.insertContentLength().value(5); + + // Lookup is not supported for non O(1) header. + { + const HeaderEntry* entry; + EXPECT_EQ(HeaderMap::Lookup::NotSupported, headers.lookup(LowerCaseString{"hello"}, &entry)); + EXPECT_EQ(nullptr, entry); + } + + // Lookup returns the entry of a O(1) header if it exists. + { + const HeaderEntry* entry; + EXPECT_EQ(HeaderMap::Lookup::Found, headers.lookup(Headers::get().ContentLength, &entry)); + EXPECT_STREQ("5", entry->value().c_str()); + } + + // Lookup returns HeaderMap::Lookup::NotFound if a O(1) header does not exist. + { + const HeaderEntry* entry; + EXPECT_EQ(HeaderMap::Lookup::NotFound, headers.lookup(Headers::get().Host, &entry)); + EXPECT_EQ(nullptr, entry); + } +} } // namespace Http } // namespace Envoy diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index a3b6d18d24fe..825cace70086 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -1,8 +1,10 @@ #include #include +#include #include #include "common/common/base64.h" +#include "common/grpc/common.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/http/message_impl.h" @@ -24,6 +26,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::AtLeast; using testing::Invoke; using testing::NiceMock; using testing::Return; @@ -37,9 +40,10 @@ namespace Tracing { class LightStepDriverTest : public Test { public: void setup(Json::Object& config, bool init_timer) { - std::unique_ptr opts(new lightstep::TracerOptions()); + std::unique_ptr opts( + new lightstep::LightStepTracerOptions()); opts->access_token = "sample_token"; - opts->tracer_attributes["lightstep.component_name"] = "component"; + opts->component_name = "component"; ON_CALL(cm_, httpAsyncClientForCluster("fake_cluster")) .WillByDefault(ReturnRef(cm_.async_client_)); @@ -162,7 +166,7 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { })); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5)) - .Times(2) + .Times(AtLeast(1)) .WillRepeatedly(Return(2)); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U)) .WillOnce(Return(5000U)); @@ -181,6 +185,10 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); msg->trailers(Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{"grpc-status", "0"}}}); + std::unique_ptr collector_response = + lightstep::Transporter::MakeCollectorResponse(); + EXPECT_NE(collector_response, nullptr); + msg->body() = Grpc::Common::serializeBody(*collector_response); callback->onSuccess(std::move(msg)); @@ -188,15 +196,56 @@ TEST_F(LightStepDriverTest, FlushSeveralSpans) { .counter("grpc.lightstep.collector.CollectorService.Report.success") .value()); + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("grpc.lightstep.collector.CollectorService.Report.total") + .value()); + EXPECT_EQ(2U, stats_.counter("tracing.lightstep.spans_sent").value()); +} + +TEST_F(LightStepDriverTest, FlushOneFailure) { + 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"}}}); + 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(2U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ .counter("grpc.lightstep.collector.CollectorService.Report.total") .value()); - EXPECT_EQ(2U, stats_.counter("tracing.lightstep.spans_sent").value()); + EXPECT_EQ(1U, stats_.counter("tracing.lightstep.spans_sent").value()); } TEST_F(LightStepDriverTest, FlushSpansTimer) { @@ -289,9 +338,10 @@ TEST_F(LightStepDriverTest, SerializeAndDeserializeContext) { EXPECT_FALSE(injected_ctx.empty()); // Context can be parsed fine. - lightstep::BinaryCarrier ctx; + const opentracing::Tracer& tracer = driver_->tracer(); std::string context = Base64::decode(injected_ctx); - ctx.ParseFromString(context); + std::istringstream iss{context, std::ios::binary}; + EXPECT_TRUE(tracer.Extract(iss)); // Supply parent context, request_headers has properly populated x-ot-span-context. SpanPtr span_with_parent = From 4af99effdbaa31ef706e50fbd344c550865dae6e Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 9 Nov 2017 14:57:21 -0800 Subject: [PATCH 02/25] Const correctness fixes. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.cc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 0229622f1665..33d96b148c7d 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -19,7 +19,7 @@ namespace { class LightStepLogger : Logger::Loggable { public: void operator()(lightstep::LogLevel level, opentracing::string_view message) const { - fmt::StringRef fmt_message{message.data(), message.size()}; + const fmt::StringRef fmt_message{message.data(), message.size()}; switch (level) { case lightstep::LogLevel::debug: ENVOY_LOG(debug, "{}", fmt_message); @@ -49,7 +49,7 @@ void LightStepDriver::LightStepTransporter::Send(const Protobuf::Message& reques lightstep::CollectorMethodName()); message->body() = Grpc::Common::serializeBody(request); - uint64_t timeout = + const uint64_t timeout = driver_.runtime().snapshot().getInteger("tracing.lightstep.request_timeout", 5000U); driver_.clusterManager() .httpAsyncClientForCluster(driver_.cluster()->name()) @@ -102,7 +102,7 @@ LightStepDriver::TlsLightStepTracer::TlsLightStepTracer( const opentracing::Tracer& LightStepDriver::TlsLightStepTracer::tracer() const { return *tracer_; } void LightStepDriver::TlsLightStepTracer::enableTimer() { - uint64_t flush_interval = + const uint64_t flush_interval = driver_.runtime().snapshot().getInteger("tracing.lightstep.flush_interval_ms", 1000U); flush_timer_->enableTimer(std::chrono::milliseconds(flush_interval)); } @@ -133,10 +133,8 @@ LightStepDriver::LightStepDriver(const Json::Object& config, tls_options.use_thread = false; tls_options.use_single_key_propagation = true; tls_options.logger_sink = LightStepLogger{}; - tls_options.max_buffered_spans = std::function{[this] { - auto result = runtime_.snapshot().getInteger("tracing.lightstep.min_flush_spans", 5U); - return result; - }}; + tls_options.max_buffered_spans = std::function{ + [this] { return runtime_.snapshot().getInteger("tracing.lightstep.min_flush_spans", 5U); }}; tls_options.metrics_observer.reset(new LightStepMetricsObserver{*this}); tls_options.transporter.reset(new LightStepTransporter{*this}); std::shared_ptr tracer = From 93ab508136526b80d81b5edf0dea88d89276c7bb Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 9 Nov 2017 15:02:00 -0800 Subject: [PATCH 03/25] Use PURE instead of "= 0". Signed-off-by: Ryan Burn --- source/common/tracing/opentracing_driver_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/tracing/opentracing_driver_impl.h b/source/common/tracing/opentracing_driver_impl.h index e8cbcbbb334a..5b78d9c02a2c 100644 --- a/source/common/tracing/opentracing_driver_impl.h +++ b/source/common/tracing/opentracing_driver_impl.h @@ -35,7 +35,7 @@ class OpenTracingDriver : public Driver, protected Logger::Loggable Date: Thu, 9 Nov 2017 15:22:16 -0800 Subject: [PATCH 04/25] Log Inject/Extract failures at debug level. Signed-off-by: Ryan Burn --- source/common/tracing/opentracing_driver_impl.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index 46339758cb00..b6ac91294bd4 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -96,7 +96,7 @@ void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) { const opentracing::expected was_successful = span_->tracer().Inject(span_->context(), oss); if (!was_successful) { - ENVOY_LOG(warn, "Failed to inject span context: {}", was_successful.error().message()); + ENVOY_LOG(debug, "Failed to inject span context: {}", was_successful.error().message()); return; } const std::string current_span_context = oss.str(); @@ -110,7 +110,7 @@ void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) { const opentracing::expected was_successful = span_->tracer().Inject(span_->context(), writer); if (!was_successful) { - ENVOY_LOG(warn, "Failed to inject span context: {}", was_successful.error().message()); + ENVOY_LOG(debug, "Failed to inject span context: {}", was_successful.error().message()); return; } } @@ -141,7 +141,7 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea if (parent_span_ctx_maybe) { parent_span_ctx = std::move(*parent_span_ctx_maybe); } else { - ENVOY_LOG(warn, "Failed to extract span context: {}", + ENVOY_LOG(debug, "Failed to extract span context: {}", parent_span_ctx_maybe.error().message()); } } else if (use_tracer_propagation) { @@ -151,7 +151,7 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea if (parent_span_ctx_maybe) { parent_span_ctx = std::move(*parent_span_ctx_maybe); } else { - ENVOY_LOG(warn, "Failed to extract span context: {}", + ENVOY_LOG(debug, "Failed to extract span context: {}", parent_span_ctx_maybe.error().message()); } } From 739cd543569c01e37bafbd40dcd17bbba1625ee2 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 9 Nov 2017 16:23:17 -0800 Subject: [PATCH 05/25] Avoid copying when deserializing LightStep response. Signed-off-by: Ryan Burn --- source/common/grpc/common.cc | 9 --------- source/common/grpc/common.h | 5 ----- source/common/tracing/BUILD | 1 + source/common/tracing/lightstep_tracer_impl.cc | 7 ++++++- 4 files changed, 7 insertions(+), 15 deletions(-) diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index d844e0663aab..0ed1faacc97f 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -218,15 +218,6 @@ Buffer::InstancePtr Common::serializeBody(const Protobuf::Message& message) { return body; } -bool Common::deserializeBody(const std::string& body, Protobuf::Message& message) { - // http://www.grpc.io/docs/guides/wire.html - // There must be at least 5 bytes to contain the message header. - if (body.size() < 5) { - return false; - } - return message.ParseFromString(body.substr(5)); -} - Http::MessagePtr Common::prepareHeaders(const std::string& upstream_cluster, const std::string& service_full_name, const std::string& method_name) { diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index 58febc119714..fa934c8d8abc 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -112,11 +112,6 @@ class Common { */ static Buffer::InstancePtr serializeBody(const Protobuf::Message& message); - /** - * Deserialize protobuf message. - */ - static bool deserializeBody(const std::string& body, Protobuf::Message& message); - /** * Prepare headers for protobuf service. */ diff --git a/source/common/tracing/BUILD b/source/common/tracing/BUILD index e550cd3f4af8..bf3ef043bccf 100644 --- a/source/common/tracing/BUILD +++ b/source/common/tracing/BUILD @@ -26,6 +26,7 @@ envoy_cc_library( "//include/envoy/tracing:http_tracer_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/access_log:access_log_formatter_lib", + "//source/common/buffer:zero_copy_input_stream_lib", "//source/common/common:base64_lib", "//source/common/common:macros", "//source/common/common:utility_lib", diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 33d96b148c7d..864a97955397 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -5,6 +5,7 @@ #include #include +#include "common/buffer/zero_copy_input_stream_impl.h" #include "common/common/base64.h" #include "common/grpc/common.h" #include "common/http/message_impl.h" @@ -62,7 +63,11 @@ void LightStepDriver::LightStepTransporter::onSuccess(Http::MessagePtr&& respons Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(), lightstep::CollectorMethodName(), true); - if (!Grpc::Common::deserializeBody(response->bodyAsString(), *active_response_)) { + // http://www.grpc.io/docs/guides/wire.html + // First 5 bytes contain the message header. + response->body()->drain(5); + Buffer::ZeroCopyInputStreamImpl stream{std::move(response->body())}; + if (!active_response_->ParseFromZeroCopyStream(&stream)) { throw EnvoyException("Failed to parse LightStep collector response"); } active_callback_->OnSuccess(); From 6270b3fe7706a1595527915127aeea6a487aa0c5 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 9 Nov 2017 16:28:11 -0800 Subject: [PATCH 06/25] Change propagation options to be const. Signed-off-by: Ryan Burn --- source/common/tracing/opentracing_driver_impl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/tracing/opentracing_driver_impl.h b/source/common/tracing/opentracing_driver_impl.h index 5b78d9c02a2c..e5e6d0af051d 100644 --- a/source/common/tracing/opentracing_driver_impl.h +++ b/source/common/tracing/opentracing_driver_impl.h @@ -24,8 +24,8 @@ class OpenTracingSpan : public Span, Logger::Loggable { SpanPtr spawnChild(const Config& config, const std::string& name, SystemTime start_time) override; private: - bool use_single_header_propagation_; - bool use_tracer_propagation_; + const bool use_single_header_propagation_; + const bool use_tracer_propagation_; std::unique_ptr span_; }; From ab3db8554a92fb92b1e37f6f4e6ad4b144ede601 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 13 Nov 2017 16:13:02 -0800 Subject: [PATCH 07/25] Fix lightstep build. Signed-off-by: Ryan Burn --- bazel/external/BUILD | 2 ++ bazel/repositories.bzl | 28 +++++++--------------------- bazel/repository_locations.bzl | 14 +++++++------- 3 files changed, 16 insertions(+), 28 deletions(-) diff --git a/bazel/external/BUILD b/bazel/external/BUILD index b67fd5ca4826..f686511e728b 100644 --- a/bazel/external/BUILD +++ b/bazel/external/BUILD @@ -4,6 +4,8 @@ cc_library( name = "all_external", srcs = [":empty.cc"], deps = [ + "@io_opentracing_cpp//:opentracing", + "@com_github_lightstep_lightstep_tracer_cpp//:lightstep_tracer", "@com_google_googletest//:gtest", ], ) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 91502915ee5e..3985f81ef573 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -238,7 +238,7 @@ def envoy_dependencies(path = "@envoy_deps//", skip_targets = []): _com_github_fmtlib_fmt() _com_github_gabime_spdlog() _com_github_gcovr_gcovr() - _com_github_opentracing_opentracing_cpp() + _io_opentracing_cpp() _com_github_lightstep_lightstep_tracer_cpp() _com_github_nodejs_http_parser() _com_github_tencent_rapidjson() @@ -313,36 +313,22 @@ def _com_github_gcovr_gcovr(): actual = "@com_github_gcovr_gcovr//:gcovr", ) -def _com_github_opentracing_opentracing_cpp(): - location = REPOSITORY_LOCATIONS[ - "com_github_opentracing_opentracing_cpp"] - native.git_repository( - name = "io_opentracing_cpp", - remote = location["remote"], - commit = location["commit"], - ) +def _io_opentracing_cpp(): + _repository_impl("io_opentracing_cpp") native.bind( name = "opentracing", actual = "@io_opentracing_cpp//:opentracing", ) def _com_github_lightstep_lightstep_tracer_cpp(): - location = REPOSITORY_LOCATIONS[ - "com_github_lightstep_lightstep_tracer_cpp"] - native.git_repository( - name = "com_lightstep_tracer_cpp", - remote = location["remote"], - commit = location["commit"], - ) - native.new_git_repository( + _repository_impl("com_github_lightstep_lightstep_tracer_cpp") + _repository_impl( name = "lightstep_vendored_googleapis", - remote = location["googleapis"]["remote"], - commit = location["googleapis"]["commit"], - build_file = "@com_lightstep_tracer_cpp//:lightstep-tracer-common/third_party/googleapis/BUILD", + build_file = "@com_github_lightstep_lightstep_tracer_cpp//:lightstep-tracer-common/third_party/googleapis/BUILD", ) native.bind( name = "lightstep", - actual = "@com_lightstep_tracer_cpp//:lightstep_tracer", + actual = "@com_github_lightstep_lightstep_tracer_cpp//:lightstep_tracer", ) def _com_github_tencent_rapidjson(): diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 866f54a93277..3b98b5cbe90f 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -25,17 +25,17 @@ REPOSITORY_LOCATIONS = dict( commit = "c0d77201039c7b119b18bc7fb991564c602dd75d", remote = "https://github.com/gcovr/gcovr", ), - com_github_opentracing_opentracing_cpp = dict( - commit = "713c15d40ae63185d2bec99bf3b03823967d7108", + io_opentracing_cpp = dict( + commit = "550c686e0e174c845a034f432a6c31a808f5f994", remote = "https://github.com/opentracing/opentracing-cpp", ), com_github_lightstep_lightstep_tracer_cpp = dict( - commit = "c9d9215b5c3652c7eb5640903697d9a683e1df76", + commit = "deb5284395075028c3e5b4eab1416fe1e597bdb7", remote = "https://github.com/lightstep/lightstep-tracer-cpp", - googleapis = dict( - commit = "d6f78d948c53f3b400bb46996eb3084359914f9b", - remote = "https://github.com/google/googleapis", - ), + ), + lightstep_vendored_googleapis = dict( + commit = "d6f78d948c53f3b400bb46996eb3084359914f9b", + remote = "https://github.com/google/googleapis", ), com_github_nodejs_http_parser = dict( commit = "feae95a3a69f111bc1897b9048d9acbc290992f9", # v2.7.1 From cee939f9f30f6da641eb159a528f2a5bd282c15f Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 14 Nov 2017 11:33:44 -0800 Subject: [PATCH 08/25] Replace nullptr branching with RELEASE_ASSERT. Signed-off-by: Ryan Burn --- source/common/tracing/opentracing_driver_impl.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index b6ac91294bd4..437f84a4d20a 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -119,9 +119,7 @@ void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) { SpanPtr OpenTracingSpan::spawnChild(const Config&, const std::string& name, SystemTime start_time) { std::unique_ptr ot_span = span_->tracer().StartSpan( name, {opentracing::ChildOf(&span_->context()), opentracing::StartTimestamp(start_time)}); - if (ot_span == nullptr) { - return nullptr; - } + RELEASE_ASSERT(ot_span != nullptr); return SpanPtr{new OpenTracingSpan{use_single_header_propagation_, use_tracer_propagation_, std::move(ot_span)}}; } @@ -157,9 +155,7 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea } active_span = tracer.StartSpan(operation_name, {opentracing::ChildOf(parent_span_ctx.get()), opentracing::StartTimestamp(start_time)}); - if (active_span == nullptr) { - return nullptr; - } + RELEASE_ASSERT(active_span != nullptr); return SpanPtr{new OpenTracingSpan{use_single_header_propagation, use_tracer_propagation, std::move(active_span)}}; } From c775ded82aaf795b8cf088aa12212880068e75b4 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 14 Nov 2017 11:37:14 -0800 Subject: [PATCH 09/25] s/header_map_callback/headerMapCallback/ Signed-off-by: Ryan Burn --- source/common/tracing/opentracing_driver_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index 437f84a4d20a..4c08392f81da 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -53,15 +53,15 @@ class OpenTracingHTTPHeadersReader : public opentracing::HTTPHeadersReader { } opentracing::expected ForeachKey(OpenTracingCb f) const override { - request_headers_.iterate(header_map_callback, static_cast(&f)); + request_headers_.iterate(headerMapCallback, static_cast(&f)); return {}; } private: const Http::HeaderMap& request_headers_; - static Http::HeaderMap::Iterate header_map_callback(const Http::HeaderEntry& header, - void* context) { + static Http::HeaderMap::Iterate headerMapCallback(const Http::HeaderEntry& header, + void* context) { OpenTracingCb* callback = static_cast(context); opentracing::string_view key{header.key().c_str(), header.key().size()}; opentracing::string_view value{header.value().c_str(), header.value().size()}; From f1db61fffaa68d67093f1d8d5989ea5e5a78c7b1 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 20 Nov 2017 12:40:38 -0800 Subject: [PATCH 10/25] Use stats to track span context injection/extraction failures. Signed-off-by: Ryan Burn --- .../common/tracing/lightstep_tracer_impl.cc | 3 ++- .../common/tracing/opentracing_driver_impl.cc | 22 +++++++++++-------- .../common/tracing/opentracing_driver_impl.h | 22 +++++++++++++++---- .../tracing/lightstep_tracer_impl_test.cc | 1 + 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 864a97955397..caff530d5147 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -116,7 +116,8 @@ LightStepDriver::LightStepDriver(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, std::unique_ptr&& options) - : cm_(cluster_manager), tracer_stats_{LIGHTSTEP_TRACER_STATS( + : OpenTracingDriver{stats}, + cm_(cluster_manager), tracer_stats_{LIGHTSTEP_TRACER_STATS( POOL_COUNTER_PREFIX(stats, "tracing.lightstep."))}, tls_(tls.allocateSlot()), runtime_(runtime), options_(std::move(options)) { Upstream::ThreadLocalCluster* cluster = cm_.get(config.getString("collector_cluster")); diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index 4c08392f81da..d5e134296459 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -74,10 +74,9 @@ class OpenTracingHTTPHeadersReader : public opentracing::HTTPHeadersReader { }; } // namespace -OpenTracingSpan::OpenTracingSpan(bool use_single_header_propagation, bool use_tracer_propagation, +OpenTracingSpan::OpenTracingSpan(OpenTracingDriver& driver, std::unique_ptr&& span) - : use_single_header_propagation_(use_single_header_propagation), - use_tracer_propagation_(use_tracer_propagation), span_(std::move(span)) {} + : driver_{driver}, span_(std::move(span)) {} void OpenTracingSpan::finishSpan() { span_->Finish(); } @@ -90,13 +89,14 @@ void OpenTracingSpan::setTag(const std::string& name, const std::string& value) } void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) { - if (use_single_header_propagation_) { + if (driver_.useSingleHeaderPropagation()) { // Inject the span context using Envoy's single-header format. std::ostringstream oss; const opentracing::expected was_successful = span_->tracer().Inject(span_->context(), oss); if (!was_successful) { ENVOY_LOG(debug, "Failed to inject span context: {}", was_successful.error().message()); + driver_.tracerStats().span_context_injection_error_.inc(); return; } const std::string current_span_context = oss.str(); @@ -104,13 +104,14 @@ void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) { Base64::encode(current_span_context.c_str(), current_span_context.length())); } - if (use_tracer_propagation_) { + if (driver_.useTracerPropagation()) { // Inject the context using the tracer's standard HTTP header format. const OpenTracingHTTPHeadersWriter writer{request_headers}; const opentracing::expected was_successful = span_->tracer().Inject(span_->context(), writer); if (!was_successful) { ENVOY_LOG(debug, "Failed to inject span context: {}", was_successful.error().message()); + driver_.tracerStats().span_context_injection_error_.inc(); return; } } @@ -120,10 +121,12 @@ SpanPtr OpenTracingSpan::spawnChild(const Config&, const std::string& name, Syst std::unique_ptr ot_span = span_->tracer().StartSpan( name, {opentracing::ChildOf(&span_->context()), opentracing::StartTimestamp(start_time)}); RELEASE_ASSERT(ot_span != nullptr); - return SpanPtr{new OpenTracingSpan{use_single_header_propagation_, use_tracer_propagation_, - std::move(ot_span)}}; + return SpanPtr{new OpenTracingSpan{driver_, std::move(ot_span)}}; } +OpenTracingDriver::OpenTracingDriver(Stats::Store& stats) + : tracer_stats_{OPENTRACING_TRACER_STATS(POOL_COUNTER_PREFIX(stats, "tracing.opentracing."))} {} + SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_headers, const std::string& operation_name, SystemTime start_time) { const bool use_single_header_propagation = this->useSingleHeaderPropagation(); @@ -141,6 +144,7 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea } else { ENVOY_LOG(debug, "Failed to extract span context: {}", parent_span_ctx_maybe.error().message()); + tracerStats().span_context_extraction_error_.inc(); } } else if (use_tracer_propagation) { const OpenTracingHTTPHeadersReader reader{request_headers}; @@ -151,13 +155,13 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea } else { ENVOY_LOG(debug, "Failed to extract span context: {}", parent_span_ctx_maybe.error().message()); + tracerStats().span_context_extraction_error_.inc(); } } active_span = tracer.StartSpan(operation_name, {opentracing::ChildOf(parent_span_ctx.get()), opentracing::StartTimestamp(start_time)}); RELEASE_ASSERT(active_span != nullptr); - return SpanPtr{new OpenTracingSpan{use_single_header_propagation, use_tracer_propagation, - std::move(active_span)}}; + return SpanPtr{new OpenTracingSpan{*this, std::move(active_span)}}; } } // namespace Tracing diff --git a/source/common/tracing/opentracing_driver_impl.h b/source/common/tracing/opentracing_driver_impl.h index e5e6d0af051d..775d73ed6d71 100644 --- a/source/common/tracing/opentracing_driver_impl.h +++ b/source/common/tracing/opentracing_driver_impl.h @@ -11,10 +11,19 @@ namespace Envoy { namespace Tracing { +#define OPENTRACING_TRACER_STATS(COUNTER) \ + COUNTER(span_context_extraction_error) \ + COUNTER(span_context_injection_error) + +struct OpenTracingTracerStats { + OPENTRACING_TRACER_STATS(GENERATE_COUNTER_STRUCT) +}; + +class OpenTracingDriver; + class OpenTracingSpan : public Span, Logger::Loggable { public: - OpenTracingSpan(bool use_single_header_propagation, bool use_tracer_propagation, - std::unique_ptr&& span); + OpenTracingSpan(OpenTracingDriver& driver, std::unique_ptr&& span); // Tracing::Span void finishSpan() override; @@ -24,13 +33,14 @@ class OpenTracingSpan : public Span, Logger::Loggable { SpanPtr spawnChild(const Config& config, const std::string& name, SystemTime start_time) override; private: - const bool use_single_header_propagation_; - const bool use_tracer_propagation_; + OpenTracingDriver& driver_; std::unique_ptr span_; }; class OpenTracingDriver : public Driver, protected Logger::Loggable { public: + explicit OpenTracingDriver(Stats::Store& stats); + // Tracer::TracingDriver SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers, const std::string& operation_name, SystemTime start_time) override; @@ -39,6 +49,10 @@ class OpenTracingDriver : public Driver, protected Logger::LoggablestartSpan(config_, request_headers_, operation_name_, start_time_); + EXPECT_EQ(1U, stats_.counter("tracing.opentracing.span_context_extraction_error").value()); std::string injected_ctx = request_headers_.OtSpanContext()->value().c_str(); EXPECT_FALSE(injected_ctx.empty()); From 94750bd2433d0bcb4e05f762a576e6f498d0b343 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 21 Nov 2017 16:11:53 -0800 Subject: [PATCH 11/25] Add TODO note for Grpc::AsyncClient. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index caff530d5147..b68e8b051559 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -42,6 +42,7 @@ LightStepDriver::LightStepTransporter::LightStepTransporter(LightStepDriver& dri void LightStepDriver::LightStepTransporter::Send(const Protobuf::Message& request, Protobuf::Message& response, lightstep::AsyncTransporter::Callback& callback) { + // TODO(rnburn): Update to use Grpc::AsyncClient when it supports abstract message classes. active_callback_ = &callback; active_response_ = &response; From 1e255d4d6db308ac0174fed2f93916c3ba823f38 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 21 Nov 2017 16:20:02 -0800 Subject: [PATCH 12/25] s/O(1) header/predefined inline header/ Signed-off-by: Ryan Burn --- include/envoy/http/header_map.h | 8 ++++---- test/common/http/header_map_impl_test.cc | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 1fa454e8e8e3..a888adc800f5 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -387,12 +387,12 @@ class HeaderMap { enum class Lookup { Found, NotFound, NotSupported }; /** - * Lookup one of the O(1) headers by key. + * Lookup one of the predefined inline headers (see ALL_INLINE_HEADERS below) by key. * @param key supplies the header key. - * @param entry is set to the header entry if it exists and if key is one of the O(1) headers; - * otherwise, nullptr. + * @param entry is set to the header entry if it exists and if key is one of the predefined inline + * headers; otherwise, nullptr. * @return Lookup::Found if lookup was successful, Lookup::NotFound if the header entry doesn't - * exist, or Lookup::NotSupported if key is not one of the O(1) headers. + * exist, or Lookup::NotSupported if key is not one of the predefined inline headers. */ virtual Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const PURE; diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 1865f0efac77..2e06089836b3 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -451,21 +451,21 @@ TEST(HeaderMapImplTest, Lookup) { headers.addCopy("hello", "world"); headers.insertContentLength().value(5); - // Lookup is not supported for non O(1) header. + // Lookup is not supported for non predefined inline headers. { const HeaderEntry* entry; EXPECT_EQ(HeaderMap::Lookup::NotSupported, headers.lookup(LowerCaseString{"hello"}, &entry)); EXPECT_EQ(nullptr, entry); } - // Lookup returns the entry of a O(1) header if it exists. + // Lookup returns the entry of a predefined inline header if it exists. { const HeaderEntry* entry; EXPECT_EQ(HeaderMap::Lookup::Found, headers.lookup(Headers::get().ContentLength, &entry)); EXPECT_STREQ("5", entry->value().c_str()); } - // Lookup returns HeaderMap::Lookup::NotFound if a O(1) header does not exist. + // Lookup returns HeaderMap::Lookup::NotFound if a predefined inline header does not exist. { const HeaderEntry* entry; EXPECT_EQ(HeaderMap::Lookup::NotFound, headers.lookup(Headers::get().Host, &entry)); From 725d002fffd7de45461bdb059f3bc8cc457de993 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Tue, 21 Nov 2017 16:26:17 -0800 Subject: [PATCH 13/25] Add note for const_cast. Signed-off-by: Ryan Burn --- source/common/http/header_map_impl.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index f159c4c787ff..a5e9745a4064 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -403,6 +403,9 @@ HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key, const HeaderEntry** entry) const { StaticLookupEntry::EntryCb cb = ConstSingleton::get().find(key.get().c_str()); if (cb) { + // The accessor callbacks for predefined inline headers take a HeaderMapImpl& as an argument; + // even though we don't make any modifications, we need to cast_cast in order to use the + // accessor. StaticLookupResponse ref_lookup_response = cb(const_cast(*this)); *entry = *ref_lookup_response.entry_; if (*entry) { From 961e195784a6e6cd78cdec3294dc59cadab5d4dc Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Nov 2017 14:55:54 -0800 Subject: [PATCH 14/25] Document OpenTracingDriver. Signed-off-by: Ryan Burn --- source/common/tracing/opentracing_driver_impl.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/source/common/tracing/opentracing_driver_impl.h b/source/common/tracing/opentracing_driver_impl.h index 775d73ed6d71..f429456f8996 100644 --- a/source/common/tracing/opentracing_driver_impl.h +++ b/source/common/tracing/opentracing_driver_impl.h @@ -37,6 +37,12 @@ class OpenTracingSpan : public Span, Logger::Loggable { std::unique_ptr span_; }; +/** + * This driver can be used by tracing libraries implementing the OpenTracing API (see + * https://github.com/opentracing/opentracing-cpp) to hook into Envoy's tracing functionality with a + * minimal amount of effort. Libraries need only provide an opentracing::Tracer implementation; the + * rest of span creation is taken care of by this class. + */ class OpenTracingDriver : public Driver, protected Logger::Loggable { public: explicit OpenTracingDriver(Stats::Store& stats); From ecf6df12f6d0c61fdc7a550c9f478797e74c87bd Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 22 Nov 2017 16:27:52 -0800 Subject: [PATCH 15/25] Control span context propagation with enum instead of bools. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.h | 7 +- .../common/tracing/opentracing_driver_impl.cc | 26 ++++--- .../common/tracing/opentracing_driver_impl.h | 11 ++- .../tracing/lightstep_tracer_impl_test.cc | 71 ++++++++++--------- 4 files changed, 68 insertions(+), 47 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index 4d8a66acb3e1..d8c242e3f822 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -45,13 +45,15 @@ class LightStepDriver : public OpenTracingDriver { // Tracer::OpenTracingDriver const opentracing::Tracer& tracer() const override; - bool useTracerPropagation() const override { return true; } - bool useSingleHeaderPropagation() const override { return false; } + PropagationMode propagationMode() const override { return propagation_mode_; } Upstream::ClusterManager& clusterManager() { return cm_; } Upstream::ClusterInfoConstSharedPtr cluster() { return cluster_; } Runtime::Loader& runtime() { return runtime_; } LightstepTracerStats& tracerStats() { return tracer_stats_; } + void setPropagationMode(PropagationMode propagation_mode) { + propagation_mode_ = propagation_mode; + } private: class LightStepTransporter : public lightstep::AsyncTransporter, Http::AsyncClient::Callbacks { @@ -103,6 +105,7 @@ class LightStepDriver : public OpenTracingDriver { ThreadLocal::SlotPtr tls_; Runtime::Loader& runtime_; std::unique_ptr options_; + PropagationMode propagation_mode_ = PropagationMode::TracerNative; }; } // Tracing diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index d5e134296459..3ae8e1c0ac27 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -89,7 +89,7 @@ void OpenTracingSpan::setTag(const std::string& name, const std::string& value) } void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) { - if (driver_.useSingleHeaderPropagation()) { + if (driver_.propagationMode() == OpenTracingDriver::PropagationMode::SingleHeader) { // Inject the span context using Envoy's single-header format. std::ostringstream oss; const opentracing::expected was_successful = @@ -102,9 +102,7 @@ void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) { const std::string current_span_context = oss.str(); request_headers.insertOtSpanContext().value( Base64::encode(current_span_context.c_str(), current_span_context.length())); - } - - if (driver_.useTracerPropagation()) { + } else { // Inject the context using the tracer's standard HTTP header format. const OpenTracingHTTPHeadersWriter writer{request_headers}; const opentracing::expected was_successful = @@ -129,16 +127,22 @@ OpenTracingDriver::OpenTracingDriver(Stats::Store& stats) SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_headers, const std::string& operation_name, SystemTime start_time) { - const bool use_single_header_propagation = this->useSingleHeaderPropagation(); - const bool use_tracer_propagation = this->useTracerPropagation(); + const PropagationMode propagation_mode = this->propagationMode(); const opentracing::Tracer& tracer = this->tracer(); std::unique_ptr active_span; std::unique_ptr parent_span_ctx; - if (use_single_header_propagation && request_headers.OtSpanContext()) { + if (propagation_mode == PropagationMode::SingleHeader && request_headers.OtSpanContext()) { + opentracing::expected> parent_span_ctx_maybe; std::string parent_context = Base64::decode(request_headers.OtSpanContext()->value().c_str()); - std::istringstream iss(parent_context); - opentracing::expected> parent_span_ctx_maybe = - tracer.Extract(iss); + + if (!parent_context.empty()) { + std::istringstream iss{parent_context, std::ios::binary}; + parent_span_ctx_maybe = tracer.Extract(iss); + } else { + parent_span_ctx_maybe = + opentracing::make_unexpected(opentracing::span_context_corrupted_error); + } + if (parent_span_ctx_maybe) { parent_span_ctx = std::move(*parent_span_ctx_maybe); } else { @@ -146,7 +150,7 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea parent_span_ctx_maybe.error().message()); tracerStats().span_context_extraction_error_.inc(); } - } else if (use_tracer_propagation) { + } else if (propagation_mode == PropagationMode::TracerNative) { const OpenTracingHTTPHeadersReader reader{request_headers}; opentracing::expected> parent_span_ctx_maybe = tracer.Extract(reader); diff --git a/source/common/tracing/opentracing_driver_impl.h b/source/common/tracing/opentracing_driver_impl.h index f429456f8996..f843eb868b7c 100644 --- a/source/common/tracing/opentracing_driver_impl.h +++ b/source/common/tracing/opentracing_driver_impl.h @@ -53,8 +53,15 @@ class OpenTracingDriver : public Driver, protected Logger::LoggablestartSpan(config_, request_headers_, operation_name_, start_time_); - EXPECT_EQ(1U, stats_.counter("tracing.opentracing.span_context_extraction_error").value()); - - std::string injected_ctx = request_headers_.OtSpanContext()->value().c_str(); - EXPECT_FALSE(injected_ctx.empty()); - - // Supply empty context. - request_headers_.removeOtSpanContext(); - SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_); - - EXPECT_EQ(nullptr, request_headers_.OtSpanContext()); - span->injectContext(request_headers_); - - injected_ctx = request_headers_.OtSpanContext()->value().c_str(); - EXPECT_FALSE(injected_ctx.empty()); - - // Context can be parsed fine. - const opentracing::Tracer& tracer = driver_->tracer(); - std::string context = Base64::decode(injected_ctx); - std::istringstream iss{context, std::ios::binary}; - EXPECT_TRUE(tracer.Extract(iss)); - - // Supply parent context, request_headers has properly populated x-ot-span-context. - SpanPtr span_with_parent = - driver_->startSpan(config_, request_headers_, operation_name_, start_time_); - request_headers_.removeOtSpanContext(); - span_with_parent->injectContext(request_headers_); - injected_ctx = request_headers_.OtSpanContext()->value().c_str(); - EXPECT_FALSE(injected_ctx.empty()); + for (OpenTracingDriver::PropagationMode propagation_mode : + {OpenTracingDriver::PropagationMode::SingleHeader, + OpenTracingDriver::PropagationMode::TracerNative}) { + driver_->setPropagationMode(propagation_mode); + + // Supply bogus context, that will be simply ignored. + const std::string invalid_context = "notvalidcontext"; + request_headers_.insertOtSpanContext().value(invalid_context); + stats_.counter("tracing.opentracing.span_context_extraction_error").reset(); + driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + EXPECT_EQ(1U, stats_.counter("tracing.opentracing.span_context_extraction_error").value()); + + std::string injected_ctx = request_headers_.OtSpanContext()->value().c_str(); + EXPECT_FALSE(injected_ctx.empty()); + + // Supply empty context. + request_headers_.removeOtSpanContext(); + SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + + EXPECT_EQ(nullptr, request_headers_.OtSpanContext()); + span->injectContext(request_headers_); + + injected_ctx = request_headers_.OtSpanContext()->value().c_str(); + EXPECT_FALSE(injected_ctx.empty()); + + // Context can be parsed fine. + const opentracing::Tracer& tracer = driver_->tracer(); + std::string context = Base64::decode(injected_ctx); + std::istringstream iss{context, std::ios::binary}; + EXPECT_TRUE(tracer.Extract(iss)); + + // Supply parent context, request_headers has properly populated x-ot-span-context. + SpanPtr span_with_parent = + driver_->startSpan(config_, request_headers_, operation_name_, start_time_); + request_headers_.removeOtSpanContext(); + span_with_parent->injectContext(request_headers_); + injected_ctx = request_headers_.OtSpanContext()->value().c_str(); + EXPECT_FALSE(injected_ctx.empty()); + } } TEST_F(LightStepDriverTest, SpawnChild) { From 0803ff2803958b3823d68e867028e6415a13b648 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 27 Nov 2017 14:59:19 -0800 Subject: [PATCH 16/25] Avoid copy associated with std::istringstream. Signed-off-by: Ryan Burn --- source/common/common/utility.cc | 11 ++++++++++ source/common/common/utility.h | 20 +++++++++++++++++++ .../common/tracing/opentracing_driver_impl.cc | 5 +++-- test/common/common/utility_test.cc | 19 ++++++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index b6e11eac6032..49e36d2d2a36 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -32,6 +32,17 @@ std::string DateFormatter::now() { ProdSystemTimeSource ProdSystemTimeSource::instance_; ProdMonotonicTimeSource ProdMonotonicTimeSource::instance_; +MemoryStreamBuffer::MemoryStreamBuffer(const char* data, size_t size) { + // std::streambuf won't modify `data`, but the interface still requires a char* for convenience, + // so we need to const_cast. + char* ptr = const_cast(data); + + this->setg(ptr, ptr, ptr + size); +} + +IMemoryStream::IMemoryStream(const char* data, size_t size) + : MemoryStreamBuffer{data, size}, std::istream{static_cast(this)} {} + bool DateUtil::timePointValid(SystemTime time_point) { return std::chrono::duration_cast(time_point.time_since_epoch()) .count() != 0; diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 599ff334f0e9..b95b5614811a 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -64,6 +64,26 @@ class ProdMonotonicTimeSource : public MonotonicTimeSource { static ProdMonotonicTimeSource instance_; }; +/** + * Class used for creating non-copying std::istream's. See IMemoryStream below. + */ +class MemoryStreamBuffer : public std::streambuf { +public: + MemoryStreamBuffer(const char* data, size_t size); +}; + +/** + * std::istream class similar to std::istringstream, except that it provides a view into a region of + * constant memory. It can be more efficient than std::istringstream because it doesn't copy the + * provided string. + * + * See https://stackoverflow.com/a/13059195/4447365. + */ +class IMemoryStream : public virtual MemoryStreamBuffer, public std::istream { +public: + IMemoryStream(const char* data, size_t size); +}; + /** * Utility class for date/time helpers. */ diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index 3ae8e1c0ac27..a05d119280cd 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -4,6 +4,7 @@ #include "common/common/assert.h" #include "common/common/base64.h" +#include "common/common/utility.h" namespace Envoy { namespace Tracing { @@ -136,8 +137,8 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea std::string parent_context = Base64::decode(request_headers.OtSpanContext()->value().c_str()); if (!parent_context.empty()) { - std::istringstream iss{parent_context, std::ios::binary}; - parent_span_ctx_maybe = tracer.Extract(iss); + IMemoryStream istream{parent_context.data(), parent_context.size()}; + parent_span_ctx_maybe = tracer.Extract(istream); } else { parent_span_ctx_maybe = opentracing::make_unexpected(opentracing::span_context_corrupted_error); diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 07c41a3f5d0f..b80c1c20316b 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -38,6 +38,25 @@ TEST(ProdSystemTimeSourceTest, All) { source.currentTime(); } +TEST(IMemoryStream, All) { + { + IMemoryStream istream{nullptr, 0}; + std::string s; + istream >> s; + EXPECT_TRUE(s.empty()); + EXPECT_TRUE(istream.eof()); + } + + { + std::string data{"123"}; + IMemoryStream istream{data.data(), data.size()}; + int x; + istream >> x; + EXPECT_EQ(123, x); + EXPECT_TRUE(istream.eof()); + } +} + TEST(StringUtil, caseInsensitiveCompare) { EXPECT_EQ(0, StringUtil::caseInsensitiveCompare("CONTENT-LENGTH", "content-length")); EXPECT_LT(0, StringUtil::caseInsensitiveCompare("CONTENT-LENGTH", "blah")); From 5d62c5185c6cb988ac9c6611aea054eaa5aeee78 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Sat, 2 Dec 2017 22:57:32 -0800 Subject: [PATCH 17/25] Ensure span-context headers aren't duplicated. Signed-off-by: Ryan Burn --- source/common/tracing/opentracing_driver_impl.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index a05d119280cd..652354eb1756 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -18,7 +18,9 @@ class OpenTracingHTTPHeadersWriter : public opentracing::HTTPHeadersWriter { // opentracing::HTTPHeadersWriter opentracing::expected Set(opentracing::string_view key, opentracing::string_view value) const override { - request_headers_.addCopy(Http::LowerCaseString{key}, value); + Http::LowerCaseString lowercase_key{key}; + request_headers_.remove(lowercase_key); + request_headers_.addCopy(std::move(lowercase_key), value); return {}; } From 3ff6aafe2813f5eb6888450f1fce0f451e1234b3 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 6 Dec 2017 11:29:00 -0800 Subject: [PATCH 18/25] Remove default case from switch statment. Signed-off-by: Ryan Burn --- source/common/tracing/opentracing_driver_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index 652354eb1756..bd69cd69d30d 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -50,9 +50,8 @@ class OpenTracingHTTPHeadersReader : public opentracing::HTTPHeadersReader { return opentracing::make_unexpected(opentracing::key_not_found_error); case Http::HeaderMap::Lookup::NotSupported: return opentracing::make_unexpected(opentracing::lookup_key_not_supported_error); - default: - NOT_REACHED; } + NOT_REACHED; } opentracing::expected ForeachKey(OpenTracingCb f) const override { From cc6a7e25aae087aa2841a62e5e64456c28502e55 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 6 Dec 2017 11:36:25 -0800 Subject: [PATCH 19/25] s/MemoryStreamBuffer/ConstMemoryStreamBuffer/ Signed-off-by: Ryan Burn --- source/common/common/utility.cc | 4 ++-- source/common/common/utility.h | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 49e36d2d2a36..270bb3250e70 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -32,7 +32,7 @@ std::string DateFormatter::now() { ProdSystemTimeSource ProdSystemTimeSource::instance_; ProdMonotonicTimeSource ProdMonotonicTimeSource::instance_; -MemoryStreamBuffer::MemoryStreamBuffer(const char* data, size_t size) { +ConstMemoryStreamBuffer::ConstMemoryStreamBuffer(const char* data, size_t size) { // std::streambuf won't modify `data`, but the interface still requires a char* for convenience, // so we need to const_cast. char* ptr = const_cast(data); @@ -41,7 +41,7 @@ MemoryStreamBuffer::MemoryStreamBuffer(const char* data, size_t size) { } IMemoryStream::IMemoryStream(const char* data, size_t size) - : MemoryStreamBuffer{data, size}, std::istream{static_cast(this)} {} + : ConstMemoryStreamBuffer{data, size}, std::istream{static_cast(this)} {} bool DateUtil::timePointValid(SystemTime time_point) { return std::chrono::duration_cast(time_point.time_since_epoch()) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index b95b5614811a..d9c79017fc36 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -67,9 +67,9 @@ class ProdMonotonicTimeSource : public MonotonicTimeSource { /** * Class used for creating non-copying std::istream's. See IMemoryStream below. */ -class MemoryStreamBuffer : public std::streambuf { +class ConstMemoryStreamBuffer : public std::streambuf { public: - MemoryStreamBuffer(const char* data, size_t size); + ConstMemoryStreamBuffer(const char* data, size_t size); }; /** @@ -79,7 +79,7 @@ class MemoryStreamBuffer : public std::streambuf { * * See https://stackoverflow.com/a/13059195/4447365. */ -class IMemoryStream : public virtual MemoryStreamBuffer, public std::istream { +class IMemoryStream : public virtual ConstMemoryStreamBuffer, public std::istream { public: IMemoryStream(const char* data, size_t size); }; From 739bf4aacf28430ffe2499d6bcfb0d3499329835 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 6 Dec 2017 11:42:30 -0800 Subject: [PATCH 20/25] s/IMemoryStream/InputConstMemoryStream/ Signed-off-by: Ryan Burn --- source/common/common/utility.cc | 2 +- source/common/common/utility.h | 6 +++--- source/common/tracing/opentracing_driver_impl.cc | 2 +- test/common/common/utility_test.cc | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 270bb3250e70..7a295d891ef5 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -40,7 +40,7 @@ ConstMemoryStreamBuffer::ConstMemoryStreamBuffer(const char* data, size_t size) this->setg(ptr, ptr, ptr + size); } -IMemoryStream::IMemoryStream(const char* data, size_t size) +InputConstMemoryStream::InputConstMemoryStream(const char* data, size_t size) : ConstMemoryStreamBuffer{data, size}, std::istream{static_cast(this)} {} bool DateUtil::timePointValid(SystemTime time_point) { diff --git a/source/common/common/utility.h b/source/common/common/utility.h index d9c79017fc36..91f46e1ac859 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -65,7 +65,7 @@ class ProdMonotonicTimeSource : public MonotonicTimeSource { }; /** - * Class used for creating non-copying std::istream's. See IMemoryStream below. + * Class used for creating non-copying std::istream's. See InputConstMemoryStream below. */ class ConstMemoryStreamBuffer : public std::streambuf { public: @@ -79,9 +79,9 @@ class ConstMemoryStreamBuffer : public std::streambuf { * * See https://stackoverflow.com/a/13059195/4447365. */ -class IMemoryStream : public virtual ConstMemoryStreamBuffer, public std::istream { +class InputConstMemoryStream : public virtual ConstMemoryStreamBuffer, public std::istream { public: - IMemoryStream(const char* data, size_t size); + InputConstMemoryStream(const char* data, size_t size); }; /** diff --git a/source/common/tracing/opentracing_driver_impl.cc b/source/common/tracing/opentracing_driver_impl.cc index bd69cd69d30d..5c54257f0b43 100644 --- a/source/common/tracing/opentracing_driver_impl.cc +++ b/source/common/tracing/opentracing_driver_impl.cc @@ -138,7 +138,7 @@ SpanPtr OpenTracingDriver::startSpan(const Config&, Http::HeaderMap& request_hea std::string parent_context = Base64::decode(request_headers.OtSpanContext()->value().c_str()); if (!parent_context.empty()) { - IMemoryStream istream{parent_context.data(), parent_context.size()}; + InputConstMemoryStream istream{parent_context.data(), parent_context.size()}; parent_span_ctx_maybe = tracer.Extract(istream); } else { parent_span_ctx_maybe = diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index b80c1c20316b..bec35a8f3c81 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -38,9 +38,9 @@ TEST(ProdSystemTimeSourceTest, All) { source.currentTime(); } -TEST(IMemoryStream, All) { +TEST(InputConstMemoryStream, All) { { - IMemoryStream istream{nullptr, 0}; + InputConstMemoryStream istream{nullptr, 0}; std::string s; istream >> s; EXPECT_TRUE(s.empty()); @@ -49,7 +49,7 @@ TEST(IMemoryStream, All) { { std::string data{"123"}; - IMemoryStream istream{data.data(), data.size()}; + InputConstMemoryStream istream{data.data(), data.size()}; int x; istream >> x; EXPECT_EQ(123, x); From cf7896b4dc0a6dd6e0721f62b8868332bc723876 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 6 Dec 2017 11:47:30 -0800 Subject: [PATCH 21/25] Fix ordering of member functions. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index d8c242e3f822..d51dd4f9f117 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -43,10 +43,6 @@ class LightStepDriver : public OpenTracingDriver { Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, std::unique_ptr&& options); - // Tracer::OpenTracingDriver - const opentracing::Tracer& tracer() const override; - PropagationMode propagationMode() const override { return propagation_mode_; } - Upstream::ClusterManager& clusterManager() { return cm_; } Upstream::ClusterInfoConstSharedPtr cluster() { return cluster_; } Runtime::Loader& runtime() { return runtime_; } @@ -55,6 +51,10 @@ class LightStepDriver : public OpenTracingDriver { propagation_mode_ = propagation_mode; } + // Tracer::OpenTracingDriver + const opentracing::Tracer& tracer() const override; + PropagationMode propagationMode() const override { return propagation_mode_; } + private: class LightStepTransporter : public lightstep::AsyncTransporter, Http::AsyncClient::Callbacks { public: From ce00ac677052bbb34b38e0d85477699c01633e78 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 6 Dec 2017 11:53:41 -0800 Subject: [PATCH 22/25] Document reason for choosing const_cast. Signed-off-by: Ryan Burn --- source/common/http/header_map_impl.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index a5e9745a4064..48e8ab7b92a4 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -406,6 +406,9 @@ HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key, // The accessor callbacks for predefined inline headers take a HeaderMapImpl& as an argument; // even though we don't make any modifications, we need to cast_cast in order to use the // accessor. + // + // Making this work without const_cast would require managing an additional const accessor + // callback for each predefined inline header and add to the complexity of the code. StaticLookupResponse ref_lookup_response = cb(const_cast(*this)); *entry = *ref_lookup_response.entry_; if (*entry) { From 861e87c90b0f28785ed36f255026c40cfd500840 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 6 Dec 2017 12:01:04 -0800 Subject: [PATCH 23/25] Don't move from std::shared_ptr. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.cc | 6 +++--- source/common/tracing/lightstep_tracer_impl.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index b68e8b051559..77e3e6de7579 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -93,9 +93,9 @@ void LightStepDriver::LightStepMetricsObserver::OnSpansSent(int num_spans) { } LightStepDriver::TlsLightStepTracer::TlsLightStepTracer( - std::shared_ptr&& tracer, LightStepDriver& driver, + const std::shared_ptr& tracer, LightStepDriver& driver, Event::Dispatcher& dispatcher) - : tracer_(std::move(tracer)), driver_(driver) { + : tracer_{tracer}, driver_{driver} { flush_timer_ = dispatcher.createTimer([this]() -> void { driver_.tracerStats().timer_flushed_.inc(); tracer_->Flush(); @@ -148,7 +148,7 @@ LightStepDriver::LightStepDriver(const Json::Object& config, lightstep::MakeLightStepTracer(std::move(tls_options)); return ThreadLocal::ThreadLocalObjectSharedPtr{ - new TlsLightStepTracer(std::move(tracer), *this, dispatcher)}; + new TlsLightStepTracer{tracer, *this, dispatcher}}; }); } diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index d51dd4f9f117..5997827d9138 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -86,7 +86,7 @@ class LightStepDriver : public OpenTracingDriver { class TlsLightStepTracer : public ThreadLocal::ThreadLocalObject { public: - TlsLightStepTracer(std::shared_ptr&& tracer, + TlsLightStepTracer(const std::shared_ptr& tracer, LightStepDriver& driver, Event::Dispatcher& dispatcher); const opentracing::Tracer& tracer() const; From 562746e301fbcdcfc29a4b95b62f9a485b9bf877 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Wed, 6 Dec 2017 19:15:19 -0800 Subject: [PATCH 24/25] Set propagation mode via constructor. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.cc | 11 ++++++----- source/common/tracing/lightstep_tracer_impl.h | 8 +++----- .../server/config/http/lightstep_http_tracer.cc | 10 +++++----- .../common/tracing/lightstep_tracer_impl_test.cc | 16 +++++++++------- 4 files changed, 23 insertions(+), 22 deletions(-) diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc index 77e3e6de7579..851a6b0eb1c9 100644 --- a/source/common/tracing/lightstep_tracer_impl.cc +++ b/source/common/tracing/lightstep_tracer_impl.cc @@ -116,11 +116,12 @@ void LightStepDriver::TlsLightStepTracer::enableTimer() { LightStepDriver::LightStepDriver(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, - std::unique_ptr&& options) - : OpenTracingDriver{stats}, - cm_(cluster_manager), tracer_stats_{LIGHTSTEP_TRACER_STATS( - POOL_COUNTER_PREFIX(stats, "tracing.lightstep."))}, - tls_(tls.allocateSlot()), runtime_(runtime), options_(std::move(options)) { + std::unique_ptr&& options, + PropagationMode propagation_mode) + : OpenTracingDriver{stats}, cm_{cluster_manager}, + 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")); if (!cluster) { throw EnvoyException(fmt::format("{} collector cluster is not defined on cluster manager level", diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index 5997827d9138..2fc06c16739c 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -41,15 +41,13 @@ class LightStepDriver : public OpenTracingDriver { public: LightStepDriver(const Json::Object& config, Upstream::ClusterManager& cluster_manager, Stats::Store& stats, ThreadLocal::SlotAllocator& tls, Runtime::Loader& runtime, - std::unique_ptr&& options); + std::unique_ptr&& options, + PropagationMode propagation_mode); Upstream::ClusterManager& clusterManager() { return cm_; } Upstream::ClusterInfoConstSharedPtr cluster() { return cluster_; } Runtime::Loader& runtime() { return runtime_; } LightstepTracerStats& tracerStats() { return tracer_stats_; } - void setPropagationMode(PropagationMode propagation_mode) { - propagation_mode_ = propagation_mode; - } // Tracer::OpenTracingDriver const opentracing::Tracer& tracer() const override; @@ -105,7 +103,7 @@ class LightStepDriver : public OpenTracingDriver { ThreadLocal::SlotPtr tls_; Runtime::Loader& runtime_; std::unique_ptr options_; - PropagationMode propagation_mode_ = PropagationMode::TracerNative; + PropagationMode propagation_mode_; }; } // Tracing diff --git a/source/server/config/http/lightstep_http_tracer.cc b/source/server/config/http/lightstep_http_tracer.cc index a7227f55b653..f9d167e1fec4 100644 --- a/source/server/config/http/lightstep_http_tracer.cc +++ b/source/server/config/http/lightstep_http_tracer.cc @@ -25,11 +25,11 @@ LightstepHttpTracerFactory::createHttpTracer(const Json::Object& json_config, StringUtil::rtrim(opts->access_token); opts->component_name = server.localInfo().clusterName(); - Tracing::DriverPtr lightstep_driver( - new Tracing::LightStepDriver(json_config, cluster_manager, server.stats(), - server.threadLocal(), server.runtime(), std::move(opts))); - return Tracing::HttpTracerPtr( - new Tracing::HttpTracerImpl(std::move(lightstep_driver), server.localInfo())); + Tracing::DriverPtr lightstep_driver{new Tracing::LightStepDriver{ + json_config, cluster_manager, server.stats(), server.threadLocal(), server.runtime(), + std::move(opts), Tracing::OpenTracingDriver::PropagationMode::TracerNative}}; + return Tracing::HttpTracerPtr{ + new Tracing::HttpTracerImpl{std::move(lightstep_driver), server.localInfo()}}; } std::string LightstepHttpTracerFactory::name() { return Config::HttpTracerNames::get().LIGHTSTEP; } diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc index 4c4c8767cca0..4e02490900ef 100644 --- a/test/common/tracing/lightstep_tracer_impl_test.cc +++ b/test/common/tracing/lightstep_tracer_impl_test.cc @@ -39,7 +39,9 @@ namespace Tracing { class LightStepDriverTest : public Test { public: - void setup(Json::Object& config, bool init_timer) { + void setup(Json::Object& config, bool init_timer, + OpenTracingDriver::PropagationMode propagation_mode = + OpenTracingDriver::PropagationMode::TracerNative) { std::unique_ptr opts( new lightstep::LightStepTracerOptions()); opts->access_token = "sample_token"; @@ -53,10 +55,12 @@ class LightStepDriverTest : public Test { EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1000))); } - driver_.reset(new LightStepDriver(config, cm_, stats_, tls_, runtime_, std::move(opts))); + driver_.reset(new LightStepDriver{config, cm_, stats_, tls_, runtime_, std::move(opts), + propagation_mode}); } - void setupValidDriver() { + void setupValidDriver(OpenTracingDriver::PropagationMode propagation_mode = + OpenTracingDriver::PropagationMode::TracerNative) { 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)); @@ -66,7 +70,7 @@ class LightStepDriverTest : public Test { )EOF"; Json::ObjectSharedPtr loader = Json::Factory::loadFromString(valid_config); - setup(*loader, true); + setup(*loader, true, propagation_mode); } const std::string operation_name_{"test"}; @@ -317,12 +321,10 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) { } TEST_F(LightStepDriverTest, SerializeAndDeserializeContext) { - setupValidDriver(); - for (OpenTracingDriver::PropagationMode propagation_mode : {OpenTracingDriver::PropagationMode::SingleHeader, OpenTracingDriver::PropagationMode::TracerNative}) { - driver_->setPropagationMode(propagation_mode); + setupValidDriver(propagation_mode); // Supply bogus context, that will be simply ignored. const std::string invalid_context = "notvalidcontext"; From a9411ad69bb5634e2862c44b53a39ac6dd983860 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Mon, 11 Dec 2017 11:44:19 -0800 Subject: [PATCH 25/25] Make propagation_mode_ const. Signed-off-by: Ryan Burn --- source/common/tracing/lightstep_tracer_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h index 2fc06c16739c..62b91016bc3f 100644 --- a/source/common/tracing/lightstep_tracer_impl.h +++ b/source/common/tracing/lightstep_tracer_impl.h @@ -103,7 +103,7 @@ class LightStepDriver : public OpenTracingDriver { ThreadLocal::SlotPtr tls_; Runtime::Loader& runtime_; std::unique_ptr options_; - PropagationMode propagation_mode_; + const PropagationMode propagation_mode_; }; } // Tracing