From 2499ea17b720a8bdeb8d4bfa2a4b4e32e1e6c7f5 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 9 Oct 2023 14:28:59 +0200 Subject: [PATCH 01/15] add sampler interface and AlwaysOnSampler --- api/BUILD | 1 + api/envoy/config/trace/v3/opentelemetry.proto | 5 + .../tracers/opentelemetry/samplers/v3/BUILD | 7 + .../samplers/v3/always_on_sampler.proto | 17 ++ source/extensions/extensions_build_config.bzl | 6 + source/extensions/extensions_metadata.yaml | 5 + source/extensions/tracers/opentelemetry/BUILD | 1 + .../opentelemetry_tracer_impl.cc | 45 +++-- .../tracers/opentelemetry/samplers/BUILD | 25 +++ .../opentelemetry/samplers/always_on/BUILD | 33 ++++ .../samplers/always_on/always_on_sampler.cc | 34 ++++ .../samplers/always_on/always_on_sampler.h | 38 ++++ .../samplers/always_on/config.cc | 27 +++ .../opentelemetry/samplers/always_on/config.h | 42 +++++ .../tracers/opentelemetry/samplers/sampler.h | 111 ++++++++++++ .../tracers/opentelemetry/tracer.cc | 24 ++- .../extensions/tracers/opentelemetry/tracer.h | 14 +- .../tracers/opentelemetry/samplers/BUILD | 23 +++ .../opentelemetry/samplers/always_on/BUILD | 37 ++++ .../always_on/always_on_sampler_test.cc | 56 ++++++ .../samplers/always_on/config_test.cc | 37 ++++ .../opentelemetry/samplers/sampler_test.cc | 168 ++++++++++++++++++ 22 files changed, 738 insertions(+), 18 deletions(-) create mode 100644 api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD create mode 100644 api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto create mode 100644 source/extensions/tracers/opentelemetry/samplers/BUILD create mode 100644 source/extensions/tracers/opentelemetry/samplers/always_on/BUILD create mode 100644 source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc create mode 100644 source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h create mode 100644 source/extensions/tracers/opentelemetry/samplers/always_on/config.cc create mode 100644 source/extensions/tracers/opentelemetry/samplers/always_on/config.h create mode 100644 source/extensions/tracers/opentelemetry/samplers/sampler.h create mode 100644 test/extensions/tracers/opentelemetry/samplers/BUILD create mode 100644 test/extensions/tracers/opentelemetry/samplers/always_on/BUILD create mode 100644 test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc create mode 100644 test/extensions/tracers/opentelemetry/samplers/sampler_test.cc diff --git a/api/BUILD b/api/BUILD index 846242a3d5442..2e53436777a14 100644 --- a/api/BUILD +++ b/api/BUILD @@ -300,6 +300,7 @@ proto_library( "//envoy/extensions/stat_sinks/graphite_statsd/v3:pkg", "//envoy/extensions/stat_sinks/open_telemetry/v3:pkg", "//envoy/extensions/stat_sinks/wasm/v3:pkg", + "//envoy/extensions/tracers/opentelemetry/samplers/v3:pkg", "//envoy/extensions/transport_sockets/alts/v3:pkg", "//envoy/extensions/transport_sockets/http_11_proxy/v3:pkg", "//envoy/extensions/transport_sockets/internal_upstream/v3:pkg", diff --git a/api/envoy/config/trace/v3/opentelemetry.proto b/api/envoy/config/trace/v3/opentelemetry.proto index e9c7430dcfdd7..6059f8ad76acf 100644 --- a/api/envoy/config/trace/v3/opentelemetry.proto +++ b/api/envoy/config/trace/v3/opentelemetry.proto @@ -2,6 +2,7 @@ syntax = "proto3"; package envoy.config.trace.v3; +import "envoy/config/core/v3/extension.proto"; import "envoy/config/core/v3/grpc_service.proto"; import "udpa/annotations/status.proto"; @@ -25,4 +26,8 @@ message OpenTelemetryConfig { // The name for the service. This will be populated in the ResourceSpan Resource attributes. // If it is not provided, it will default to "unknown_service:envoy". string service_name = 2; + + // optional sampler which is used in the opentelemetry tracer + // [#extension-category: envoy.tracers.opentelemetry.samplers] + optional core.v3.TypedExtensionConfig sampler = 3; } diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD new file mode 100644 index 0000000000000..e8aa6b3a4bfd5 --- /dev/null +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD @@ -0,0 +1,7 @@ +load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package") + +licenses(["notice"]) # Apache 2 + +api_proto_package( + deps = ["@com_github_cncf_udpa//udpa/annotations:pkg"], +) diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto new file mode 100644 index 0000000000000..ae465a3bd5d4d --- /dev/null +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto @@ -0,0 +1,17 @@ +syntax = "proto3"; + +package envoy.extensions.tracers.opentelemetry.samplers.v3; + +import "udpa/annotations/status.proto"; + +option java_package = "io.envoyproxy.envoy.extensions.tracers.opentelemetry.samplers.v3"; +option java_outer_classname = "AlwaysOnSamplerConfigProto"; +option java_multiple_files = true; +option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/tracers/opentelemetry/samplers/v3;samplersv3"; +option (udpa.annotations.file_status).package_version_status = ACTIVE; + +// [#protodoc-title: Always On Sampler config] +// [#extension: envoy.tracers.opentelemetry.samplers.always_on] + +message AlwaysOnSamplerConfig { +} \ No newline at end of file diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index 2578c123b020e..27df83cae30d3 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -260,6 +260,12 @@ EXTENSIONS = { "envoy.tracers.skywalking": "//source/extensions/tracers/skywalking:config", "envoy.tracers.opentelemetry": "//source/extensions/tracers/opentelemetry:config", + # + # Sampler + # + + "envoy.tracers.opentelemetry.samplers.always_on": "//source/extensions/tracers/opentelemetry/samplers/always_on:config", + # # Transport sockets # diff --git a/source/extensions/extensions_metadata.yaml b/source/extensions/extensions_metadata.yaml index 3cec56ae01d5a..9deec64ab669a 100644 --- a/source/extensions/extensions_metadata.yaml +++ b/source/extensions/extensions_metadata.yaml @@ -1136,6 +1136,11 @@ envoy.tracers.opentelemetry: status: wip type_urls: - envoy.config.trace.v3.OpenTelemetryConfig +envoy.tracers.opentelemetry.samplers.all: + categories: + - envoy.tracers.opentelemetry.samplers + security_posture: unknown + status: wip envoy.tracers.skywalking: categories: - envoy.tracers diff --git a/source/extensions/tracers/opentelemetry/BUILD b/source/extensions/tracers/opentelemetry/BUILD index 3a36a3f91fd50..5b05a2da875e9 100644 --- a/source/extensions/tracers/opentelemetry/BUILD +++ b/source/extensions/tracers/opentelemetry/BUILD @@ -41,6 +41,7 @@ envoy_cc_library( "//source/common/config:utility_lib", "//source/common/tracing:http_tracer_lib", "//source/extensions/tracers/common:factory_base_lib", + "//source/extensions/tracers/opentelemetry/samplers:sampler_lib", "@envoy_api//envoy/config/trace/v3:pkg_cc_proto", "@opentelemetry_proto//:trace_cc_proto", ], diff --git a/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc b/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc index a3119c187e836..c63566bc2d398 100644 --- a/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc +++ b/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc @@ -8,6 +8,7 @@ #include "source/common/common/logger.h" #include "source/common/config/utility.h" #include "source/common/tracing/http_tracer_impl.h" +#include "source/extensions/tracers/opentelemetry/samplers/sampler.h" #include "opentelemetry/proto/collector/trace/v1/trace_service.pb.h" #include "opentelemetry/proto/trace/v1/trace.pb.h" @@ -26,23 +27,37 @@ Driver::Driver(const envoy::config::trace::v3::OpenTelemetryConfig& opentelemetr tracing_stats_{OPENTELEMETRY_TRACER_STATS( POOL_COUNTER_PREFIX(context.serverFactoryContext().scope(), "tracing.opentelemetry"))} { auto& factory_context = context.serverFactoryContext(); - // Create the tracer in Thread Local Storage. - tls_slot_ptr_->set([opentelemetry_config, &factory_context, this](Event::Dispatcher& dispatcher) { - OpenTelemetryGrpcTraceExporterPtr exporter; - if (opentelemetry_config.has_grpc_service()) { - Grpc::AsyncClientFactoryPtr&& factory = - factory_context.clusterManager().grpcAsyncClientManager().factoryForGrpcService( - opentelemetry_config.grpc_service(), factory_context.scope(), true); - const Grpc::RawAsyncClientSharedPtr& async_client_shared_ptr = - factory->createUncachedRawAsyncClient(); - exporter = std::make_unique(async_client_shared_ptr); + + // Create the samper if configured + SamplerSharedPtr sampler; + if (opentelemetry_config.has_sampler()) { + auto& sampler_config = opentelemetry_config.sampler(); + auto* factory = Envoy::Config::Utility::getFactory(sampler_config); + if (!factory) { + throw EnvoyException(fmt::format("Sampler factory not found: '{}'", sampler_config.name())); } - TracerPtr tracer = std::make_unique( - std::move(exporter), factory_context.timeSource(), factory_context.api().randomGenerator(), - factory_context.runtime(), dispatcher, tracing_stats_, opentelemetry_config.service_name()); + sampler = factory->createSampler(sampler_config.typed_config(), context); + } + + // Create the tracer in Thread Local Storage. + tls_slot_ptr_->set( + [opentelemetry_config, &factory_context, this, sampler](Event::Dispatcher& dispatcher) { + OpenTelemetryGrpcTraceExporterPtr exporter; + if (opentelemetry_config.has_grpc_service()) { + Grpc::AsyncClientFactoryPtr&& factory = + factory_context.clusterManager().grpcAsyncClientManager().factoryForGrpcService( + opentelemetry_config.grpc_service(), factory_context.scope(), true); + const Grpc::RawAsyncClientSharedPtr& async_client_shared_ptr = + factory->createUncachedRawAsyncClient(); + exporter = std::make_unique(async_client_shared_ptr); + } + TracerPtr tracer = std::make_unique( + std::move(exporter), factory_context.timeSource(), + factory_context.api().randomGenerator(), factory_context.runtime(), dispatcher, + tracing_stats_, opentelemetry_config.service_name(), sampler); - return std::make_shared(std::move(tracer)); - }); + return std::make_shared(std::move(tracer)); + }); } Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, diff --git a/source/extensions/tracers/opentelemetry/samplers/BUILD b/source/extensions/tracers/opentelemetry/samplers/BUILD new file mode 100644 index 0000000000000..32a1005b11e80 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/BUILD @@ -0,0 +1,25 @@ +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_extension_package", +) + +licenses(["notice"]) # Apache 2 + +envoy_extension_package() + +envoy_cc_library( + name = "sampler_lib", + srcs = [ + ], + hdrs = [ + "sampler.h", + ], + deps = [ + "//envoy/config:typed_config_interface", + "//envoy/server:tracer_config_interface", + "//source/common/common:logger_lib", + "//source/common/config:utility_lib", + "@opentelemetry_proto//:trace_cc_proto", + ], +) diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/BUILD b/source/extensions/tracers/opentelemetry/samplers/always_on/BUILD new file mode 100644 index 0000000000000..744607330d570 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/BUILD @@ -0,0 +1,33 @@ +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_extension", + "envoy_cc_library", + "envoy_extension_package", +) + +licenses(["notice"]) # Apache 2 + +envoy_extension_package() + +envoy_cc_extension( + name = "config", + srcs = ["config.cc"], + hdrs = ["config.h"], + deps = [ + ":always_on_sampler_lib", + "//envoy/registry", + "//source/common/config:utility_lib", + "@envoy_api//envoy/extensions/tracers/opentelemetry/samplers/v3:pkg_cc_proto", + ], +) + +envoy_cc_library( + name = "always_on_sampler_lib", + srcs = ["always_on_sampler.cc"], + hdrs = ["always_on_sampler.h"], + deps = [ + "//source/common/config:datasource_lib", + "//source/extensions/tracers/opentelemetry:opentelemetry_tracer_lib", + "//source/extensions/tracers/opentelemetry/samplers:sampler_lib", + ], +) diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc new file mode 100644 index 0000000000000..4ca6bffd3b09b --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc @@ -0,0 +1,34 @@ +#include "source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h" + +#include +#include +#include + +#include "source/common/config/datasource.h" +#include "source/extensions/tracers/opentelemetry/span_context.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +SamplingResult +AlwaysOnSampler::shouldSample(const absl::StatusOr& parent_context, + const std::string& /*trace_id*/, const std::string& /*name*/, + ::opentelemetry::proto::trace::v1::Span::SpanKind /*spankind*/, + const std::map& /*attributes*/, + const std::set& /*links*/) { + SamplingResult result; + result.decision = Decision::RECORD_AND_SAMPLE; + if (parent_context.ok()) { + result.tracestate = parent_context.value().tracestate(); + } + return result; +} + +std::string AlwaysOnSampler::getDescription() const { return "AlwaysOnSampler"; } + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h new file mode 100644 index 0000000000000..aaf0db4fa4bbc --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h @@ -0,0 +1,38 @@ +#pragma once + +#include "envoy/server/factory_context.h" + +#include "source/common/common/logger.h" +#include "source/common/config/datasource.h" +#include "source/extensions/tracers/opentelemetry/samplers/sampler.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * @brief A sampler which samples every span variable. + * https://opentelemetry.io/docs/specs/otel/trace/sdk/#alwayson + * - Returns RECORD_AND_SAMPLE always. + * - Description MUST be AlwaysOnSampler. + * + */ +class AlwaysOnSampler : public Sampler, Logger::Loggable { +public: + explicit AlwaysOnSampler(const Protobuf::Message& /*config*/, + Server::Configuration::TracerFactoryContext& /*context*/) {} + SamplingResult shouldSample(const absl::StatusOr& parent_context, + const std::string& trace_id, const std::string& name, + ::opentelemetry::proto::trace::v1::Span::SpanKind spankind, + const std::map& attributes, + const std::set& links) override; + std::string getDescription() const override; + +private: +}; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/config.cc b/source/extensions/tracers/opentelemetry/samplers/always_on/config.cc new file mode 100644 index 0000000000000..99288c4bf4696 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/config.cc @@ -0,0 +1,27 @@ +#include "source/extensions/tracers/opentelemetry/samplers/always_on/config.h" + +#include "envoy/server/tracer_config.h" + +#include "source/common/config/utility.h" +#include "source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +SamplerSharedPtr +AlwaysOnSamplerFactory::createSampler(const Protobuf::Message& config, + Server::Configuration::TracerFactoryContext& context) { + return std::make_shared(config, context); +} + +/** + * Static registration for the Env sampler factory. @see RegisterFactory. + */ +REGISTER_FACTORY(AlwaysOnSamplerFactory, SamplerFactory); + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/config.h b/source/extensions/tracers/opentelemetry/samplers/always_on/config.h new file mode 100644 index 0000000000000..5f93f43c0f1fd --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/config.h @@ -0,0 +1,42 @@ +#pragma once + +#include + +#include "envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.pb.h" +#include "envoy/registry/registry.h" + +#include "source/extensions/tracers/opentelemetry/samplers/sampler.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +/** + * Config registration for the AlwaysOnSampler. @see SamplerFactory. + */ +class AlwaysOnSamplerFactory : public SamplerFactory { +public: + /** + * @brief Create a Sampler. @see AlwaysOnSampler + * + * @param config Protobuf config for the sampler. + * @param context A reference to the TracerFactoryContext. + * @return SamplerSharedPtr + */ + SamplerSharedPtr createSampler(const Protobuf::Message& config, + Server::Configuration::TracerFactoryContext& context) override; + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return std::make_unique< + envoy::extensions::tracers::opentelemetry::samplers::v3::AlwaysOnSamplerConfig>(); + } + std::string name() const override { return "envoy.tracers.opentelemetry.samplers.always_on"; } +}; + +DECLARE_FACTORY(AlwaysOnSamplerFactory); + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/samplers/sampler.h b/source/extensions/tracers/opentelemetry/samplers/sampler.h new file mode 100644 index 0000000000000..7627cc8e8fe94 --- /dev/null +++ b/source/extensions/tracers/opentelemetry/samplers/sampler.h @@ -0,0 +1,111 @@ +#pragma once + +#include +#include +#include +#include + +#include "envoy/config/typed_config.h" +#include "envoy/server/tracer_config.h" +#include "envoy/tracing/trace_context.h" + +#include "absl/status/statusor.h" +#include "opentelemetry/proto/trace/v1/trace.pb.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +class SpanContext; + +enum class Decision { + // IsRecording will be false, the Span will not be recorded and all events and attributes will be + // dropped. + DROP, + // IsRecording will be true, but the Sampled flag MUST NOT be set. + RECORD_ONLY, + // IsRecording will be true and the Sampled flag MUST be set. + RECORD_AND_SAMPLE +}; + +struct SamplingResult { + /// @see Decision + Decision decision; + // A set of span Attributes that will also be added to the Span. Can be nullptr. + std::unique_ptr> attributes; + // A Tracestate that will be associated with the Span. If the sampler + // returns an empty Tracestate here, the Tracestate will be cleared, so samplers SHOULD normally + // return the passed-in Tracestate if they do not intend to change it + std::string tracestate; + + inline bool isRecording() const { + return decision == Decision::RECORD_ONLY || decision == Decision::RECORD_AND_SAMPLE; + } + + inline bool isSampled() const { return decision == Decision::RECORD_AND_SAMPLE; } +}; + +/** + * @brief The base type for all samplers + * see https://opentelemetry.io/docs/specs/otel/trace/sdk/#sampler + * + */ +class Sampler { +public: + virtual ~Sampler() = default; + + /** + * @brief Decides if a trace should be sampled. + * + * @param parent_context Span context decsribing the parent span. The Span's SpanContext may be + * invalid to indicate a root span. + * @param trace_id Trace id of the Span to be created. If the parent SpanContext contains a valid + * TraceId, they MUST always match. + * @param name Name of the Span to be created. + * @param spankind Span kind of the Span to be created. + * @param attributes Initial set of Attributes of the Span to be created. + * @param links Collection of links that will be associated with the Span to be created. + * @return SamplingResult @see SamplingResult + */ + virtual SamplingResult shouldSample(const absl::StatusOr& parent_context, + const std::string& trace_id, const std::string& name, + ::opentelemetry::proto::trace::v1::Span::SpanKind spankind, + const std::map& attributes, + const std::set& links) PURE; + + /** + * @brief Returns a sampler description or name. + * + * @return The sampler name or short description with the configuration. + */ + virtual std::string getDescription() const PURE; +}; + +using SamplerSharedPtr = std::shared_ptr; + +/* + * A factory for creating a sampler + */ +class SamplerFactory : public Envoy::Config::TypedFactory { +public: + ~SamplerFactory() override = default; + + /** + * @brief Creates a sampler + * @param config The sampler protobuf config. + * @param context The TracerFactoryContext. + * @return SamplerSharedPtr A sampler. + */ + virtual SamplerSharedPtr createSampler(const Protobuf::Message& config, + Server::Configuration::TracerFactoryContext& context) PURE; + + std::string category() const override { return "envoy.tracers.opentelemetry.samplers"; } +}; + +using SamplerFactoryPtr = std::unique_ptr; + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/opentelemetry/tracer.cc b/source/extensions/tracers/opentelemetry/tracer.cc index a344a253ab31e..0cd52768c4456 100644 --- a/source/extensions/tracers/opentelemetry/tracer.cc +++ b/source/extensions/tracers/opentelemetry/tracer.cc @@ -94,9 +94,9 @@ void Span::setTag(absl::string_view name, absl::string_view value) { Tracer::Tracer(OpenTelemetryGrpcTraceExporterPtr exporter, Envoy::TimeSource& time_source, Random::RandomGenerator& random, Runtime::Loader& runtime, Event::Dispatcher& dispatcher, OpenTelemetryTracerStats tracing_stats, - const std::string& service_name) + const std::string& service_name, SamplerSharedPtr sampler) : exporter_(std::move(exporter)), time_source_(time_source), random_(random), runtime_(runtime), - tracing_stats_(tracing_stats), service_name_(service_name) { + tracing_stats_(tracing_stats), service_name_(service_name), sampler_(sampler) { if (service_name.empty()) { service_name_ = std::string{kDefaultServiceName}; } @@ -142,6 +142,23 @@ void Tracer::flushSpans() { span_buffer_.clear(); } +void Tracer::callSampler(const absl::StatusOr span_context, Span& new_span, + const std::string& operation_name) { + if (!sampler_) { + return; + } + const auto sampling_result = sampler_->shouldSample( + span_context, operation_name, new_span.getTraceIdAsHex(), new_span.spankind(), {}, {}); + new_span.setSampled(sampling_result.isSampled()); + + if (sampling_result.attributes) { + for (auto const& attribute : *sampling_result.attributes) { + new_span.setTag(attribute.first, attribute.second); + } + } + new_span.setTracestate(sampling_result.tracestate); +} + void Tracer::sendSpan(::opentelemetry::proto::trace::v1::Span& span) { span_buffer_.push_back(span); const uint64_t min_flush_spans = @@ -162,6 +179,8 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str new_span.setTraceId(absl::StrCat(Hex::uint64ToHex(trace_id_high), Hex::uint64ToHex(trace_id))); uint64_t span_id = random_.random(); new_span.setId(Hex::uint64ToHex(span_id)); + absl::StatusOr span_context = absl::InvalidArgumentError("no parent span"); + callSampler(span_context, new_span, operation_name); return std::make_unique(new_span); } @@ -183,6 +202,7 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str if (!previous_span_context.tracestate().empty()) { new_span.setTracestate(std::string{previous_span_context.tracestate()}); } + callSampler(previous_span_context, new_span, operation_name); return std::make_unique(new_span); } diff --git a/source/extensions/tracers/opentelemetry/tracer.h b/source/extensions/tracers/opentelemetry/tracer.h index f80cb12fd3f76..905520ab7b491 100644 --- a/source/extensions/tracers/opentelemetry/tracer.h +++ b/source/extensions/tracers/opentelemetry/tracer.h @@ -11,6 +11,7 @@ #include "source/common/common/logger.h" #include "source/extensions/tracers/common/factory_base.h" #include "source/extensions/tracers/opentelemetry/grpc_trace_exporter.h" +#include "source/extensions/tracers/opentelemetry/samplers/sampler.h" #include "absl/strings/escaping.h" #include "span_context.h" @@ -28,6 +29,8 @@ struct OpenTelemetryTracerStats { OPENTELEMETRY_TRACER_STATS(GENERATE_COUNTER_STRUCT) }; +class Span; + /** * OpenTelemetry Tracer. It is stored in TLS and contains the exporter. */ @@ -35,7 +38,8 @@ class Tracer : Logger::Loggable { public: Tracer(OpenTelemetryGrpcTraceExporterPtr exporter, Envoy::TimeSource& time_source, Random::RandomGenerator& random, Runtime::Loader& runtime, Event::Dispatcher& dispatcher, - OpenTelemetryTracerStats tracing_stats, const std::string& service_name); + OpenTelemetryTracerStats tracing_stats, const std::string& service_name, + SamplerSharedPtr sampler); void sendSpan(::opentelemetry::proto::trace::v1::Span& span); @@ -55,6 +59,9 @@ class Tracer : Logger::Loggable { */ void flushSpans(); + void callSampler(const absl::StatusOr span_context, Span& new_span, + const std::string& operation_name); + OpenTelemetryGrpcTraceExporterPtr exporter_; Envoy::TimeSource& time_source_; Random::RandomGenerator& random_; @@ -63,6 +70,7 @@ class Tracer : Logger::Loggable { Event::TimerPtr flush_timer_; OpenTelemetryTracerStats tracing_stats_; std::string service_name_; + SamplerSharedPtr sampler_; }; /** @@ -109,6 +117,10 @@ class Span : Logger::Loggable, public Tracing::Span { std::string getTraceIdAsHex() const override { return absl::BytesToHexString(span_.trace_id()); }; + ::opentelemetry::proto::trace::v1::Span::SpanKind spankind() const { return span_.kind(); } + + std::string tracestate() const { return span_.trace_state(); } + /** * Sets the span's id. */ diff --git a/test/extensions/tracers/opentelemetry/samplers/BUILD b/test/extensions/tracers/opentelemetry/samplers/BUILD new file mode 100644 index 0000000000000..55414e7854c95 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/BUILD @@ -0,0 +1,23 @@ +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_package", +) + +licenses(["notice"]) # Apache 2 + +envoy_package() + +envoy_cc_test( + name = "sampler_test", + srcs = ["sampler_test.cc"], + deps = [ + "//envoy/registry", + "//source/extensions/tracers/opentelemetry:opentelemetry_tracer_lib", + "//source/extensions/tracers/opentelemetry/samplers:sampler_lib", + "//test/mocks/server:tracer_factory_context_mocks", + "//test/test_common:registry_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/trace/v3:pkg_cc_proto", + ], +) diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/BUILD b/test/extensions/tracers/opentelemetry/samplers/always_on/BUILD new file mode 100644 index 0000000000000..342679cf14a00 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/BUILD @@ -0,0 +1,37 @@ +load( + "//bazel:envoy_build_system.bzl", + "envoy_package", +) +load( + "//test/extensions:extensions_build_system.bzl", + "envoy_extension_cc_test", +) + +licenses(["notice"]) # Apache 2 + +envoy_package() + +envoy_extension_cc_test( + name = "config_test", + srcs = ["config_test.cc"], + extension_names = ["envoy.tracers.opentelemetry.samplers.always_on"], + deps = [ + "//envoy/registry", + "//source/extensions/tracers/opentelemetry/samplers/always_on:always_on_sampler_lib", + "//source/extensions/tracers/opentelemetry/samplers/always_on:config", + "//test/mocks/server:tracer_factory_context_mocks", + "//test/test_common:utility_lib", + ], +) + +envoy_extension_cc_test( + name = "always_on_sampler_test", + srcs = ["always_on_sampler_test.cc"], + extension_names = ["envoy.tracers.opentelemetry.samplers.always_on"], + deps = [ + "//source/extensions/tracers/opentelemetry/samplers/always_on:always_on_sampler_lib", + "//test/mocks/server:tracer_factory_context_mocks", + "//test/test_common:utility_lib", + "@envoy_api//envoy/extensions/tracers/opentelemetry/samplers/v3:pkg_cc_proto", + ], +) diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc new file mode 100644 index 0000000000000..c533ce778c530 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc @@ -0,0 +1,56 @@ +#include + +#include "envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.pb.h" + +#include "source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h" +#include "source/extensions/tracers/opentelemetry/span_context.h" + +#include "test/mocks/server/tracer_factory_context.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +TEST(AlwaysOnSamplerTest, TestWithInvalidParentContext) { + envoy::extensions::tracers::opentelemetry::samplers::v3::AlwaysOnSamplerConfig config; + NiceMock context; + auto sampler = std::make_shared(config, context); + EXPECT_STREQ(sampler->getDescription().c_str(), "AlwaysOnSampler"); + + absl::StatusOr span_context = absl::InvalidArgumentError("no parent span"); + auto sampling_result = + sampler->shouldSample(span_context, "operation_name", "12345", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::RECORD_AND_SAMPLE); + EXPECT_EQ(sampling_result.attributes, nullptr); + EXPECT_STREQ(sampling_result.tracestate.c_str(), ""); + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +TEST(AlwaysOnSamplerTest, TestWithValidParentContext) { + envoy::extensions::tracers::opentelemetry::samplers::v3::AlwaysOnSamplerConfig config; + NiceMock context; + auto sampler = std::make_shared(config, context); + EXPECT_STREQ(sampler->getDescription().c_str(), "AlwaysOnSampler"); + + SpanContext ctx("0", "12345", "45678", false, "some_tracestate"); + absl::StatusOr span_context = ctx; + auto sampling_result = + sampler->shouldSample(span_context, "operation_name", "12345", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); + EXPECT_EQ(sampling_result.decision, Decision::RECORD_AND_SAMPLE); + EXPECT_EQ(sampling_result.attributes, nullptr); + EXPECT_STREQ(sampling_result.tracestate.c_str(), "some_tracestate"); + EXPECT_TRUE(sampling_result.isRecording()); + EXPECT_TRUE(sampling_result.isSampled()); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy \ No newline at end of file diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc b/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc new file mode 100644 index 0000000000000..463d5aec2a874 --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc @@ -0,0 +1,37 @@ +#include "envoy/registry/registry.h" + +#include "source/extensions/tracers/opentelemetry/samplers/always_on/config.h" + +#include "test/mocks/server/tracer_factory_context.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +TEST(AlwaysOnSamplerFactoryTest, Test) { + auto* factory = Registry::FactoryRegistry::getFactory( + "envoy.tracers.opentelemetry.samplers.always_on"); + ASSERT_NE(factory, nullptr); + EXPECT_STREQ(factory->name().c_str(), "envoy.tracers.opentelemetry.samplers.always_on"); + EXPECT_NE(factory->createEmptyConfigProto(), nullptr); + + envoy::config::core::v3::TypedExtensionConfig typed_config; + const std::string yaml = R"EOF( + name: envoy.tracers.opentelemetry.samplers.always_on + typed_config: + "@type": type.googleapis.com/envoy.extensions.tracers.opentelemetry.samplers.v3.AlwaysOnSamplerConfig + )EOF"; + TestUtility::loadFromYaml(yaml, typed_config); + NiceMock context; + EXPECT_NE(factory->createSampler(typed_config.typed_config(), context), nullptr); + EXPECT_STREQ(factory->name().c_str(), "envoy.tracers.opentelemetry.samplers.always_on"); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy \ No newline at end of file diff --git a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc new file mode 100644 index 0000000000000..ef020a5b0ffaa --- /dev/null +++ b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc @@ -0,0 +1,168 @@ +#include + +#include "envoy/config/trace/v3/opentelemetry.pb.h" +#include "envoy/registry/registry.h" + +#include "source/common/tracing/http_tracer_impl.h" +#include "source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.h" +#include "source/extensions/tracers/opentelemetry/samplers/sampler.h" +#include "source/extensions/tracers/opentelemetry/span_context.h" + +#include "test/mocks/server/tracer_factory_context.h" +#include "test/test_common/registry.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace OpenTelemetry { + +using ::testing::NiceMock; +using ::testing::StrictMock; + +class TestSampler : public Sampler { +public: + MOCK_METHOD(SamplingResult, shouldSample, + ((const absl::StatusOr&), (const std::string&), (const std::string&), + (::opentelemetry::proto::trace::v1::Span::SpanKind), + (const std::map&), (const std::set&)), + (override)); + MOCK_METHOD(std::string, getDescription, (), (const, override)); +}; + +class TestSamplerFactory : public SamplerFactory { +public: + MOCK_METHOD(SamplerSharedPtr, createSampler, + (const Protobuf::Message& message, + Server::Configuration::TracerFactoryContext& context)); + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return std::make_unique(); + } + + std::string name() const override { return "envoy.tracers.opentelemetry.samplers.testsampler"; } +}; + +class SamplerFactoryTest : public testing::Test { + +protected: + NiceMock config; + NiceMock stream_info; + Tracing::TestTraceContextImpl trace_context{}; + NiceMock context; +}; + +TEST_F(SamplerFactoryTest, TestWithoutSampler) { + // using StrictMock, calls to SamplerFactory would cause a test failure + auto test_sampler = std::make_shared>(); + StrictMock sampler_factory; + Registry::InjectFactory sampler_factory_registration(sampler_factory); + + // no sampler configured + const std::string yaml_string = R"EOF( + grpc_service: + envoy_grpc: + cluster_name: fake-cluster + timeout: 0.250s + service_name: my-service + )EOF"; + + envoy::config::trace::v3::OpenTelemetryConfig opentelemetry_config; + TestUtility::loadFromYaml(yaml_string, opentelemetry_config); + + auto driver = std::make_unique(opentelemetry_config, context); + + driver->startSpan(config, trace_context, stream_info, "operation_name", + {Tracing::Reason::Sampling, true}); +} + +TEST_F(SamplerFactoryTest, TestWithInvalidSampler) { + // using StrictMock, calls to SamplerFactory would cause a test failure + auto test_sampler = std::make_shared>(); + StrictMock sampler_factory; + Registry::InjectFactory sampler_factory_registration(sampler_factory); + + // invalid sampler configured + const std::string yaml_string = R"EOF( + grpc_service: + envoy_grpc: + cluster_name: fake-cluster + timeout: 0.250s + service_name: my-service + sampler: + name: envoy.tracers.opentelemetry.samplers.testsampler + typed_config: + "@type": type.googleapis.com/google.protobuf.Value + )EOF"; + + envoy::config::trace::v3::OpenTelemetryConfig opentelemetry_config; + TestUtility::loadFromYaml(yaml_string, opentelemetry_config); + + EXPECT_THROW(std::make_unique(opentelemetry_config, context), EnvoyException); +} + +TEST_F(SamplerFactoryTest, TestWithSampler) { + auto test_sampler = std::make_shared>(); + TestSamplerFactory sampler_factory; + Registry::InjectFactory sampler_factory_registration(sampler_factory); + + EXPECT_CALL(sampler_factory, createSampler(_, _)).WillOnce(Return(test_sampler)); + + const std::string yaml_string = R"EOF( + grpc_service: + envoy_grpc: + cluster_name: fake-cluster + timeout: 0.250s + service_name: my-service + sampler: + name: envoy.tracers.opentelemetry.samplers.testsampler + typed_config: + "@type": type.googleapis.com/google.protobuf.Struct + )EOF"; + + envoy::config::trace::v3::OpenTelemetryConfig opentelemetry_config; + TestUtility::loadFromYaml(yaml_string, opentelemetry_config); + + auto driver = std::make_unique(opentelemetry_config, context); + + EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)); + driver->startSpan(config, trace_context, stream_info, "operation_name", + {Tracing::Reason::Sampling, true}); + + // now, let shouldSample return a result containing additonal attributes + EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)) + .WillOnce([](const absl::StatusOr&, const std::string&, const std::string&, + ::opentelemetry::proto::trace::v1::Span::SpanKind, + const std::map&, const std::set&) { + SamplingResult res; + std::map attributes; + attributes["key"] = "value"; + attributes["another_key"] = "another_value"; + res.attributes = + std::make_unique>(std::move(attributes)); + return res; + }); + driver->startSpan(config, trace_context, stream_info, "operation_name", + {Tracing::Reason::Sampling, true}); +} + +TEST(SamplingResultTest, TestSamplingResult) { + SamplingResult result; + result.decision = Decision::RECORD_AND_SAMPLE; + EXPECT_TRUE(result.isRecording()); + EXPECT_TRUE(result.isSampled()); + result.decision = Decision::RECORD_ONLY; + EXPECT_TRUE(result.isRecording()); + EXPECT_FALSE(result.isSampled()); + result.decision = Decision::DROP; + EXPECT_FALSE(result.isRecording()); + EXPECT_FALSE(result.isSampled()); +} + +} // namespace OpenTelemetry +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From e0f41ba55a1b41346040eed291c3fee477eb6231 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Fri, 13 Oct 2023 08:20:47 +0200 Subject: [PATCH 02/15] review feedback: mainly docs, new lines --- api/envoy/config/trace/v3/opentelemetry.proto | 7 ++++++- .../opentelemetry/samplers/v3/always_on_sampler.proto | 8 +++++++- api/versioning/BUILD | 1 + source/extensions/extensions_build_config.bzl | 2 +- .../opentelemetry/samplers/always_on/always_on_sampler.h | 2 +- .../samplers/always_on/always_on_sampler_test.cc | 2 +- .../opentelemetry/samplers/always_on/config_test.cc | 2 +- 7 files changed, 18 insertions(+), 6 deletions(-) diff --git a/api/envoy/config/trace/v3/opentelemetry.proto b/api/envoy/config/trace/v3/opentelemetry.proto index 6059f8ad76acf..9c880422ccf32 100644 --- a/api/envoy/config/trace/v3/opentelemetry.proto +++ b/api/envoy/config/trace/v3/opentelemetry.proto @@ -27,7 +27,12 @@ message OpenTelemetryConfig { // If it is not provided, it will default to "unknown_service:envoy". string service_name = 2; - // optional sampler which is used in the opentelemetry tracer + + // Specifies the sampler to be used by the OpenTelemetry tracer. + // The configured sampler implements the Sampler interface defined by the OpenTelemetry specification. + // This field can be left empty. In this case, the default Envoy sampling decision is used. + // See: https://opentelemetry.io/docs/specs/otel/trace/sdk/#sampler + // See: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/observability/tracing#trace-context-propagation // [#extension-category: envoy.tracers.opentelemetry.samplers] optional core.v3.TypedExtensionConfig sampler = 3; } diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto index ae465a3bd5d4d..307a472c11213 100644 --- a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto @@ -11,7 +11,13 @@ option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/tra option (udpa.annotations.file_status).package_version_status = ACTIVE; // [#protodoc-title: Always On Sampler config] +// Configuration for the "AlwaysOn" Sampler extension. +// The sampler follows the the "AlwaysOn" implementation from the OpenTelemetry +// SDK specification. +// +// See: +// `AlwaysOn specification `_ // [#extension: envoy.tracers.opentelemetry.samplers.always_on] message AlwaysOnSamplerConfig { -} \ No newline at end of file +} diff --git a/api/versioning/BUILD b/api/versioning/BUILD index 902e803f3e9a6..2fa09b2aae7de 100644 --- a/api/versioning/BUILD +++ b/api/versioning/BUILD @@ -239,6 +239,7 @@ proto_library( "//envoy/extensions/stat_sinks/graphite_statsd/v3:pkg", "//envoy/extensions/stat_sinks/open_telemetry/v3:pkg", "//envoy/extensions/stat_sinks/wasm/v3:pkg", + "//envoy/extensions/tracers/opentelemetry/samplers/v3:pkg", "//envoy/extensions/transport_sockets/alts/v3:pkg", "//envoy/extensions/transport_sockets/http_11_proxy/v3:pkg", "//envoy/extensions/transport_sockets/internal_upstream/v3:pkg", diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index 27df83cae30d3..18a87efaca333 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -261,7 +261,7 @@ EXTENSIONS = { "envoy.tracers.opentelemetry": "//source/extensions/tracers/opentelemetry:config", # - # Sampler + # OpenTelemetry tracer samplers # "envoy.tracers.opentelemetry.samplers.always_on": "//source/extensions/tracers/opentelemetry/samplers/always_on:config", diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h index aaf0db4fa4bbc..745eef96a3653 100644 --- a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h @@ -12,7 +12,7 @@ namespace Tracers { namespace OpenTelemetry { /** - * @brief A sampler which samples every span variable. + * @brief A sampler which samples every span. * https://opentelemetry.io/docs/specs/otel/trace/sdk/#alwayson * - Returns RECORD_AND_SAMPLE always. * - Description MUST be AlwaysOnSampler. diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc index c533ce778c530..b0f394330b5b1 100644 --- a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc @@ -53,4 +53,4 @@ TEST(AlwaysOnSamplerTest, TestWithValidParentContext) { } // namespace OpenTelemetry } // namespace Tracers } // namespace Extensions -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc b/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc index 463d5aec2a874..2829c1295d112 100644 --- a/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc @@ -34,4 +34,4 @@ TEST(AlwaysOnSamplerFactoryTest, Test) { } // namespace OpenTelemetry } // namespace Tracers } // namespace Extensions -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From 4d17c894d5de85db4fb6dd0e512c3392b7d9cb6a Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Fri, 13 Oct 2023 09:46:41 +0200 Subject: [PATCH 03/15] remove optional, run proto_format script, change extension name in extensions_metadata.yaml --- api/envoy/config/trace/v3/opentelemetry.proto | 3 +-- api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD | 2 ++ .../tracers/opentelemetry/samplers/v3/always_on_sampler.proto | 2 +- source/extensions/extensions_metadata.yaml | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/trace/v3/opentelemetry.proto b/api/envoy/config/trace/v3/opentelemetry.proto index 9c880422ccf32..ccb57e3dda7b0 100644 --- a/api/envoy/config/trace/v3/opentelemetry.proto +++ b/api/envoy/config/trace/v3/opentelemetry.proto @@ -27,12 +27,11 @@ message OpenTelemetryConfig { // If it is not provided, it will default to "unknown_service:envoy". string service_name = 2; - // Specifies the sampler to be used by the OpenTelemetry tracer. // The configured sampler implements the Sampler interface defined by the OpenTelemetry specification. // This field can be left empty. In this case, the default Envoy sampling decision is used. // See: https://opentelemetry.io/docs/specs/otel/trace/sdk/#sampler // See: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/observability/tracing#trace-context-propagation // [#extension-category: envoy.tracers.opentelemetry.samplers] - optional core.v3.TypedExtensionConfig sampler = 3; + core.v3.TypedExtensionConfig sampler = 3; } diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD index e8aa6b3a4bfd5..ee92fb652582e 100644 --- a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD @@ -1,3 +1,5 @@ +# DO NOT EDIT. This file is generated by tools/proto_format/proto_sync.py. + load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package") licenses(["notice"]) # Apache 2 diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto index 307a472c11213..66c7224a7fcbb 100644 --- a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto @@ -5,7 +5,7 @@ package envoy.extensions.tracers.opentelemetry.samplers.v3; import "udpa/annotations/status.proto"; option java_package = "io.envoyproxy.envoy.extensions.tracers.opentelemetry.samplers.v3"; -option java_outer_classname = "AlwaysOnSamplerConfigProto"; +option java_outer_classname = "AlwaysOnSamplerProto"; option java_multiple_files = true; option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/tracers/opentelemetry/samplers/v3;samplersv3"; option (udpa.annotations.file_status).package_version_status = ACTIVE; diff --git a/source/extensions/extensions_metadata.yaml b/source/extensions/extensions_metadata.yaml index 9deec64ab669a..4a659d993f80c 100644 --- a/source/extensions/extensions_metadata.yaml +++ b/source/extensions/extensions_metadata.yaml @@ -1136,7 +1136,7 @@ envoy.tracers.opentelemetry: status: wip type_urls: - envoy.config.trace.v3.OpenTelemetryConfig -envoy.tracers.opentelemetry.samplers.all: +envoy.tracers.opentelemetry.samplers.always_on: categories: - envoy.tracers.opentelemetry.samplers security_posture: unknown From 806b9f57358bd63db25a60cd6aec10e7e5a93744 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Fri, 13 Oct 2023 12:17:15 +0200 Subject: [PATCH 04/15] remove redundant setSampled(), already called in startSpan extend unit test to check sampling result --- .../opentelemetry_tracer_impl.cc | 1 - .../opentelemetry/samplers/sampler_test.cc | 29 +++++++++++++++---- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc b/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc index c63566bc2d398..3c0124b5fba57 100644 --- a/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc +++ b/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc @@ -72,7 +72,6 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, // No propagation header, so we can create a fresh span with the given decision. Tracing::SpanPtr new_open_telemetry_span = tracer.startSpan(config, operation_name, stream_info.startTime(), tracing_decision); - new_open_telemetry_span->setSampled(tracing_decision.traced); return new_open_telemetry_span; } else { // Try to extract the span context. If we can't, just return a null span. diff --git a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc index ef020a5b0ffaa..4dbeb3bb669ae 100644 --- a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc @@ -128,16 +128,31 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { auto driver = std::make_unique(opentelemetry_config, context); - EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)); - driver->startSpan(config, trace_context, stream_info, "operation_name", - {Tracing::Reason::Sampling, true}); + // shouldSample returns a result without additonal attributes and Decision::RECORD_AND_SAMPLE + EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)) + .WillOnce([](const absl::StatusOr&, const std::string&, const std::string&, + ::opentelemetry::proto::trace::v1::Span::SpanKind, + const std::map&, const std::set&) { + SamplingResult res; + res.decision = Decision::RECORD_AND_SAMPLE; + return res; + }); - // now, let shouldSample return a result containing additonal attributes + Tracing::SpanPtr tracing_span = driver->startSpan( + config, trace_context, stream_info, "operation_name", {Tracing::Reason::Sampling, true}); + // startSpan returns a Tracing::SpanPtr. Tracing::Span has no sampled() method. + // We know that the underlying span is Extensions::Tracers::OpenTelemetry::Span + // So the dynamic_cast should be safe. + std::unique_ptr span(dynamic_cast(tracing_span.release())); + EXPECT_TRUE(span->sampled()); + + // shouldSamples return a result containing additional attributes and Decision::DROP EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)) .WillOnce([](const absl::StatusOr&, const std::string&, const std::string&, ::opentelemetry::proto::trace::v1::Span::SpanKind, const std::map&, const std::set&) { SamplingResult res; + res.decision = Decision::DROP; std::map attributes; attributes["key"] = "value"; attributes["another_key"] = "another_value"; @@ -145,8 +160,10 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { std::make_unique>(std::move(attributes)); return res; }); - driver->startSpan(config, trace_context, stream_info, "operation_name", - {Tracing::Reason::Sampling, true}); + tracing_span = driver->startSpan(config, trace_context, stream_info, "operation_name", + {Tracing::Reason::Sampling, true}); + std::unique_ptr unsampled_span(dynamic_cast(tracing_span.release())); + EXPECT_FALSE(unsampled_span->sampled()); } TEST(SamplingResultTest, TestSamplingResult) { From 713498a641b0b94923bab3c654184fe0877032d2 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Fri, 13 Oct 2023 13:24:26 +0200 Subject: [PATCH 05/15] change datatype from set to vector --- .../samplers/always_on/always_on_sampler.cc | 2 +- .../samplers/always_on/always_on_sampler.h | 2 +- .../tracers/opentelemetry/samplers/sampler.h | 4 ++-- .../tracers/opentelemetry/samplers/sampler_test.cc | 10 +++++++--- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc index 4ca6bffd3b09b..7904bcbc71165 100644 --- a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc @@ -17,7 +17,7 @@ AlwaysOnSampler::shouldSample(const absl::StatusOr& parent_context, const std::string& /*trace_id*/, const std::string& /*name*/, ::opentelemetry::proto::trace::v1::Span::SpanKind /*spankind*/, const std::map& /*attributes*/, - const std::set& /*links*/) { + const std::vector& /*links*/) { SamplingResult result; result.decision = Decision::RECORD_AND_SAMPLE; if (parent_context.ok()) { diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h index 745eef96a3653..4c1ee29a18c3a 100644 --- a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h @@ -26,7 +26,7 @@ class AlwaysOnSampler : public Sampler, Logger::Loggable { const std::string& trace_id, const std::string& name, ::opentelemetry::proto::trace::v1::Span::SpanKind spankind, const std::map& attributes, - const std::set& links) override; + const std::vector& links) override; std::string getDescription() const override; private: diff --git a/source/extensions/tracers/opentelemetry/samplers/sampler.h b/source/extensions/tracers/opentelemetry/samplers/sampler.h index 7627cc8e8fe94..9645d2f6124fc 100644 --- a/source/extensions/tracers/opentelemetry/samplers/sampler.h +++ b/source/extensions/tracers/opentelemetry/samplers/sampler.h @@ -2,8 +2,8 @@ #include #include -#include #include +#include #include "envoy/config/typed_config.h" #include "envoy/server/tracer_config.h" @@ -72,7 +72,7 @@ class Sampler { const std::string& trace_id, const std::string& name, ::opentelemetry::proto::trace::v1::Span::SpanKind spankind, const std::map& attributes, - const std::set& links) PURE; + const std::vector& links) PURE; /** * @brief Returns a sampler description or name. diff --git a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc index 4dbeb3bb669ae..1471288630796 100644 --- a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc @@ -28,7 +28,7 @@ class TestSampler : public Sampler { MOCK_METHOD(SamplingResult, shouldSample, ((const absl::StatusOr&), (const std::string&), (const std::string&), (::opentelemetry::proto::trace::v1::Span::SpanKind), - (const std::map&), (const std::set&)), + (const std::map&), (const std::vector&)), (override)); MOCK_METHOD(std::string, getDescription, (), (const, override)); }; @@ -132,9 +132,10 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)) .WillOnce([](const absl::StatusOr&, const std::string&, const std::string&, ::opentelemetry::proto::trace::v1::Span::SpanKind, - const std::map&, const std::set&) { + const std::map&, const std::vector&) { SamplingResult res; res.decision = Decision::RECORD_AND_SAMPLE; + res.tracestate = "this_is=tracesate"; return res; }); @@ -145,12 +146,13 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { // So the dynamic_cast should be safe. std::unique_ptr span(dynamic_cast(tracing_span.release())); EXPECT_TRUE(span->sampled()); + EXPECT_STREQ(span->tracestate().c_str(), "this_is=tracesate"); // shouldSamples return a result containing additional attributes and Decision::DROP EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)) .WillOnce([](const absl::StatusOr&, const std::string&, const std::string&, ::opentelemetry::proto::trace::v1::Span::SpanKind, - const std::map&, const std::set&) { + const std::map&, const std::vector&) { SamplingResult res; res.decision = Decision::DROP; std::map attributes; @@ -158,12 +160,14 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { attributes["another_key"] = "another_value"; res.attributes = std::make_unique>(std::move(attributes)); + res.tracestate = "this_is=another_tracesate"; return res; }); tracing_span = driver->startSpan(config, trace_context, stream_info, "operation_name", {Tracing::Reason::Sampling, true}); std::unique_ptr unsampled_span(dynamic_cast(tracing_span.release())); EXPECT_FALSE(unsampled_span->sampled()); + EXPECT_STREQ(unsampled_span->tracestate().c_str(), "this_is=another_tracesate"); } TEST(SamplingResultTest, TestSamplingResult) { From 090514729700b5b6b3485d643a640a6483a4a754 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Fri, 13 Oct 2023 13:36:58 +0200 Subject: [PATCH 06/15] extract sampler creation to tryCreateSamper --- .../opentelemetry_tracer_impl.cc | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc b/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc index 3c0124b5fba57..20ed669bb41bf 100644 --- a/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc +++ b/source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.cc @@ -21,14 +21,11 @@ namespace Extensions { namespace Tracers { namespace OpenTelemetry { -Driver::Driver(const envoy::config::trace::v3::OpenTelemetryConfig& opentelemetry_config, - Server::Configuration::TracerFactoryContext& context) - : tls_slot_ptr_(context.serverFactoryContext().threadLocal().allocateSlot()), - tracing_stats_{OPENTELEMETRY_TRACER_STATS( - POOL_COUNTER_PREFIX(context.serverFactoryContext().scope(), "tracing.opentelemetry"))} { - auto& factory_context = context.serverFactoryContext(); +namespace { - // Create the samper if configured +SamplerSharedPtr +tryCreateSamper(const envoy::config::trace::v3::OpenTelemetryConfig& opentelemetry_config, + Server::Configuration::TracerFactoryContext& context) { SamplerSharedPtr sampler; if (opentelemetry_config.has_sampler()) { auto& sampler_config = opentelemetry_config.sampler(); @@ -38,6 +35,20 @@ Driver::Driver(const envoy::config::trace::v3::OpenTelemetryConfig& opentelemetr } sampler = factory->createSampler(sampler_config.typed_config(), context); } + return sampler; +} + +} // namespace + +Driver::Driver(const envoy::config::trace::v3::OpenTelemetryConfig& opentelemetry_config, + Server::Configuration::TracerFactoryContext& context) + : tls_slot_ptr_(context.serverFactoryContext().threadLocal().allocateSlot()), + tracing_stats_{OPENTELEMETRY_TRACER_STATS( + POOL_COUNTER_PREFIX(context.serverFactoryContext().scope(), "tracing.opentelemetry"))} { + auto& factory_context = context.serverFactoryContext(); + + // Create the sampler if configured + SamplerSharedPtr sampler = tryCreateSamper(opentelemetry_config, context); // Create the tracer in Thread Local Storage. tls_slot_ptr_->set( From 99c14e8fda3ebf198c459a9d8a856832d1ba9a11 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Fri, 13 Oct 2023 13:56:14 +0200 Subject: [PATCH 07/15] turn callSampler into a free function (not a member of Tracer) --- .../tracers/opentelemetry/tracer.cc | 42 ++++++++++--------- .../extensions/tracers/opentelemetry/tracer.h | 5 --- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/tracer.cc b/source/extensions/tracers/opentelemetry/tracer.cc index 0cd52768c4456..298fb7a5b3203 100644 --- a/source/extensions/tracers/opentelemetry/tracer.cc +++ b/source/extensions/tracers/opentelemetry/tracer.cc @@ -24,6 +24,27 @@ constexpr absl::string_view kDefaultServiceName = "unknown_service:envoy"; using opentelemetry::proto::collector::trace::v1::ExportTraceServiceRequest; +namespace { + +void callSampler(SamplerSharedPtr sampler, const absl::StatusOr span_context, + Span& new_span, const std::string& operation_name) { + if (!sampler) { + return; + } + const auto sampling_result = sampler->shouldSample( + span_context, operation_name, new_span.getTraceIdAsHex(), new_span.spankind(), {}, {}); + new_span.setSampled(sampling_result.isSampled()); + + if (sampling_result.attributes) { + for (auto const& attribute : *sampling_result.attributes) { + new_span.setTag(attribute.first, attribute.second); + } + } + new_span.setTracestate(sampling_result.tracestate); +} + +} // namespace + Span::Span(const Tracing::Config& config, const std::string& name, SystemTime start_time, Envoy::TimeSource& time_source, Tracer& parent_tracer) : parent_tracer_(parent_tracer), time_source_(time_source) { @@ -142,23 +163,6 @@ void Tracer::flushSpans() { span_buffer_.clear(); } -void Tracer::callSampler(const absl::StatusOr span_context, Span& new_span, - const std::string& operation_name) { - if (!sampler_) { - return; - } - const auto sampling_result = sampler_->shouldSample( - span_context, operation_name, new_span.getTraceIdAsHex(), new_span.spankind(), {}, {}); - new_span.setSampled(sampling_result.isSampled()); - - if (sampling_result.attributes) { - for (auto const& attribute : *sampling_result.attributes) { - new_span.setTag(attribute.first, attribute.second); - } - } - new_span.setTracestate(sampling_result.tracestate); -} - void Tracer::sendSpan(::opentelemetry::proto::trace::v1::Span& span) { span_buffer_.push_back(span); const uint64_t min_flush_spans = @@ -180,7 +184,7 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str uint64_t span_id = random_.random(); new_span.setId(Hex::uint64ToHex(span_id)); absl::StatusOr span_context = absl::InvalidArgumentError("no parent span"); - callSampler(span_context, new_span, operation_name); + callSampler(sampler_, span_context, new_span, operation_name); return std::make_unique(new_span); } @@ -202,7 +206,7 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str if (!previous_span_context.tracestate().empty()) { new_span.setTracestate(std::string{previous_span_context.tracestate()}); } - callSampler(previous_span_context, new_span, operation_name); + callSampler(sampler_, previous_span_context, new_span, operation_name); return std::make_unique(new_span); } diff --git a/source/extensions/tracers/opentelemetry/tracer.h b/source/extensions/tracers/opentelemetry/tracer.h index 905520ab7b491..e93a89d4bf3c0 100644 --- a/source/extensions/tracers/opentelemetry/tracer.h +++ b/source/extensions/tracers/opentelemetry/tracer.h @@ -29,8 +29,6 @@ struct OpenTelemetryTracerStats { OPENTELEMETRY_TRACER_STATS(GENERATE_COUNTER_STRUCT) }; -class Span; - /** * OpenTelemetry Tracer. It is stored in TLS and contains the exporter. */ @@ -59,9 +57,6 @@ class Tracer : Logger::Loggable { */ void flushSpans(); - void callSampler(const absl::StatusOr span_context, Span& new_span, - const std::string& operation_name); - OpenTelemetryGrpcTraceExporterPtr exporter_; Envoy::TimeSource& time_source_; Random::RandomGenerator& random_; From 6969f55184d91ed2c7a2146b2084d1c7ccd453dd Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 16 Oct 2023 06:57:48 +0200 Subject: [PATCH 08/15] change shouldSample(): absl::StatusOr -> absl::optional --- .../opentelemetry/samplers/always_on/always_on_sampler.cc | 4 ++-- .../opentelemetry/samplers/always_on/always_on_sampler.h | 2 +- source/extensions/tracers/opentelemetry/samplers/sampler.h | 4 ++-- source/extensions/tracers/opentelemetry/tracer.cc | 5 ++--- .../samplers/always_on/always_on_sampler_test.cc | 5 ++--- .../tracers/opentelemetry/samplers/sampler_test.cc | 6 +++--- 6 files changed, 12 insertions(+), 14 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc index 7904bcbc71165..8c11e8d9d6a31 100644 --- a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc @@ -13,14 +13,14 @@ namespace Tracers { namespace OpenTelemetry { SamplingResult -AlwaysOnSampler::shouldSample(const absl::StatusOr& parent_context, +AlwaysOnSampler::shouldSample(const absl::optional parent_context, const std::string& /*trace_id*/, const std::string& /*name*/, ::opentelemetry::proto::trace::v1::Span::SpanKind /*spankind*/, const std::map& /*attributes*/, const std::vector& /*links*/) { SamplingResult result; result.decision = Decision::RECORD_AND_SAMPLE; - if (parent_context.ok()) { + if (parent_context.has_value()) { result.tracestate = parent_context.value().tracestate(); } return result; diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h index 4c1ee29a18c3a..2d53a511fa294 100644 --- a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.h @@ -22,7 +22,7 @@ class AlwaysOnSampler : public Sampler, Logger::Loggable { public: explicit AlwaysOnSampler(const Protobuf::Message& /*config*/, Server::Configuration::TracerFactoryContext& /*context*/) {} - SamplingResult shouldSample(const absl::StatusOr& parent_context, + SamplingResult shouldSample(const absl::optional parent_context, const std::string& trace_id, const std::string& name, ::opentelemetry::proto::trace::v1::Span::SpanKind spankind, const std::map& attributes, diff --git a/source/extensions/tracers/opentelemetry/samplers/sampler.h b/source/extensions/tracers/opentelemetry/samplers/sampler.h index 9645d2f6124fc..78b0b60254a35 100644 --- a/source/extensions/tracers/opentelemetry/samplers/sampler.h +++ b/source/extensions/tracers/opentelemetry/samplers/sampler.h @@ -9,7 +9,7 @@ #include "envoy/server/tracer_config.h" #include "envoy/tracing/trace_context.h" -#include "absl/status/statusor.h" +#include "absl/types/optional.h" #include "opentelemetry/proto/trace/v1/trace.pb.h" namespace Envoy { @@ -68,7 +68,7 @@ class Sampler { * @param links Collection of links that will be associated with the Span to be created. * @return SamplingResult @see SamplingResult */ - virtual SamplingResult shouldSample(const absl::StatusOr& parent_context, + virtual SamplingResult shouldSample(const absl::optional parent_context, const std::string& trace_id, const std::string& name, ::opentelemetry::proto::trace::v1::Span::SpanKind spankind, const std::map& attributes, diff --git a/source/extensions/tracers/opentelemetry/tracer.cc b/source/extensions/tracers/opentelemetry/tracer.cc index 298fb7a5b3203..b99a331759eff 100644 --- a/source/extensions/tracers/opentelemetry/tracer.cc +++ b/source/extensions/tracers/opentelemetry/tracer.cc @@ -26,7 +26,7 @@ using opentelemetry::proto::collector::trace::v1::ExportTraceServiceRequest; namespace { -void callSampler(SamplerSharedPtr sampler, const absl::StatusOr span_context, +void callSampler(SamplerSharedPtr sampler, const absl::optional span_context, Span& new_span, const std::string& operation_name) { if (!sampler) { return; @@ -183,8 +183,7 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str new_span.setTraceId(absl::StrCat(Hex::uint64ToHex(trace_id_high), Hex::uint64ToHex(trace_id))); uint64_t span_id = random_.random(); new_span.setId(Hex::uint64ToHex(span_id)); - absl::StatusOr span_context = absl::InvalidArgumentError("no parent span"); - callSampler(sampler_, span_context, new_span, operation_name); + callSampler(sampler_, absl::nullopt, new_span, operation_name); return std::make_unique(new_span); } diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc index b0f394330b5b1..606abe76faa27 100644 --- a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc @@ -21,7 +21,7 @@ TEST(AlwaysOnSamplerTest, TestWithInvalidParentContext) { auto sampler = std::make_shared(config, context); EXPECT_STREQ(sampler->getDescription().c_str(), "AlwaysOnSampler"); - absl::StatusOr span_context = absl::InvalidArgumentError("no parent span"); + absl::optional span_context; auto sampling_result = sampler->shouldSample(span_context, "operation_name", "12345", ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); @@ -38,8 +38,7 @@ TEST(AlwaysOnSamplerTest, TestWithValidParentContext) { auto sampler = std::make_shared(config, context); EXPECT_STREQ(sampler->getDescription().c_str(), "AlwaysOnSampler"); - SpanContext ctx("0", "12345", "45678", false, "some_tracestate"); - absl::StatusOr span_context = ctx; + SpanContext span_context("0", "12345", "45678", false, "some_tracestate"); auto sampling_result = sampler->shouldSample(span_context, "operation_name", "12345", ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); diff --git a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc index 1471288630796..d456452818973 100644 --- a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc @@ -26,7 +26,7 @@ using ::testing::StrictMock; class TestSampler : public Sampler { public: MOCK_METHOD(SamplingResult, shouldSample, - ((const absl::StatusOr&), (const std::string&), (const std::string&), + ((const absl::optional), (const std::string&), (const std::string&), (::opentelemetry::proto::trace::v1::Span::SpanKind), (const std::map&), (const std::vector&)), (override)); @@ -130,7 +130,7 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { // shouldSample returns a result without additonal attributes and Decision::RECORD_AND_SAMPLE EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)) - .WillOnce([](const absl::StatusOr&, const std::string&, const std::string&, + .WillOnce([](const absl::optional, const std::string&, const std::string&, ::opentelemetry::proto::trace::v1::Span::SpanKind, const std::map&, const std::vector&) { SamplingResult res; @@ -150,7 +150,7 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { // shouldSamples return a result containing additional attributes and Decision::DROP EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)) - .WillOnce([](const absl::StatusOr&, const std::string&, const std::string&, + .WillOnce([](const absl::optional, const std::string&, const std::string&, ::opentelemetry::proto::trace::v1::Span::SpanKind, const std::map&, const std::vector&) { SamplingResult res; From 226cc78206e349b4b4549fcd73bc68a3119ac7cd Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 16 Oct 2023 07:33:38 +0200 Subject: [PATCH 09/15] tracer: do not call setSampled() if a tracer is set --- .../tracers/opentelemetry/tracer.cc | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/tracer.cc b/source/extensions/tracers/opentelemetry/tracer.cc index b99a331759eff..b18cb1e5ecf36 100644 --- a/source/extensions/tracers/opentelemetry/tracer.cc +++ b/source/extensions/tracers/opentelemetry/tracer.cc @@ -177,13 +177,16 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str const Tracing::Decision tracing_decision) { // Create an Tracers::OpenTelemetry::Span class that will contain the OTel span. Span new_span = Span(config, operation_name, start_time, time_source_, *this); - new_span.setSampled(tracing_decision.traced); uint64_t trace_id_high = random_.random(); uint64_t trace_id = random_.random(); new_span.setTraceId(absl::StrCat(Hex::uint64ToHex(trace_id_high), Hex::uint64ToHex(trace_id))); uint64_t span_id = random_.random(); new_span.setId(Hex::uint64ToHex(span_id)); - callSampler(sampler_, absl::nullopt, new_span, operation_name); + if (sampler_) { + callSampler(sampler_, absl::nullopt, new_span, operation_name); + } else { + new_span.setSampled(tracing_decision.traced); + } return std::make_unique(new_span); } @@ -192,7 +195,7 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str const SpanContext& previous_span_context) { // Create a new span and populate details from the span context. Span new_span = Span(config, operation_name, start_time, time_source_, *this); - new_span.setSampled(previous_span_context.sampled()); + new_span.setTraceId(previous_span_context.traceId()); if (!previous_span_context.parentId().empty()) { new_span.setParentId(previous_span_context.parentId()); @@ -200,12 +203,16 @@ Tracing::SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::str // Generate a new identifier for the span id. uint64_t span_id = random_.random(); new_span.setId(Hex::uint64ToHex(span_id)); - // Respect the previous span's sampled flag. - new_span.setSampled(previous_span_context.sampled()); - if (!previous_span_context.tracestate().empty()) { - new_span.setTracestate(std::string{previous_span_context.tracestate()}); + if (sampler_) { + // Sampler should make a sampling decision and set tracestate + callSampler(sampler_, previous_span_context, new_span, operation_name); + } else { + // Respect the previous span's sampled flag. + new_span.setSampled(previous_span_context.sampled()); + if (!previous_span_context.tracestate().empty()) { + new_span.setTracestate(std::string{previous_span_context.tracestate()}); + } } - callSampler(sampler_, previous_span_context, new_span, operation_name); return std::make_unique(new_span); } From cdc8c58229238f2b517861a8938faf49fe213bda Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 16 Oct 2023 07:42:21 +0200 Subject: [PATCH 10/15] fix spell check findings. ./tools/spelling/check_spelling_pedantic.py fix --- .../opentelemetry/samplers/always_on/always_on_sampler.cc | 2 +- source/extensions/tracers/opentelemetry/samplers/sampler.h | 2 +- test/extensions/tracers/opentelemetry/samplers/sampler_test.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc index 8c11e8d9d6a31..3bc0aa87ab3d5 100644 --- a/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc +++ b/source/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler.cc @@ -15,7 +15,7 @@ namespace OpenTelemetry { SamplingResult AlwaysOnSampler::shouldSample(const absl::optional parent_context, const std::string& /*trace_id*/, const std::string& /*name*/, - ::opentelemetry::proto::trace::v1::Span::SpanKind /*spankind*/, + ::opentelemetry::proto::trace::v1::Span::SpanKind /*kind*/, const std::map& /*attributes*/, const std::vector& /*links*/) { SamplingResult result; diff --git a/source/extensions/tracers/opentelemetry/samplers/sampler.h b/source/extensions/tracers/opentelemetry/samplers/sampler.h index 78b0b60254a35..fd2be0bb647e9 100644 --- a/source/extensions/tracers/opentelemetry/samplers/sampler.h +++ b/source/extensions/tracers/opentelemetry/samplers/sampler.h @@ -58,7 +58,7 @@ class Sampler { /** * @brief Decides if a trace should be sampled. * - * @param parent_context Span context decsribing the parent span. The Span's SpanContext may be + * @param parent_context Span context describing the parent span. The Span's SpanContext may be * invalid to indicate a root span. * @param trace_id Trace id of the Span to be created. If the parent SpanContext contains a valid * TraceId, they MUST always match. diff --git a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc index d456452818973..30ffac30f261d 100644 --- a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc @@ -128,7 +128,7 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { auto driver = std::make_unique(opentelemetry_config, context); - // shouldSample returns a result without additonal attributes and Decision::RECORD_AND_SAMPLE + // shouldSample returns a result without additional attributes and Decision::RECORD_AND_SAMPLE EXPECT_CALL(*test_sampler, shouldSample(_, _, _, _, _, _)) .WillOnce([](const absl::optional, const std::string&, const std::string&, ::opentelemetry::proto::trace::v1::Span::SpanKind, From 3500d48d26bf6a757232d720fd9cf1cf4654ea0a Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 16 Oct 2023 10:50:22 +0200 Subject: [PATCH 11/15] modify rst files, doc build works now --- docs/root/api-v3/config/trace/opentelemetry/samplers.rst | 7 +++++++ docs/root/api-v3/config/trace/trace.rst | 1 + 2 files changed, 8 insertions(+) create mode 100644 docs/root/api-v3/config/trace/opentelemetry/samplers.rst diff --git a/docs/root/api-v3/config/trace/opentelemetry/samplers.rst b/docs/root/api-v3/config/trace/opentelemetry/samplers.rst new file mode 100644 index 0000000000000..911759e6154bb --- /dev/null +++ b/docs/root/api-v3/config/trace/opentelemetry/samplers.rst @@ -0,0 +1,7 @@ +Samplers that can be configured with the OpenTelemetry Tracer: + +.. toctree:: + :glob: + :maxdepth: 3 + + ../../../extensions/tracers/opentelemetry/samplers/v3/* diff --git a/docs/root/api-v3/config/trace/trace.rst b/docs/root/api-v3/config/trace/trace.rst index 8f8d039a18d81..adc3daceb6e30 100644 --- a/docs/root/api-v3/config/trace/trace.rst +++ b/docs/root/api-v3/config/trace/trace.rst @@ -12,3 +12,4 @@ HTTP tracers :maxdepth: 2 v3/* + opentelemetry/samplers From 7b8c758f39d90b3dff454a4518beedac775c1edb Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 16 Oct 2023 12:40:52 +0200 Subject: [PATCH 12/15] Enforce line break in rendered docu --- api/envoy/config/trace/v3/opentelemetry.proto | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/envoy/config/trace/v3/opentelemetry.proto b/api/envoy/config/trace/v3/opentelemetry.proto index ccb57e3dda7b0..208fd7ccee1f2 100644 --- a/api/envoy/config/trace/v3/opentelemetry.proto +++ b/api/envoy/config/trace/v3/opentelemetry.proto @@ -30,7 +30,9 @@ message OpenTelemetryConfig { // Specifies the sampler to be used by the OpenTelemetry tracer. // The configured sampler implements the Sampler interface defined by the OpenTelemetry specification. // This field can be left empty. In this case, the default Envoy sampling decision is used. + // // See: https://opentelemetry.io/docs/specs/otel/trace/sdk/#sampler + // // See: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/observability/tracing#trace-context-propagation // [#extension-category: envoy.tracers.opentelemetry.samplers] core.v3.TypedExtensionConfig sampler = 3; From 156500d68581e0943c9b250603042c2b4e225962 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 16 Oct 2023 13:38:08 +0200 Subject: [PATCH 13/15] fix wording --- .../tracers/opentelemetry/samplers/v3/always_on_sampler.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto index 66c7224a7fcbb..565df1416c235 100644 --- a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto @@ -12,7 +12,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // [#protodoc-title: Always On Sampler config] // Configuration for the "AlwaysOn" Sampler extension. -// The sampler follows the the "AlwaysOn" implementation from the OpenTelemetry +// The sampler follows the "AlwaysOn" implementation from the OpenTelemetry // SDK specification. // // See: From 6942e4f984da4b4820f3678abcf224286dad346c Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 16 Oct 2023 15:00:36 +0200 Subject: [PATCH 14/15] add title to Always On Sampler config --- docs/root/api-v3/config/trace/opentelemetry/samplers.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/root/api-v3/config/trace/opentelemetry/samplers.rst b/docs/root/api-v3/config/trace/opentelemetry/samplers.rst index 911759e6154bb..705155f640b95 100644 --- a/docs/root/api-v3/config/trace/opentelemetry/samplers.rst +++ b/docs/root/api-v3/config/trace/opentelemetry/samplers.rst @@ -1,3 +1,6 @@ +OpenTelemetry Samplers +====================== + Samplers that can be configured with the OpenTelemetry Tracer: .. toctree:: From f7950e95d6da8a9faa540cd338dede8286c807c7 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Tue, 17 Oct 2023 07:43:21 +0200 Subject: [PATCH 15/15] Add comment to tests --- api/envoy/config/trace/v3/opentelemetry.proto | 4 +--- .../opentelemetry/samplers/v3/always_on_sampler.proto | 2 +- .../samplers/always_on/always_on_sampler_test.cc | 5 +++-- .../tracers/opentelemetry/samplers/always_on/config_test.cc | 1 + .../tracers/opentelemetry/samplers/sampler_test.cc | 4 ++++ 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/api/envoy/config/trace/v3/opentelemetry.proto b/api/envoy/config/trace/v3/opentelemetry.proto index 208fd7ccee1f2..7af9840d18d4c 100644 --- a/api/envoy/config/trace/v3/opentelemetry.proto +++ b/api/envoy/config/trace/v3/opentelemetry.proto @@ -31,9 +31,7 @@ message OpenTelemetryConfig { // The configured sampler implements the Sampler interface defined by the OpenTelemetry specification. // This field can be left empty. In this case, the default Envoy sampling decision is used. // - // See: https://opentelemetry.io/docs/specs/otel/trace/sdk/#sampler - // - // See: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/observability/tracing#trace-context-propagation + // See: `OpenTelemetry sampler specification `_ // [#extension-category: envoy.tracers.opentelemetry.samplers] core.v3.TypedExtensionConfig sampler = 3; } diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto index 565df1416c235..241dc06eb1fc7 100644 --- a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/always_on_sampler.proto @@ -16,7 +16,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // SDK specification. // // See: -// `AlwaysOn specification `_ +// `AlwaysOn sampler specification `_ // [#extension: envoy.tracers.opentelemetry.samplers.always_on] message AlwaysOnSamplerConfig { diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc index 606abe76faa27..79d37ca9bfe80 100644 --- a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_test.cc @@ -15,15 +15,15 @@ namespace Extensions { namespace Tracers { namespace OpenTelemetry { +// Verify sampler being invoked with an invalid span context TEST(AlwaysOnSamplerTest, TestWithInvalidParentContext) { envoy::extensions::tracers::opentelemetry::samplers::v3::AlwaysOnSamplerConfig config; NiceMock context; auto sampler = std::make_shared(config, context); EXPECT_STREQ(sampler->getDescription().c_str(), "AlwaysOnSampler"); - absl::optional span_context; auto sampling_result = - sampler->shouldSample(span_context, "operation_name", "12345", + sampler->shouldSample(absl::nullopt, "operation_name", "12345", ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {}); EXPECT_EQ(sampling_result.decision, Decision::RECORD_AND_SAMPLE); EXPECT_EQ(sampling_result.attributes, nullptr); @@ -32,6 +32,7 @@ TEST(AlwaysOnSamplerTest, TestWithInvalidParentContext) { EXPECT_TRUE(sampling_result.isSampled()); } +// Verify sampler being invoked with a valid span context TEST(AlwaysOnSamplerTest, TestWithValidParentContext) { envoy::extensions::tracers::opentelemetry::samplers::v3::AlwaysOnSamplerConfig config; NiceMock context; diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc b/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc index 2829c1295d112..226cd58e34f39 100644 --- a/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/config_test.cc @@ -12,6 +12,7 @@ namespace Extensions { namespace Tracers { namespace OpenTelemetry { +// Test create sampler via factory TEST(AlwaysOnSamplerFactoryTest, Test) { auto* factory = Registry::FactoryRegistry::getFactory( "envoy.tracers.opentelemetry.samplers.always_on"); diff --git a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc index 30ffac30f261d..f80103b4345cb 100644 --- a/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/sampler_test.cc @@ -55,6 +55,7 @@ class SamplerFactoryTest : public testing::Test { NiceMock context; }; +// Test OTLP tracer without a sampler TEST_F(SamplerFactoryTest, TestWithoutSampler) { // using StrictMock, calls to SamplerFactory would cause a test failure auto test_sampler = std::make_shared>(); @@ -79,6 +80,7 @@ TEST_F(SamplerFactoryTest, TestWithoutSampler) { {Tracing::Reason::Sampling, true}); } +// Test config containing an unknown sampler TEST_F(SamplerFactoryTest, TestWithInvalidSampler) { // using StrictMock, calls to SamplerFactory would cause a test failure auto test_sampler = std::make_shared>(); @@ -104,6 +106,7 @@ TEST_F(SamplerFactoryTest, TestWithInvalidSampler) { EXPECT_THROW(std::make_unique(opentelemetry_config, context), EnvoyException); } +// Test OTLP tracer with a sampler TEST_F(SamplerFactoryTest, TestWithSampler) { auto test_sampler = std::make_shared>(); TestSamplerFactory sampler_factory; @@ -170,6 +173,7 @@ TEST_F(SamplerFactoryTest, TestWithSampler) { EXPECT_STREQ(unsampled_span->tracestate().c_str(), "this_is=another_tracesate"); } +// Test sampling result decision TEST(SamplingResultTest, TestSamplingResult) { SamplingResult result; result.decision = Decision::RECORD_AND_SAMPLE;