From d445cc8af6e50278628a9b8bc62d39d39a9af547 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 16 May 2019 15:42:50 +0700 Subject: [PATCH 01/48] Prepare sending proto3 to zipkin Signed-off-by: Dhi Aurrahman --- api/bazel/repositories.bzl | 23 ++++++++ api/bazel/repository_locations.bzl | 8 +++ api/envoy/config/trace/v2/trace.proto | 11 ++++ source/common/http/headers.h | 1 + source/extensions/tracers/zipkin/BUILD | 1 + .../extensions/tracers/zipkin/span_buffer.cc | 11 ++++ .../extensions/tracers/zipkin/span_buffer.h | 9 ++- .../tracers/zipkin/zipkin_core_types.cc | 56 +++++++++++++++++++ .../tracers/zipkin/zipkin_core_types.h | 15 +++++ .../tracers/zipkin/zipkin_tracer_impl.cc | 38 ++++++++----- .../tracers/zipkin/zipkin_tracer_impl.h | 26 ++++++--- 11 files changed, 178 insertions(+), 21 deletions(-) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index 724718f7495f..764d90ba29e1 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -36,6 +36,11 @@ def api_dependencies(): locations = REPOSITORY_LOCATIONS, build_file_content = KAFKASOURCE_BUILD_CONTENT, ) + envoy_http_archive( + name = "com_github_apache_zipkinapi", + locations = REPOSITORY_LOCATIONS, + build_file_content = ZIPKINAPI_BUILD_CONTENT, + ) GOOGLEAPIS_BUILD_CONTENT = """ load("@com_google_protobuf//:protobuf.bzl", "cc_proto_library", "py_proto_library") @@ -303,3 +308,21 @@ filegroup( ) """ + +ZIPKINAPI_BUILD_CONTENT = """ +load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library") +load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") +api_proto_library( + name = "zipkin", + srcs = [ + "zipkin.proto", + ], + visibility = ["//visibility:public"], +) +go_proto_library( + name = "zipkin_go_proto", + importpath = "zipkin", + proto = ":zipkin", + visibility = ["//visibility:public"], +) +""" diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index b3bcb016651b..4d9f0bbe47b2 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -18,6 +18,9 @@ PROMETHEUS_SHA = "783bdaf8ee0464b35ec0c8704871e1e72afa0005c3f3587f65d9d6694bf391 KAFKA_SOURCE_SHA = "ae7a1696c0a0302b43c5b21e515c37e6ecd365941f68a510a7e442eebddf39a1" # 2.2.0-rc2 +ZIPKINAPI_RELEASE = "0.2.0" +ZIPKINAPI_SHA256 = "cc34d57c51a52e8b9ea8ea5ca51e25016531eabc6c47be9b11d8fc29a5d266ae" + REPOSITORY_LOCATIONS = dict( bazel_skylib = dict( sha256 = BAZEL_SKYLIB_SHA256, @@ -54,4 +57,9 @@ REPOSITORY_LOCATIONS = dict( strip_prefix = "kafka-2.2.0-rc2/clients/src/main/resources/common/message", urls = ["https://github.com/apache/kafka/archive/2.2.0-rc2.zip"], ), + com_github_apache_zipkinapi = dict( + sha256 = ZIPKINAPI_SHA256, + strip_prefix = "incubator-zipkin-api-" + ZIPKINAPI_RELEASE, + urls = ["https://github.com/apache/incubator-zipkin-api/archive/" + ZIPKINAPI_RELEASE + ".tar.gz"], + ), ) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index e21766b9a2a0..26c7a49f9d99 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -80,6 +80,17 @@ message ZipkinConfig { // Determines whether client and server spans will shared the same span id. // The default value is true. google.protobuf.BoolValue shared_span_context = 4; + + // Available zipkin collector endpoint versions. + enum CollectorEndpointVersion { + DEFAULT = 0; + HTTP_JSON_V1 = 1; + HTTP_PROTO = 2; + } + + // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1``` will be + // used. + CollectorEndpointVersion collector_endpoint_version = 6; } // DynamicOtConfig is used to dynamically load a tracer from a shared library diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 38403792c545..3b8d9ec878ee 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -130,6 +130,7 @@ class HeaderValues { const std::string GrpcWebText{"application/grpc-web-text"}; const std::string GrpcWebTextProto{"application/grpc-web-text+proto"}; const std::string Json{"application/json"}; + const std::string Protobuf{"application/x-protobuf"}; } ContentTypeValues; struct { diff --git a/source/extensions/tracers/zipkin/BUILD b/source/extensions/tracers/zipkin/BUILD index 77937e6dbbbf..34a32b87fab3 100644 --- a/source/extensions/tracers/zipkin/BUILD +++ b/source/extensions/tracers/zipkin/BUILD @@ -57,6 +57,7 @@ envoy_cc_library( "//source/common/singleton:const_singleton", "//source/common/tracing:http_tracer_lib", "//source/extensions/tracers:well_known_names", + "@com_github_apache_zipkinapi//:zipkin_cc", ], ) diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index 387d851a9f91..2f2bc5cd40ff 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -32,6 +32,17 @@ std::string SpanBuffer::toStringifiedJsonArray() { return stringified_json_array; } +const zipkin::proto3::ListOfSpans SpanBuffer::toProtoListOfSpans() const { + zipkin::proto3::ListOfSpans spans; + if (pendingSpans()) { + for (const auto& span : span_buffer_) { + auto* mutable_span = spans.add_spans(); + mutable_span->MergeFrom(span.toProtoSpan()); + } + } + return spans; +} + } // namespace Zipkin } // namespace Tracers } // namespace Extensions diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index f81ddc41e003..16d0481af617 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -2,6 +2,8 @@ #include "extensions/tracers/zipkin/zipkin_core_types.h" +#include "zipkin.pb.h" + namespace Envoy { namespace Extensions { namespace Tracers { @@ -51,7 +53,7 @@ class SpanBuffer { /** * @return the number of spans currently buffered. */ - uint64_t pendingSpans() { return span_buffer_.size(); } + uint64_t pendingSpans() const { return span_buffer_.size(); } /** * @return the contents of the buffer as a stringified array of JSONs, where @@ -59,6 +61,11 @@ class SpanBuffer { */ std::string toStringifiedJsonArray(); + /** + * @return the contents of the buffer as a zipkin::proto3::ListOfSpans. + */ + const zipkin::proto3::ListOfSpans toProtoListOfSpans() const; + private: // We use a pre-allocated vector to improve performance std::vector span_buffer_; diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index e99e87312077..896fed805a77 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -1,5 +1,7 @@ #include "extensions/tracers/zipkin/zipkin_core_types.h" +#include "common/common/byte_order.h" +#include "common/common/empty_string.h" #include "common/common/utility.h" #include "extensions/tracers/zipkin/span_context.h" @@ -55,6 +57,22 @@ const std::string Endpoint::toJson() { return json_string; } +const zipkin::proto3::Endpoint Endpoint::toProtoEndpoint() const { + zipkin::proto3::Endpoint endpoint; + if (!address_) { + endpoint.set_ipv4(EMPTY_STRING); + endpoint.set_port(0); + } else { + if (address_->ip()->version() == Network::Address::IpVersion::v4) { + endpoint.set_ipv4(address_->ip()->addressAsString()); + } else { + endpoint.set_ipv6(address_->ip()->addressAsString()); + } + } + endpoint.set_service_name(service_name_); + return endpoint; +} + Annotation::Annotation(const Annotation& ann) { timestamp_ = ann.timestamp(); value_ = ann.value(); @@ -219,6 +237,44 @@ const std::string Span::toJson() { return json_string; } +const zipkin::proto3::Span Span::toProtoSpan() const { + zipkin::proto3::Span span; + + // TODO(dio): pack ids. + span.set_id("id"); + span.set_trace_id("traceid"); + span.set_name(name_); + + if (parent_id_ && parent_id_.has_value()) { + span.set_parent_id("parentid"); + } + + if (timestamp_) { + span.set_timestamp(timestamp_.value()); + } + + if (duration_) { + span.set_duration(duration_.value()); + } + + for (const auto& annotation : annotations_) { + if (annotation.isSetEndpoint()) { + if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { + span.mutable_local_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); + } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { + span.mutable_remote_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); + } + } + } + + for (const auto& binary_annotation : binary_annotations_) { + auto& tags = *span.mutable_tags(); + tags[binary_annotation.key()] = binary_annotation.value(); + } + + return span; +} + void Span::finish() { // Assumption: Span will have only one annotation when this method is called SpanContext context(*this); diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index 9060432c25a5..ca2b16ed531b 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -13,6 +13,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "zipkin.pb.h" namespace Envoy { namespace Extensions { @@ -94,6 +95,13 @@ class Endpoint : public ZipkinBase { */ const std::string toJson() override; + /** + * Builds protobuf message representation of this endpoint. + * + * @return zipkin::proto3::Endpoint the protobuf message representation of this endpoint. + */ + const zipkin::proto3::Endpoint toProtoEndpoint() const; + private: std::string service_name_; Network::Address::InstanceConstSharedPtr address_; @@ -518,6 +526,13 @@ class Span : public ZipkinBase { */ const std::string toJson() override; + /** + * Builds protobuf message representation of this span. + * + * @return zipkin::proto3::Span the protobuf message representation of this span. + */ + const zipkin::proto3::Span toProtoSpan() const; + /** * Associates a Tracer object with the span. The tracer's reportSpan() method is invoked * by the span's finish() method so that the tracer can decide what to do with the span diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index fbe84ff78662..99f377a3ae86 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -76,9 +76,13 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, Config::Utility::checkCluster(TracerNames::get().Zipkin, zipkin_config.collector_cluster(), cm_); cluster_ = cm_.get(zipkin_config.collector_cluster())->info(); - std::string collector_endpoint = ZipkinCoreConstants::get().DEFAULT_COLLECTOR_ENDPOINT; + CollectorInfo collector; if (!zipkin_config.collector_endpoint().empty()) { - collector_endpoint = zipkin_config.collector_endpoint(); + collector.endpoint = zipkin_config.collector_endpoint(); + } + if (zipkin_config.collector_endpoint_version() != + envoy::config::trace::v2::ZipkinConfig::DEFAULT) { + collector.version = zipkin_config.collector_endpoint_version(); } const bool trace_id_128bit = zipkin_config.trace_id_128bit(); @@ -86,12 +90,12 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, const bool shared_span_context = PROTOBUF_GET_WRAPPED_OR_DEFAULT( zipkin_config, shared_span_context, ZipkinCoreConstants::get().DEFAULT_SHARED_SPAN_CONTEXT); - tls_->set([this, collector_endpoint, &random_generator, trace_id_128bit, shared_span_context]( + tls_->set([this, collector, &random_generator, trace_id_128bit, shared_span_context]( Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { TracerPtr tracer(new Tracer(local_info_.clusterName(), local_info_.address(), random_generator, trace_id_128bit, shared_span_context, time_source_)); tracer->setReporter( - ReporterImpl::NewInstance(std::ref(*this), std::ref(dispatcher), collector_endpoint)); + ReporterImpl::NewInstance(std::ref(*this), std::ref(dispatcher), collector)); return ThreadLocal::ThreadLocalObjectSharedPtr{new TlsTracer(std::move(tracer), *this)}; }); } @@ -125,8 +129,8 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa } ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, - const std::string& collector_endpoint) - : driver_(driver), collector_endpoint_(collector_endpoint) { + const CollectorInfo& collector) + : driver_(driver), collector_(collector) { flush_timer_ = dispatcher.createTimer([this]() -> void { driver_.tracerStats().timer_flushed_.inc(); flushSpans(); @@ -141,8 +145,8 @@ ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, } ReporterPtr ReporterImpl::NewInstance(Driver& driver, Event::Dispatcher& dispatcher, - const std::string& collector_endpoint) { - return ReporterPtr(new ReporterImpl(driver, dispatcher, collector_endpoint)); + const CollectorInfo& collector) { + return ReporterPtr(new ReporterImpl(driver, dispatcher, collector)); } // TODO(fabolive): Need to avoid the copy to improve performance. @@ -167,16 +171,24 @@ void ReporterImpl::flushSpans() { if (span_buffer_.pendingSpans()) { driver_.tracerStats().spans_sent_.add(span_buffer_.pendingSpans()); - const std::string request_body = span_buffer_.toStringifiedJsonArray(); Http::MessagePtr message(new Http::RequestMessageImpl()); message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); - message->headers().insertPath().value(collector_endpoint_); + message->headers().insertPath().value(collector_.endpoint); message->headers().insertHost().value(driver_.cluster()->name()); - message->headers().insertContentType().value().setReference( - Http::Headers::get().ContentTypeValues.Json); + + std::string payload; + if (collector_.version == envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO) { + message->headers().insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Protobuf); + span_buffer_.toProtoListOfSpans().SerializeToString(&payload); + } else { + message->headers().insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Json); + payload = span_buffer_.toStringifiedJsonArray(); + } Buffer::InstancePtr body(new Buffer::OwnedImpl()); - body->add(request_body); + body->add(payload); message->body() = std::move(body); const uint64_t timeout = diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h index daa0bb70a1e9..ad3a07b04b41 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h @@ -137,10 +137,23 @@ class Driver : public Tracing::Driver { TimeSource& time_source_; }; +/** + * Information about the Zipkin collector. + */ +struct CollectorInfo { + // The Zipkin collector endpoint/path to receive the collected metrics. e.g. /api/v1/spans. + std::string endpoint{ZipkinCoreConstants::get().DEFAULT_COLLECTOR_ENDPOINT}; + + // The version of the collector. This is related to endpoint's supported payload specification and + // transport. + envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion version{ + envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1}; +}; + /** * This class derives from the abstract Zipkin::Reporter. - * It buffers spans and relies on Http::AsyncClient to send spans to - * Zipkin using JSON over HTTP. + * It buffers spans and relies on Http::AsyncClient to send spans to Zipkin collector depends on the + * chosen collector endpoint version. By default it sends JSON over HTTP. * * Two runtime parameters control the span buffering/flushing behavior, namely: * tracing.zipkin.min_flush_spans and tracing.zipkin.flush_interval_ms. @@ -158,12 +171,11 @@ class ReporterImpl : public Reporter, Http::AsyncClient::Callbacks { * * @param driver ZipkinDriver to be associated with the reporter. * @param dispatcher Controls the timer used to flush buffered spans. - * @param collector_endpoint String representing the Zipkin endpoint to be used + * @param CollectorInfo represents the Zipkin endpoint information to be used. * when making HTTP POST requests carrying spans. This value comes from the * Zipkin-related tracing configuration. */ - ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, - const std::string& collector_endpoint); + ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, const CollectorInfo& collector); /** * Implementation of Zipkin::Reporter::reportSpan(). @@ -191,7 +203,7 @@ class ReporterImpl : public Reporter, Http::AsyncClient::Callbacks { * @return Pointer to the newly-created ZipkinReporter. */ static ReporterPtr NewInstance(Driver& driver, Event::Dispatcher& dispatcher, - const std::string& collector_endpoint); + const CollectorInfo& collector); private: /** @@ -207,7 +219,7 @@ class ReporterImpl : public Reporter, Http::AsyncClient::Callbacks { Driver& driver_; Event::TimerPtr flush_timer_; SpanBuffer span_buffer_; - const std::string collector_endpoint_; + const CollectorInfo collector_; }; } // namespace Zipkin } // namespace Tracers From ac13b27e6b26f095705ef1ee92364b5ee8e07c94 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 16 May 2019 16:47:58 +0700 Subject: [PATCH 02/48] Generate ids Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/util.cc | 14 ++++++++++++++ source/extensions/tracers/zipkin/util.h | 7 +++++++ .../tracers/zipkin/zipkin_core_types.cc | 15 +++++++++------ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/zipkin/util.cc b/source/extensions/tracers/zipkin/util.cc index d18eff673b04..8253f29c7724 100644 --- a/source/extensions/tracers/zipkin/util.cc +++ b/source/extensions/tracers/zipkin/util.cc @@ -55,6 +55,20 @@ uint64_t Util::generateRandom64(TimeSource& time_source) { return rand_64(); } +std::string Util::bytesOf(uint64_t value) { + std::string data; + data.reserve(8); + data.push_back(value & 0x00000000000000FF); + data.push_back((value & 0x000000000000FF00) >> 8); + data.push_back((value & 0x0000000000FF0000) >> 16); + data.push_back((value & 0x00000000FF000000) >> 24); + data.push_back((value & 0x000000FF00000000) >> 32); + data.push_back((value & 0x0000FF0000000000) >> 40); + data.push_back((value & 0x00FF000000000000) >> 48); + data.push_back((value & 0xFF00000000000000) >> 56); + return data; +} + } // namespace Zipkin } // namespace Tracers } // namespace Extensions diff --git a/source/extensions/tracers/zipkin/util.h b/source/extensions/tracers/zipkin/util.h index ce86f73080e9..b37a520a9ea0 100644 --- a/source/extensions/tracers/zipkin/util.h +++ b/source/extensions/tracers/zipkin/util.h @@ -48,6 +48,13 @@ class Util { * Returns a randomly-generated 64-bit integer number. */ static uint64_t generateRandom64(TimeSource& time_source); + + /** + * Returns byte string representation of an number. + * @param value Number that will be represented in byte string. + * @return std::string byte string representation of a number. + */ + static std::string bytesOf(uint64_t value); }; } // namespace Zipkin diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index 896fed805a77..b76a25cc4eba 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -240,15 +240,17 @@ const std::string Span::toJson() { const zipkin::proto3::Span Span::toProtoSpan() const { zipkin::proto3::Span span; - // TODO(dio): pack ids. - span.set_id("id"); - span.set_trace_id("traceid"); - span.set_name(name_); + span.set_trace_id(trace_id_high_.has_value() + ? Util::bytesOf(trace_id_high_.value()) + Util::bytesOf(trace_id_) + : Util::bytesOf(trace_id_)); - if (parent_id_ && parent_id_.has_value()) { - span.set_parent_id("parentid"); + if (parent_id_ && parent_id_.value()) { + span.set_parent_id(Util::bytesOf(parent_id_.value())); } + span.set_id(Util::bytesOf(id_)); + span.set_name(name_); + if (timestamp_) { span.set_timestamp(timestamp_.value()); } @@ -262,6 +264,7 @@ const zipkin::proto3::Span Span::toProtoSpan() const { if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { span.mutable_local_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { + span.set_kind(zipkin::proto3::Span::SERVER); span.mutable_remote_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); } } From 7fdbeb9892bf18ff9aa4a5251c320b37bd2cfc85 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 16 May 2019 16:50:26 +0700 Subject: [PATCH 03/48] Add newline Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/util.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/tracers/zipkin/util.h b/source/extensions/tracers/zipkin/util.h index b37a520a9ea0..74e3f488d6d5 100644 --- a/source/extensions/tracers/zipkin/util.h +++ b/source/extensions/tracers/zipkin/util.h @@ -51,6 +51,7 @@ class Util { /** * Returns byte string representation of an number. + * * @param value Number that will be represented in byte string. * @return std::string byte string representation of a number. */ From 07dfb1d912fdbd1c6f5763170e7d0889d8f121e7 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 May 2019 05:07:18 +0700 Subject: [PATCH 04/48] Add test for endpoint proto Signed-off-by: Dhi Aurrahman --- .../tracers/zipkin/zipkin_core_types.cc | 2 ++ test/extensions/tracers/zipkin/BUILD | 1 + test/extensions/tracers/zipkin/config_test.cc | 1 + .../tracers/zipkin/zipkin_core_types_test.cc | 34 +++++++++++++++++-- 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index b76a25cc4eba..411ae55781ef 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -68,7 +68,9 @@ const zipkin::proto3::Endpoint Endpoint::toProtoEndpoint() const { } else { endpoint.set_ipv6(address_->ip()->addressAsString()); } + endpoint.set_port(address_->ip()->port()); } + endpoint.set_service_name(service_name_); return endpoint; } diff --git a/test/extensions/tracers/zipkin/BUILD b/test/extensions/tracers/zipkin/BUILD index 9f98e8c3ba15..a481ea737220 100644 --- a/test/extensions/tracers/zipkin/BUILD +++ b/test/extensions/tracers/zipkin/BUILD @@ -31,6 +31,7 @@ envoy_extension_cc_test( "//source/common/common:utility_lib", "//source/common/network:address_lib", "//source/common/network:utility_lib", + "//source/common/protobuf:utility_lib", "//source/common/runtime:runtime_lib", "//source/extensions/tracers/zipkin:zipkin_lib", "//test/mocks:common_lib", diff --git a/test/extensions/tracers/zipkin/config_test.cc b/test/extensions/tracers/zipkin/config_test.cc index 313c0c18286d..f10a8713d50a 100644 --- a/test/extensions/tracers/zipkin/config_test.cc +++ b/test/extensions/tracers/zipkin/config_test.cc @@ -47,6 +47,7 @@ TEST(ZipkinTracerConfigTest, ZipkinHttpTracerWithTypedConfig) { "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig collector_cluster: fake_cluster collector_endpoint: /api/v1/spans + collector_endpoint_version: HTTP-PROTO )EOF"; envoy::config::trace::v2::Tracing configuration; diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index 62f7b9b840c1..025d74ee8dbb 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -1,6 +1,8 @@ #include "common/common/utility.h" #include "common/network/address_impl.h" #include "common/network/utility.h" +#include "common/protobuf/utility.h" +#include "common/common/base64.h" #include "extensions/tracers/zipkin/zipkin_core_constants.h" #include "extensions/tracers/zipkin/zipkin_core_types.h" @@ -15,11 +17,32 @@ namespace Tracers { namespace Zipkin { namespace { +void expectValidEndpointProto(const Endpoint& endpoint) { + const auto addr = endpoint.address(); + const auto version = + addr ? (addr->ip()->version() == Network::Address::IpVersion::v4 ? "ipv4" : "ipv6") : "ipv4"; + const auto ip = addr ? Base64::encode(addr->ip()->addressAsString().c_str(), + addr->ip()->addressAsString().size(), false) + : ""; + const auto port = addr ? addr->ip()->port() : 0; + const std::string expected_yaml = fmt::format( + R"EOF( +{}: {} +port: {} +serviceName: {} +)EOF", + version, ip, port, endpoint.serviceName()); + zipkin::proto3::Endpoint expected_msg; + MessageUtil::loadFromYaml(expected_yaml, expected_msg); + EXPECT_EQ(endpoint.toProtoEndpoint().DebugString(), expected_msg.DebugString()); +} + TEST(ZipkinCoreTypesEndpointTest, defaultConstructor) { Endpoint ep; EXPECT_EQ("", ep.serviceName()); EXPECT_EQ(R"({"ipv4":"","port":0,"serviceName":""})", ep.toJson()); + expectValidEndpointProto(ep); Network::Address::InstanceConstSharedPtr addr = Network::Utility::parseInternetAddress("127.0.0.1"); @@ -34,8 +57,10 @@ TEST(ZipkinCoreTypesEndpointTest, defaultConstructor) { ep.setServiceName("my_service"); EXPECT_EQ("my_service", ep.serviceName()); - EXPECT_EQ(R"({"ipv6":"2001:db8:85a3::8a2e:370:4444","port":7334,"serviceName":"my_service"})", - ep.toJson()); + const std::string expected_json = + R"({"ipv6":"2001:db8:85a3::8a2e:370:4444","port":7334,"serviceName":"my_service"})"; + EXPECT_EQ(expected_json, ep.toJson()); + expectValidEndpointProto(ep); } TEST(ZipkinCoreTypesEndpointTest, customConstructor) { @@ -52,6 +77,7 @@ TEST(ZipkinCoreTypesEndpointTest, customConstructor) { EXPECT_EQ(R"({"ipv6":"2001:db8:85a3::8a2e:370:4444","port":7334,"serviceName":"my_service"})", ep.toJson()); + expectValidEndpointProto(ep); } TEST(ZipkinCoreTypesEndpointTest, copyOperator) { @@ -65,6 +91,8 @@ TEST(ZipkinCoreTypesEndpointTest, copyOperator) { EXPECT_EQ(ep1.serviceName(), ep2.serviceName()); EXPECT_EQ(ep1.toJson(), ep2.toJson()); + expectValidEndpointProto(ep1); + expectValidEndpointProto(ep2); } TEST(ZipkinCoreTypesEndpointTest, assignmentOperator) { @@ -78,6 +106,8 @@ TEST(ZipkinCoreTypesEndpointTest, assignmentOperator) { EXPECT_EQ(ep1.serviceName(), ep2.serviceName()); EXPECT_EQ(ep1.toJson(), ep2.toJson()); + expectValidEndpointProto(ep1); + expectValidEndpointProto(ep2); } TEST(ZipkinCoreTypesAnnotationTest, defaultConstructor) { From 07c897bd739e2222ad01766c78d157efe9c22fc9 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 May 2019 22:28:25 +0700 Subject: [PATCH 05/48] Add test for zipkin_core_types Signed-off-by: Dhi Aurrahman --- api/bazel/repository_locations.bzl | 6 +- .../tracers/zipkin/zipkin_core_types.cc | 16 +-- .../tracers/zipkin/zipkin_core_types.h | 21 ++++ .../tracers/zipkin/zipkin_tracer_impl.cc | 1 + .../tracers/zipkin/zipkin_core_types_test.cc | 100 +++++++++++++++++- 5 files changed, 129 insertions(+), 15 deletions(-) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 4d9f0bbe47b2..151f370e721b 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -18,8 +18,8 @@ PROMETHEUS_SHA = "783bdaf8ee0464b35ec0c8704871e1e72afa0005c3f3587f65d9d6694bf391 KAFKA_SOURCE_SHA = "ae7a1696c0a0302b43c5b21e515c37e6ecd365941f68a510a7e442eebddf39a1" # 2.2.0-rc2 -ZIPKINAPI_RELEASE = "0.2.0" -ZIPKINAPI_SHA256 = "cc34d57c51a52e8b9ea8ea5ca51e25016531eabc6c47be9b11d8fc29a5d266ae" +ZIPKINAPI_RELEASE = "0.2.1" +ZIPKINAPI_SHA256 = "98a62b36493f189cf47eb2c07c541212d09b3b497f66f698ac7bc5b6305d742a" REPOSITORY_LOCATIONS = dict( bazel_skylib = dict( @@ -60,6 +60,6 @@ REPOSITORY_LOCATIONS = dict( com_github_apache_zipkinapi = dict( sha256 = ZIPKINAPI_SHA256, strip_prefix = "incubator-zipkin-api-" + ZIPKINAPI_RELEASE, - urls = ["https://github.com/apache/incubator-zipkin-api/archive/" + ZIPKINAPI_RELEASE + ".tar.gz"], + urls = ["https://github.com/apache/incubator-zipkin-api/archive/v" + ZIPKINAPI_RELEASE + ".tar.gz"], ), ) diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index 411ae55781ef..b3eb508c96b2 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -241,16 +241,9 @@ const std::string Span::toJson() { const zipkin::proto3::Span Span::toProtoSpan() const { zipkin::proto3::Span span; - - span.set_trace_id(trace_id_high_.has_value() - ? Util::bytesOf(trace_id_high_.value()) + Util::bytesOf(trace_id_) - : Util::bytesOf(trace_id_)); - - if (parent_id_ && parent_id_.value()) { - span.set_parent_id(Util::bytesOf(parent_id_.value())); - } - - span.set_id(Util::bytesOf(id_)); + span.set_trace_id(traceIdAsByteString()); + span.set_parent_id(parentIdAsByteString()); + span.set_id(idAsByteString()); span.set_name(name_); if (timestamp_) { @@ -264,6 +257,7 @@ const zipkin::proto3::Span Span::toProtoSpan() const { for (const auto& annotation : annotations_) { if (annotation.isSetEndpoint()) { if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { + span.set_kind(zipkin::proto3::Span::CLIENT); span.mutable_local_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { span.set_kind(zipkin::proto3::Span::SERVER); @@ -272,8 +266,8 @@ const zipkin::proto3::Span Span::toProtoSpan() const { } } + auto& tags = *span.mutable_tags(); for (const auto& binary_annotation : binary_annotations_) { - auto& tags = *span.mutable_tags(); tags[binary_annotation.key()] = binary_annotation.value(); } diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index ca2b16ed531b..c9e0a49a3ec7 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -451,6 +451,11 @@ class Span : public ZipkinBase { */ const std::string idAsHexString() const { return Hex::uint64ToHex(id_); } + /** + * @return the span's id as a byte string. + */ + const std::string idAsByteString() const { return Util::bytesOf(id_); } + /** * @return the span's name. */ @@ -468,6 +473,13 @@ class Span : public ZipkinBase { return parent_id_ ? Hex::uint64ToHex(parent_id_.value()) : EMPTY_HEX_STRING_; } + /** + * @return the span's parent id as a byte string. + */ + const std::string parentIdAsByteString() const { + return parent_id_ ? Util::bytesOf(parent_id_.value()) : EMPTY_HEX_STRING_; + } + /** * @return whether or not the debug attribute is set */ @@ -502,6 +514,15 @@ class Span : public ZipkinBase { : Hex::uint64ToHex(trace_id_); } + /** + * @return the span's trace id as a byte string. + */ + const std::string traceIdAsByteString() const { + return trace_id_high_.has_value() + ? Util::bytesOf(trace_id_high_.value()) + Util::bytesOf(trace_id_) + : Util::bytesOf(trace_id_); + } + /** * @return the span's start time (monotonic, used to calculate duration). */ diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index 99f377a3ae86..ddb61a35c758 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -180,6 +180,7 @@ void ReporterImpl::flushSpans() { if (collector_.version == envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO) { message->headers().insertContentType().value().setReference( Http::Headers::get().ContentTypeValues.Protobuf); + // TODO(dio): use grpc message serializer so no round trip to std::string is required. span_buffer_.toProtoListOfSpans().SerializeToString(&payload); } else { message->headers().insertContentType().value().setReference( diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index 025d74ee8dbb..d1fd56678f94 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -1,8 +1,8 @@ +#include "common/common/base64.h" #include "common/common/utility.h" #include "common/network/address_impl.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" -#include "common/common/base64.h" #include "extensions/tracers/zipkin/zipkin_core_constants.h" #include "extensions/tracers/zipkin/zipkin_core_types.h" @@ -339,6 +339,20 @@ TEST(ZipkinCoreTypesSpanTest, defaultConstructor) { R"("annotations":[],"binaryAnnotations":[]})", span.toJson()); + std::string expected_yaml = fmt::format( + R"EOF( +traceId: {} +parentId: {} +id: {} +)EOF", + Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), + Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), + Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size())); + + zipkin::proto3::Span expected_msg; + MessageUtil::loadFromYaml(expected_yaml, expected_msg); + EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); + uint64_t id = Util::generateRandom64(test_time.timeSystem()); std::string id_hex = Hex::uint64ToHex(id); span.setId(id); @@ -427,6 +441,30 @@ TEST(ZipkinCoreTypesSpanTest, defaultConstructor) { R"({"ipv4":"192.168.1.2","port":3306,"serviceName":"my_service_name"}}]})", span.toJson()); + expected_yaml = fmt::format( + R"EOF( +traceId: {} +parentId: {} +id: {} +kind: CLIENT +name: span_name +timestamp: {} +duration: {} +localEndpoint: + serviceName: my_service_name + ipv4: {} + port: 3306 +tags: + lc: my_component_name +)EOF", + Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), + Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), + Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), + /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); + + MessageUtil::loadFromYaml(expected_yaml, expected_msg); + EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); + // Test the copy-semantics flavor of addAnnotation and addBinaryAnnotation ann.setValue(Zipkin::ZipkinCoreConstants::get().SERVER_SEND); @@ -478,6 +516,36 @@ TEST(ZipkinCoreTypesSpanTest, defaultConstructor) { R"("serviceName":"my_service_name"}}]})", span.toJson()); + expected_yaml = fmt::format( + R"EOF( +traceId: {} +parentId: {} +id: {} +kind: SERVER +name: span_name +timestamp: {} +duration: {} +localEndpoint: + serviceName: my_service_name + ipv4: {} + port: 3306 +remoteEndpoint: + serviceName: my_service_name + ipv4: {} + port: 3306 +tags: + "http.return_code": "400" + lc: my_component_name +)EOF", + Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), + Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), + Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), + /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11), + /* remoteEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); + + MessageUtil::loadFromYaml(expected_yaml, expected_msg); + EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); + // Test setSourceServiceName and setDestinationServiceName ann.setValue(Zipkin::ZipkinCoreConstants::get().CLIENT_RECV); // NOLINT(bugprone-use-after-move) @@ -515,6 +583,36 @@ TEST(ZipkinCoreTypesSpanTest, defaultConstructor) { R"("endpoint":{"ipv4":"192.168.1.2","port":3306,)" R"("serviceName":"my_service_name"}}]})", span.toJson()); + + expected_yaml = fmt::format( + R"EOF( +traceId: {} +parentId: {} +id: {} +kind: SERVER +name: span_name +timestamp: {} +duration: {} +localEndpoint: + serviceName: NEW_SERVICE_NAME + ipv4: {} + port: 3306 +remoteEndpoint: + serviceName: NEW_SERVICE_NAME + ipv4: {} + port: 3306 +tags: + "http.return_code": "400" + lc: my_component_name +)EOF", + Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), + Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), + Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), + /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11), + /* remoteEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); + + MessageUtil::loadFromYaml(expected_yaml, expected_msg); + EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); } TEST(ZipkinCoreTypesSpanTest, copyConstructor) { From 9aafd95f4470569a3aa421271f08e37d51b5f9cf Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 May 2019 23:50:36 +0700 Subject: [PATCH 06/48] Add span buffer test Signed-off-by: Dhi Aurrahman --- .../extensions/tracers/zipkin/span_buffer.h | 5 + .../tracers/zipkin/span_buffer_test.cc | 33 ++++ .../tracers/zipkin/zipkin_tracer_impl_test.cc | 151 ++++++++++-------- 3 files changed, 120 insertions(+), 69 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 16d0481af617..8a5b1e677dd4 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -66,6 +66,11 @@ class SpanBuffer { */ const zipkin::proto3::ListOfSpans toProtoListOfSpans() const; + /** + * @return the current buffered spans. + */ + std::vector spans() const { return span_buffer_; } + private: // We use a pre-allocated vector to improve performance std::vector span_buffer_; diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 320b6a909bb0..33fb445851d7 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -1,3 +1,6 @@ +#include "common/common/base64.h" +#include "common/protobuf/utility.h" + #include "extensions/tracers/zipkin/span_buffer.h" #include "test/test_common/test_time.h" @@ -10,13 +13,34 @@ namespace Tracers { namespace Zipkin { namespace { +void expectValidBufferedProtoListOfSpans(const SpanBuffer& buffer) { + std::string expected_yaml = "spans:"; + for (const auto& span : buffer.spans()) { + const std::string expected_span_yaml = fmt::format( + R"EOF( +- traceId: {} + parentId: {} + id: {} +)EOF", + Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), + Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), + Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size())); + expected_yaml += expected_span_yaml; + } + zipkin::proto3::ListOfSpans expected_msg; + MessageUtil::loadFromYaml(expected_yaml, expected_msg); + EXPECT_EQ(buffer.toProtoListOfSpans().DebugString(), expected_msg.DebugString()); +} + TEST(ZipkinSpanBufferTest, defaultConstructorEndToEnd) { DangerousDeprecatedTestTime test_time; SpanBuffer buffer; EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); + EXPECT_EQ(buffer.toProtoListOfSpans().DebugString(), ""); EXPECT_FALSE(buffer.addSpan(Span(test_time.timeSystem()))); + expectValidBufferedProtoListOfSpans(buffer); buffer.allocateBuffer(2); EXPECT_EQ(0ULL, buffer.pendingSpans()); @@ -32,10 +56,12 @@ TEST(ZipkinSpanBufferTest, defaultConstructorEndToEnd) { R"("binaryAnnotations":[])" "}]"; EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); buffer.addSpan(Span(test_time.timeSystem())); buffer.addSpan(Span(test_time.timeSystem())); @@ -57,10 +83,12 @@ TEST(ZipkinSpanBufferTest, defaultConstructorEndToEnd) { "]"; EXPECT_EQ(2ULL, buffer.pendingSpans()); EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); } TEST(ZipkinSpanBufferTest, sizeConstructorEndtoEnd) { @@ -69,6 +97,7 @@ TEST(ZipkinSpanBufferTest, sizeConstructorEndtoEnd) { EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); buffer.addSpan(Span(test_time.timeSystem())); EXPECT_EQ(1ULL, buffer.pendingSpans()); @@ -80,10 +109,12 @@ TEST(ZipkinSpanBufferTest, sizeConstructorEndtoEnd) { R"("binaryAnnotations":[])" "}]"; EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); buffer.addSpan(Span(test_time.timeSystem())); buffer.addSpan(Span(test_time.timeSystem())); @@ -104,10 +135,12 @@ TEST(ZipkinSpanBufferTest, sizeConstructorEndtoEnd) { "}]"; EXPECT_EQ(2ULL, buffer.pendingSpans()); EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); + expectValidBufferedProtoListOfSpans(buffer); } } // namespace diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index c918f3817207..681abe92c9c2 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -55,19 +55,71 @@ class ZipkinDriverTest : public testing::Test { random_, time_source_); } - void setupValidDriver() { + void setupValidDriver(const std::string& version) { EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); - const std::string yaml_string = R"EOF( + const std::string yaml_string = fmt::format(R"EOF( collector_cluster: fake_cluster collector_endpoint: /api/v1/spans - )EOF"; + collector_endpoint_version: {} + )EOF", + version); envoy::config::trace::v2::ZipkinConfig zipkin_config; MessageUtil::loadFromYaml(yaml_string, zipkin_config); setup(zipkin_config, true); } + void expectValidFlushSeveralSpans(const std::string& version, const std::string& content_type) { + setupValidDriver(version); + + Http::MockAsyncClientRequest request(&cm_.async_client_); + Http::AsyncClient::Callbacks* callback; + const absl::optional timeout(std::chrono::seconds(5)); + + EXPECT_CALL(cm_.async_client_, + send_(_, _, Http::AsyncClient::RequestOptions().setTimeout(timeout))) + .WillOnce( + Invoke([&](Http::MessagePtr& message, Http::AsyncClient::Callbacks& callbacks, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callback = &callbacks; + + EXPECT_EQ("/api/v1/spans", message->headers().Path()->value().getStringView()); + EXPECT_EQ("fake_cluster", message->headers().Host()->value().getStringView()); + EXPECT_EQ(content_type, message->headers().ContentType()->value().getStringView()); + + return &request; + })); + + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.zipkin.min_flush_spans", 5)) + .Times(2) + .WillRepeatedly(Return(2)); + EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.zipkin.request_timeout", 5000U)) + .WillOnce(Return(5000U)); + + Tracing::SpanPtr first_span = driver_->startSpan( + config_, request_headers_, operation_name_, start_time_, {Tracing::Reason::Sampling, true}); + first_span->finishSpan(); + + Tracing::SpanPtr second_span = driver_->startSpan( + config_, request_headers_, operation_name_, start_time_, {Tracing::Reason::Sampling, true}); + second_span->finishSpan(); + + Http::MessagePtr msg(new Http::ResponseMessageImpl( + Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "202"}}})); + + callback->onSuccess(std::move(msg)); + + EXPECT_EQ(2U, stats_.counter("tracing.zipkin.spans_sent").value()); + EXPECT_EQ(1U, stats_.counter("tracing.zipkin.reports_sent").value()); + EXPECT_EQ(0U, stats_.counter("tracing.zipkin.reports_dropped").value()); + EXPECT_EQ(0U, stats_.counter("tracing.zipkin.reports_failed").value()); + + callback->onFailure(Http::AsyncClient::FailureReason::Reset); + + EXPECT_EQ(1U, stats_.counter("tracing.zipkin.reports_failed").value()); + } + // TODO(#4160): Currently time_system_ is initialized from DangerousDeprecatedTestTime, which uses // real time, not mock-time. When that is switched to use mock-time instead, I think // generateRandom64() may not be as random as we want, and we'll need to inject entropy @@ -132,58 +184,19 @@ TEST_F(ZipkinDriverTest, InitializeDriver) { } TEST_F(ZipkinDriverTest, FlushSeveralSpans) { - setupValidDriver(); - - Http::MockAsyncClientRequest request(&cm_.async_client_); - Http::AsyncClient::Callbacks* callback; - const absl::optional timeout(std::chrono::seconds(5)); - - EXPECT_CALL(cm_.async_client_, - send_(_, _, Http::AsyncClient::RequestOptions().setTimeout(timeout))) - .WillOnce( - Invoke([&](Http::MessagePtr& message, Http::AsyncClient::Callbacks& callbacks, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - callback = &callbacks; - - EXPECT_EQ("/api/v1/spans", message->headers().Path()->value().getStringView()); - EXPECT_EQ("fake_cluster", message->headers().Host()->value().getStringView()); - EXPECT_EQ("application/json", - message->headers().ContentType()->value().getStringView()); - - return &request; - })); - - EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.zipkin.min_flush_spans", 5)) - .Times(2) - .WillRepeatedly(Return(2)); - EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.zipkin.request_timeout", 5000U)) - .WillOnce(Return(5000U)); - - Tracing::SpanPtr first_span = driver_->startSpan(config_, request_headers_, operation_name_, - start_time_, {Tracing::Reason::Sampling, true}); - first_span->finishSpan(); - - Tracing::SpanPtr second_span = driver_->startSpan(config_, request_headers_, operation_name_, - start_time_, {Tracing::Reason::Sampling, true}); - second_span->finishSpan(); - - Http::MessagePtr msg(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "202"}}})); - - callback->onSuccess(std::move(msg)); - - EXPECT_EQ(2U, stats_.counter("tracing.zipkin.spans_sent").value()); - EXPECT_EQ(1U, stats_.counter("tracing.zipkin.reports_sent").value()); - EXPECT_EQ(0U, stats_.counter("tracing.zipkin.reports_dropped").value()); - EXPECT_EQ(0U, stats_.counter("tracing.zipkin.reports_failed").value()); + expectValidFlushSeveralSpans("DEFAULT", "application/json"); +} - callback->onFailure(Http::AsyncClient::FailureReason::Reset); +TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpJsonV1) { + expectValidFlushSeveralSpans("HTTP_JSON_V1", "application/json"); +} - EXPECT_EQ(1U, stats_.counter("tracing.zipkin.reports_failed").value()); +TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpProto) { + expectValidFlushSeveralSpans("HTTP_PROTO", "application/x-protobuf"); } TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { - setupValidDriver(); + setupValidDriver("DEFAULT"); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; @@ -225,7 +238,7 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { } TEST_F(ZipkinDriverTest, FlushSpansTimer) { - setupValidDriver(); + setupValidDriver("DEFAULT"); const absl::optional timeout(std::chrono::seconds(5)); EXPECT_CALL(cm_.async_client_, @@ -252,7 +265,7 @@ TEST_F(ZipkinDriverTest, FlushSpansTimer) { } TEST_F(ZipkinDriverTest, NoB3ContextSampledTrue) { - setupValidDriver(); + setupValidDriver("DEFAULT"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -266,7 +279,7 @@ TEST_F(ZipkinDriverTest, NoB3ContextSampledTrue) { } TEST_F(ZipkinDriverTest, NoB3ContextSampledFalse) { - setupValidDriver(); + setupValidDriver("DEFAULT"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -280,7 +293,7 @@ TEST_F(ZipkinDriverTest, NoB3ContextSampledFalse) { } TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleTrue) { - setupValidDriver(); + setupValidDriver("DEFAULT"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -296,7 +309,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleTrue) { } TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleFalse) { - setupValidDriver(); + setupValidDriver("DEFAULT"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -312,7 +325,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleFalse) { } TEST_F(ZipkinDriverTest, PropagateB3NotSampled) { - setupValidDriver(); + setupValidDriver("DEFAULT"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -334,7 +347,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NotSampled) { } TEST_F(ZipkinDriverTest, PropagateB3NotSampledWithFalse) { - setupValidDriver(); + setupValidDriver("DEFAULT"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -356,7 +369,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NotSampledWithFalse) { } TEST_F(ZipkinDriverTest, PropagateB3SampledWithTrue) { - setupValidDriver(); + setupValidDriver("DEFAULT"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -378,7 +391,7 @@ TEST_F(ZipkinDriverTest, PropagateB3SampledWithTrue) { } TEST_F(ZipkinDriverTest, PropagateB3SampleFalse) { - setupValidDriver(); + setupValidDriver("DEFAULT"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -395,7 +408,7 @@ TEST_F(ZipkinDriverTest, PropagateB3SampleFalse) { } TEST_F(ZipkinDriverTest, ZipkinSpanTest) { - setupValidDriver(); + setupValidDriver("DEFAULT"); // ==== // Test effective setTag() @@ -475,7 +488,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) { - setupValidDriver(); + setupValidDriver("DEFAULT"); const std::string trace_id = Hex::uint64ToHex(generateRandom64()); const std::string span_id = Hex::uint64ToHex(generateRandom64()); @@ -499,7 +512,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersEmptyParentSpanTest) { - setupValidDriver(); + setupValidDriver("DEFAULT"); // Root span so have same trace and span id const std::string id = Hex::uint64ToHex(generateRandom64()); @@ -520,7 +533,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersEmptyParentSpanTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3Headers128TraceIdTest) { - setupValidDriver(); + setupValidDriver("DEFAULT"); const uint64_t trace_id_high = generateRandom64(); const uint64_t trace_id_low = generateRandom64(); @@ -548,7 +561,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3Headers128TraceIdTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { - setupValidDriver(); + setupValidDriver("DEFAULT"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, std::string("xyz")); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_SPAN_ID, @@ -562,7 +575,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { - setupValidDriver(); + setupValidDriver("DEFAULT"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -576,7 +589,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { - setupValidDriver(); + setupValidDriver("DEFAULT"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -591,7 +604,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ExplicitlySetSampledFalse) { - setupValidDriver(); + setupValidDriver("DEFAULT"); Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_, {Tracing::Reason::Sampling, true}); @@ -608,7 +621,7 @@ TEST_F(ZipkinDriverTest, ExplicitlySetSampledFalse) { } TEST_F(ZipkinDriverTest, ExplicitlySetSampledTrue) { - setupValidDriver(); + setupValidDriver("DEFAULT"); Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_, {Tracing::Reason::Sampling, false}); @@ -625,7 +638,7 @@ TEST_F(ZipkinDriverTest, ExplicitlySetSampledTrue) { } TEST_F(ZipkinDriverTest, DuplicatedHeader) { - setupValidDriver(); + setupValidDriver("DEFAULT"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_SPAN_ID, From 8685f632b7457e96d7acdf953e07c34b134f26cf Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 17 May 2019 23:55:25 +0700 Subject: [PATCH 07/48] Fix Signed-off-by: Dhi Aurrahman --- api/envoy/config/trace/v2/trace.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 26c7a49f9d99..1a39732a5ad0 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -88,7 +88,7 @@ message ZipkinConfig { HTTP_PROTO = 2; } - // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1``` will be + // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be // used. CollectorEndpointVersion collector_endpoint_version = 6; } From 4c5810084f8f4dac30ed015f0989205dff2e1966 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sat, 18 May 2019 00:09:58 +0700 Subject: [PATCH 08/48] Fix Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/zipkin_core_types.cc | 1 - source/extensions/tracers/zipkin/zipkin_core_types.h | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index b3eb508c96b2..9288437c984d 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -1,7 +1,6 @@ #include "extensions/tracers/zipkin/zipkin_core_types.h" #include "common/common/byte_order.h" -#include "common/common/empty_string.h" #include "common/common/utility.h" #include "extensions/tracers/zipkin/span_context.h" diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index c9e0a49a3ec7..341fe069faa5 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -6,6 +6,7 @@ #include "envoy/common/time.h" #include "envoy/network/address.h" +#include "common/common/empty_string.h" #include "common/common/hex.h" #include "extensions/tracers/zipkin/tracer_interface.h" @@ -477,7 +478,7 @@ class Span : public ZipkinBase { * @return the span's parent id as a byte string. */ const std::string parentIdAsByteString() const { - return parent_id_ ? Util::bytesOf(parent_id_.value()) : EMPTY_HEX_STRING_; + return parent_id_ ? Util::bytesOf(parent_id_.value()) : EMPTY_STRING; } /** From c6f86c87c83dcf44a6a792016c221bdae89d4cb8 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sat, 18 May 2019 06:26:30 +0700 Subject: [PATCH 09/48] Rename bytesOf to toByteString Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/util.cc | 14 -------------- source/extensions/tracers/zipkin/util.h | 8 +++++++- .../extensions/tracers/zipkin/zipkin_core_types.h | 12 +++++++----- 3 files changed, 14 insertions(+), 20 deletions(-) diff --git a/source/extensions/tracers/zipkin/util.cc b/source/extensions/tracers/zipkin/util.cc index 8253f29c7724..d18eff673b04 100644 --- a/source/extensions/tracers/zipkin/util.cc +++ b/source/extensions/tracers/zipkin/util.cc @@ -55,20 +55,6 @@ uint64_t Util::generateRandom64(TimeSource& time_source) { return rand_64(); } -std::string Util::bytesOf(uint64_t value) { - std::string data; - data.reserve(8); - data.push_back(value & 0x00000000000000FF); - data.push_back((value & 0x000000000000FF00) >> 8); - data.push_back((value & 0x0000000000FF0000) >> 16); - data.push_back((value & 0x00000000FF000000) >> 24); - data.push_back((value & 0x000000FF00000000) >> 32); - data.push_back((value & 0x0000FF0000000000) >> 40); - data.push_back((value & 0x00FF000000000000) >> 48); - data.push_back((value & 0xFF00000000000000) >> 56); - return data; -} - } // namespace Zipkin } // namespace Tracers } // namespace Extensions diff --git a/source/extensions/tracers/zipkin/util.h b/source/extensions/tracers/zipkin/util.h index 74e3f488d6d5..11c2c4324351 100644 --- a/source/extensions/tracers/zipkin/util.h +++ b/source/extensions/tracers/zipkin/util.h @@ -5,6 +5,8 @@ #include "envoy/common/time.h" +#include "common/common/byte_order.h" + namespace Envoy { namespace Extensions { namespace Tracers { @@ -53,9 +55,13 @@ class Util { * Returns byte string representation of an number. * * @param value Number that will be represented in byte string. + * @param flip indicates to flip order or not. * @return std::string byte string representation of a number. */ - static std::string bytesOf(uint64_t value); + template static std::string toByteString(Type value, bool flip = false) { + auto bytes = flip ? toEndianness(value) : value; + return std::string(reinterpret_cast(&bytes), sizeof(Type)); + } }; } // namespace Zipkin diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index 341fe069faa5..909b51eed659 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -455,7 +455,7 @@ class Span : public ZipkinBase { /** * @return the span's id as a byte string. */ - const std::string idAsByteString() const { return Util::bytesOf(id_); } + const std::string idAsByteString() const { return Util::toByteString(id_); } /** * @return the span's name. @@ -478,7 +478,7 @@ class Span : public ZipkinBase { * @return the span's parent id as a byte string. */ const std::string parentIdAsByteString() const { - return parent_id_ ? Util::bytesOf(parent_id_.value()) : EMPTY_STRING; + return parent_id_ ? Util::toByteString(parent_id_.value()) : EMPTY_STRING; } /** @@ -519,9 +519,11 @@ class Span : public ZipkinBase { * @return the span's trace id as a byte string. */ const std::string traceIdAsByteString() const { - return trace_id_high_.has_value() - ? Util::bytesOf(trace_id_high_.value()) + Util::bytesOf(trace_id_) - : Util::bytesOf(trace_id_); + // TODO(dio): Make sure this is the right interpretation of + // https://github.com/apache/incubator-zipkin-api/blob/v0.2.1/zipkin.proto#L60-L61. + return trace_id_high_.has_value() ? Util::toByteString(trace_id_high_.value(), true) + + Util::toByteString(trace_id_, true) + : Util::toByteString(trace_id_, true); } /** From c775be0a907aa057039e797cc4bc5fe4450a1377 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sat, 18 May 2019 07:11:26 +0700 Subject: [PATCH 10/48] Add changelog Signed-off-by: Dhi Aurrahman --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index e1ac8a33084c..d1dc7b74d00b 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -32,6 +32,7 @@ Version history `. * server: ``--define manual_stamp=manual_stamp`` was added to allow server stamping outside of binary rules. more info in the `bazel docs `_. +* tracing: added support to the Zipkin reporter for sending list of spans as protobuf message over HTTP. * upstream: added :ref:`upstream_cx_pool_overflow ` for the connection pool circuit breaker. * upstream: an EDS management server can now force removal of a host that is still passing active health checking by first marking the host as failed via EDS health check and subsequently removing From d60b37f06337272b4f2deb90fb348691f7671d85 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 27 May 2019 11:39:31 +0700 Subject: [PATCH 11/48] Review comments Signed-off-by: Dhi Aurrahman --- .../tracers/zipkin/zipkin_core_types.cc | 17 +++++++++-------- .../tracers/zipkin/zipkin_core_types.h | 5 +++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index 9288437c984d..ac51df482e80 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -58,10 +58,7 @@ const std::string Endpoint::toJson() { const zipkin::proto3::Endpoint Endpoint::toProtoEndpoint() const { zipkin::proto3::Endpoint endpoint; - if (!address_) { - endpoint.set_ipv4(EMPTY_STRING); - endpoint.set_port(0); - } else { + if (address_) { if (address_->ip()->version() == Network::Address::IpVersion::v4) { endpoint.set_ipv4(address_->ip()->addressAsString()); } else { @@ -70,7 +67,10 @@ const zipkin::proto3::Endpoint Endpoint::toProtoEndpoint() const { endpoint.set_port(address_->ip()->port()); } - endpoint.set_service_name(service_name_); + if (!service_name_.empty()) { + endpoint.set_service_name(service_name_); + } + return endpoint; } @@ -241,7 +241,9 @@ const std::string Span::toJson() { const zipkin::proto3::Span Span::toProtoSpan() const { zipkin::proto3::Span span; span.set_trace_id(traceIdAsByteString()); - span.set_parent_id(parentIdAsByteString()); + if (parent_id_ && parent_id_.value()) { + span.set_parent_id(parentIdAsByteString()); + } span.set_id(idAsByteString()); span.set_name(name_); @@ -257,11 +259,10 @@ const zipkin::proto3::Span Span::toProtoSpan() const { if (annotation.isSetEndpoint()) { if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { span.set_kind(zipkin::proto3::Span::CLIENT); - span.mutable_local_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { span.set_kind(zipkin::proto3::Span::SERVER); - span.mutable_remote_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); } + span.mutable_local_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); } } diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index 909b51eed659..0ebdb91bc697 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -6,8 +6,8 @@ #include "envoy/common/time.h" #include "envoy/network/address.h" -#include "common/common/empty_string.h" #include "common/common/hex.h" +#include "common/common/assert.h" #include "extensions/tracers/zipkin/tracer_interface.h" #include "extensions/tracers/zipkin/util.h" @@ -478,7 +478,8 @@ class Span : public ZipkinBase { * @return the span's parent id as a byte string. */ const std::string parentIdAsByteString() const { - return parent_id_ ? Util::toByteString(parent_id_.value()) : EMPTY_STRING; + ASSERT(parent_id_); + return Util::toByteString(parent_id_.value()); } /** From d03f5f4a821800bb164ebbf3c7f4367b33f5bbf5 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 27 May 2019 12:50:19 +0700 Subject: [PATCH 12/48] Fix Signed-off-by: Dhi Aurrahman --- .../tracers/zipkin/zipkin_core_types.cc | 2 +- .../tracers/zipkin/span_buffer_test.cc | 4 +++- .../tracers/zipkin/zipkin_core_types_test.cc | 19 +++++-------------- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index ac51df482e80..995912413343 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -36,7 +36,7 @@ const std::string Endpoint::toJson() { writer.String(""); writer.Key(ZipkinJsonFieldNames::get().ENDPOINT_PORT.c_str()); writer.Uint(0); - } else { + } if (address_) { if (address_->ip()->version() == Network::Address::IpVersion::v4) { // IPv4 writer.Key(ZipkinJsonFieldNames::get().ENDPOINT_IPV4.c_str()); diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 33fb445851d7..4f3995cad2ab 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -23,7 +23,9 @@ void expectValidBufferedProtoListOfSpans(const SpanBuffer& buffer) { id: {} )EOF", Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), - Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), + span.isSetParentId() ? Base64::encode(span.parentIdAsByteString().c_str(), + span.parentIdAsByteString().size()) + : "", Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size())); expected_yaml += expected_span_yaml; } diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index d1fd56678f94..ca6992a58c03 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -327,7 +327,6 @@ TEST(ZipkinCoreTypesSpanTest, defaultConstructor) { EXPECT_EQ(0ULL, span.annotations().size()); EXPECT_EQ(0ULL, span.binaryAnnotations().size()); EXPECT_EQ("0000000000000000", span.idAsHexString()); - EXPECT_EQ("0000000000000000", span.parentIdAsHexString()); EXPECT_EQ("0000000000000000", span.traceIdAsHexString()); EXPECT_EQ(0LL, span.startTime()); EXPECT_FALSE(span.debug()); @@ -346,7 +345,9 @@ parentId: {} id: {} )EOF", Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), - Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), + span.isSetParentId() + ? Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()) + : "", Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size())); zipkin::proto3::Span expected_msg; @@ -529,10 +530,6 @@ duration: {} serviceName: my_service_name ipv4: {} port: 3306 -remoteEndpoint: - serviceName: my_service_name - ipv4: {} - port: 3306 tags: "http.return_code": "400" lc: my_component_name @@ -540,8 +537,7 @@ duration: {} Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), - /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11), - /* remoteEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); + /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); MessageUtil::loadFromYaml(expected_yaml, expected_msg); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); @@ -597,10 +593,6 @@ duration: {} serviceName: NEW_SERVICE_NAME ipv4: {} port: 3306 -remoteEndpoint: - serviceName: NEW_SERVICE_NAME - ipv4: {} - port: 3306 tags: "http.return_code": "400" lc: my_component_name @@ -608,8 +600,7 @@ duration: {} Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), - /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11), - /* remoteEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); + /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); MessageUtil::loadFromYaml(expected_yaml, expected_msg); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); From a75668bcb55fa4b5c6ea99213b3a4e0a701e1a11 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 27 May 2019 12:57:22 +0700 Subject: [PATCH 13/48] Fix format Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/zipkin_core_types.cc | 3 ++- source/extensions/tracers/zipkin/zipkin_core_types.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index 995912413343..6b7e46f616c9 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -36,7 +36,8 @@ const std::string Endpoint::toJson() { writer.String(""); writer.Key(ZipkinJsonFieldNames::get().ENDPOINT_PORT.c_str()); writer.Uint(0); - } if (address_) { + } + if (address_) { if (address_->ip()->version() == Network::Address::IpVersion::v4) { // IPv4 writer.Key(ZipkinJsonFieldNames::get().ENDPOINT_IPV4.c_str()); diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index 0ebdb91bc697..0c51fa58eced 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -6,8 +6,8 @@ #include "envoy/common/time.h" #include "envoy/network/address.h" -#include "common/common/hex.h" #include "common/common/assert.h" +#include "common/common/hex.h" #include "extensions/tracers/zipkin/tracer_interface.h" #include "extensions/tracers/zipkin/util.h" From 53d0dedc20f0a810ae3b477646056a9651e94e6a Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 16 Aug 2019 01:56:50 +0700 Subject: [PATCH 14/48] Fix tests Signed-off-by: Dhi Aurrahman --- test/extensions/tracers/zipkin/span_buffer_test.cc | 3 ++- .../tracers/zipkin/zipkin_core_types_test.cc | 12 +++++++----- .../tracers/zipkin/zipkin_tracer_impl_test.cc | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 4f3995cad2ab..f2c85492320d 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -1,4 +1,5 @@ #include "common/common/base64.h" +#include "common/protobuf/message_validator_impl.h" #include "common/protobuf/utility.h" #include "extensions/tracers/zipkin/span_buffer.h" @@ -30,7 +31,7 @@ void expectValidBufferedProtoListOfSpans(const SpanBuffer& buffer) { expected_yaml += expected_span_yaml; } zipkin::proto3::ListOfSpans expected_msg; - MessageUtil::loadFromYaml(expected_yaml, expected_msg); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(buffer.toProtoListOfSpans().DebugString(), expected_msg.DebugString()); } diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index ca6992a58c03..95b4826d7582 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -2,6 +2,7 @@ #include "common/common/utility.h" #include "common/network/address_impl.h" #include "common/network/utility.h" +#include "common/protobuf/message_validator_impl.h" #include "common/protobuf/utility.h" #include "extensions/tracers/zipkin/zipkin_core_constants.h" @@ -33,7 +34,8 @@ serviceName: {} )EOF", version, ip, port, endpoint.serviceName()); zipkin::proto3::Endpoint expected_msg; - MessageUtil::loadFromYaml(expected_yaml, expected_msg); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, + ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(endpoint.toProtoEndpoint().DebugString(), expected_msg.DebugString()); } @@ -351,7 +353,7 @@ id: {} Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size())); zipkin::proto3::Span expected_msg; - MessageUtil::loadFromYaml(expected_yaml, expected_msg); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); uint64_t id = Util::generateRandom64(test_time.timeSystem()); @@ -463,7 +465,7 @@ duration: {} Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - MessageUtil::loadFromYaml(expected_yaml, expected_msg); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); // Test the copy-semantics flavor of addAnnotation and addBinaryAnnotation @@ -539,7 +541,7 @@ duration: {} Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - MessageUtil::loadFromYaml(expected_yaml, expected_msg); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); // Test setSourceServiceName and setDestinationServiceName @@ -602,7 +604,7 @@ duration: {} Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - MessageUtil::loadFromYaml(expected_yaml, expected_msg); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); } diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index bccc493f5f3e..0b63b61e3945 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -57,7 +57,7 @@ class ZipkinDriverTest : public testing::Test { } void setupValidDriver(const std::string& version) { - EXPECT_CALL(cm_, get("fake_cluster")).WillRepeatedly(Return(&cm_.thread_local_cluster_)); + EXPECT_CALL(cm_, get(Eq("fake_cluster"))).WillRepeatedly(Return(&cm_.thread_local_cluster_)); const std::string yaml_string = fmt::format(R"EOF( collector_cluster: fake_cluster From 12015fef4014d2d1c7259364507b55efb222a865 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 16 Aug 2019 01:57:42 +0700 Subject: [PATCH 15/48] Fix format Signed-off-by: Dhi Aurrahman --- test/extensions/tracers/zipkin/span_buffer_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index f2c85492320d..35e452703178 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -31,7 +31,8 @@ void expectValidBufferedProtoListOfSpans(const SpanBuffer& buffer) { expected_yaml += expected_span_yaml; } zipkin::proto3::ListOfSpans expected_msg; - MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, + ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(buffer.toProtoListOfSpans().DebugString(), expected_msg.DebugString()); } From 4c7542f8f2b8d4579a1e5d6d4c24011ae3601305 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 16 Aug 2019 13:02:15 +0700 Subject: [PATCH 16/48] Fix format Signed-off-by: Dhi Aurrahman --- .../tracers/zipkin/zipkin_core_types_test.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index 95b4826d7582..798baada923f 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -353,7 +353,8 @@ id: {} Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size())); zipkin::proto3::Span expected_msg; - MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, + ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); uint64_t id = Util::generateRandom64(test_time.timeSystem()); @@ -465,7 +466,8 @@ duration: {} Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, + ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); // Test the copy-semantics flavor of addAnnotation and addBinaryAnnotation @@ -541,7 +543,8 @@ duration: {} Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, + ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); // Test setSourceServiceName and setDestinationServiceName @@ -604,7 +607,8 @@ duration: {} Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - MessageUtil::loadFromYaml(expected_yaml, expected_msg, ProtobufMessage::getStrictValidationVisitor()); + MessageUtil::loadFromYaml(expected_yaml, expected_msg, + ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); } From f7601d0343e94f03eb4b6c72b7b2a684f9f7ad62 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 16 Aug 2019 13:37:47 +0700 Subject: [PATCH 17/48] apache -> openzipkin Signed-off-by: Dhi Aurrahman --- api/bazel/repositories.bzl | 2 +- api/bazel/repository_locations.bzl | 4 ++-- source/extensions/tracers/zipkin/BUILD | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index f9cfe99574a0..b5b191c24233 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -35,7 +35,7 @@ def api_dependencies(): build_file_content = KAFKASOURCE_BUILD_CONTENT, ) envoy_http_archive( - name = "com_github_apache_zipkinapi", + name = "com_github_openzipkin_zipkinapi", locations = REPOSITORY_LOCATIONS, build_file_content = ZIPKINAPI_BUILD_CONTENT, ) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 84cb0f848d66..72a47df50c3b 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -57,9 +57,9 @@ REPOSITORY_LOCATIONS = dict( strip_prefix = "kafka-2.2.0-rc2/clients/src/main/resources/common/message", urls = ["https://github.com/apache/kafka/archive/2.2.0-rc2.zip"], ), - com_github_apache_zipkinapi = dict( + com_github_openzipkin_zipkinapi = dict( sha256 = ZIPKINAPI_SHA256, strip_prefix = "incubator-zipkin-api-" + ZIPKINAPI_RELEASE, - urls = ["https://github.com/apache/incubator-zipkin-api/archive/v" + ZIPKINAPI_RELEASE + ".tar.gz"], + urls = ["https://github.com/openzipkin/zipkin-api/archive/v" + ZIPKINAPI_RELEASE + ".tar.gz"], ), ) diff --git a/source/extensions/tracers/zipkin/BUILD b/source/extensions/tracers/zipkin/BUILD index 34a32b87fab3..652496900174 100644 --- a/source/extensions/tracers/zipkin/BUILD +++ b/source/extensions/tracers/zipkin/BUILD @@ -57,7 +57,7 @@ envoy_cc_library( "//source/common/singleton:const_singleton", "//source/common/tracing:http_tracer_lib", "//source/extensions/tracers:well_known_names", - "@com_github_apache_zipkinapi//:zipkin_cc", + "@com_github_openzipkin_zipkinapi//:zipkin_cc", ], ) From 2a2746ada92020f23d52c1892f0349d2e9efce75 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 16 Aug 2019 13:53:11 +0700 Subject: [PATCH 18/48] Fix sha256 Signed-off-by: Dhi Aurrahman --- api/bazel/repository_locations.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 72a47df50c3b..47f112aafa72 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -19,7 +19,7 @@ PROMETHEUS_SHA = "783bdaf8ee0464b35ec0c8704871e1e72afa0005c3f3587f65d9d6694bf391 KAFKA_SOURCE_SHA = "ae7a1696c0a0302b43c5b21e515c37e6ecd365941f68a510a7e442eebddf39a1" # 2.2.0-rc2 ZIPKINAPI_RELEASE = "0.2.1" -ZIPKINAPI_SHA256 = "98a62b36493f189cf47eb2c07c541212d09b3b497f66f698ac7bc5b6305d742a" +ZIPKINAPI_SHA256 = "91f5b69951951c12fa8eb784a2d57ef25e5206b5da8e1db46466ccadbc9d036e" REPOSITORY_LOCATIONS = dict( bazel_skylib = dict( @@ -59,7 +59,7 @@ REPOSITORY_LOCATIONS = dict( ), com_github_openzipkin_zipkinapi = dict( sha256 = ZIPKINAPI_SHA256, - strip_prefix = "incubator-zipkin-api-" + ZIPKINAPI_RELEASE, + strip_prefix = "zipkin-api-" + ZIPKINAPI_RELEASE, urls = ["https://github.com/openzipkin/zipkin-api/archive/v" + ZIPKINAPI_RELEASE + ".tar.gz"], ), ) From e4cf785dcc4ac917a38d918f75cbba92c40442f2 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sat, 17 Aug 2019 12:39:53 +0700 Subject: [PATCH 19/48] Fix format Signed-off-by: Dhi Aurrahman --- api/bazel/repositories.bzl | 1 + api/bazel/repository_locations.bzl | 7 +-- api/envoy/config/trace/v2/trace.proto | 16 +++++- .../extensions/tracers/zipkin/span_buffer.cc | 20 +++++++ .../extensions/tracers/zipkin/span_buffer.h | 6 +++ .../tracers/zipkin/zipkin_core_types.cc | 53 +++++++++++++++++++ .../tracers/zipkin/zipkin_core_types.h | 10 ++++ .../tracers/zipkin/zipkin_tracer_impl.cc | 6 +++ .../tracers/zipkin/zipkin_core_types_test.cc | 4 ++ 9 files changed, 118 insertions(+), 5 deletions(-) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index b5b191c24233..a82cae27ac74 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -161,6 +161,7 @@ load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") api_proto_library( name = "zipkin", srcs = [ + "zipkin-jsonv2.proto", "zipkin.proto", ], visibility = ["//visibility:public"], diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 47f112aafa72..711c20ebd38a 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -19,7 +19,8 @@ PROMETHEUS_SHA = "783bdaf8ee0464b35ec0c8704871e1e72afa0005c3f3587f65d9d6694bf391 KAFKA_SOURCE_SHA = "ae7a1696c0a0302b43c5b21e515c37e6ecd365941f68a510a7e442eebddf39a1" # 2.2.0-rc2 ZIPKINAPI_RELEASE = "0.2.1" -ZIPKINAPI_SHA256 = "91f5b69951951c12fa8eb784a2d57ef25e5206b5da8e1db46466ccadbc9d036e" +ZIPKINAPI_GIT_SHA = "821bc013ecc730f4c76f33c50bd5e8f4b6fa8446" +ZIPKINAPI_SHA256 = "f4ee5f8f87d95393fd7e4440ff8c0dbe49405cf16cab8c34eec37106551c3e46" REPOSITORY_LOCATIONS = dict( bazel_skylib = dict( @@ -59,7 +60,7 @@ REPOSITORY_LOCATIONS = dict( ), com_github_openzipkin_zipkinapi = dict( sha256 = ZIPKINAPI_SHA256, - strip_prefix = "zipkin-api-" + ZIPKINAPI_RELEASE, - urls = ["https://github.com/openzipkin/zipkin-api/archive/v" + ZIPKINAPI_RELEASE + ".tar.gz"], + strip_prefix = "zipkin-api-" + ZIPKINAPI_GIT_SHA, + urls = ["https://github.com/openzipkin/zipkin-api/archive/" + ZIPKINAPI_GIT_SHA + ".tar.gz"], ), ) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 0622b681597c..0079648b8327 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -84,11 +84,23 @@ message ZipkinConfig { // The default value is true. google.protobuf.BoolValue shared_span_context = 4; - // Available zipkin collector endpoint versions. + // Available Zipkin collector endpoint versions. enum CollectorEndpointVersion { + // Currently this falbacks to HTTP_JSON_V1. DEFAULT = 0; + + // Zipkin API v1, JSON over HTTP. HTTP_JSON_V1 = 1; - HTTP_PROTO = 2; + + // Zipkin API v2, JSON over HTTP. + HTTP_JSON_V2 = 2; + + // Zipkin API v2, protobuf over HTTP. + HTTP_PROTO = 3; + + // Zipkin API v2, GRPC API. + // [#not-implemented-hide:] + GRPC = 4; } // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index 2f2bc5cd40ff..f8b6bd52e203 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -1,5 +1,7 @@ #include "extensions/tracers/zipkin/span_buffer.h" +#include "common/protobuf/protobuf.h" + namespace Envoy { namespace Extensions { namespace Tracers { @@ -32,6 +34,24 @@ std::string SpanBuffer::toStringifiedJsonArray() { return stringified_json_array; } +const std::string SpanBuffer::toJsonListOfSpans() const { + std::string stringified_json_array = "["; + if (pendingSpans()) { + std::string payload; + Protobuf::util::MessageToJsonString(span_buffer_[0].toJsonSpan(), &payload); + stringified_json_array += payload; + const uint64_t size = span_buffer_.size(); + for (uint64_t i = 1; i < size; i++) { + stringified_json_array += ","; + Protobuf::util::MessageToJsonString(span_buffer_[i].toJsonSpan(), &payload); + stringified_json_array += payload; + } + } + stringified_json_array += "]"; + + return stringified_json_array; +} + const zipkin::proto3::ListOfSpans SpanBuffer::toProtoListOfSpans() const { zipkin::proto3::ListOfSpans spans; if (pendingSpans()) { diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 26d5ee5231d3..0634a3c4fc89 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -2,6 +2,7 @@ #include "extensions/tracers/zipkin/zipkin_core_types.h" +#include "zipkin-jsonv2.pb.h" #include "zipkin.pb.h" namespace Envoy { @@ -61,6 +62,11 @@ class SpanBuffer { */ std::string toStringifiedJsonArray(); + /** + * @return the contents of the buffer as a zipkin::proto3::ListOfSpans. + */ + const std::string toJsonListOfSpans() const; + /** * @return the contents of the buffer as a zipkin::proto3::ListOfSpans. */ diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index 513fd4ceeb19..51459807fc9f 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -57,6 +57,24 @@ const std::string Endpoint::toJson() { return json_string; } +const zipkin::jsonv2::Endpoint Endpoint::toJsonEndpoint() const { + zipkin::jsonv2::Endpoint endpoint; + if (address_) { + if (address_->ip()->version() == Network::Address::IpVersion::v4) { + endpoint.set_ipv4(address_->ip()->addressAsString()); + } else { + endpoint.set_ipv6(address_->ip()->addressAsString()); + } + endpoint.set_port(address_->ip()->port()); + } + + if (!service_name_.empty()) { + endpoint.set_service_name(service_name_); + } + + return endpoint; +} + const zipkin::proto3::Endpoint Endpoint::toProtoEndpoint() const { zipkin::proto3::Endpoint endpoint; if (address_) { @@ -239,6 +257,41 @@ const std::string Span::toJson() { return json_string; } +const zipkin::jsonv2::Span Span::toJsonSpan() const { + zipkin::jsonv2::Span span; + span.set_trace_id(traceIdAsHexString()); + if (parent_id_ && parent_id_.value()) { + span.set_parent_id(Hex::uint64ToHex(parent_id_.value())); + } + span.set_id(Hex::uint64ToHex(id_)); + span.set_name(name_); + + if (timestamp_) { + span.set_timestamp(timestamp_.value()); + } + + if (duration_) { + span.set_duration(duration_.value()); + } + + for (const auto& annotation : annotations_) { + if (annotation.isSetEndpoint()) { + if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { + span.set_kind("CLIENT"); + } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { + span.set_kind("SERVER"); + } + span.mutable_local_endpoint()->MergeFrom(annotation.endpoint().toJsonEndpoint()); + } + } + + auto& tags = *span.mutable_tags(); + for (const auto& binary_annotation : binary_annotations_) { + tags[binary_annotation.key()] = binary_annotation.value(); + } + return span; +} + const zipkin::proto3::Span Span::toProtoSpan() const { zipkin::proto3::Span span; span.set_trace_id(traceIdAsByteString()); diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index d9391fdf8e09..9d9c9243298b 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -14,6 +14,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "zipkin-jsonv2.pb.h" #include "zipkin.pb.h" namespace Envoy { @@ -103,6 +104,13 @@ class Endpoint : public ZipkinBase { */ const zipkin::proto3::Endpoint toProtoEndpoint() const; + /** + * Builds protobuf message representation of this endpoint. + * + * @return zipkin::jsonv2::Endpoint the protobuf message representation of this endpoint. + */ + const zipkin::jsonv2::Endpoint toJsonEndpoint() const; + private: std::string service_name_; Network::Address::InstanceConstSharedPtr address_; @@ -551,6 +559,8 @@ class Span : public ZipkinBase { */ const std::string toJson() override; + const zipkin::jsonv2::Span toJsonSpan() const; + /** * Builds protobuf message representation of this span. * diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index ddb61a35c758..c4141c07d39a 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -182,6 +182,11 @@ void ReporterImpl::flushSpans() { Http::Headers::get().ContentTypeValues.Protobuf); // TODO(dio): use grpc message serializer so no round trip to std::string is required. span_buffer_.toProtoListOfSpans().SerializeToString(&payload); + } else if (collector_.version == envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V2) { + message->headers().insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Json); + // TODO(dio): Handle the returned status. + payload = span_buffer_.toJsonListOfSpans(); } else { message->headers().insertContentType().value().setReference( Http::Headers::get().ContentTypeValues.Json); @@ -189,6 +194,7 @@ void ReporterImpl::flushSpans() { } Buffer::InstancePtr body(new Buffer::OwnedImpl()); + std::cerr << payload << "\n"; body->add(payload); message->body() = std::move(body); diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index 798baada923f..f5a978159fdc 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -470,6 +470,10 @@ duration: {} ProtobufMessage::getStrictValidationVisitor()); EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); + std::string json; + const auto status = Protobuf::util::MessageToJsonString(span.toJsonSpan(), &json); + std::cerr << json << "\n"; + // Test the copy-semantics flavor of addAnnotation and addBinaryAnnotation ann.setValue(Zipkin::ZipkinCoreConstants::get().SERVER_SEND); From 66ea6dd89388f8b9cf66437caffd29eb3fb41cb1 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 20 Aug 2019 23:34:14 +0700 Subject: [PATCH 20/48] Change approach to use serializer Signed-off-by: Dhi Aurrahman --- api/bazel/repositories.bzl | 11 +- api/bazel/repository_locations.bzl | 2 +- api/envoy/config/trace/v2/trace.proto | 9 +- .../extensions/tracers/zipkin/span_buffer.cc | 207 ++++++++++++++++-- .../extensions/tracers/zipkin/span_buffer.h | 67 ++++-- source/extensions/tracers/zipkin/tracer.h | 2 +- .../tracers/zipkin/tracer_interface.h | 21 ++ source/extensions/tracers/zipkin/util.h | 12 +- .../tracers/zipkin/zipkin_core_constants.h | 3 + .../tracers/zipkin/zipkin_core_types.cc | 114 +--------- .../tracers/zipkin/zipkin_core_types.h | 32 +-- .../tracers/zipkin/zipkin_tracer_impl.cc | 48 ++-- .../tracers/zipkin/zipkin_tracer_impl.h | 17 +- test/extensions/tracers/zipkin/config_test.cc | 4 +- .../tracers/zipkin/span_buffer_test.cc | 57 +---- test/extensions/tracers/zipkin/tracer_test.cc | 2 +- .../tracers/zipkin/zipkin_core_types_test.cc | 126 ----------- .../tracers/zipkin/zipkin_tracer_impl_test.cc | 46 ++-- 18 files changed, 365 insertions(+), 415 deletions(-) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index a82cae27ac74..c4d514d72724 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -156,8 +156,10 @@ filegroup( """ ZIPKINAPI_BUILD_CONTENT = """ -load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library") + +load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library", "api_go_proto_library") load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") + api_proto_library( name = "zipkin", srcs = [ @@ -166,10 +168,9 @@ api_proto_library( ], visibility = ["//visibility:public"], ) -go_proto_library( - name = "zipkin_go_proto", - importpath = "zipkin", + +api_go_proto_library( + name = "zipkin", proto = ":zipkin", - visibility = ["//visibility:public"], ) """ diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 711c20ebd38a..64f6f29e9c9b 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -18,7 +18,7 @@ PROMETHEUS_SHA = "783bdaf8ee0464b35ec0c8704871e1e72afa0005c3f3587f65d9d6694bf391 KAFKA_SOURCE_SHA = "ae7a1696c0a0302b43c5b21e515c37e6ecd365941f68a510a7e442eebddf39a1" # 2.2.0-rc2 -ZIPKINAPI_RELEASE = "0.2.1" +# ZIPKINAPI_RELEASE = "0.2.2" ZIPKINAPI_GIT_SHA = "821bc013ecc730f4c76f33c50bd5e8f4b6fa8446" ZIPKINAPI_SHA256 = "f4ee5f8f87d95393fd7e4440ff8c0dbe49405cf16cab8c34eec37106551c3e46" diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 0079648b8327..316d7e1ce229 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -86,26 +86,25 @@ message ZipkinConfig { // Available Zipkin collector endpoint versions. enum CollectorEndpointVersion { - // Currently this falbacks to HTTP_JSON_V1. - DEFAULT = 0; + // If not set, it fallbacks to ``HTTP_JSON_V1``. + NOT_SET = 0; // Zipkin API v1, JSON over HTTP. HTTP_JSON_V1 = 1; // Zipkin API v2, JSON over HTTP. - HTTP_JSON_V2 = 2; + HTTP_JSON = 2; // Zipkin API v2, protobuf over HTTP. HTTP_PROTO = 3; - // Zipkin API v2, GRPC API. // [#not-implemented-hide:] GRPC = 4; } // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be // used. - CollectorEndpointVersion collector_endpoint_version = 6; + CollectorEndpointVersion collector_endpoint_version = 5; } // DynamicOtConfig is used to dynamically load a tracer from a shared library diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index f8b6bd52e203..104e98aaf870 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -2,13 +2,28 @@ #include "common/protobuf/protobuf.h" +#include "extensions/tracers/zipkin/util.h" +#include "extensions/tracers/zipkin/zipkin_core_constants.h" + namespace Envoy { namespace Extensions { namespace Tracers { namespace Zipkin { +SpanBuffer::SpanBuffer( + const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, + const bool shared_span_context) + : serializer_{makeSerializer(version, shared_span_context)} {} + +SpanBuffer::SpanBuffer( + const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, + const bool shared_span_context, uint64_t size) + : serializer_{makeSerializer(version, shared_span_context)} { + allocateBuffer(size); +} + // TODO(fabolive): Need to avoid the copy to improve performance. -bool SpanBuffer::addSpan(const Span& span) { +bool SpanBuffer::addSpan(Span&& span) { if (span_buffer_.size() == span_buffer_.capacity()) { // Buffer full return false; @@ -34,17 +49,54 @@ std::string SpanBuffer::toStringifiedJsonArray() { return stringified_json_array; } -const std::string SpanBuffer::toJsonListOfSpans() const { +SerializerPtr SpanBuffer::makeSerializer( + const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, + const bool shared_span_context) { + switch (version) { + case envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1: + return std::make_unique(); + case envoy::config::trace::v2::ZipkinConfig::HTTP_JSON: + return std::make_unique(shared_span_context); + case envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO: + return std::make_unique(shared_span_context); + default: + // TODO(dio): Throw if it is required to be explicit when specifying version. + return std::make_unique(); + } +} + +std::string JsonV1Serializer::serialize(std::vector&& zipkin_spans) { std::string stringified_json_array = "["; - if (pendingSpans()) { - std::string payload; - Protobuf::util::MessageToJsonString(span_buffer_[0].toJsonSpan(), &payload); - stringified_json_array += payload; - const uint64_t size = span_buffer_.size(); + + if (!zipkin_spans.empty()) { + stringified_json_array += zipkin_spans[0].toJson(); + const uint64_t size = zipkin_spans.size(); for (uint64_t i = 1; i < size; i++) { stringified_json_array += ","; - Protobuf::util::MessageToJsonString(span_buffer_[i].toJsonSpan(), &payload); - stringified_json_array += payload; + stringified_json_array += zipkin_spans[i].toJson(); + } + } + stringified_json_array += "]"; + + return stringified_json_array; +} + +JsonV2Serializer::JsonV2Serializer(const bool shared_span_context) + : shared_span_context_{shared_span_context} {} + +std::string JsonV2Serializer::serialize(std::vector&& zipkin_spans) { + zipkin::jsonv2::ListOfSpans spans; + for (const Span& zipkin_span : zipkin_spans) { + spans.MergeFrom(toListOfSpans(zipkin_span)); + } + + std::string stringified_json_array = "["; + for (ssize_t i = 0; i < spans.spans_size(); i++) { + std::string entry; + Protobuf::util::MessageToJsonString(spans.spans()[i], &entry); + stringified_json_array += entry; + if (i != spans.spans_size() - 1) { + stringified_json_array += ","; } } stringified_json_array += "]"; @@ -52,17 +104,144 @@ const std::string SpanBuffer::toJsonListOfSpans() const { return stringified_json_array; } -const zipkin::proto3::ListOfSpans SpanBuffer::toProtoListOfSpans() const { +const zipkin::jsonv2::ListOfSpans JsonV2Serializer::toListOfSpans(const Span& zipkin_span) const { + zipkin::jsonv2::ListOfSpans spans; + for (const auto& annotation : zipkin_span.annotations()) { + zipkin::jsonv2::Span span; + + if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { + span.set_kind(ZipkinCoreConstants::get().KIND_CLIENT); + } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { + span.set_shared(shared_span_context_ && zipkin_span.annotations().size() > 1); + span.set_kind(ZipkinCoreConstants::get().KIND_SERVER); + } else { + continue; + } + + if (annotation.isSetEndpoint()) { + span.set_timestamp(annotation.timestamp()); + span.mutable_local_endpoint()->MergeFrom(toProtoEndpoint(annotation.endpoint())); + } + + span.set_trace_id(zipkin_span.traceIdAsHexString()); + if (zipkin_span.isSetParentId()) { + span.set_parent_id(zipkin_span.parentIdAsHexString()); + } + + span.set_id(zipkin_span.idAsHexString()); + span.set_name(zipkin_span.name()); + + if (zipkin_span.isSetDuration()) { + span.set_duration(zipkin_span.duration()); + } + + auto& tags = *span.mutable_tags(); + for (const auto& binary_annotation : zipkin_span.binaryAnnotations()) { + tags[binary_annotation.key()] = binary_annotation.value(); + } + + auto* mutable_span = spans.add_spans(); + mutable_span->MergeFrom(span); + } + return spans; +} + +const zipkin::jsonv2::Endpoint +JsonV2Serializer::toProtoEndpoint(const Endpoint& zipkin_endpoint) const { + zipkin::jsonv2::Endpoint endpoint; + Network::Address::InstanceConstSharedPtr address = zipkin_endpoint.address(); + if (address) { + if (address->ip()->version() == Network::Address::IpVersion::v4) { + endpoint.set_ipv4(address->ip()->addressAsString()); + } else { + endpoint.set_ipv6(address->ip()->addressAsString()); + } + endpoint.set_port(address->ip()->port()); + } + + const std::string& service_name = zipkin_endpoint.serviceName(); + if (!service_name.empty()) { + endpoint.set_service_name(service_name); + } + + return endpoint; +} + +ProtobufSerializer::ProtobufSerializer(const bool shared_span_context) + : shared_span_context_{shared_span_context} {} + +std::string ProtobufSerializer::serialize(std::vector&& zipkin_spans) { zipkin::proto3::ListOfSpans spans; - if (pendingSpans()) { - for (const auto& span : span_buffer_) { - auto* mutable_span = spans.add_spans(); - mutable_span->MergeFrom(span.toProtoSpan()); + for (const Span& zipkin_span : zipkin_spans) { + spans.MergeFrom(toListOfSpans(zipkin_span)); + } + std::string serialized; + spans.SerializeToString(&serialized); + return serialized; +} + +const zipkin::proto3::ListOfSpans ProtobufSerializer::toListOfSpans(const Span& zipkin_span) const { + zipkin::proto3::ListOfSpans spans; + for (const auto& annotation : zipkin_span.annotations()) { + zipkin::proto3::Span span; + if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { + span.set_kind(zipkin::proto3::Span::CLIENT); + } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { + span.set_shared(shared_span_context_ && zipkin_span.annotations().size() > 1); + span.set_kind(zipkin::proto3::Span::SERVER); + } else { + continue; } + + if (annotation.isSetEndpoint()) { + span.set_timestamp(annotation.timestamp()); + span.mutable_local_endpoint()->MergeFrom(toProtoEndpoint(annotation.endpoint())); + } + + span.set_trace_id(zipkin_span.traceIdAsByteString()); + if (zipkin_span.isSetParentId()) { + span.set_parent_id(zipkin_span.parentIdAsByteString()); + } + + span.set_id(zipkin_span.idAsByteString()); + span.set_name(zipkin_span.name()); + + if (zipkin_span.isSetDuration()) { + span.set_duration(zipkin_span.duration()); + } + + auto& tags = *span.mutable_tags(); + for (const auto& binary_annotation : zipkin_span.binaryAnnotations()) { + tags[binary_annotation.key()] = binary_annotation.value(); + } + + auto* mutable_span = spans.add_spans(); + mutable_span->MergeFrom(span); } return spans; } +const zipkin::proto3::Endpoint +ProtobufSerializer::toProtoEndpoint(const Endpoint& zipkin_endpoint) const { + zipkin::proto3::Endpoint endpoint; + Network::Address::InstanceConstSharedPtr address = zipkin_endpoint.address(); + if (address) { + if (address->ip()->version() == Network::Address::IpVersion::v4) { + endpoint.set_ipv4(Util::toByteString(address->ip()->ipv4()->address())); + } else { + endpoint.set_ipv6(Util::toByteString(address->ip()->ipv6()->address())); + } + endpoint.set_port(address->ip()->port()); + } + + const std::string& service_name = zipkin_endpoint.serviceName(); + if (!service_name.empty()) { + endpoint.set_service_name(service_name); + } + + return endpoint; +} + } // namespace Zipkin } // namespace Tracers } // namespace Extensions diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 0634a3c4fc89..19dd402865ea 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -1,5 +1,8 @@ #pragma once +#include "envoy/config/trace/v2/trace.pb.h" + +#include "extensions/tracers/zipkin/tracer_interface.h" #include "extensions/tracers/zipkin/zipkin_core_types.h" #include "zipkin-jsonv2.pb.h" @@ -20,14 +23,16 @@ class SpanBuffer { * Constructor that creates an empty buffer. Space needs to be allocated by invoking * the method allocateBuffer(size). */ - SpanBuffer() = default; + SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, + const bool shared_span_context); /** * Constructor that initializes a buffer with the given size. * * @param size The desired buffer size. */ - SpanBuffer(uint64_t size) { allocateBuffer(size); } + SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, + const bool shared_span_context, uint64_t size); /** * Allocates space for an empty buffer or resizes a previously-allocated one. @@ -43,7 +48,7 @@ class SpanBuffer { * * @return true if the span was successfully added, or false if the buffer was full. */ - bool addSpan(const Span& span); + bool addSpan(Span&& span); /** * Empties the buffer. This method is supposed to be called when all buffered spans @@ -54,7 +59,7 @@ class SpanBuffer { /** * @return the number of spans currently buffered. */ - uint64_t pendingSpans() const { return span_buffer_.size(); } + uint64_t pendingSpans() { return span_buffer_.size(); } /** * @return the contents of the buffer as a stringified array of JSONs, where @@ -63,23 +68,53 @@ class SpanBuffer { std::string toStringifiedJsonArray(); /** - * @return the contents of the buffer as a zipkin::proto3::ListOfSpans. - */ - const std::string toJsonListOfSpans() const; - - /** - * @return the contents of the buffer as a zipkin::proto3::ListOfSpans. - */ - const zipkin::proto3::ListOfSpans toProtoListOfSpans() const; - - /** - * @return the current buffered spans. + * @return std::string */ - std::vector spans() const { return span_buffer_; } + std::string serialize() { return serializer_->serialize(std::move(span_buffer_)); } private: + SerializerPtr + makeSerializer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, + const bool shared_span_context); + // We use a pre-allocated vector to improve performance std::vector span_buffer_; + SerializerPtr serializer_; +}; + +using SpanBufferPtr = std::unique_ptr; + +class JsonV1Serializer : public Serializer { +public: + JsonV1Serializer() = default; + + std::string serialize(std::vector&& pending_spans) override; +}; + +class JsonV2Serializer : public Serializer { +public: + JsonV2Serializer(const bool shared_span_context); + + std::string serialize(std::vector&& pending_spans) override; + +private: + const zipkin::jsonv2::ListOfSpans toListOfSpans(const Span& zipkin_span) const; + const zipkin::jsonv2::Endpoint toProtoEndpoint(const Endpoint& zipkin_endpoint) const; + + const bool shared_span_context_; +}; + +class ProtobufSerializer : public Serializer { +public: + ProtobufSerializer(const bool shared_span_context); + + std::string serialize(std::vector&& pending_spans) override; + +private: + const zipkin::proto3::ListOfSpans toListOfSpans(const Span& zipkin_span) const; + const zipkin::proto3::Endpoint toProtoEndpoint(const Endpoint& zipkin_endpoint) const; + + const bool shared_span_context_; }; } // namespace Zipkin diff --git a/source/extensions/tracers/zipkin/tracer.h b/source/extensions/tracers/zipkin/tracer.h index 190b68631b63..effbb4b8eddc 100644 --- a/source/extensions/tracers/zipkin/tracer.h +++ b/source/extensions/tracers/zipkin/tracer.h @@ -33,7 +33,7 @@ class Reporter { * * @param span The span that needs action. */ - virtual void reportSpan(const Span& span) PURE; + virtual void reportSpan(Span&& span) PURE; }; using ReporterPtr = std::unique_ptr; diff --git a/source/extensions/tracers/zipkin/tracer_interface.h b/source/extensions/tracers/zipkin/tracer_interface.h index 4c55ab90980b..1f31b5871660 100644 --- a/source/extensions/tracers/zipkin/tracer_interface.h +++ b/source/extensions/tracers/zipkin/tracer_interface.h @@ -1,5 +1,9 @@ #pragma once +#include +#include +#include + #include "envoy/common/pure.h" namespace Envoy { @@ -31,6 +35,23 @@ class TracerInterface { virtual void reportSpan(Span&& span) PURE; }; +/** + * Buffered pending spans serializer. + */ +class Serializer { +public: + virtual ~Serializer() = default; + + /** + * Serialize buffered pending spans. + * + * @return std::string serialized buffered pending spans. + */ + virtual std::string serialize(std::vector&& spans) PURE; +}; + +using SerializerPtr = std::unique_ptr; + } // namespace Zipkin } // namespace Tracers } // namespace Extensions diff --git a/source/extensions/tracers/zipkin/util.h b/source/extensions/tracers/zipkin/util.h index 11c2c4324351..df23a3925b6a 100644 --- a/source/extensions/tracers/zipkin/util.h +++ b/source/extensions/tracers/zipkin/util.h @@ -51,6 +51,16 @@ class Util { */ static uint64_t generateRandom64(TimeSource& time_source); + /** + * Returns byte string representation of an number. + * + * @param value Number that will be represented in byte string. + * @return std::string byte string representation of a number. + */ + template static std::string toByteString(Type value) { + return std::string(reinterpret_cast(&value), sizeof(Type)); + } + /** * Returns byte string representation of an number. * @@ -58,7 +68,7 @@ class Util { * @param flip indicates to flip order or not. * @return std::string byte string representation of a number. */ - template static std::string toByteString(Type value, bool flip = false) { + template static std::string toFlipableByteString(Type value, bool flip) { auto bytes = flip ? toEndianness(value) : value; return std::string(reinterpret_cast(&bytes), sizeof(Type)); } diff --git a/source/extensions/tracers/zipkin/zipkin_core_constants.h b/source/extensions/tracers/zipkin/zipkin_core_constants.h index 7384df786a19..0180aeb8f22c 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_constants.h +++ b/source/extensions/tracers/zipkin/zipkin_core_constants.h @@ -13,6 +13,9 @@ namespace Zipkin { class ZipkinCoreConstantValues { public: + const std::string KIND_CLIENT = "CLIENT"; + const std::string KIND_SERVER = "SERVER"; + const std::string CLIENT_SEND = "cs"; const std::string CLIENT_RECV = "cr"; const std::string SERVER_SEND = "ss"; diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.cc b/source/extensions/tracers/zipkin/zipkin_core_types.cc index 51459807fc9f..16c9d688a437 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.cc +++ b/source/extensions/tracers/zipkin/zipkin_core_types.cc @@ -1,6 +1,5 @@ #include "extensions/tracers/zipkin/zipkin_core_types.h" -#include "common/common/byte_order.h" #include "common/common/utility.h" #include "extensions/tracers/zipkin/span_context.h" @@ -36,8 +35,7 @@ const std::string Endpoint::toJson() { writer.String(""); writer.Key(ZipkinJsonFieldNames::get().ENDPOINT_PORT.c_str()); writer.Uint(0); - } - if (address_) { + } else { if (address_->ip()->version() == Network::Address::IpVersion::v4) { // IPv4 writer.Key(ZipkinJsonFieldNames::get().ENDPOINT_IPV4.c_str()); @@ -57,42 +55,6 @@ const std::string Endpoint::toJson() { return json_string; } -const zipkin::jsonv2::Endpoint Endpoint::toJsonEndpoint() const { - zipkin::jsonv2::Endpoint endpoint; - if (address_) { - if (address_->ip()->version() == Network::Address::IpVersion::v4) { - endpoint.set_ipv4(address_->ip()->addressAsString()); - } else { - endpoint.set_ipv6(address_->ip()->addressAsString()); - } - endpoint.set_port(address_->ip()->port()); - } - - if (!service_name_.empty()) { - endpoint.set_service_name(service_name_); - } - - return endpoint; -} - -const zipkin::proto3::Endpoint Endpoint::toProtoEndpoint() const { - zipkin::proto3::Endpoint endpoint; - if (address_) { - if (address_->ip()->version() == Network::Address::IpVersion::v4) { - endpoint.set_ipv4(address_->ip()->addressAsString()); - } else { - endpoint.set_ipv6(address_->ip()->addressAsString()); - } - endpoint.set_port(address_->ip()->port()); - } - - if (!service_name_.empty()) { - endpoint.set_service_name(service_name_); - } - - return endpoint; -} - Annotation::Annotation(const Annotation& ann) { timestamp_ = ann.timestamp(); value_ = ann.value(); @@ -257,77 +219,6 @@ const std::string Span::toJson() { return json_string; } -const zipkin::jsonv2::Span Span::toJsonSpan() const { - zipkin::jsonv2::Span span; - span.set_trace_id(traceIdAsHexString()); - if (parent_id_ && parent_id_.value()) { - span.set_parent_id(Hex::uint64ToHex(parent_id_.value())); - } - span.set_id(Hex::uint64ToHex(id_)); - span.set_name(name_); - - if (timestamp_) { - span.set_timestamp(timestamp_.value()); - } - - if (duration_) { - span.set_duration(duration_.value()); - } - - for (const auto& annotation : annotations_) { - if (annotation.isSetEndpoint()) { - if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { - span.set_kind("CLIENT"); - } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { - span.set_kind("SERVER"); - } - span.mutable_local_endpoint()->MergeFrom(annotation.endpoint().toJsonEndpoint()); - } - } - - auto& tags = *span.mutable_tags(); - for (const auto& binary_annotation : binary_annotations_) { - tags[binary_annotation.key()] = binary_annotation.value(); - } - return span; -} - -const zipkin::proto3::Span Span::toProtoSpan() const { - zipkin::proto3::Span span; - span.set_trace_id(traceIdAsByteString()); - if (parent_id_ && parent_id_.value()) { - span.set_parent_id(parentIdAsByteString()); - } - span.set_id(idAsByteString()); - span.set_name(name_); - - if (timestamp_) { - span.set_timestamp(timestamp_.value()); - } - - if (duration_) { - span.set_duration(duration_.value()); - } - - for (const auto& annotation : annotations_) { - if (annotation.isSetEndpoint()) { - if (annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND) { - span.set_kind(zipkin::proto3::Span::CLIENT); - } else if (annotation.value() == ZipkinCoreConstants::get().SERVER_RECV) { - span.set_kind(zipkin::proto3::Span::SERVER); - } - span.mutable_local_endpoint()->MergeFrom(annotation.endpoint().toProtoEndpoint()); - } - } - - auto& tags = *span.mutable_tags(); - for (const auto& binary_annotation : binary_annotations_) { - tags[binary_annotation.key()] = binary_annotation.value(); - } - - return span; -} - void Span::finish() { // Assumption: Span will have only one annotation when this method is called SpanContext context(*this); @@ -350,6 +241,9 @@ void Span::finish() { cr.setTimestamp(stop_timestamp); cr.setValue(ZipkinCoreConstants::get().CLIENT_RECV); annotations_.push_back(std::move(cr)); + } + + if (monotonic_start_time_) { const int64_t monotonic_stop_time = std::chrono::duration_cast( time_source_.monotonicTime().time_since_epoch()) .count(); diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index 9d9c9243298b..95cef6a4ecaf 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -14,8 +14,6 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" -#include "zipkin-jsonv2.pb.h" -#include "zipkin.pb.h" namespace Envoy { namespace Extensions { @@ -97,20 +95,6 @@ class Endpoint : public ZipkinBase { */ const std::string toJson() override; - /** - * Builds protobuf message representation of this endpoint. - * - * @return zipkin::proto3::Endpoint the protobuf message representation of this endpoint. - */ - const zipkin::proto3::Endpoint toProtoEndpoint() const; - - /** - * Builds protobuf message representation of this endpoint. - * - * @return zipkin::jsonv2::Endpoint the protobuf message representation of this endpoint. - */ - const zipkin::jsonv2::Endpoint toJsonEndpoint() const; - private: std::string service_name_; Network::Address::InstanceConstSharedPtr address_; @@ -528,11 +512,10 @@ class Span : public ZipkinBase { * @return the span's trace id as a byte string. */ const std::string traceIdAsByteString() const { - // TODO(dio): Make sure this is the right interpretation of // https://github.com/apache/incubator-zipkin-api/blob/v0.2.1/zipkin.proto#L60-L61. - return trace_id_high_.has_value() ? Util::toByteString(trace_id_high_.value(), true) + - Util::toByteString(trace_id_, true) - : Util::toByteString(trace_id_, true); + return trace_id_high_.has_value() ? Util::toFlipableByteString(trace_id_high_.value(), true) + + Util::toFlipableByteString(trace_id_, true) + : Util::toFlipableByteString(trace_id_, true); } /** @@ -559,15 +542,6 @@ class Span : public ZipkinBase { */ const std::string toJson() override; - const zipkin::jsonv2::Span toJsonSpan() const; - - /** - * Builds protobuf message representation of this span. - * - * @return zipkin::proto3::Span the protobuf message representation of this span. - */ - const zipkin::proto3::Span toProtoSpan() const; - /** * Associates a Tracer object with the span. The tracer's reportSpan() method is invoked * by the span's finish() method so that the tracer can decide what to do with the span diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index c4141c07d39a..c5bdeccf53c1 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -80,8 +80,9 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, if (!zipkin_config.collector_endpoint().empty()) { collector.endpoint = zipkin_config.collector_endpoint(); } + if (zipkin_config.collector_endpoint_version() != - envoy::config::trace::v2::ZipkinConfig::DEFAULT) { + envoy::config::trace::v2::ZipkinConfig::NOT_SET) { collector.version = zipkin_config.collector_endpoint_version(); } @@ -89,6 +90,7 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, const bool shared_span_context = PROTOBUF_GET_WRAPPED_OR_DEFAULT( zipkin_config, shared_span_context, ZipkinCoreConstants::get().DEFAULT_SHARED_SPAN_CONTEXT); + collector.shared_span_context = shared_span_context; tls_->set([this, collector, &random_generator, trace_id_128bit, shared_span_context]( Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { @@ -130,7 +132,9 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, const CollectorInfo& collector) - : driver_(driver), collector_(collector) { + : driver_(driver), + collector_(collector), span_buffer_{ + new SpanBuffer(collector.version, collector.shared_span_context)} { flush_timer_ = dispatcher.createTimer([this]() -> void { driver_.tracerStats().timer_flushed_.inc(); flushSpans(); @@ -139,7 +143,7 @@ ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, const uint64_t min_flush_spans = driver_.runtime().snapshot().getInteger("tracing.zipkin.min_flush_spans", 5U); - span_buffer_.allocateBuffer(min_flush_spans); + span_buffer_->allocateBuffer(min_flush_spans); enableTimer(); } @@ -150,13 +154,13 @@ ReporterPtr ReporterImpl::NewInstance(Driver& driver, Event::Dispatcher& dispatc } // TODO(fabolive): Need to avoid the copy to improve performance. -void ReporterImpl::reportSpan(const Span& span) { - span_buffer_.addSpan(span); +void ReporterImpl::reportSpan(Span&& span) { + span_buffer_->addSpan(std::move(span)); const uint64_t min_flush_spans = driver_.runtime().snapshot().getInteger("tracing.zipkin.min_flush_spans", 5U); - if (span_buffer_.pendingSpans() == min_flush_spans) { + if (span_buffer_->pendingSpans() == min_flush_spans) { flushSpans(); } } @@ -168,34 +172,20 @@ void ReporterImpl::enableTimer() { } void ReporterImpl::flushSpans() { - if (span_buffer_.pendingSpans()) { - driver_.tracerStats().spans_sent_.add(span_buffer_.pendingSpans()); - + if (span_buffer_->pendingSpans()) { + driver_.tracerStats().spans_sent_.add(span_buffer_->pendingSpans()); + const std::string request_body = span_buffer_->serialize(); Http::MessagePtr message(new Http::RequestMessageImpl()); message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); message->headers().insertPath().value(collector_.endpoint); message->headers().insertHost().value(driver_.cluster()->name()); - - std::string payload; - if (collector_.version == envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO) { - message->headers().insertContentType().value().setReference( - Http::Headers::get().ContentTypeValues.Protobuf); - // TODO(dio): use grpc message serializer so no round trip to std::string is required. - span_buffer_.toProtoListOfSpans().SerializeToString(&payload); - } else if (collector_.version == envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V2) { - message->headers().insertContentType().value().setReference( - Http::Headers::get().ContentTypeValues.Json); - // TODO(dio): Handle the returned status. - payload = span_buffer_.toJsonListOfSpans(); - } else { - message->headers().insertContentType().value().setReference( - Http::Headers::get().ContentTypeValues.Json); - payload = span_buffer_.toStringifiedJsonArray(); - } + message->headers().insertContentType().value().setReference( + collector_.version == envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO + ? Http::Headers::get().ContentTypeValues.Protobuf + : Http::Headers::get().ContentTypeValues.Json); Buffer::InstancePtr body(new Buffer::OwnedImpl()); - std::cerr << payload << "\n"; - body->add(payload); + body->add(request_body); message->body() = std::move(body); const uint64_t timeout = @@ -205,7 +195,7 @@ void ReporterImpl::flushSpans() { .send(std::move(message), *this, Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(timeout))); - span_buffer_.clear(); + span_buffer_->clear(); } } diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h index ad5063834c60..25be2b7dc2fd 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h @@ -141,19 +141,22 @@ class Driver : public Tracing::Driver { * Information about the Zipkin collector. */ struct CollectorInfo { - // The Zipkin collector endpoint/path to receive the collected metrics. e.g. /api/v1/spans. + // The Zipkin collector endpoint/path to receive the collected trace data. e.g. /api/v1/spans if + // HTTP_JSON_V1 or /api/v2/spans otherwise. std::string endpoint{ZipkinCoreConstants::get().DEFAULT_COLLECTOR_ENDPOINT}; // The version of the collector. This is related to endpoint's supported payload specification and // transport. envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion version{ envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1}; + + bool shared_span_context{true}; }; /** * This class derives from the abstract Zipkin::Reporter. - * It buffers spans and relies on Http::AsyncClient to send spans to Zipkin collector depends on the - * chosen collector endpoint version. By default it sends JSON over HTTP. + * It buffers spans and relies on Http::AsyncClient to send spans to + * Zipkin using JSON over HTTP. * * Two runtime parameters control the span buffering/flushing behavior, namely: * tracing.zipkin.min_flush_spans and tracing.zipkin.flush_interval_ms. @@ -171,7 +174,7 @@ class ReporterImpl : public Reporter, Http::AsyncClient::Callbacks { * * @param driver ZipkinDriver to be associated with the reporter. * @param dispatcher Controls the timer used to flush buffered spans. - * @param CollectorInfo represents the Zipkin endpoint information to be used. + * @param collector holds the endpoint version and path information. * when making HTTP POST requests carrying spans. This value comes from the * Zipkin-related tracing configuration. */ @@ -184,7 +187,7 @@ class ReporterImpl : public Reporter, Http::AsyncClient::Callbacks { * * @param span The span to be buffered. */ - void reportSpan(const Span& span) override; + void reportSpan(Span&& span) override; // Http::AsyncClient::Callbacks. // The callbacks below record Zipkin-span-related stats. @@ -196,7 +199,7 @@ class ReporterImpl : public Reporter, Http::AsyncClient::Callbacks { * * @param driver ZipkinDriver to be associated with the reporter. * @param dispatcher Controls the timer used to flush buffered spans. - * @param collector_endpoint String representing the Zipkin endpoint to be used + * @param collector holds the endpoint version and path information. * when making HTTP POST requests carrying spans. This value comes from the * Zipkin-related tracing configuration. * @@ -218,8 +221,8 @@ class ReporterImpl : public Reporter, Http::AsyncClient::Callbacks { Driver& driver_; Event::TimerPtr flush_timer_; - SpanBuffer span_buffer_; const CollectorInfo collector_; + SpanBufferPtr span_buffer_; }; } // namespace Zipkin } // namespace Tracers diff --git a/test/extensions/tracers/zipkin/config_test.cc b/test/extensions/tracers/zipkin/config_test.cc index 8e7940d1bdb8..8b211fa74d10 100644 --- a/test/extensions/tracers/zipkin/config_test.cc +++ b/test/extensions/tracers/zipkin/config_test.cc @@ -49,8 +49,8 @@ TEST(ZipkinTracerConfigTest, ZipkinHttpTracerWithTypedConfig) { typed_config: "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig collector_cluster: fake_cluster - collector_endpoint: /api/v1/spans - collector_endpoint_version: HTTP-PROTO + collector_endpoint: /api/v2/spans + collector_endpoint_version: HTTP_PROTO )EOF"; envoy::config::trace::v2::Tracing configuration; diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 35e452703178..35063fc23b71 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -1,7 +1,3 @@ -#include "common/common/base64.h" -#include "common/protobuf/message_validator_impl.h" -#include "common/protobuf/utility.h" - #include "extensions/tracers/zipkin/span_buffer.h" #include "test/test_common/test_time.h" @@ -14,41 +10,17 @@ namespace Tracers { namespace Zipkin { namespace { -void expectValidBufferedProtoListOfSpans(const SpanBuffer& buffer) { - std::string expected_yaml = "spans:"; - for (const auto& span : buffer.spans()) { - const std::string expected_span_yaml = fmt::format( - R"EOF( -- traceId: {} - parentId: {} - id: {} -)EOF", - Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), - span.isSetParentId() ? Base64::encode(span.parentIdAsByteString().c_str(), - span.parentIdAsByteString().size()) - : "", - Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size())); - expected_yaml += expected_span_yaml; - } - zipkin::proto3::ListOfSpans expected_msg; - MessageUtil::loadFromYaml(expected_yaml, expected_msg, - ProtobufMessage::getStrictValidationVisitor()); - EXPECT_EQ(buffer.toProtoListOfSpans().DebugString(), expected_msg.DebugString()); -} - TEST(ZipkinSpanBufferTest, defaultConstructorEndToEnd) { DangerousDeprecatedTestTime test_time; - SpanBuffer buffer; + SpanBuffer buffer(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, true); EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); - EXPECT_EQ(buffer.toProtoListOfSpans().DebugString(), ""); + EXPECT_EQ("[]", buffer.serialize()); EXPECT_FALSE(buffer.addSpan(Span(test_time.timeSystem()))); - expectValidBufferedProtoListOfSpans(buffer); buffer.allocateBuffer(2); EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); + EXPECT_EQ("[]", buffer.serialize()); buffer.addSpan(Span(test_time.timeSystem())); EXPECT_EQ(1ULL, buffer.pendingSpans()); @@ -59,13 +31,11 @@ TEST(ZipkinSpanBufferTest, defaultConstructorEndToEnd) { R"("annotations":[],)" R"("binaryAnnotations":[])" "}]"; - EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); + EXPECT_EQ(expected_json_array_string, buffer.serialize()); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); + EXPECT_EQ("[]", buffer.serialize()); buffer.addSpan(Span(test_time.timeSystem())); buffer.addSpan(Span(test_time.timeSystem())); @@ -87,21 +57,18 @@ TEST(ZipkinSpanBufferTest, defaultConstructorEndToEnd) { "]"; EXPECT_EQ(2ULL, buffer.pendingSpans()); EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); + EXPECT_EQ("[]", buffer.serialize()); } TEST(ZipkinSpanBufferTest, sizeConstructorEndtoEnd) { DangerousDeprecatedTestTime test_time; - SpanBuffer buffer(2); + SpanBuffer buffer(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, true, 2); EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); + EXPECT_EQ("[]", buffer.serialize()); buffer.addSpan(Span(test_time.timeSystem())); EXPECT_EQ(1ULL, buffer.pendingSpans()); @@ -113,12 +80,10 @@ TEST(ZipkinSpanBufferTest, sizeConstructorEndtoEnd) { R"("binaryAnnotations":[])" "}]"; EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); + EXPECT_EQ("[]", buffer.serialize()); buffer.addSpan(Span(test_time.timeSystem())); buffer.addSpan(Span(test_time.timeSystem())); @@ -139,12 +104,10 @@ TEST(ZipkinSpanBufferTest, sizeConstructorEndtoEnd) { "}]"; EXPECT_EQ(2ULL, buffer.pendingSpans()); EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.toStringifiedJsonArray()); - expectValidBufferedProtoListOfSpans(buffer); + EXPECT_EQ("[]", buffer.serialize()); } } // namespace diff --git a/test/extensions/tracers/zipkin/tracer_test.cc b/test/extensions/tracers/zipkin/tracer_test.cc index 04fdbe3e07b4..bc5f9b9e32a9 100644 --- a/test/extensions/tracers/zipkin/tracer_test.cc +++ b/test/extensions/tracers/zipkin/tracer_test.cc @@ -28,7 +28,7 @@ namespace { class TestReporterImpl : public Reporter { public: TestReporterImpl(int value) : value_(value) {} - void reportSpan(const Span& span) override { reported_spans_.push_back(span); } + void reportSpan(Span&& span) override { reported_spans_.push_back(span); } int getValue() { return value_; } std::vector& reportedSpans() { return reported_spans_; } diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index f5a978159fdc..7965aea1d429 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -18,33 +18,11 @@ namespace Tracers { namespace Zipkin { namespace { -void expectValidEndpointProto(const Endpoint& endpoint) { - const auto addr = endpoint.address(); - const auto version = - addr ? (addr->ip()->version() == Network::Address::IpVersion::v4 ? "ipv4" : "ipv6") : "ipv4"; - const auto ip = addr ? Base64::encode(addr->ip()->addressAsString().c_str(), - addr->ip()->addressAsString().size(), false) - : ""; - const auto port = addr ? addr->ip()->port() : 0; - const std::string expected_yaml = fmt::format( - R"EOF( -{}: {} -port: {} -serviceName: {} -)EOF", - version, ip, port, endpoint.serviceName()); - zipkin::proto3::Endpoint expected_msg; - MessageUtil::loadFromYaml(expected_yaml, expected_msg, - ProtobufMessage::getStrictValidationVisitor()); - EXPECT_EQ(endpoint.toProtoEndpoint().DebugString(), expected_msg.DebugString()); -} - TEST(ZipkinCoreTypesEndpointTest, defaultConstructor) { Endpoint ep; EXPECT_EQ("", ep.serviceName()); EXPECT_EQ(R"({"ipv4":"","port":0,"serviceName":""})", ep.toJson()); - expectValidEndpointProto(ep); Network::Address::InstanceConstSharedPtr addr = Network::Utility::parseInternetAddress("127.0.0.1"); @@ -62,7 +40,6 @@ TEST(ZipkinCoreTypesEndpointTest, defaultConstructor) { const std::string expected_json = R"({"ipv6":"2001:db8:85a3::8a2e:370:4444","port":7334,"serviceName":"my_service"})"; EXPECT_EQ(expected_json, ep.toJson()); - expectValidEndpointProto(ep); } TEST(ZipkinCoreTypesEndpointTest, customConstructor) { @@ -79,7 +56,6 @@ TEST(ZipkinCoreTypesEndpointTest, customConstructor) { EXPECT_EQ(R"({"ipv6":"2001:db8:85a3::8a2e:370:4444","port":7334,"serviceName":"my_service"})", ep.toJson()); - expectValidEndpointProto(ep); } TEST(ZipkinCoreTypesEndpointTest, copyOperator) { @@ -93,8 +69,6 @@ TEST(ZipkinCoreTypesEndpointTest, copyOperator) { EXPECT_EQ(ep1.serviceName(), ep2.serviceName()); EXPECT_EQ(ep1.toJson(), ep2.toJson()); - expectValidEndpointProto(ep1); - expectValidEndpointProto(ep2); } TEST(ZipkinCoreTypesEndpointTest, assignmentOperator) { @@ -108,8 +82,6 @@ TEST(ZipkinCoreTypesEndpointTest, assignmentOperator) { EXPECT_EQ(ep1.serviceName(), ep2.serviceName()); EXPECT_EQ(ep1.toJson(), ep2.toJson()); - expectValidEndpointProto(ep1); - expectValidEndpointProto(ep2); } TEST(ZipkinCoreTypesAnnotationTest, defaultConstructor) { @@ -340,23 +312,6 @@ TEST(ZipkinCoreTypesSpanTest, defaultConstructor) { R"("annotations":[],"binaryAnnotations":[]})", span.toJson()); - std::string expected_yaml = fmt::format( - R"EOF( -traceId: {} -parentId: {} -id: {} -)EOF", - Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), - span.isSetParentId() - ? Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()) - : "", - Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size())); - - zipkin::proto3::Span expected_msg; - MessageUtil::loadFromYaml(expected_yaml, expected_msg, - ProtobufMessage::getStrictValidationVisitor()); - EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); - uint64_t id = Util::generateRandom64(test_time.timeSystem()); std::string id_hex = Hex::uint64ToHex(id); span.setId(id); @@ -445,35 +400,6 @@ id: {} R"({"ipv4":"192.168.1.2","port":3306,"serviceName":"my_service_name"}}]})", span.toJson()); - expected_yaml = fmt::format( - R"EOF( -traceId: {} -parentId: {} -id: {} -kind: CLIENT -name: span_name -timestamp: {} -duration: {} -localEndpoint: - serviceName: my_service_name - ipv4: {} - port: 3306 -tags: - lc: my_component_name -)EOF", - Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), - Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), - Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), - /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - - MessageUtil::loadFromYaml(expected_yaml, expected_msg, - ProtobufMessage::getStrictValidationVisitor()); - EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); - - std::string json; - const auto status = Protobuf::util::MessageToJsonString(span.toJsonSpan(), &json); - std::cerr << json << "\n"; - // Test the copy-semantics flavor of addAnnotation and addBinaryAnnotation ann.setValue(Zipkin::ZipkinCoreConstants::get().SERVER_SEND); @@ -525,32 +451,6 @@ duration: {} R"("serviceName":"my_service_name"}}]})", span.toJson()); - expected_yaml = fmt::format( - R"EOF( -traceId: {} -parentId: {} -id: {} -kind: SERVER -name: span_name -timestamp: {} -duration: {} -localEndpoint: - serviceName: my_service_name - ipv4: {} - port: 3306 -tags: - "http.return_code": "400" - lc: my_component_name -)EOF", - Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), - Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), - Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), - /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - - MessageUtil::loadFromYaml(expected_yaml, expected_msg, - ProtobufMessage::getStrictValidationVisitor()); - EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); - // Test setSourceServiceName and setDestinationServiceName ann.setValue(Zipkin::ZipkinCoreConstants::get().CLIENT_RECV); // NOLINT(bugprone-use-after-move) @@ -588,32 +488,6 @@ duration: {} R"("endpoint":{"ipv4":"192.168.1.2","port":3306,)" R"("serviceName":"my_service_name"}}]})", span.toJson()); - - expected_yaml = fmt::format( - R"EOF( -traceId: {} -parentId: {} -id: {} -kind: SERVER -name: span_name -timestamp: {} -duration: {} -localEndpoint: - serviceName: NEW_SERVICE_NAME - ipv4: {} - port: 3306 -tags: - "http.return_code": "400" - lc: my_component_name -)EOF", - Base64::encode(span.traceIdAsByteString().c_str(), span.traceIdAsByteString().size()), - Base64::encode(span.parentIdAsByteString().c_str(), span.parentIdAsByteString().size()), - Base64::encode(span.idAsByteString().c_str(), span.idAsByteString().size()), span.timestamp(), - /* duration= */ 3000, /* localEndpoint.ipv4= */ Base64::encode("192.168.1.2", 11)); - - MessageUtil::loadFromYaml(expected_yaml, expected_msg, - ProtobufMessage::getStrictValidationVisitor()); - EXPECT_EQ(span.toProtoSpan().DebugString(), expected_msg.DebugString()); } TEST(ZipkinCoreTypesSpanTest, copyConstructor) { diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index 0b63b61e3945..c979622b58fd 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -185,19 +185,23 @@ TEST_F(ZipkinDriverTest, InitializeDriver) { } TEST_F(ZipkinDriverTest, FlushSeveralSpans) { - expectValidFlushSeveralSpans("DEFAULT", "application/json"); + expectValidFlushSeveralSpans("NOT_SET", "application/json"); } TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpJsonV1) { expectValidFlushSeveralSpans("HTTP_JSON_V1", "application/json"); } +TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpJson) { + expectValidFlushSeveralSpans("HTTP_JSON", "application/json"); +} + TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpProto) { expectValidFlushSeveralSpans("HTTP_PROTO", "application/x-protobuf"); } TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; @@ -239,7 +243,7 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { } TEST_F(ZipkinDriverTest, FlushSpansTimer) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); const absl::optional timeout(std::chrono::seconds(5)); EXPECT_CALL(cm_.async_client_, @@ -266,7 +270,7 @@ TEST_F(ZipkinDriverTest, FlushSpansTimer) { } TEST_F(ZipkinDriverTest, NoB3ContextSampledTrue) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -280,7 +284,7 @@ TEST_F(ZipkinDriverTest, NoB3ContextSampledTrue) { } TEST_F(ZipkinDriverTest, NoB3ContextSampledFalse) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -294,7 +298,7 @@ TEST_F(ZipkinDriverTest, NoB3ContextSampledFalse) { } TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleTrue) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -310,7 +314,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleTrue) { } TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleFalse) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -326,7 +330,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleFalse) { } TEST_F(ZipkinDriverTest, PropagateB3NotSampled) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -348,7 +352,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NotSampled) { } TEST_F(ZipkinDriverTest, PropagateB3NotSampledWithFalse) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -370,7 +374,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NotSampledWithFalse) { } TEST_F(ZipkinDriverTest, PropagateB3SampledWithTrue) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -392,7 +396,7 @@ TEST_F(ZipkinDriverTest, PropagateB3SampledWithTrue) { } TEST_F(ZipkinDriverTest, PropagateB3SampleFalse) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -409,7 +413,7 @@ TEST_F(ZipkinDriverTest, PropagateB3SampleFalse) { } TEST_F(ZipkinDriverTest, ZipkinSpanTest) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); // ==== // Test effective setTag() @@ -489,7 +493,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); const std::string trace_id = Hex::uint64ToHex(generateRandom64()); const std::string span_id = Hex::uint64ToHex(generateRandom64()); @@ -513,7 +517,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersEmptyParentSpanTest) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); // Root span so have same trace and span id const std::string id = Hex::uint64ToHex(generateRandom64()); @@ -534,7 +538,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersEmptyParentSpanTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3Headers128TraceIdTest) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); const uint64_t trace_id_high = generateRandom64(); const uint64_t trace_id_low = generateRandom64(); @@ -562,7 +566,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3Headers128TraceIdTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, std::string("xyz")); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_SPAN_ID, @@ -576,7 +580,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -590,7 +594,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -605,7 +609,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ExplicitlySetSampledFalse) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_, {Tracing::Reason::Sampling, true}); @@ -622,7 +626,7 @@ TEST_F(ZipkinDriverTest, ExplicitlySetSampledFalse) { } TEST_F(ZipkinDriverTest, ExplicitlySetSampledTrue) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_, {Tracing::Reason::Sampling, false}); @@ -639,7 +643,7 @@ TEST_F(ZipkinDriverTest, ExplicitlySetSampledTrue) { } TEST_F(ZipkinDriverTest, DuplicatedHeader) { - setupValidDriver("DEFAULT"); + setupValidDriver("NOT_SET"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_SPAN_ID, From 240c9a1b5c25f46a55095bab2812fb74f2ff9730 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 11:34:14 +0700 Subject: [PATCH 21/48] Comments and add tests Signed-off-by: Dhi Aurrahman --- .../extensions/tracers/zipkin/span_buffer.cc | 16 - .../extensions/tracers/zipkin/span_buffer.h | 33 +- source/extensions/tracers/zipkin/util.h | 8 +- .../tracers/zipkin/zipkin_core_types.h | 6 +- .../tracers/zipkin/span_buffer_test.cc | 337 ++++++++++++++---- 5 files changed, 301 insertions(+), 99 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index 104e98aaf870..3e4923c20ec7 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -33,22 +33,6 @@ bool SpanBuffer::addSpan(Span&& span) { return true; } -std::string SpanBuffer::toStringifiedJsonArray() { - std::string stringified_json_array = "["; - - if (pendingSpans()) { - stringified_json_array += span_buffer_[0].toJson(); - const uint64_t size = span_buffer_.size(); - for (uint64_t i = 1; i < size; i++) { - stringified_json_array += ","; - stringified_json_array += span_buffer_[i].toJson(); - } - } - stringified_json_array += "]"; - - return stringified_json_array; -} - SerializerPtr SpanBuffer::makeSerializer( const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, const bool shared_span_context) { diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 19dd402865ea..6ec6e27c398d 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -62,13 +62,8 @@ class SpanBuffer { uint64_t pendingSpans() { return span_buffer_.size(); } /** - * @return the contents of the buffer as a stringified array of JSONs, where - * each JSON in the array corresponds to one Zipkin span. - */ - std::string toStringifiedJsonArray(); - - /** - * @return std::string + * @return std::string the contents of the buffer, a collection of serialized pending Zipkin + * spans. */ std::string serialize() { return serializer_->serialize(std::move(span_buffer_)); } @@ -84,17 +79,33 @@ class SpanBuffer { using SpanBufferPtr = std::unique_ptr; +/** + * JsonV2Serializer implements Zipkin::Serializer that serializes list of Zipkin spans into JSON + * Zipkin v1 array. + */ class JsonV1Serializer : public Serializer { public: JsonV1Serializer() = default; + /** + * Serialize list of Zipkin spans into Zipkin v1 JSON array. + * @return std::string serialized pending spans as Zipkin v1 JSON array. + */ std::string serialize(std::vector&& pending_spans) override; }; +/** + * JsonV2Serializer implements Zipkin::Serializer that serializes list of Zipkin spans into JSON + * Zipkin v2 array. + */ class JsonV2Serializer : public Serializer { public: JsonV2Serializer(const bool shared_span_context); + /** + * Serialize list of Zipkin spans into Zipkin v2 JSON array. + * @return std::string serialized pending spans as Zipkin v2 JSON array. + */ std::string serialize(std::vector&& pending_spans) override; private: @@ -104,10 +115,18 @@ class JsonV2Serializer : public Serializer { const bool shared_span_context_; }; +/** + * JsonV2Serializer implements Zipkin::Serializer that serializes list of Zipkin spans into + * stringified (SerializeToString) protobuf message. + */ class ProtobufSerializer : public Serializer { public: ProtobufSerializer(const bool shared_span_context); + /** + * Serialize list of Zipkin spans into Zipkin v2 zipkin::proto3::ListOfSpans. + * @return std::string serialized pending spans as Zipkin zipkin::proto3::ListOfSpans. + */ std::string serialize(std::vector&& pending_spans) override; private: diff --git a/source/extensions/tracers/zipkin/util.h b/source/extensions/tracers/zipkin/util.h index df23a3925b6a..8b30a155ae04 100644 --- a/source/extensions/tracers/zipkin/util.h +++ b/source/extensions/tracers/zipkin/util.h @@ -52,7 +52,7 @@ class Util { static uint64_t generateRandom64(TimeSource& time_source); /** - * Returns byte string representation of an number. + * Returns byte string representation of a number. * * @param value Number that will be represented in byte string. * @return std::string byte string representation of a number. @@ -62,14 +62,14 @@ class Util { } /** - * Returns byte string representation of an number. + * Returns big endian byte string representation of a number. * * @param value Number that will be represented in byte string. * @param flip indicates to flip order or not. * @return std::string byte string representation of a number. */ - template static std::string toFlipableByteString(Type value, bool flip) { - auto bytes = flip ? toEndianness(value) : value; + template static std::string toBigEndianByteString(Type value) { + auto bytes = toEndianness(value); return std::string(reinterpret_cast(&bytes), sizeof(Type)); } }; diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index 95cef6a4ecaf..4a6f7eb8ee47 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -513,9 +513,9 @@ class Span : public ZipkinBase { */ const std::string traceIdAsByteString() const { // https://github.com/apache/incubator-zipkin-api/blob/v0.2.1/zipkin.proto#L60-L61. - return trace_id_high_.has_value() ? Util::toFlipableByteString(trace_id_high_.value(), true) + - Util::toFlipableByteString(trace_id_, true) - : Util::toFlipableByteString(trace_id_, true); + return trace_id_high_.has_value() ? Util::toBigEndianByteString(trace_id_high_.value()) + + Util::toBigEndianByteString(trace_id_) + : Util::toBigEndianByteString(trace_id_); } /** diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 35063fc23b71..9f6dcdc8eb8b 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -1,5 +1,8 @@ +#include "common/network/utility.h" + #include "extensions/tracers/zipkin/span_buffer.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/test_time.h" #include "gtest/gtest.h" @@ -10,28 +13,61 @@ namespace Tracers { namespace Zipkin { namespace { -TEST(ZipkinSpanBufferTest, defaultConstructorEndToEnd) { +Endpoint createEndpoint() { + Endpoint endpoint; + endpoint.setAddress(Envoy::Network::Utility::parseInternetAddress("1.2.3.4", 8080, false)); + endpoint.setServiceName("service1"); + return endpoint; +} + +Annotation createAnnotation(const absl::string_view value) { + Annotation annotation; + annotation.setValue(value.data()); + annotation.setTimestamp(1566058071601051); + annotation.setEndpoint(createEndpoint()); + return annotation; +} + +BinaryAnnotation createTag() { + BinaryAnnotation tag; + tag.setKey("component"); + tag.setValue("proxy"); + return tag; +} + +Span createSpan(const std::vector& annotation_values) { + DangerousDeprecatedTestTime test_time; + Span span = Span(test_time.timeSystem()); + span.setId(1); + span.setTraceId(1); + span.setDuration(100); + std::vector annotations; + for (absl::string_view value : annotation_values) { + annotations.push_back(createAnnotation(value)); + } + span.setAnnotations(annotations); + span.setBinaryAnnotations({createTag()}); + return span; +} + +void expectSerializedBuffer(SpanBuffer& buffer, const bool delay_allocation, + const std::vector& expected) { DangerousDeprecatedTestTime test_time; - SpanBuffer buffer(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, true); EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.serialize()); - EXPECT_FALSE(buffer.addSpan(Span(test_time.timeSystem()))); - buffer.allocateBuffer(2); + if (delay_allocation) { + EXPECT_FALSE(buffer.addSpan(Span(test_time.timeSystem()))); + buffer.allocateBuffer(2); + } + EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.serialize()); buffer.addSpan(Span(test_time.timeSystem())); EXPECT_EQ(1ULL, buffer.pendingSpans()); - std::string expected_json_array_string = "[{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}]"; - EXPECT_EQ(expected_json_array_string, buffer.serialize()); + EXPECT_EQ(expected.at(0), buffer.serialize()); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); @@ -39,75 +75,238 @@ TEST(ZipkinSpanBufferTest, defaultConstructorEndToEnd) { buffer.addSpan(Span(test_time.timeSystem())); buffer.addSpan(Span(test_time.timeSystem())); - expected_json_array_string = "[" - "{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}," - "{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}" - "]"; + EXPECT_EQ(2ULL, buffer.pendingSpans()); - EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); + EXPECT_EQ(expected.at(1), buffer.serialize()); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.serialize()); } -TEST(ZipkinSpanBufferTest, sizeConstructorEndtoEnd) { - DangerousDeprecatedTestTime test_time; - SpanBuffer buffer(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, true, 2); +template std::string serializedMessageToJson(const std::string& serialized) { + Type message; + message.ParseFromString(serialized); + std::string json; + Protobuf::util::MessageToJsonString(message, &json); + return json; +} - EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.serialize()); +TEST(ZipkinSpanBufferTest, ConstructBuffer) { + const bool shared = true; + SpanBuffer buffer1(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, shared); + const bool delay_allocation = true; + expectSerializedBuffer(buffer1, delay_allocation, + {"[{" + R"("traceId":"0000000000000000",)" + R"("name":"",)" + R"("id":"0000000000000000",)" + R"("annotations":[],)" + R"("binaryAnnotations":[])" + "}]", + "[" + "{" + R"("traceId":"0000000000000000",)" + R"("name":"",)" + R"("id":"0000000000000000",)" + R"("annotations":[],)" + R"("binaryAnnotations":[])" + "}," + "{" + R"("traceId":"0000000000000000",)" + R"("name":"",)" + R"("id":"0000000000000000",)" + R"("annotations":[],)" + R"("binaryAnnotations":[])" + "}" + "]"}); - buffer.addSpan(Span(test_time.timeSystem())); - EXPECT_EQ(1ULL, buffer.pendingSpans()); - std::string expected_json_array_string = "[{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}]"; - EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); + SpanBuffer buffer2(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, shared, 2); + expectSerializedBuffer(buffer2, !delay_allocation, + {"[{" + R"("traceId":"0000000000000000",)" + R"("name":"",)" + R"("id":"0000000000000000",)" + R"("annotations":[],)" + R"("binaryAnnotations":[])" + "}]", + "[" + "{" + R"("traceId":"0000000000000000",)" + R"("name":"",)" + R"("id":"0000000000000000",)" + R"("annotations":[],)" + R"("binaryAnnotations":[])" + "}," + "{" + R"("traceId":"0000000000000000",)" + R"("name":"",)" + R"("id":"0000000000000000",)" + R"("annotations":[],)" + R"("binaryAnnotations":[])" + "}" + "]"}); - buffer.clear(); - EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.serialize()); + SpanBuffer buffer3(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, shared); + expectSerializedBuffer(buffer3, delay_allocation, {"[]", "[]"}); +} - buffer.addSpan(Span(test_time.timeSystem())); - buffer.addSpan(Span(test_time.timeSystem())); - expected_json_array_string = "[" - "{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}," - "{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}]"; - EXPECT_EQ(2ULL, buffer.pendingSpans()); - EXPECT_EQ(expected_json_array_string, buffer.toStringifiedJsonArray()); +TEST(ZipkinSpanBufferTest, SerializeSpan) { + const bool shared = true; + SpanBuffer buffer1(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, shared, 2); + buffer1.addSpan(createSpan({"cs"})); + EXPECT_EQ("[{" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"CLIENT",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"})" + "}]", + buffer1.serialize()); - buffer.clear(); - EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.serialize()); + SpanBuffer buffer2(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, shared, 2); + buffer2.addSpan(createSpan({"cs", "sr"})); + EXPECT_EQ("[{" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"CLIENT",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"}},)" + R"({)" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"SERVER",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"},)" + R"("shared":true)" + "}]", + buffer2.serialize()); + + SpanBuffer buffer3(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, !shared, 2); + buffer3.addSpan(createSpan({"cs", "sr"})); + EXPECT_EQ("[{" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"CLIENT",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"}},)" + R"({)" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"SERVER",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"1.2.3.4",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"})" + "}]", + buffer3.serialize()); + + SpanBuffer buffer4(envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO, shared, 2); + buffer4.addSpan(createSpan({"cs"})); + EXPECT_EQ("{" + R"("spans":[{)" + R"("traceId":"AAAAAAAAAAE=",)" + R"("id":"AQAAAAAAAAA=",)" + R"("kind":"CLIENT",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"AQIDBA==",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"})" + "}]}", + serializedMessageToJson(buffer4.serialize())); + + SpanBuffer buffer5(envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO, shared, 2); + buffer5.addSpan(createSpan({"cs", "sr"})); + EXPECT_EQ("{" + R"("spans":[{)" + R"("traceId":"AAAAAAAAAAE=",)" + R"("id":"AQAAAAAAAAA=",)" + R"("kind":"CLIENT",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"AQIDBA==",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"}},)" + R"({)" + R"("traceId":"AAAAAAAAAAE=",)" + R"("id":"AQAAAAAAAAA=",)" + R"("kind":"SERVER",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"AQIDBA==",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"},)" + R"("shared":true)" + "}]}", + serializedMessageToJson(buffer5.serialize())); + + SpanBuffer buffer6(envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO, !shared, 2); + buffer6.addSpan(createSpan({"cs", "sr"})); + EXPECT_EQ("{" + R"("spans":[{)" + R"("traceId":"AAAAAAAAAAE=",)" + R"("id":"AQAAAAAAAAA=",)" + R"("kind":"CLIENT",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"AQIDBA==",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"}},)" + R"({)" + R"("traceId":"AAAAAAAAAAE=",)" + R"("id":"AQAAAAAAAAA=",)" + R"("kind":"SERVER",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv4":"AQIDBA==",)" + R"("port":8080},)" + R"("tags":{)" + R"("component":"proxy"})" + "}]}", + serializedMessageToJson(buffer6.serialize())); } } // namespace From 8c0480d5d3beb49931115e372cf664e65b483085 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 11:39:12 +0700 Subject: [PATCH 22/48] Fix docs Signed-off-by: Dhi Aurrahman --- docs/root/intro/version_history.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 8b72c43aeef7..97752523ba9a 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -21,12 +21,13 @@ Version history * http: added the ability to :ref:`merge adjacent slashes` in the path. * listeners: added :ref:`continue_on_listener_filters_timeout ` to configure whether a listener will still create a connection when listener filters time out. * listeners: added :ref:`HTTP inspector listener filter `. -* rbac: added support for DNS SAN as :ref:`principal_name `. * lua: extended `httpCall()` and `respond()` APIs to accept headers with entry values that can be a string or table of strings. * performance: new buffer implementation enabled by default (to disable add "--use-libevent-buffers 1" to the command-line arguments when starting Envoy). * router: added :ref:`rq_retry_skipped_request_not_complete ` counter stat to router stats. +* rbac: added support for DNS SAN as :ref:`principal_name `. * router check tool: add coverage reporting & enforcement. * router check tool: add comprehensive coverage reporting. +* tracing: added support to the Zipkin reporter for sending list of spans as Zipkin JSON v2 and protobuf message over HTTP. * tls: added verification of IP address SAN fields in certificates against configured SANs in the certificate validation context. * upstream: added network filter chains to upstream connections, see :ref:`filters`. @@ -128,7 +129,6 @@ Version history * sandbox: added :ref:`CSRF sandbox `. * server: ``--define manual_stamp=manual_stamp`` was added to allow server stamping outside of binary rules. more info in the `bazel docs `_. -* tracing: added support to the Zipkin reporter for sending list of spans as protobuf message over HTTP. * tool: added :repo:`proto ` support for :ref:`router check tool ` tests. * server: added :ref:`server state ` statistic. * server: added :ref:`initialization_time_ms` statistic. From acfa29db80dd54f9f60fe25592d9d9f6870a78ec Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 11:48:29 +0700 Subject: [PATCH 23/48] Fix Signed-off-by: Dhi Aurrahman --- docs/root/intro/version_history.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 83d5ff60ead3..478368da9a41 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -28,13 +28,12 @@ Version history * http: added the ability to :ref:`merge adjacent slashes` in the path. * listeners: added :ref:`continue_on_listener_filters_timeout ` to configure whether a listener will still create a connection when listener filters time out. * listeners: added :ref:`HTTP inspector listener filter `. -* redis: added :ref:`read_policy ` to allow reading from redis replicas for Redis Cluster deployments. -* rbac: added support for DNS SAN as :ref:`principal_name `. * lua: extended `httpCall()` and `respond()` APIs to accept headers with entry values that can be a string or table of strings. * performance: new buffer implementation enabled by default (to disable add "--use-libevent-buffers 1" to the command-line arguments when starting Envoy). * rbac: added conditions to the policy, see :ref:`condition `. -* router: added :ref:`rq_retry_skipped_request_not_complete ` counter stat to router stats. * rbac: added support for DNS SAN as :ref:`principal_name `. +* redis: added :ref:`read_policy ` to allow reading from redis replicas for Redis Cluster deployments. +* router: added :ref:`rq_retry_skipped_request_not_complete ` counter stat to router stats. * router check tool: add coverage reporting & enforcement. * router check tool: add comprehensive coverage reporting. * tracing: added support to the Zipkin reporter for sending list of spans as Zipkin JSON v2 and protobuf message over HTTP. From 50aec33f4d838611c47543ab459b89ae19dbbb1d Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 12:33:26 +0700 Subject: [PATCH 24/48] absl::StrAppend and absl::StrCat Signed-off-by: Dhi Aurrahman --- .../extensions/tracers/zipkin/span_buffer.cc | 25 ++++++++++--------- .../tracers/zipkin/span_buffer_test.cc | 2 +- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index 3e4923c20ec7..7ca256e832d8 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -53,14 +53,12 @@ std::string JsonV1Serializer::serialize(std::vector&& zipkin_spans) { std::string stringified_json_array = "["; if (!zipkin_spans.empty()) { - stringified_json_array += zipkin_spans[0].toJson(); - const uint64_t size = zipkin_spans.size(); - for (uint64_t i = 1; i < size; i++) { - stringified_json_array += ","; - stringified_json_array += zipkin_spans[i].toJson(); + absl::StrAppend(&stringified_json_array, zipkin_spans[0].toJson()); + for (uint64_t i = 1; i < zipkin_spans.size(); i++) { + absl::StrAppend(&stringified_json_array, absl::StrCat(",", zipkin_spans[i].toJson())); } } - stringified_json_array += "]"; + absl::StrAppend(&stringified_json_array, "]"); return stringified_json_array; } @@ -75,15 +73,18 @@ std::string JsonV2Serializer::serialize(std::vector&& zipkin_spans) { } std::string stringified_json_array = "["; - for (ssize_t i = 0; i < spans.spans_size(); i++) { + const ssize_t spans_size = spans.spans_size(); + if (spans_size > 0) { std::string entry; - Protobuf::util::MessageToJsonString(spans.spans()[i], &entry); - stringified_json_array += entry; - if (i != spans.spans_size() - 1) { - stringified_json_array += ","; + Protobuf::util::MessageToJsonString(spans.spans()[0], &entry); + absl::StrAppend(&stringified_json_array, entry); + for (ssize_t i = 1; i < spans_size; i++) { + entry.clear(); + Protobuf::util::MessageToJsonString(spans.spans()[i], &entry); + absl::StrAppend(&stringified_json_array, absl::StrCat(",", entry)); } } - stringified_json_array += "]"; + absl::StrAppend(&stringified_json_array, "]"); return stringified_json_array; } diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 9f6dcdc8eb8b..896f7078e609 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -94,8 +94,8 @@ template std::string serializedMessageToJson(const std::string& TEST(ZipkinSpanBufferTest, ConstructBuffer) { const bool shared = true; - SpanBuffer buffer1(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, shared); const bool delay_allocation = true; + SpanBuffer buffer1(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, shared); expectSerializedBuffer(buffer1, delay_allocation, {"[{" R"("traceId":"0000000000000000",)" From bd462dc35c57c7b5211d440dbaa8caaa48ab53ab Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 12:38:44 +0700 Subject: [PATCH 25/48] Remove wrong entry Signed-off-by: Dhi Aurrahman --- docs/root/intro/version_history.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 478368da9a41..821c9e31895e 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -139,7 +139,6 @@ Version history * sandbox: added :ref:`CSRF sandbox `. * server: ``--define manual_stamp=manual_stamp`` was added to allow server stamping outside of binary rules. more info in the `bazel docs `_. -* tool: added :repo:`proto ` support for :ref:`router check tool ` tests. * server: added :ref:`server state ` statistic. * server: added :ref:`initialization_time_ms` statistic. * subset: added :ref:`list_as_any` option to From 273c92cd830c40759a048faf0a1fead885a205af Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 14:20:24 +0700 Subject: [PATCH 26/48] Fix clang-tidy Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/span_buffer.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index 7ca256e832d8..4e553f73512e 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -55,7 +55,7 @@ std::string JsonV1Serializer::serialize(std::vector&& zipkin_spans) { if (!zipkin_spans.empty()) { absl::StrAppend(&stringified_json_array, zipkin_spans[0].toJson()); for (uint64_t i = 1; i < zipkin_spans.size(); i++) { - absl::StrAppend(&stringified_json_array, absl::StrCat(",", zipkin_spans[i].toJson())); + absl::StrAppend(&stringified_json_array, ",", zipkin_spans[i].toJson()); } } absl::StrAppend(&stringified_json_array, "]"); @@ -81,7 +81,7 @@ std::string JsonV2Serializer::serialize(std::vector&& zipkin_spans) { for (ssize_t i = 1; i < spans_size; i++) { entry.clear(); Protobuf::util::MessageToJsonString(spans.spans()[i], &entry); - absl::StrAppend(&stringified_json_array, absl::StrCat(",", entry)); + absl::StrAppend(&stringified_json_array, ",", entry); } } absl::StrAppend(&stringified_json_array, "]"); From 756261b6ae07464f5d5582d1342dba064aa33aaf Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 14:35:45 +0700 Subject: [PATCH 27/48] Add missing comments Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/span_buffer.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 6ec6e27c398d..93280020c240 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -22,6 +22,10 @@ class SpanBuffer { /** * Constructor that creates an empty buffer. Space needs to be allocated by invoking * the method allocateBuffer(size). + * + * @param version The selected Zipkin collector version. + * @param shared_span_context To determine whether client and server spans will shared the same + * span id. */ SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, const bool shared_span_context); @@ -29,6 +33,9 @@ class SpanBuffer { /** * Constructor that initializes a buffer with the given size. * + * @param version The selected Zipkin collector version. + * @param shared_span_context To determine whether client and server spans will shared the same + * span id. * @param size The desired buffer size. */ SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, From 49f6bf55ea092e266d2688601cbd445d27b17c6d Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 14:48:03 +0700 Subject: [PATCH 28/48] Remove unused headers Signed-off-by: Dhi Aurrahman --- test/extensions/tracers/zipkin/zipkin_core_types_test.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc index 7965aea1d429..62f7b9b840c1 100644 --- a/test/extensions/tracers/zipkin/zipkin_core_types_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_core_types_test.cc @@ -1,9 +1,6 @@ -#include "common/common/base64.h" #include "common/common/utility.h" #include "common/network/address_impl.h" #include "common/network/utility.h" -#include "common/protobuf/message_validator_impl.h" -#include "common/protobuf/utility.h" #include "extensions/tracers/zipkin/zipkin_core_constants.h" #include "extensions/tracers/zipkin/zipkin_core_types.h" @@ -37,9 +34,8 @@ TEST(ZipkinCoreTypesEndpointTest, defaultConstructor) { ep.setServiceName("my_service"); EXPECT_EQ("my_service", ep.serviceName()); - const std::string expected_json = - R"({"ipv6":"2001:db8:85a3::8a2e:370:4444","port":7334,"serviceName":"my_service"})"; - EXPECT_EQ(expected_json, ep.toJson()); + EXPECT_EQ(R"({"ipv6":"2001:db8:85a3::8a2e:370:4444","port":7334,"serviceName":"my_service"})", + ep.toJson()); } TEST(ZipkinCoreTypesEndpointTest, customConstructor) { @@ -301,6 +297,7 @@ TEST(ZipkinCoreTypesSpanTest, defaultConstructor) { EXPECT_EQ(0ULL, span.annotations().size()); EXPECT_EQ(0ULL, span.binaryAnnotations().size()); EXPECT_EQ("0000000000000000", span.idAsHexString()); + EXPECT_EQ("0000000000000000", span.parentIdAsHexString()); EXPECT_EQ("0000000000000000", span.traceIdAsHexString()); EXPECT_EQ(0LL, span.startTime()); EXPECT_FALSE(span.debug()); From 3d3fcae3ab3bd0a8afd0553d41df0edf11cd104f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 21 Aug 2019 18:00:19 +0700 Subject: [PATCH 29/48] Fix clang-tidy again Signed-off-by: Dhi Aurrahman --- api/envoy/config/trace/v2/trace.proto | 5 +- .../extensions/tracers/zipkin/span_buffer.cc | 3 +- .../tracers/zipkin/zipkin_tracer_impl.cc | 2 + .../tracers/zipkin/zipkin_tracer_impl.h | 3 +- .../tracers/zipkin/span_buffer_test.cc | 65 +++++++++++++++---- 5 files changed, 61 insertions(+), 17 deletions(-) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 316d7e1ce229..4c0f140138a1 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -65,6 +65,7 @@ message LightstepConfig { string access_token_file = 2 [(validate.rules).string.min_bytes = 1]; } +// Configuration for the Zipkin tracer. message ZipkinConfig { // The cluster manager cluster that hosts the Zipkin collectors. Note that the // Zipkin cluster must be defined in the :ref:`Bootstrap static cluster @@ -87,10 +88,10 @@ message ZipkinConfig { // Available Zipkin collector endpoint versions. enum CollectorEndpointVersion { // If not set, it fallbacks to ``HTTP_JSON_V1``. - NOT_SET = 0; + NOT_SET = 0 [deprecated = true]; // Zipkin API v1, JSON over HTTP. - HTTP_JSON_V1 = 1; + HTTP_JSON_V1 = 1 [deprecated = true]; // Zipkin API v2, JSON over HTTP. HTTP_JSON = 2; diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index 4e553f73512e..3eecb6fed1f5 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -44,8 +44,7 @@ SerializerPtr SpanBuffer::makeSerializer( case envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO: return std::make_unique(shared_span_context); default: - // TODO(dio): Throw if it is required to be explicit when specifying version. - return std::make_unique(); + NOT_REACHED_GCOVR_EXCL_LINE; } } diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index c5bdeccf53c1..65679d33e563 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -83,6 +83,8 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, if (zipkin_config.collector_endpoint_version() != envoy::config::trace::v2::ZipkinConfig::NOT_SET) { + // TODO(dio): Throw an error when we require user to explicitly setting up the collector + // endpoint version. collector.version = zipkin_config.collector_endpoint_version(); } diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h index 25be2b7dc2fd..bd68c4b95928 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h @@ -146,7 +146,8 @@ struct CollectorInfo { std::string endpoint{ZipkinCoreConstants::get().DEFAULT_COLLECTOR_ENDPOINT}; // The version of the collector. This is related to endpoint's supported payload specification and - // transport. + // transport. Currently it defaults to envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1. In + // the future, we will throw when collector_endpoint_version is not specified. envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion version{ envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1}; diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 896f7078e609..2191926a24f0 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -13,18 +13,23 @@ namespace Tracers { namespace Zipkin { namespace { -Endpoint createEndpoint() { +enum class IpType { V4, V6 }; + +Endpoint createEndpoint(const IpType ip_type) { Endpoint endpoint; - endpoint.setAddress(Envoy::Network::Utility::parseInternetAddress("1.2.3.4", 8080, false)); + endpoint.setAddress(ip_type == IpType::V6 + ? Envoy::Network::Utility::parseInternetAddress( + "2001:db8:85a3::8a2e:370:4444", 7334, true) + : Envoy::Network::Utility::parseInternetAddress("1.2.3.4", 8080, false)); endpoint.setServiceName("service1"); return endpoint; } -Annotation createAnnotation(const absl::string_view value) { +Annotation createAnnotation(const absl::string_view value, const IpType ip_type) { Annotation annotation; annotation.setValue(value.data()); annotation.setTimestamp(1566058071601051); - annotation.setEndpoint(createEndpoint()); + annotation.setEndpoint(createEndpoint(ip_type)); return annotation; } @@ -35,15 +40,16 @@ BinaryAnnotation createTag() { return tag; } -Span createSpan(const std::vector& annotation_values) { +Span createSpan(const std::vector& annotation_values, const IpType ip_type) { DangerousDeprecatedTestTime test_time; Span span = Span(test_time.timeSystem()); span.setId(1); span.setTraceId(1); span.setDuration(100); std::vector annotations; + annotations.reserve(annotation_values.size()); for (absl::string_view value : annotation_values) { - annotations.push_back(createAnnotation(value)); + annotations.push_back(createAnnotation(value, ip_type)); } span.setAnnotations(annotations); span.setBinaryAnnotations({createTag()}); @@ -154,7 +160,7 @@ TEST(ZipkinSpanBufferTest, ConstructBuffer) { TEST(ZipkinSpanBufferTest, SerializeSpan) { const bool shared = true; SpanBuffer buffer1(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, shared, 2); - buffer1.addSpan(createSpan({"cs"})); + buffer1.addSpan(createSpan({"cs"}, IpType::V4)); EXPECT_EQ("[{" R"("traceId":"0000000000000001",)" R"("id":"0000000000000001",)" @@ -170,8 +176,25 @@ TEST(ZipkinSpanBufferTest, SerializeSpan) { "}]", buffer1.serialize()); + SpanBuffer buffer1_v6(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, shared, 2); + buffer1_v6.addSpan(createSpan({"cs"}, IpType::V6)); + EXPECT_EQ("[{" + R"("traceId":"0000000000000001",)" + R"("id":"0000000000000001",)" + R"("kind":"CLIENT",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv6":"2001:db8:85a3::8a2e:370:4444",)" + R"("port":7334},)" + R"("tags":{)" + R"("component":"proxy"})" + "}]", + buffer1_v6.serialize()); + SpanBuffer buffer2(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, shared, 2); - buffer2.addSpan(createSpan({"cs", "sr"})); + buffer2.addSpan(createSpan({"cs", "sr"}, IpType::V4)); EXPECT_EQ("[{" R"("traceId":"0000000000000001",)" R"("id":"0000000000000001",)" @@ -201,7 +224,7 @@ TEST(ZipkinSpanBufferTest, SerializeSpan) { buffer2.serialize()); SpanBuffer buffer3(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, !shared, 2); - buffer3.addSpan(createSpan({"cs", "sr"})); + buffer3.addSpan(createSpan({"cs", "sr"}, IpType::V4)); EXPECT_EQ("[{" R"("traceId":"0000000000000001",)" R"("id":"0000000000000001",)" @@ -230,7 +253,7 @@ TEST(ZipkinSpanBufferTest, SerializeSpan) { buffer3.serialize()); SpanBuffer buffer4(envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO, shared, 2); - buffer4.addSpan(createSpan({"cs"})); + buffer4.addSpan(createSpan({"cs"}, IpType::V4)); EXPECT_EQ("{" R"("spans":[{)" R"("traceId":"AAAAAAAAAAE=",)" @@ -247,8 +270,26 @@ TEST(ZipkinSpanBufferTest, SerializeSpan) { "}]}", serializedMessageToJson(buffer4.serialize())); + SpanBuffer buffer4_v6(envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO, shared, 2); + buffer4_v6.addSpan(createSpan({"cs"}, IpType::V6)); + EXPECT_EQ("{" + R"("spans":[{)" + R"("traceId":"AAAAAAAAAAE=",)" + R"("id":"AQAAAAAAAAA=",)" + R"("kind":"CLIENT",)" + R"("timestamp":"1566058071601051",)" + R"("duration":"100",)" + R"("localEndpoint":{)" + R"("serviceName":"service1",)" + R"("ipv6":"IAENuIWjAAAAAIouA3BERA==",)" + R"("port":7334},)" + R"("tags":{)" + R"("component":"proxy"})" + "}]}", + serializedMessageToJson(buffer4_v6.serialize())); + SpanBuffer buffer5(envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO, shared, 2); - buffer5.addSpan(createSpan({"cs", "sr"})); + buffer5.addSpan(createSpan({"cs", "sr"}, IpType::V4)); EXPECT_EQ("{" R"("spans":[{)" R"("traceId":"AAAAAAAAAAE=",)" @@ -279,7 +320,7 @@ TEST(ZipkinSpanBufferTest, SerializeSpan) { serializedMessageToJson(buffer5.serialize())); SpanBuffer buffer6(envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO, !shared, 2); - buffer6.addSpan(createSpan({"cs", "sr"})); + buffer6.addSpan(createSpan({"cs", "sr"}, IpType::V4)); EXPECT_EQ("{" R"("spans":[{)" R"("traceId":"AAAAAAAAAAE=",)" From 57625de02ea6d3072ff706ef7d8dc1615ab947ca Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 22 Aug 2019 05:33:01 +0700 Subject: [PATCH 30/48] Fix alpha ordering later Signed-off-by: Dhi Aurrahman --- docs/root/intro/version_history.rst | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 06198f4d4fcd..ad34f543de57 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -34,8 +34,6 @@ Version history * lua: extended `httpCall()` and `respond()` APIs to accept headers with entry values that can be a string or table of strings. * performance: new buffer implementation enabled by default (to disable add "--use-libevent-buffers 1" to the command-line arguments when starting Envoy). * rbac: added conditions to the policy, see :ref:`condition `. -* rbac: added support for DNS SAN as :ref:`principal_name `. -* redis: added :ref:`read_policy ` to allow reading from redis replicas for Redis Cluster deployments. * router: added :ref:`rq_retry_skipped_request_not_complete ` counter stat to router stats. * router check tool: add coverage reporting & enforcement. * router check tool: add comprehensive coverage reporting. From f303859a31eea211121cd2a10e569365569e6e47 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 22 Aug 2019 17:35:13 +0700 Subject: [PATCH 31/48] Comments Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/zipkin_core_types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index 4a6f7eb8ee47..0aac3fe545e6 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -512,7 +512,7 @@ class Span : public ZipkinBase { * @return the span's trace id as a byte string. */ const std::string traceIdAsByteString() const { - // https://github.com/apache/incubator-zipkin-api/blob/v0.2.1/zipkin.proto#L60-L61. + // https://github.com/openzipkin/zipkin-api/blob/v0.2.1/zipkin.proto#L60-L61. return trace_id_high_.has_value() ? Util::toBigEndianByteString(trace_id_high_.value()) + Util::toBigEndianByteString(trace_id_) : Util::toBigEndianByteString(trace_id_); From 93cbf23f2c8228d7b26352522cb417bfce6644f3 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 22 Aug 2019 21:11:00 +0700 Subject: [PATCH 32/48] Comments Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/span_buffer.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 93280020c240..10c6924d78de 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -87,7 +87,7 @@ class SpanBuffer { using SpanBufferPtr = std::unique_ptr; /** - * JsonV2Serializer implements Zipkin::Serializer that serializes list of Zipkin spans into JSON + * JsonV1Serializer implements Zipkin::Serializer that serializes list of Zipkin spans into JSON * Zipkin v1 array. */ class JsonV1Serializer : public Serializer { @@ -123,7 +123,7 @@ class JsonV2Serializer : public Serializer { }; /** - * JsonV2Serializer implements Zipkin::Serializer that serializes list of Zipkin spans into + * ProtobufSerializer implements Zipkin::Serializer that serializes list of Zipkin spans into * stringified (SerializeToString) protobuf message. */ class ProtobufSerializer : public Serializer { From 19f3bb4c0af5f8da87c611cec43e972deb55f056 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 23 Aug 2019 07:47:01 +0700 Subject: [PATCH 33/48] Use vector Signed-off-by: Dhi Aurrahman --- .../extensions/tracers/zipkin/span_buffer.cc | 23 +++++++++++-------- .../extensions/tracers/zipkin/span_buffer.h | 2 +- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index 3eecb6fed1f5..aa186fa1ddc3 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -66,20 +66,22 @@ JsonV2Serializer::JsonV2Serializer(const bool shared_span_context) : shared_span_context_{shared_span_context} {} std::string JsonV2Serializer::serialize(std::vector&& zipkin_spans) { - zipkin::jsonv2::ListOfSpans spans; + std::vector spans; + spans.reserve(zipkin_spans.size() * 2); for (const Span& zipkin_span : zipkin_spans) { - spans.MergeFrom(toListOfSpans(zipkin_span)); + const std::vector& converted = toListOfSpans(zipkin_span); + std::copy(converted.begin(), converted.end(), std::back_inserter(spans)); } std::string stringified_json_array = "["; - const ssize_t spans_size = spans.spans_size(); + const uint64_t spans_size = spans.size(); if (spans_size > 0) { std::string entry; - Protobuf::util::MessageToJsonString(spans.spans()[0], &entry); + Protobuf::util::MessageToJsonString(spans.at(0), &entry); absl::StrAppend(&stringified_json_array, entry); - for (ssize_t i = 1; i < spans_size; i++) { + for (uint64_t i = 1; i < spans_size; i++) { entry.clear(); - Protobuf::util::MessageToJsonString(spans.spans()[i], &entry); + Protobuf::util::MessageToJsonString(spans.at(i), &entry); absl::StrAppend(&stringified_json_array, ",", entry); } } @@ -88,8 +90,10 @@ std::string JsonV2Serializer::serialize(std::vector&& zipkin_spans) { return stringified_json_array; } -const zipkin::jsonv2::ListOfSpans JsonV2Serializer::toListOfSpans(const Span& zipkin_span) const { - zipkin::jsonv2::ListOfSpans spans; +const std::vector +JsonV2Serializer::toListOfSpans(const Span& zipkin_span) const { + std::vector spans; + spans.reserve(zipkin_span.annotations().size()); for (const auto& annotation : zipkin_span.annotations()) { zipkin::jsonv2::Span span; @@ -124,8 +128,7 @@ const zipkin::jsonv2::ListOfSpans JsonV2Serializer::toListOfSpans(const Span& zi tags[binary_annotation.key()] = binary_annotation.value(); } - auto* mutable_span = spans.add_spans(); - mutable_span->MergeFrom(span); + spans.push_back(span); } return spans; } diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 10c6924d78de..7b7bdc8fe7c3 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -116,7 +116,7 @@ class JsonV2Serializer : public Serializer { std::string serialize(std::vector&& pending_spans) override; private: - const zipkin::jsonv2::ListOfSpans toListOfSpans(const Span& zipkin_span) const; + const std::vector toListOfSpans(const Span& zipkin_span) const; const zipkin::jsonv2::Endpoint toProtoEndpoint(const Endpoint& zipkin_endpoint) const; const bool shared_span_context_; From ba7bf9c5c632e227633af7ce4c642e91eaf446bf Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 23 Aug 2019 13:14:00 +0700 Subject: [PATCH 34/48] Use 0.2.2 release Signed-off-by: Dhi Aurrahman --- api/bazel/repository_locations.bzl | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 64f6f29e9c9b..ec800b6cd4d2 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -18,9 +18,8 @@ PROMETHEUS_SHA = "783bdaf8ee0464b35ec0c8704871e1e72afa0005c3f3587f65d9d6694bf391 KAFKA_SOURCE_SHA = "ae7a1696c0a0302b43c5b21e515c37e6ecd365941f68a510a7e442eebddf39a1" # 2.2.0-rc2 -# ZIPKINAPI_RELEASE = "0.2.2" -ZIPKINAPI_GIT_SHA = "821bc013ecc730f4c76f33c50bd5e8f4b6fa8446" -ZIPKINAPI_SHA256 = "f4ee5f8f87d95393fd7e4440ff8c0dbe49405cf16cab8c34eec37106551c3e46" +ZIPKINAPI_RELEASE = "0.2.2" # Aug 23, 2019 +ZIPKINAPI_SHA256 = "688c4fe170821dd589f36ec45aaadc03a618a40283bc1f97da8fa11686fc816b" REPOSITORY_LOCATIONS = dict( bazel_skylib = dict( @@ -60,7 +59,7 @@ REPOSITORY_LOCATIONS = dict( ), com_github_openzipkin_zipkinapi = dict( sha256 = ZIPKINAPI_SHA256, - strip_prefix = "zipkin-api-" + ZIPKINAPI_GIT_SHA, - urls = ["https://github.com/openzipkin/zipkin-api/archive/" + ZIPKINAPI_GIT_SHA + ".tar.gz"], + strip_prefix = "zipkin-api-" + ZIPKINAPI_RELEASE, + urls = ["https://github.com/openzipkin/zipkin-api/archive/" + ZIPKINAPI_RELEASE + ".tar.gz"], ), ) From 199d28eb81ad5802a1f0351e6401472aca1f1eb6 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 23 Aug 2019 13:14:43 +0700 Subject: [PATCH 35/48] Fix Signed-off-by: Dhi Aurrahman --- api/bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index ec800b6cd4d2..e1587ddd1036 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -18,7 +18,7 @@ PROMETHEUS_SHA = "783bdaf8ee0464b35ec0c8704871e1e72afa0005c3f3587f65d9d6694bf391 KAFKA_SOURCE_SHA = "ae7a1696c0a0302b43c5b21e515c37e6ecd365941f68a510a7e442eebddf39a1" # 2.2.0-rc2 -ZIPKINAPI_RELEASE = "0.2.2" # Aug 23, 2019 +ZIPKINAPI_RELEASE = "0.2.2" # Aug 23, 2019 ZIPKINAPI_SHA256 = "688c4fe170821dd589f36ec45aaadc03a618a40283bc1f97da8fa11686fc816b" REPOSITORY_LOCATIONS = dict( From 10f220d884ffd0deade0c56d11560305e5fdd4aa Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 23 Aug 2019 20:10:26 +0700 Subject: [PATCH 36/48] Cleanup Signed-off-by: Dhi Aurrahman --- .../extensions/tracers/zipkin/span_buffer.cc | 63 ++++---- .../tracers/zipkin/span_buffer_test.cc | 141 +++++++++--------- 2 files changed, 98 insertions(+), 106 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index aa186fa1ddc3..cfe444d02c79 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -5,6 +5,8 @@ #include "extensions/tracers/zipkin/util.h" #include "extensions/tracers/zipkin/zipkin_core_constants.h" +#include "absl/strings/str_join.h" + namespace Envoy { namespace Extensions { namespace Tracers { @@ -24,10 +26,18 @@ SpanBuffer::SpanBuffer( // TODO(fabolive): Need to avoid the copy to improve performance. bool SpanBuffer::addSpan(Span&& span) { - if (span_buffer_.size() == span_buffer_.capacity()) { - // Buffer full + const auto& annotations = span.annotations(); + if (span_buffer_.size() == span_buffer_.capacity() || annotations.empty() || + annotations.end() == + std::find_if(annotations.begin(), annotations.end(), [](const auto& annotation) { + return annotation.value() == ZipkinCoreConstants::get().CLIENT_SEND || + annotation.value() == ZipkinCoreConstants::get().SERVER_RECV; + })) { + + // Buffer full or invalid span. return false; } + span_buffer_.push_back(std::move(span)); return true; @@ -49,45 +59,28 @@ SerializerPtr SpanBuffer::makeSerializer( } std::string JsonV1Serializer::serialize(std::vector&& zipkin_spans) { - std::string stringified_json_array = "["; - - if (!zipkin_spans.empty()) { - absl::StrAppend(&stringified_json_array, zipkin_spans[0].toJson()); - for (uint64_t i = 1; i < zipkin_spans.size(); i++) { - absl::StrAppend(&stringified_json_array, ",", zipkin_spans[i].toJson()); - } - } - absl::StrAppend(&stringified_json_array, "]"); - - return stringified_json_array; + const std::string serialized_elements = + absl::StrJoin(zipkin_spans, ",", [](std::string* element, Span zipkin_span) { + absl::StrAppend(element, zipkin_span.toJson()); + }); + return absl::StrCat("[", serialized_elements, "]"); } JsonV2Serializer::JsonV2Serializer(const bool shared_span_context) : shared_span_context_{shared_span_context} {} std::string JsonV2Serializer::serialize(std::vector&& zipkin_spans) { - std::vector spans; - spans.reserve(zipkin_spans.size() * 2); - for (const Span& zipkin_span : zipkin_spans) { - const std::vector& converted = toListOfSpans(zipkin_span); - std::copy(converted.begin(), converted.end(), std::back_inserter(spans)); - } - - std::string stringified_json_array = "["; - const uint64_t spans_size = spans.size(); - if (spans_size > 0) { - std::string entry; - Protobuf::util::MessageToJsonString(spans.at(0), &entry); - absl::StrAppend(&stringified_json_array, entry); - for (uint64_t i = 1; i < spans_size; i++) { - entry.clear(); - Protobuf::util::MessageToJsonString(spans.at(i), &entry); - absl::StrAppend(&stringified_json_array, ",", entry); - } - } - absl::StrAppend(&stringified_json_array, "]"); - - return stringified_json_array; + const std::string serialized_elements = + absl::StrJoin(zipkin_spans, ",", [this](std::string* out, const Span& zipkin_span) { + absl::StrAppend(out, + absl::StrJoin(toListOfSpans(zipkin_span), ",", + [](std::string* element, const zipkin::jsonv2::Span& span) { + std::string entry; + Protobuf::util::MessageToJsonString(span, &entry); + absl::StrAppend(element, entry); + })); + }); + return absl::StrCat("[", serialized_elements, "]"); } const std::vector diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 2191926a24f0..9122e51acced 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -2,7 +2,6 @@ #include "extensions/tracers/zipkin/span_buffer.h" -#include "test/test_common/simulated_time_system.h" #include "test/test_common/test_time.h" #include "gtest/gtest.h" @@ -42,7 +41,7 @@ BinaryAnnotation createTag() { Span createSpan(const std::vector& annotation_values, const IpType ip_type) { DangerousDeprecatedTestTime test_time; - Span span = Span(test_time.timeSystem()); + Span span(test_time.timeSystem()); span.setId(1); span.setTraceId(1); span.setDuration(100); @@ -57,33 +56,31 @@ Span createSpan(const std::vector& annotation_values, const I } void expectSerializedBuffer(SpanBuffer& buffer, const bool delay_allocation, - const std::vector& expected) { + const std::vector& expected_list) { DangerousDeprecatedTestTime test_time; EXPECT_EQ(0ULL, buffer.pendingSpans()); EXPECT_EQ("[]", buffer.serialize()); if (delay_allocation) { - EXPECT_FALSE(buffer.addSpan(Span(test_time.timeSystem()))); - buffer.allocateBuffer(2); + EXPECT_FALSE(buffer.addSpan(createSpan({"cs", "sr"}, IpType::V4))); + buffer.allocateBuffer(expected_list.size() + 1); } - EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.serialize()); - - buffer.addSpan(Span(test_time.timeSystem())); - EXPECT_EQ(1ULL, buffer.pendingSpans()); - EXPECT_EQ(expected.at(0), buffer.serialize()); + // Add span after allocation, but missing required annotations should be false. + EXPECT_FALSE(buffer.addSpan(Span(test_time.timeSystem()))); + EXPECT_FALSE(buffer.addSpan(createSpan({"aa"}, IpType::V4))); - buffer.clear(); - EXPECT_EQ(0ULL, buffer.pendingSpans()); - EXPECT_EQ("[]", buffer.serialize()); - - buffer.addSpan(Span(test_time.timeSystem())); - buffer.addSpan(Span(test_time.timeSystem())); + for (uint64_t i = 0; i < expected_list.size(); i++) { + buffer.addSpan(createSpan({"cs", "sr"}, IpType::V4)); + EXPECT_EQ(i + 1, buffer.pendingSpans()); + EXPECT_EQ(expected_list.at(i), buffer.serialize()); + } - EXPECT_EQ(2ULL, buffer.pendingSpans()); - EXPECT_EQ(expected.at(1), buffer.serialize()); + // Add a valid span. Valid means can be serialized to v2. + EXPECT_TRUE(buffer.addSpan(createSpan({"cs"}, IpType::V4))); + // While the span is vald, buffer is full. + EXPECT_FALSE(buffer.addSpan(createSpan({"cs", "sr"}, IpType::V4))); buffer.clear(); EXPECT_EQ(0ULL, buffer.pendingSpans()); @@ -99,62 +96,64 @@ template std::string serializedMessageToJson(const std::string& } TEST(ZipkinSpanBufferTest, ConstructBuffer) { + const std::string expected1 = R"([{"traceId":"0000000000000001",)" + R"("name":"",)" + R"("id":"0000000000000001",)" + R"("duration":100,)" + R"("annotations":[{"timestamp":1566058071601051,)" + R"("value":"cs",)" + R"("endpoint":{"ipv4":"1.2.3.4",)" + R"("port":8080,)" + R"("serviceName":"service1"}},)" + R"({"timestamp":1566058071601051,)" + R"("value":"sr",)" + R"("endpoint":{"ipv4":"1.2.3.4",)" + R"("port":8080,)" + R"("serviceName":"service1"}}],)" + R"("binaryAnnotations":[{"key":"component",)" + R"("value":"proxy"}]}])"; + + const std::string expected2 = R"([{"traceId":"0000000000000001",)" + R"("name":"",)" + R"("id":"0000000000000001",)" + R"("duration":100,)" + R"("annotations":[{"timestamp":1566058071601051,)" + R"("value":"cs",)" + R"("endpoint":{"ipv4":"1.2.3.4",)" + R"("port":8080,)" + R"("serviceName":"service1"}},)" + R"({"timestamp":1566058071601051,)" + R"("value":"sr",)" + R"("endpoint":{"ipv4":"1.2.3.4",)" + R"("port":8080,)" + R"("serviceName":"service1"}}],)" + R"("binaryAnnotations":[{"key":"component",)" + R"("value":"proxy"}]},)" + R"({"traceId":"0000000000000001",)" + R"("name":"",)" + R"("id":"0000000000000001",)" + R"("duration":100,)" + R"("annotations":[{"timestamp":1566058071601051,)" + R"("value":"cs",)" + R"("endpoint":{"ipv4":"1.2.3.4",)" + R"("port":8080,)" + R"("serviceName":"service1"}},)" + R"({"timestamp":1566058071601051,)" + R"("value":"sr",)" + R"("endpoint":{"ipv4":"1.2.3.4",)" + R"("port":8080,)" + R"("serviceName":"service1"}}],)" + R"("binaryAnnotations":[{"key":"component",)" + R"("value":"proxy"}]}])"; const bool shared = true; const bool delay_allocation = true; - SpanBuffer buffer1(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, shared); - expectSerializedBuffer(buffer1, delay_allocation, - {"[{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}]", - "[" - "{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}," - "{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}" - "]"}); - SpanBuffer buffer2(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, shared, 2); - expectSerializedBuffer(buffer2, !delay_allocation, - {"[{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}]", - "[" - "{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}," - "{" - R"("traceId":"0000000000000000",)" - R"("name":"",)" - R"("id":"0000000000000000",)" - R"("annotations":[],)" - R"("binaryAnnotations":[])" - "}" - "]"}); + SpanBuffer buffer1(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, shared); + expectSerializedBuffer(buffer1, delay_allocation, {expected1, expected2}); - SpanBuffer buffer3(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON, shared); - expectSerializedBuffer(buffer3, delay_allocation, {"[]", "[]"}); + // Prepare 3 slots, since we will add one more inside the `expectSerializedBuffer` function. + SpanBuffer buffer2(envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1, shared, 3); + expectSerializedBuffer(buffer2, !delay_allocation, {expected1, expected2}); } TEST(ZipkinSpanBufferTest, SerializeSpan) { From 7971c730762eb572e8bd4b9b9b80427b3f507ca8 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 23 Aug 2019 20:11:41 +0700 Subject: [PATCH 37/48] Fix Signed-off-by: Dhi Aurrahman --- test/extensions/tracers/zipkin/span_buffer_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/tracers/zipkin/span_buffer_test.cc b/test/extensions/tracers/zipkin/span_buffer_test.cc index 9122e51acced..1ef6e88e6485 100644 --- a/test/extensions/tracers/zipkin/span_buffer_test.cc +++ b/test/extensions/tracers/zipkin/span_buffer_test.cc @@ -79,7 +79,7 @@ void expectSerializedBuffer(SpanBuffer& buffer, const bool delay_allocation, // Add a valid span. Valid means can be serialized to v2. EXPECT_TRUE(buffer.addSpan(createSpan({"cs"}, IpType::V4))); - // While the span is vald, buffer is full. + // While the span is valid, however the buffer is full. EXPECT_FALSE(buffer.addSpan(createSpan({"cs", "sr"}, IpType::V4))); buffer.clear(); From 6a39da45c6058f3a469a25c68358e21fa77ac648 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 23 Aug 2019 20:36:02 +0700 Subject: [PATCH 38/48] Comments Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/span_buffer.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 7b7bdc8fe7c3..d837401d0348 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -23,7 +23,8 @@ class SpanBuffer { * Constructor that creates an empty buffer. Space needs to be allocated by invoking * the method allocateBuffer(size). * - * @param version The selected Zipkin collector version. + * @param version The selected Zipkin collector version. @see + * api/envoy/config/trace/v2/trace.proto. * @param shared_span_context To determine whether client and server spans will shared the same * span id. */ @@ -33,7 +34,7 @@ class SpanBuffer { /** * Constructor that initializes a buffer with the given size. * - * @param version The selected Zipkin collector version. + * @param version The selected Zipkin collector version. @see api/envoy/config/trace/v2/trace.proto. * @param shared_span_context To determine whether client and server spans will shared the same * span id. * @param size The desired buffer size. From d91e756ed06b15216871f2220e616e5b48813aaf Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 23 Aug 2019 20:37:20 +0700 Subject: [PATCH 39/48] Fix Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/span_buffer.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index d837401d0348..7af34dfc1c2c 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -34,7 +34,8 @@ class SpanBuffer { /** * Constructor that initializes a buffer with the given size. * - * @param version The selected Zipkin collector version. @see api/envoy/config/trace/v2/trace.proto. + * @param version The selected Zipkin collector version. @see + * api/envoy/config/trace/v2/trace.proto. * @param shared_span_context To determine whether client and server spans will shared the same * span id. * @param size The desired buffer size. From b00b4707a8ca569087d345887198b790101f95c9 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Sat, 24 Aug 2019 05:32:38 +0700 Subject: [PATCH 40/48] Comments Signed-off-by: Dhi Aurrahman --- api/envoy/config/trace/v2/trace.proto | 13 +++--- docs/root/intro/deprecated.rst | 1 + .../extensions/tracers/zipkin/span_buffer.h | 8 ++-- .../tracers/zipkin/zipkin_tracer_impl.cc | 10 +---- .../tracers/zipkin/zipkin_tracer_impl_test.cc | 42 +++++++++---------- 5 files changed, 33 insertions(+), 41 deletions(-) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 4c0f140138a1..9a784acf664d 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -81,26 +81,23 @@ message ZipkinConfig { // trace instance. The default value is false, which will result in a 64 bit trace id being used. bool trace_id_128bit = 3; - // Determines whether client and server spans will shared the same span id. + // Determines whether client and server spans will share the same span context. // The default value is true. google.protobuf.BoolValue shared_span_context = 4; // Available Zipkin collector endpoint versions. enum CollectorEndpointVersion { - // If not set, it fallbacks to ``HTTP_JSON_V1``. - NOT_SET = 0 [deprecated = true]; - // Zipkin API v1, JSON over HTTP. - HTTP_JSON_V1 = 1 [deprecated = true]; + HTTP_JSON_V1 = 0 [deprecated = true]; // Zipkin API v2, JSON over HTTP. - HTTP_JSON = 2; + HTTP_JSON = 1; // Zipkin API v2, protobuf over HTTP. - HTTP_PROTO = 3; + HTTP_PROTO = 2; // [#not-implemented-hide:] - GRPC = 4; + GRPC = 3; } // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be diff --git a/docs/root/intro/deprecated.rst b/docs/root/intro/deprecated.rst index 6ca0b034b759..61c8bc0741f9 100644 --- a/docs/root/intro/deprecated.rst +++ b/docs/root/intro/deprecated.rst @@ -14,6 +14,7 @@ Version 1.12.0 (pending) ======================== * The ORIGINAL_DST_LB :ref:`load balancing policy ` is deprecated, use CLUSTER_PROVIDED policy instead when configuring an :ref:`original destination cluster `. * The :option:`--allow-unknown-fields` command-line option, use :option:`--allow-unknown-static-fields` instead. +* The use of HTTP_JSON_V1 :ref:`Zipkin collector endpoint version ` or not explicitly specifying it is deprecated, use HTTP_JSON or HTTP_PROTO instead. Version 1.11.0 (July 11, 2019) ============================== diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 7af34dfc1c2c..927c9f388d9f 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -25,8 +25,8 @@ class SpanBuffer { * * @param version The selected Zipkin collector version. @see * api/envoy/config/trace/v2/trace.proto. - * @param shared_span_context To determine whether client and server spans will shared the same - * span id. + * @param shared_span_context To determine whether client and server spans will share the same + * span context. */ SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, const bool shared_span_context); @@ -36,8 +36,8 @@ class SpanBuffer { * * @param version The selected Zipkin collector version. @see * api/envoy/config/trace/v2/trace.proto. - * @param shared_span_context To determine whether client and server spans will shared the same - * span id. + * @param shared_span_context To determine whether client and server spans will share the same + * span context. * @param size The desired buffer size. */ SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index 65679d33e563..254cca541631 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -80,14 +80,8 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, if (!zipkin_config.collector_endpoint().empty()) { collector.endpoint = zipkin_config.collector_endpoint(); } - - if (zipkin_config.collector_endpoint_version() != - envoy::config::trace::v2::ZipkinConfig::NOT_SET) { - // TODO(dio): Throw an error when we require user to explicitly setting up the collector - // endpoint version. - collector.version = zipkin_config.collector_endpoint_version(); - } - + // The current default version of collector_endpoint_version is HTTP_JSON_V1. + collector.version = zipkin_config.collector_endpoint_version(); const bool trace_id_128bit = zipkin_config.trace_id_128bit(); const bool shared_span_context = PROTOBUF_GET_WRAPPED_OR_DEFAULT( diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index 15289a1f6793..5566ed107e27 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -185,7 +185,7 @@ TEST_F(ZipkinDriverTest, InitializeDriver) { } TEST_F(ZipkinDriverTest, FlushSeveralSpans) { - expectValidFlushSeveralSpans("NOT_SET", "application/json"); + expectValidFlushSeveralSpans("HTTP_JSON_V1", "application/json"); } TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpJsonV1) { @@ -201,7 +201,7 @@ TEST_F(ZipkinDriverTest, FlushSeveralSpansHttpProto) { } TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; @@ -243,7 +243,7 @@ TEST_F(ZipkinDriverTest, FlushOneSpanReportFailure) { } TEST_F(ZipkinDriverTest, FlushSpansTimer) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); const absl::optional timeout(std::chrono::seconds(5)); EXPECT_CALL(cm_.async_client_, @@ -270,7 +270,7 @@ TEST_F(ZipkinDriverTest, FlushSpansTimer) { } TEST_F(ZipkinDriverTest, NoB3ContextSampledTrue) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -284,7 +284,7 @@ TEST_F(ZipkinDriverTest, NoB3ContextSampledTrue) { } TEST_F(ZipkinDriverTest, NoB3ContextSampledFalse) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -298,7 +298,7 @@ TEST_F(ZipkinDriverTest, NoB3ContextSampledFalse) { } TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleTrue) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -314,7 +314,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleTrue) { } TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleFalse) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -330,7 +330,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NoSampleDecisionSampleFalse) { } TEST_F(ZipkinDriverTest, PropagateB3NotSampled) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -352,7 +352,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NotSampled) { } TEST_F(ZipkinDriverTest, PropagateB3NotSampledWithFalse) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -374,7 +374,7 @@ TEST_F(ZipkinDriverTest, PropagateB3NotSampledWithFalse) { } TEST_F(ZipkinDriverTest, PropagateB3SampledWithTrue) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_SPAN_ID)); EXPECT_EQ(nullptr, request_headers_.get(ZipkinCoreConstants::get().X_B3_TRACE_ID)); @@ -396,7 +396,7 @@ TEST_F(ZipkinDriverTest, PropagateB3SampledWithTrue) { } TEST_F(ZipkinDriverTest, PropagateB3SampleFalse) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -413,7 +413,7 @@ TEST_F(ZipkinDriverTest, PropagateB3SampleFalse) { } TEST_F(ZipkinDriverTest, ZipkinSpanTest) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); // ==== // Test effective setTag() @@ -493,7 +493,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); const std::string trace_id = Hex::uint64ToHex(generateRandom64()); const std::string span_id = Hex::uint64ToHex(generateRandom64()); @@ -517,7 +517,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersEmptyParentSpanTest) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); // Root span so have same trace and span id const std::string id = Hex::uint64ToHex(generateRandom64()); @@ -538,7 +538,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersEmptyParentSpanTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3Headers128TraceIdTest) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); const uint64_t trace_id_high = generateRandom64(); const uint64_t trace_id_low = generateRandom64(); @@ -566,7 +566,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3Headers128TraceIdTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, std::string("xyz")); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_SPAN_ID, @@ -580,7 +580,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -594,7 +594,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidSpanIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); @@ -609,7 +609,7 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidParentIdB3HeadersTest) { } TEST_F(ZipkinDriverTest, ExplicitlySetSampledFalse) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_, {Tracing::Reason::Sampling, true}); @@ -626,7 +626,7 @@ TEST_F(ZipkinDriverTest, ExplicitlySetSampledFalse) { } TEST_F(ZipkinDriverTest, ExplicitlySetSampledTrue) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_, {Tracing::Reason::Sampling, false}); @@ -643,7 +643,7 @@ TEST_F(ZipkinDriverTest, ExplicitlySetSampledTrue) { } TEST_F(ZipkinDriverTest, DuplicatedHeader) { - setupValidDriver("NOT_SET"); + setupValidDriver("HTTP_JSON_V1"); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID, Hex::uint64ToHex(generateRandom64())); request_headers_.addReferenceKey(ZipkinCoreConstants::get().X_B3_SPAN_ID, From a1c8cc723e27194b060477413b3c8a49b0cfd86c Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 27 Aug 2019 23:44:55 +0700 Subject: [PATCH 41/48] Add comment on immediate deprecation Signed-off-by: Dhi Aurrahman --- api/envoy/config/trace/v2/trace.proto | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 9a784acf664d..3ded1904f868 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -88,6 +88,12 @@ message ZipkinConfig { // Available Zipkin collector endpoint versions. enum CollectorEndpointVersion { // Zipkin API v1, JSON over HTTP. + // [#comment: The default implementation of Zipkin client before this field is added was only v1 + // and the way user configure this was by not explicitly specifying the version. Consequently, + // before this is added, the corresponding Zipkin collector expected to receive v1 payload. + // Hence the motivation of adding HTTP_JSON_V1 as the default is to avoid a breaking change when + // user upgrading Envoy with this change. Furthermore, we also immediately deprecate this field, + // since in Zipkin realm this v1 version is considered to be not preferrable anymore.] HTTP_JSON_V1 = 0 [deprecated = true]; // Zipkin API v2, JSON over HTTP. From f413994d2267905b535f70b02f8667ba5fe248cf Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 27 Aug 2019 23:46:21 +0700 Subject: [PATCH 42/48] Fix Signed-off-by: Dhi Aurrahman --- api/envoy/config/trace/v2/trace.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/trace/v2/trace.proto b/api/envoy/config/trace/v2/trace.proto index 3ded1904f868..2845b742dacf 100644 --- a/api/envoy/config/trace/v2/trace.proto +++ b/api/envoy/config/trace/v2/trace.proto @@ -93,7 +93,7 @@ message ZipkinConfig { // before this is added, the corresponding Zipkin collector expected to receive v1 payload. // Hence the motivation of adding HTTP_JSON_V1 as the default is to avoid a breaking change when // user upgrading Envoy with this change. Furthermore, we also immediately deprecate this field, - // since in Zipkin realm this v1 version is considered to be not preferrable anymore.] + // since in Zipkin realm this v1 version is considered to be not preferable anymore.] HTTP_JSON_V1 = 0 [deprecated = true]; // Zipkin API v2, JSON over HTTP. From 17e24d8f235d173dd5d20f016dcccc30a6ae2d92 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 28 Aug 2019 17:28:40 +0700 Subject: [PATCH 43/48] Comments Signed-off-by: Dhi Aurrahman --- .../extensions/tracers/zipkin/span_buffer.cc | 8 ++++---- source/extensions/tracers/zipkin/span_buffer.h | 18 +++++++++--------- .../tracers/zipkin/tracer_interface.h | 2 +- source/extensions/tracers/zipkin/util.cc | 15 +++------------ .../tracers/zipkin/zipkin_core_types.h | 10 ++++++---- .../tracers/zipkin/zipkin_tracer_impl.cc | 16 ++++++++-------- .../tracers/zipkin/zipkin_tracer_impl.h | 7 ++++--- 7 files changed, 35 insertions(+), 41 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index cfe444d02c79..8195039aa5ee 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -58,7 +58,7 @@ SerializerPtr SpanBuffer::makeSerializer( } } -std::string JsonV1Serializer::serialize(std::vector&& zipkin_spans) { +std::string JsonV1Serializer::serialize(const std::vector& zipkin_spans) { const std::string serialized_elements = absl::StrJoin(zipkin_spans, ",", [](std::string* element, Span zipkin_span) { absl::StrAppend(element, zipkin_span.toJson()); @@ -69,7 +69,7 @@ std::string JsonV1Serializer::serialize(std::vector&& zipkin_spans) { JsonV2Serializer::JsonV2Serializer(const bool shared_span_context) : shared_span_context_{shared_span_context} {} -std::string JsonV2Serializer::serialize(std::vector&& zipkin_spans) { +std::string JsonV2Serializer::serialize(const std::vector& zipkin_spans) { const std::string serialized_elements = absl::StrJoin(zipkin_spans, ",", [this](std::string* out, const Span& zipkin_span) { absl::StrAppend(out, @@ -121,7 +121,7 @@ JsonV2Serializer::toListOfSpans(const Span& zipkin_span) const { tags[binary_annotation.key()] = binary_annotation.value(); } - spans.push_back(span); + spans.push_back(std::move(span)); } return spans; } @@ -150,7 +150,7 @@ JsonV2Serializer::toProtoEndpoint(const Endpoint& zipkin_endpoint) const { ProtobufSerializer::ProtobufSerializer(const bool shared_span_context) : shared_span_context_{shared_span_context} {} -std::string ProtobufSerializer::serialize(std::vector&& zipkin_spans) { +std::string ProtobufSerializer::serialize(const std::vector& zipkin_spans) { zipkin::proto3::ListOfSpans spans; for (const Span& zipkin_span : zipkin_spans) { spans.MergeFrom(toListOfSpans(zipkin_span)); diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index 927c9f388d9f..d1bb7124f365 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -29,7 +29,7 @@ class SpanBuffer { * span context. */ SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, - const bool shared_span_context); + bool shared_span_context); /** * Constructor that initializes a buffer with the given size. @@ -41,7 +41,7 @@ class SpanBuffer { * @param size The desired buffer size. */ SpanBuffer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, - const bool shared_span_context, uint64_t size); + bool shared_span_context, uint64_t size); /** * Allocates space for an empty buffer or resizes a previously-allocated one. @@ -74,12 +74,12 @@ class SpanBuffer { * @return std::string the contents of the buffer, a collection of serialized pending Zipkin * spans. */ - std::string serialize() { return serializer_->serialize(std::move(span_buffer_)); } + std::string serialize() { return serializer_->serialize(span_buffer_); } private: SerializerPtr makeSerializer(const envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion& version, - const bool shared_span_context); + bool shared_span_context); // We use a pre-allocated vector to improve performance std::vector span_buffer_; @@ -100,7 +100,7 @@ class JsonV1Serializer : public Serializer { * Serialize list of Zipkin spans into Zipkin v1 JSON array. * @return std::string serialized pending spans as Zipkin v1 JSON array. */ - std::string serialize(std::vector&& pending_spans) override; + std::string serialize(const std::vector& pending_spans) override; }; /** @@ -109,13 +109,13 @@ class JsonV1Serializer : public Serializer { */ class JsonV2Serializer : public Serializer { public: - JsonV2Serializer(const bool shared_span_context); + JsonV2Serializer(bool shared_span_context); /** * Serialize list of Zipkin spans into Zipkin v2 JSON array. * @return std::string serialized pending spans as Zipkin v2 JSON array. */ - std::string serialize(std::vector&& pending_spans) override; + std::string serialize(const std::vector& pending_spans) override; private: const std::vector toListOfSpans(const Span& zipkin_span) const; @@ -130,13 +130,13 @@ class JsonV2Serializer : public Serializer { */ class ProtobufSerializer : public Serializer { public: - ProtobufSerializer(const bool shared_span_context); + ProtobufSerializer(bool shared_span_context); /** * Serialize list of Zipkin spans into Zipkin v2 zipkin::proto3::ListOfSpans. * @return std::string serialized pending spans as Zipkin zipkin::proto3::ListOfSpans. */ - std::string serialize(std::vector&& pending_spans) override; + std::string serialize(const std::vector& pending_spans) override; private: const zipkin::proto3::ListOfSpans toListOfSpans(const Span& zipkin_span) const; diff --git a/source/extensions/tracers/zipkin/tracer_interface.h b/source/extensions/tracers/zipkin/tracer_interface.h index 1f31b5871660..c56e130e2868 100644 --- a/source/extensions/tracers/zipkin/tracer_interface.h +++ b/source/extensions/tracers/zipkin/tracer_interface.h @@ -47,7 +47,7 @@ class Serializer { * * @return std::string serialized buffered pending spans. */ - virtual std::string serialize(std::vector&& spans) PURE; + virtual std::string serialize(const std::vector& spans) PURE; }; using SerializerPtr = std::unique_ptr; diff --git a/source/extensions/tracers/zipkin/util.cc b/source/extensions/tracers/zipkin/util.cc index d18eff673b04..a8c6a17558d3 100644 --- a/source/extensions/tracers/zipkin/util.cc +++ b/source/extensions/tracers/zipkin/util.cc @@ -11,6 +11,8 @@ #include "rapidjson/stringbuffer.h" #include "rapidjson/writer.h" +#include "absl/strings/str_join.h" + namespace Envoy { namespace Extensions { namespace Tracers { @@ -33,18 +35,7 @@ void Util::mergeJsons(std::string& target, const std::string& source, void Util::addArrayToJson(std::string& target, const std::vector& json_array, const std::string& field_name) { - std::string stringified_json_array = "["; - - if (!json_array.empty()) { - stringified_json_array += json_array[0]; - for (auto it = json_array.begin() + 1; it != json_array.end(); it++) { - stringified_json_array += ","; - stringified_json_array += *it; - } - } - stringified_json_array += "]"; - - mergeJsons(target, stringified_json_array, field_name); + mergeJsons(target, absl::StrCat("[", absl::StrJoin(json_array, ","), "]"), field_name); } uint64_t Util::generateRandom64(TimeSource& time_source) { diff --git a/source/extensions/tracers/zipkin/zipkin_core_types.h b/source/extensions/tracers/zipkin/zipkin_core_types.h index 0aac3fe545e6..9de9f4871620 100644 --- a/source/extensions/tracers/zipkin/zipkin_core_types.h +++ b/source/extensions/tracers/zipkin/zipkin_core_types.h @@ -12,6 +12,7 @@ #include "extensions/tracers/zipkin/tracer_interface.h" #include "extensions/tracers/zipkin/util.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -504,7 +505,7 @@ class Span : public ZipkinBase { */ const std::string traceIdAsHexString() const { return trace_id_high_.has_value() - ? Hex::uint64ToHex(trace_id_high_.value()) + Hex::uint64ToHex(trace_id_) + ? absl::StrCat(Hex::uint64ToHex(trace_id_high_.value()), Hex::uint64ToHex(trace_id_)) : Hex::uint64ToHex(trace_id_); } @@ -513,9 +514,10 @@ class Span : public ZipkinBase { */ const std::string traceIdAsByteString() const { // https://github.com/openzipkin/zipkin-api/blob/v0.2.1/zipkin.proto#L60-L61. - return trace_id_high_.has_value() ? Util::toBigEndianByteString(trace_id_high_.value()) + - Util::toBigEndianByteString(trace_id_) - : Util::toBigEndianByteString(trace_id_); + return trace_id_high_.has_value() + ? absl::StrCat(Util::toBigEndianByteString(trace_id_high_.value()), + Util::toBigEndianByteString(trace_id_)) + : Util::toBigEndianByteString(trace_id_); } /** diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index 254cca541631..344b4395905c 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -78,15 +78,15 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, CollectorInfo collector; if (!zipkin_config.collector_endpoint().empty()) { - collector.endpoint = zipkin_config.collector_endpoint(); + collector.endpoint_ = zipkin_config.collector_endpoint(); } // The current default version of collector_endpoint_version is HTTP_JSON_V1. - collector.version = zipkin_config.collector_endpoint_version(); + collector.version_ = zipkin_config.collector_endpoint_version(); const bool trace_id_128bit = zipkin_config.trace_id_128bit(); const bool shared_span_context = PROTOBUF_GET_WRAPPED_OR_DEFAULT( zipkin_config, shared_span_context, ZipkinCoreConstants::get().DEFAULT_SHARED_SPAN_CONTEXT); - collector.shared_span_context = shared_span_context; + collector.shared_span_context_ = shared_span_context; tls_->set([this, collector, &random_generator, trace_id_128bit, shared_span_context]( Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { @@ -129,8 +129,8 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, const CollectorInfo& collector) : driver_(driver), - collector_(collector), span_buffer_{ - new SpanBuffer(collector.version, collector.shared_span_context)} { + collector_(collector), span_buffer_{std::make_unique( + collector.version_, collector.shared_span_context_)} { flush_timer_ = dispatcher.createTimer([this]() -> void { driver_.tracerStats().timer_flushed_.inc(); flushSpans(); @@ -146,7 +146,7 @@ ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, ReporterPtr ReporterImpl::NewInstance(Driver& driver, Event::Dispatcher& dispatcher, const CollectorInfo& collector) { - return ReporterPtr(new ReporterImpl(driver, dispatcher, collector)); + return ReporterPtr(std::make_unique(driver, dispatcher, collector)); } // TODO(fabolive): Need to avoid the copy to improve performance. @@ -173,10 +173,10 @@ void ReporterImpl::flushSpans() { const std::string request_body = span_buffer_->serialize(); Http::MessagePtr message(new Http::RequestMessageImpl()); message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); - message->headers().insertPath().value(collector_.endpoint); + message->headers().insertPath().value(collector_.endpoint_); message->headers().insertHost().value(driver_.cluster()->name()); message->headers().insertContentType().value().setReference( - collector_.version == envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO + collector_.version_ == envoy::config::trace::v2::ZipkinConfig::HTTP_PROTO ? Http::Headers::get().ContentTypeValues.Protobuf : Http::Headers::get().ContentTypeValues.Json); diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h index bd68c4b95928..4cecd015d6a3 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.h +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.h @@ -11,6 +11,7 @@ #include "extensions/tracers/zipkin/span_buffer.h" #include "extensions/tracers/zipkin/tracer.h" +#include "extensions/tracers/zipkin/zipkin_core_constants.h" namespace Envoy { namespace Extensions { @@ -143,15 +144,15 @@ class Driver : public Tracing::Driver { struct CollectorInfo { // The Zipkin collector endpoint/path to receive the collected trace data. e.g. /api/v1/spans if // HTTP_JSON_V1 or /api/v2/spans otherwise. - std::string endpoint{ZipkinCoreConstants::get().DEFAULT_COLLECTOR_ENDPOINT}; + std::string endpoint_{ZipkinCoreConstants::get().DEFAULT_COLLECTOR_ENDPOINT}; // The version of the collector. This is related to endpoint's supported payload specification and // transport. Currently it defaults to envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1. In // the future, we will throw when collector_endpoint_version is not specified. - envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion version{ + envoy::config::trace::v2::ZipkinConfig::CollectorEndpointVersion version_{ envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1}; - bool shared_span_context{true}; + bool shared_span_context_{ZipkinCoreConstants::get().DEFAULT_SHARED_SPAN_CONTEXT}; }; /** From e602b50ca626a500e64884341e3b5ba2a2df8fe9 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 28 Aug 2019 17:29:15 +0700 Subject: [PATCH 44/48] Fix Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/util.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/tracers/zipkin/util.cc b/source/extensions/tracers/zipkin/util.cc index a8c6a17558d3..18eea42daf87 100644 --- a/source/extensions/tracers/zipkin/util.cc +++ b/source/extensions/tracers/zipkin/util.cc @@ -7,12 +7,11 @@ #include "common/common/hex.h" #include "common/common/utility.h" +#include "absl/strings/str_join.h" #include "rapidjson/document.h" #include "rapidjson/stringbuffer.h" #include "rapidjson/writer.h" -#include "absl/strings/str_join.h" - namespace Envoy { namespace Extensions { namespace Tracers { From 3f4c927fa888facab2a6179771f6dff714e15c7e Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 28 Aug 2019 17:40:57 +0700 Subject: [PATCH 45/48] More const-ness Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/span_buffer.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.h b/source/extensions/tracers/zipkin/span_buffer.h index d1bb7124f365..a5718600129e 100644 --- a/source/extensions/tracers/zipkin/span_buffer.h +++ b/source/extensions/tracers/zipkin/span_buffer.h @@ -71,10 +71,14 @@ class SpanBuffer { uint64_t pendingSpans() { return span_buffer_.size(); } /** + * Serializes std::vector span_buffer_ to std::string as payload for the reporter when the + * reporter does spans flushing. This function does only serialization and does not clear + * span_buffer_. + * * @return std::string the contents of the buffer, a collection of serialized pending Zipkin * spans. */ - std::string serialize() { return serializer_->serialize(span_buffer_); } + std::string serialize() const { return serializer_->serialize(span_buffer_); } private: SerializerPtr From 527f62cacb2111a62765a3b55af5986a6ee1a531 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 28 Aug 2019 18:00:45 +0700 Subject: [PATCH 46/48] make_unique and make_shared Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/tracer.cc | 4 ++-- .../tracers/zipkin/zipkin_tracer_impl.cc | 20 ++++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/source/extensions/tracers/zipkin/tracer.cc b/source/extensions/tracers/zipkin/tracer.cc index 8b21bef8f23a..5485fc643186 100644 --- a/source/extensions/tracers/zipkin/tracer.cc +++ b/source/extensions/tracers/zipkin/tracer.cc @@ -28,7 +28,7 @@ SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::string& span } // Create an all-new span, with no parent id - SpanPtr span_ptr(new Span(time_source_)); + SpanPtr span_ptr = std::make_unique(time_source_); span_ptr->setName(span_name); uint64_t random_number = random_generator_.random(); span_ptr->setId(random_number); @@ -57,7 +57,7 @@ SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::string& span SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::string& span_name, SystemTime timestamp, SpanContext& previous_context) { - SpanPtr span_ptr(new Span(time_source_)); + SpanPtr span_ptr = std::make_unique(time_source_); Annotation annotation; uint64_t timestamp_micro; diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index 344b4395905c..eae1a46fc369 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -58,7 +58,7 @@ Tracing::SpanPtr ZipkinSpan::spawnChild(const Tracing::Config& config, const std SystemTime start_time) { SpanContext context(span_); return Tracing::SpanPtr{ - new ZipkinSpan(*tracer_.startSpan(config, name, start_time, context), tracer_)}; + std::make_unique(*tracer_.startSpan(config, name, start_time, context), tracer_)}; } Driver::TlsTracer::TlsTracer(TracerPtr&& tracer, Driver& driver) @@ -90,11 +90,13 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, tls_->set([this, collector, &random_generator, trace_id_128bit, shared_span_context]( Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { - TracerPtr tracer(new Tracer(local_info_.clusterName(), local_info_.address(), random_generator, - trace_id_128bit, shared_span_context, time_source_)); + TracerPtr tracer = + std::make_unique(local_info_.clusterName(), local_info_.address(), random_generator, + trace_id_128bit, shared_span_context, time_source_); tracer->setReporter( ReporterImpl::NewInstance(std::ref(*this), std::ref(dispatcher), collector)); - return ThreadLocal::ThreadLocalObjectSharedPtr{new TlsTracer(std::move(tracer), *this)}; + return ThreadLocal::ThreadLocalObjectSharedPtr{ + std::make_shared(std::move(tracer), *this)}; }); } @@ -119,11 +121,11 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa } } catch (const ExtractorException& e) { - return Tracing::SpanPtr(new Tracing::NullSpan()); + return Tracing::SpanPtr{std::make_unique()}; } - ZipkinSpanPtr active_span(new ZipkinSpan(*new_zipkin_span, tracer)); - return active_span; + // Return the active Zipkin span. + return ZipkinSpanPtr{std::make_unique(*new_zipkin_span, tracer)}; } ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, @@ -171,7 +173,7 @@ void ReporterImpl::flushSpans() { if (span_buffer_->pendingSpans()) { driver_.tracerStats().spans_sent_.add(span_buffer_->pendingSpans()); const std::string request_body = span_buffer_->serialize(); - Http::MessagePtr message(new Http::RequestMessageImpl()); + Http::MessagePtr message = std::make_unique(); message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); message->headers().insertPath().value(collector_.endpoint_); message->headers().insertHost().value(driver_.cluster()->name()); @@ -180,7 +182,7 @@ void ReporterImpl::flushSpans() { ? Http::Headers::get().ContentTypeValues.Protobuf : Http::Headers::get().ContentTypeValues.Json); - Buffer::InstancePtr body(new Buffer::OwnedImpl()); + Buffer::InstancePtr body = std::make_unique(); body->add(request_body); message->body() = std::move(body); From c409f9186a7b45bc91edec3dc604bdc227f71f76 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 29 Aug 2019 07:34:05 +0700 Subject: [PATCH 47/48] Comments Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/tracer.cc | 2 +- source/extensions/tracers/zipkin/tracer.h | 8 +++++++- .../tracers/zipkin/zipkin_tracer_impl.cc | 15 +++++++-------- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/source/extensions/tracers/zipkin/tracer.cc b/source/extensions/tracers/zipkin/tracer.cc index 5485fc643186..aff1d659c12d 100644 --- a/source/extensions/tracers/zipkin/tracer.cc +++ b/source/extensions/tracers/zipkin/tracer.cc @@ -56,7 +56,7 @@ SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::string& span } SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::string& span_name, - SystemTime timestamp, SpanContext& previous_context) { + SystemTime timestamp, const SpanContext& previous_context) { SpanPtr span_ptr = std::make_unique(time_source_); Annotation annotation; uint64_t timestamp_micro; diff --git a/source/extensions/tracers/zipkin/tracer.h b/source/extensions/tracers/zipkin/tracer.h index effbb4b8eddc..d51e0645844a 100644 --- a/source/extensions/tracers/zipkin/tracer.h +++ b/source/extensions/tracers/zipkin/tracer.h @@ -72,6 +72,7 @@ class Tracer : public TracerInterface { * @param config The tracing configuration * @param span_name Name of the new span. * @param start_time The time indicating the beginning of the span. + * @return SpanPtr The root span. */ SpanPtr startSpan(const Tracing::Config&, const std::string& span_name, SystemTime timestamp); @@ -82,12 +83,15 @@ class Tracer : public TracerInterface { * @param span_name Name of the new span. * @param start_time The time indicating the beginning of the span. * @param previous_context The context of the span preceding the one to be created. + * @return SpanPtr The child span. */ SpanPtr startSpan(const Tracing::Config&, const std::string& span_name, SystemTime timestamp, - SpanContext& previous_context); + const SpanContext& previous_context); /** * TracerInterface::reportSpan. + * + * @param span The span to be reported. */ void reportSpan(Span&& span) override; @@ -103,6 +107,8 @@ class Tracer : public TracerInterface { /** * Associates a Reporter object with this Tracer. + * + * @param The span reporter. */ void setReporter(ReporterPtr reporter); diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index eae1a46fc369..75bc8d8f4b70 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -56,9 +56,9 @@ void ZipkinSpan::setSampled(bool sampled) { span_.setSampled(sampled); } Tracing::SpanPtr ZipkinSpan::spawnChild(const Tracing::Config& config, const std::string& name, SystemTime start_time) { - SpanContext context(span_); - return Tracing::SpanPtr{ - std::make_unique(*tracer_.startSpan(config, name, start_time, context), tracer_)}; + SpanContext previous_context(span_); + return std::make_unique( + *tracer_.startSpan(config, name, start_time, previous_context), tracer_); } Driver::TlsTracer::TlsTracer(TracerPtr&& tracer, Driver& driver) @@ -95,8 +95,7 @@ Driver::Driver(const envoy::config::trace::v2::ZipkinConfig& zipkin_config, trace_id_128bit, shared_span_context, time_source_); tracer->setReporter( ReporterImpl::NewInstance(std::ref(*this), std::ref(dispatcher), collector)); - return ThreadLocal::ThreadLocalObjectSharedPtr{ - std::make_shared(std::move(tracer), *this)}; + return std::make_shared(std::move(tracer), *this); }); } @@ -121,11 +120,11 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa } } catch (const ExtractorException& e) { - return Tracing::SpanPtr{std::make_unique()}; + return std::make_unique(); } // Return the active Zipkin span. - return ZipkinSpanPtr{std::make_unique(*new_zipkin_span, tracer)}; + return std::make_unique(*new_zipkin_span, tracer); } ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, @@ -148,7 +147,7 @@ ReporterImpl::ReporterImpl(Driver& driver, Event::Dispatcher& dispatcher, ReporterPtr ReporterImpl::NewInstance(Driver& driver, Event::Dispatcher& dispatcher, const CollectorInfo& collector) { - return ReporterPtr(std::make_unique(driver, dispatcher, collector)); + return std::make_unique(driver, dispatcher, collector); } // TODO(fabolive): Need to avoid the copy to improve performance. From 39767243e57af47ec40a594ef7cc4cce7b0501cd Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 29 Aug 2019 07:36:08 +0700 Subject: [PATCH 48/48] Remove TODOs Signed-off-by: Dhi Aurrahman --- source/extensions/tracers/zipkin/span_buffer.cc | 1 - source/extensions/tracers/zipkin/zipkin_tracer_impl.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/source/extensions/tracers/zipkin/span_buffer.cc b/source/extensions/tracers/zipkin/span_buffer.cc index 8195039aa5ee..66bb96a9463b 100644 --- a/source/extensions/tracers/zipkin/span_buffer.cc +++ b/source/extensions/tracers/zipkin/span_buffer.cc @@ -24,7 +24,6 @@ SpanBuffer::SpanBuffer( allocateBuffer(size); } -// TODO(fabolive): Need to avoid the copy to improve performance. bool SpanBuffer::addSpan(Span&& span) { const auto& annotations = span.annotations(); if (span_buffer_.size() == span_buffer_.capacity() || annotations.empty() || diff --git a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc index 75bc8d8f4b70..3b269f742c01 100644 --- a/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc +++ b/source/extensions/tracers/zipkin/zipkin_tracer_impl.cc @@ -150,7 +150,6 @@ ReporterPtr ReporterImpl::NewInstance(Driver& driver, Event::Dispatcher& dispatc return std::make_unique(driver, dispatcher, collector); } -// TODO(fabolive): Need to avoid the copy to improve performance. void ReporterImpl::reportSpan(Span&& span) { span_buffer_->addSpan(std::move(span));