From 105e52c506aab7f0b9dbcb2f7c0d6c49dc01e05c Mon Sep 17 00:00:00 2001 From: zirain Date: Fri, 28 Feb 2025 22:30:51 +0800 Subject: [PATCH 1/3] tracer: support request in cel sampler Signed-off-by: zirain --- envoy/tracing/trace_context.h | 3 -- .../opentelemetry/samplers/cel/cel_sampler.cc | 13 +++-- .../samplers/cel/cel_sampler_test.cc | 51 +++++++++++++++++-- test/test_common/utility.h | 33 ++++++++++++ 4 files changed, 90 insertions(+), 10 deletions(-) diff --git a/envoy/tracing/trace_context.h b/envoy/tracing/trace_context.h index fb2bd7eafba76..ebf5572c5da62 100644 --- a/envoy/tracing/trace_context.h +++ b/envoy/tracing/trace_context.h @@ -88,9 +88,6 @@ class TraceContext { */ virtual void remove(absl::string_view key) PURE; -private: - friend class TraceContextHandler; - /** * Optional HTTP request headers map. This is valid for HTTP protocol or any protocol that * that provides HTTP request headers. diff --git a/source/extensions/tracers/opentelemetry/samplers/cel/cel_sampler.cc b/source/extensions/tracers/opentelemetry/samplers/cel/cel_sampler.cc index bf2ad5f96fdd5..aa31bcec9c375 100644 --- a/source/extensions/tracers/opentelemetry/samplers/cel/cel_sampler.cc +++ b/source/extensions/tracers/opentelemetry/samplers/cel/cel_sampler.cc @@ -23,13 +23,18 @@ SamplingResult CELSampler::shouldSample(const StreamInfo::StreamInfo& stream_inf const absl::optional parent_context, const std::string& /*trace_id*/, const std::string& /*name*/, OTelSpanKind /*kind*/, - OptRef, + OptRef trace_context, const std::vector& /*links*/) { Protobuf::Arena arena; - auto eval_status = Expr::evaluate(*compiled_expr_, arena, &local_info_, stream_info, - nullptr /* request_headers */, nullptr /* response_headers */, - nullptr /* response_trailers */); + const ::Envoy::Http::RequestHeaderMap* request_headers = nullptr; + if (trace_context.has_value() && trace_context.value().get().requestHeaders().has_value()) { + request_headers = &trace_context.value().get().requestHeaders().value().get(); + } + + auto eval_status = Expr::evaluate( + *compiled_expr_, arena, &local_info_, stream_info, request_headers /* request_headers */, + nullptr /* response_headers */, nullptr /* response_trailers */); SamplingResult result; if (!eval_status.has_value() || eval_status.value().IsError()) { result.decision = Decision::Drop; diff --git a/test/extensions/tracers/opentelemetry/samplers/cel/cel_sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/cel/cel_sampler_test.cc index 505e66e38dbc6..fa6463eadf954 100644 --- a/test/extensions/tracers/opentelemetry/samplers/cel/cel_sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/cel/cel_sampler_test.cc @@ -118,13 +118,14 @@ TEST_F(CELSamplerTest, TestEval) { "envoy.tracers.opentelemetry.samplers.cel"); sampler_ = factory->createSampler(typed_config.typed_config(), context_); - Tracing::TestTraceContextImpl request_headers{ - {":authority", "test.com"}, {":path", "/"}, {":method", "GET"}}; + Http::TestRequestHeaderMapImpl request_headers = { + {":authority", "test.com"}, {":path", "/test-path"}, {":method", "GET"}}; + Tracing::TestRequestHeaderTraceContextImpl trace_context{request_headers}; SpanContext span_context("0", "12345", "45678", false, "some_tracestate"); auto sampling_result = sampler_->shouldSample( stream_info_, span_context, "operation_name", "12345", - ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, request_headers, {}); + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, trace_context, {}); EXPECT_EQ(sampling_result.decision, Decision::Drop); EXPECT_EQ(sampling_result.attributes, nullptr); EXPECT_STREQ(sampling_result.tracestate.c_str(), ""); @@ -179,6 +180,50 @@ TEST_F(CELSamplerTest, TestDecisionDrop) { EXPECT_FALSE(sampling_result.isSampled()); } +TEST_F(CELSamplerTest, TestHeader) { + envoy::config::core::v3::TypedExtensionConfig typed_config; + const std::string yaml = R"EOF( + name: envoy.tracers.opentelemetry.samplers.cel + typed_config: + "@type": type.googleapis.com/envoy.extensions.tracers.opentelemetry.samplers.v3.CELSamplerConfig + expression: + cel_expr_parsed: + expr: + id: 3 + call_expr: + function: _==_ + args: + - id: 2 + select_expr: + operand: + id: 1 + ident_expr: + name: request + field: path + - id: 4 + const_expr: + string_value: "/test-path" + )EOF"; + TestUtility::loadFromYaml(yaml, typed_config); + auto* factory = Registry::FactoryRegistry::getFactory( + "envoy.tracers.opentelemetry.samplers.cel"); + sampler_ = factory->createSampler(typed_config.typed_config(), context_); + + Http::TestRequestHeaderMapImpl request_headers = { + {":authority", "test.com"}, {":path", "/test-path"}, {":method", "GET"}}; + Tracing::TestRequestHeaderTraceContextImpl trace_context{request_headers}; + + SpanContext span_context("0", "12345", "45678", false, "some_tracestate"); + auto sampling_result = sampler_->shouldSample( + stream_info_, span_context, "operation_name", "12345", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, trace_context, {}); + EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample); + 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 diff --git a/test/test_common/utility.h b/test/test_common/utility.h index c45889b37b281..3c844d85814cd 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -934,6 +934,39 @@ class TestTraceContextImpl : public Tracing::TraceContext { absl::flat_hash_map context_map_; }; +class TestRequestHeaderTraceContextImpl : public Tracing::TraceContext { +public: + TestRequestHeaderTraceContextImpl(Http::RequestHeaderMap& request_headers) + : request_headers_(request_headers) {} + + absl::string_view protocol() const override { return request_headers_.getProtocolValue(); } + absl::string_view host() const override { return request_headers_.getHostValue(); } + absl::string_view path() const override { return request_headers_.getPathValue(); } + absl::string_view method() const override { return request_headers_.getMethodValue(); } + void forEach(IterateCallback callback) const override { + request_headers_.iterate([cb = std::move(callback)](const Http::HeaderEntry& entry) { + if (cb(entry.key().getStringView(), entry.value().getStringView())) { + return Http::HeaderMap::Iterate::Continue; + } + return Http::HeaderMap::Iterate::Break; + }); + } + absl::optional get(absl::string_view key) const override { + Http::LowerCaseString lower_key{std::string(key)}; + const auto entry = request_headers_.get(lower_key); + if (!entry.empty()) { + return entry[0]->value().getStringView(); + } + return absl::nullopt; + } + void set(absl::string_view, absl::string_view) override {} + void remove(absl::string_view) override {} + OptRef requestHeaders() const override { return request_headers_; }; + OptRef requestHeaders() override { return request_headers_; }; + + Http::RequestHeaderMap& request_headers_; +}; + } // namespace Tracing namespace Http { From ad7c43c7f44425cea21501b9f33fbddae2ed858e Mon Sep 17 00:00:00 2001 From: zirain Date: Sat, 1 Mar 2025 11:17:07 +0800 Subject: [PATCH 2/3] coverage Signed-off-by: zirain --- .../samplers/cel/cel_sampler_test.cc | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/extensions/tracers/opentelemetry/samplers/cel/cel_sampler_test.cc b/test/extensions/tracers/opentelemetry/samplers/cel/cel_sampler_test.cc index fa6463eadf954..4a39c113ea276 100644 --- a/test/extensions/tracers/opentelemetry/samplers/cel/cel_sampler_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/cel/cel_sampler_test.cc @@ -133,6 +133,50 @@ TEST_F(CELSamplerTest, TestEval) { EXPECT_FALSE(sampling_result.isSampled()); } +TEST_F(CELSamplerTest, TestEvalFail) { + envoy::config::core::v3::TypedExtensionConfig typed_config; + const std::string yaml = R"EOF( + name: envoy.tracers.opentelemetry.samplers.cel + typed_config: + "@type": type.googleapis.com/envoy.extensions.tracers.opentelemetry.samplers.v3.CELSamplerConfig + expression: + cel_expr_parsed: + expr: + id: 3 + call_expr: + function: _==_ + args: + - id: 2 + select_expr: + operand: + id: 1 + ident_expr: + name: req + field: path + - id: 4 + const_expr: + string_value: "/test-1234-deny" + )EOF"; + TestUtility::loadFromYaml(yaml, typed_config); + auto* factory = Registry::FactoryRegistry::getFactory( + "envoy.tracers.opentelemetry.samplers.cel"); + sampler_ = factory->createSampler(typed_config.typed_config(), context_); + + Http::TestRequestHeaderMapImpl request_headers = { + {":authority", "test.com"}, {":path", "/test-path"}, {":method", "GET"}}; + Tracing::TestRequestHeaderTraceContextImpl trace_context{request_headers}; + + SpanContext span_context("0", "12345", "45678", false, "some_tracestate"); + auto sampling_result = sampler_->shouldSample( + stream_info_, span_context, "operation_name", "12345", + ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, trace_context, {}); + EXPECT_EQ(sampling_result.decision, Decision::Drop); + EXPECT_EQ(sampling_result.attributes, nullptr); + EXPECT_STREQ(sampling_result.tracestate.c_str(), ""); + EXPECT_FALSE(sampling_result.isRecording()); + EXPECT_FALSE(sampling_result.isSampled()); +} + TEST_F(CELSamplerTest, TestDecisionDrop) { envoy::config::core::v3::TypedExtensionConfig typed_config; const std::string yaml = R"EOF( From b386dad6bd4777b10bb168e4a135dbaa57a0faa4 Mon Sep 17 00:00:00 2001 From: zirain Date: Thu, 6 Mar 2025 10:29:57 +0800 Subject: [PATCH 3/3] address review comment Signed-off-by: zirain --- .../tracers/opentelemetry/samplers/cel/cel_sampler.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/samplers/cel/cel_sampler.cc b/source/extensions/tracers/opentelemetry/samplers/cel/cel_sampler.cc index aa31bcec9c375..87ebb25232f0a 100644 --- a/source/extensions/tracers/opentelemetry/samplers/cel/cel_sampler.cc +++ b/source/extensions/tracers/opentelemetry/samplers/cel/cel_sampler.cc @@ -28,8 +28,8 @@ SamplingResult CELSampler::shouldSample(const StreamInfo::StreamInfo& stream_inf Protobuf::Arena arena; const ::Envoy::Http::RequestHeaderMap* request_headers = nullptr; - if (trace_context.has_value() && trace_context.value().get().requestHeaders().has_value()) { - request_headers = &trace_context.value().get().requestHeaders().value().get(); + if (trace_context.has_value() && trace_context->requestHeaders().has_value()) { + request_headers = trace_context->requestHeaders().ptr(); } auto eval_status = Expr::evaluate(