From 3bd518d78f646d6f504afe46e9d2dbfd4c8fcb18 Mon Sep 17 00:00:00 2001 From: William Date: Mon, 27 Jan 2025 19:22:45 -0800 Subject: [PATCH] add propagate_unsampled field Signed-off-by: William --- .../v3/http_connection_manager.proto | 6 +- changelogs/current.yaml | 6 +- envoy/tracing/trace_config.h | 5 ++ source/common/http/conn_manager_impl.cc | 4 ++ source/common/tracing/tracer_config_impl.h | 12 +++- test/common/http/conn_manager_impl_test.cc | 66 ++++++++++++++++++- .../http/conn_manager_impl_test_base.cc | 1 + test/common/http/conn_manager_utility_test.cc | 2 +- .../http_connection_manager/config_test.cc | 38 +++++++++++ 9 files changed, 133 insertions(+), 7 deletions(-) diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index ce549d6a9017..e91d31b27105 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -129,7 +129,7 @@ message HttpConnectionManager { UNESCAPE_AND_FORWARD = 4; } - // [#next-free-field: 11] + // [#next-free-field: 12] message Tracing { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager.Tracing"; @@ -171,6 +171,10 @@ message HttpConnectionManager { // Default: 100% type.v3.Percent overall_sampling = 5; + // Whether tracing metadata should be propagated upstream when the request is not sampled. + // Default: true. + google.protobuf.BoolValue propagate_unsampled = 11; + // Whether to annotate spans with additional data. If true, spans will include logs for stream // events. bool verbose = 6; diff --git a/changelogs/current.yaml b/changelogs/current.yaml index c5eb24f4a9c5..34b7fc83b117 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -99,6 +99,11 @@ new_features: change: | Added :ref:`max_metadata_size ` to make HTTP/2 metadata limits configurable. +- area: http + change: | + Added :ref:`propagate_unsampled + ` + to support not modifying the existing trace context when requests aren't sampled. - area: redis change: | Added support for multi-key commands on transactions. @@ -106,5 +111,4 @@ new_features: change: | Reporting a locality_stats to LRS server when rq_issued > 0, disable by setting runtime guard ``envoy.reloadable_features.report_load_with_rq_issued`` to ``false``. - deprecated: diff --git a/envoy/tracing/trace_config.h b/envoy/tracing/trace_config.h index c556aa592f38..ff7e5a0da4a6 100644 --- a/envoy/tracing/trace_config.h +++ b/envoy/tracing/trace_config.h @@ -90,6 +90,11 @@ class ConnectionManagerTracingConfig : public TracingConfig { */ virtual bool spawnUpstreamSpan() const PURE; + /** + * @return whether to propagate span information for unsampled requests. + */ + virtual bool propagateUnsampled() const PURE; + /** * @return true if spans should be annotated with more detailed information. */ diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index f5f10cdb6d59..002477d9e221 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1452,6 +1452,10 @@ void ConnectionManagerImpl::ActiveStream::traceRequest() { ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason, connection_manager_.config_->tracingStats()); + if (!connection_manager_tracing_config_->propagateUnsampled()) { + return; + } + Tracing::HttpTraceContext trace_context(*request_headers_); active_span_ = connection_manager_.tracer().startSpan( *this, trace_context, filter_manager_.streamInfo(), tracing_decision); diff --git a/source/common/tracing/tracer_config_impl.h b/source/common/tracing/tracer_config_impl.h index 9ea8e1fe58b9..0ec632789400 100644 --- a/source/common/tracing/tracer_config_impl.h +++ b/source/common/tracing/tracer_config_impl.h @@ -71,6 +71,9 @@ class ConnectionManagerTracingConfigImpl : public ConnectionManagerTracingConfig tracing_config, overall_sampling, 10000, 10000)}; overall_sampling_.set_numerator(overall_sampling_numerator); overall_sampling_.set_denominator(envoy::type::v3::FractionalPercent::TEN_THOUSAND); + propagate_unsampled_ = tracing_config.has_propagate_unsampled() + ? tracing_config.propagate_unsampled().value() + : true; verbose_ = tracing_config.verbose(); max_path_tag_length_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(tracing_config, max_path_tag_length, @@ -82,11 +85,12 @@ class ConnectionManagerTracingConfigImpl : public ConnectionManagerTracingConfig envoy::type::v3::FractionalPercent client_sampling, envoy::type::v3::FractionalPercent random_sampling, envoy::type::v3::FractionalPercent overall_sampling, - bool verbose, uint32_t max_path_tag_length) + bool propagate_unsampled, bool verbose, + uint32_t max_path_tag_length) : operation_name_(operation_name), custom_tags_(custom_tags), client_sampling_(client_sampling), random_sampling_(random_sampling), - overall_sampling_(overall_sampling), verbose_(verbose), - max_path_tag_length_(max_path_tag_length) {} + overall_sampling_(overall_sampling), propagate_unsampled_(propagate_unsampled), + verbose_(verbose), max_path_tag_length_(max_path_tag_length) {} ConnectionManagerTracingConfigImpl() = default; @@ -99,6 +103,7 @@ class ConnectionManagerTracingConfigImpl : public ConnectionManagerTracingConfig const envoy::type::v3::FractionalPercent& getOverallSampling() const override { return overall_sampling_; } + bool propagateUnsampled() const override { return propagate_unsampled_; } const Tracing::CustomTagMap& getCustomTags() const override { return custom_tags_; } Tracing::OperationName operationName() const override { return operation_name_; } @@ -113,6 +118,7 @@ class ConnectionManagerTracingConfigImpl : public ConnectionManagerTracingConfig envoy::type::v3::FractionalPercent client_sampling_; envoy::type::v3::FractionalPercent random_sampling_; envoy::type::v3::FractionalPercent overall_sampling_; + bool propagate_unsampled_{}; bool verbose_{}; uint32_t max_path_tag_length_{}; bool spawn_upstream_span_{}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 3d1ad6854d04..feb97f02afcc 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1576,7 +1576,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { } tracing_config_ = std::make_unique( TracingConnectionManagerConfig{Tracing::OperationName::Ingress, conn_tracing_tags, percent1, - percent2, percent1, false, 256}); + percent2, percent1, true, false, 256}); NiceMock route_tracing; ON_CALL(route_tracing, getClientSampling()).WillByDefault(ReturnRef(percent1)); ON_CALL(route_tracing, getRandomSampling()).WillByDefault(ReturnRef(percent2)); @@ -1865,6 +1865,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato percent1, percent2, percent1, + true, false, 256}); @@ -1949,6 +1950,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato percent1, percent2, percent1, + true, false, 256}); @@ -2035,6 +2037,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato percent1, percent2, percent1, + true, false, 256}); @@ -2113,6 +2116,7 @@ TEST_F(HttpConnectionManagerImplTest, percent1, percent2, percent1, + true, false, 256}); @@ -2209,6 +2213,66 @@ TEST_F(HttpConnectionManagerImplTest, NoHCMTracingConfigAndActiveSpanWouldBeNull conn_manager_->onData(fake_input, false); } +TEST_F(HttpConnectionManagerImplTest, SkipUnsampledSpan) { + setup(); + envoy::type::v3::FractionalPercent zero_percent; + tracing_config_ = std::make_unique( + TracingConnectionManagerConfig{Tracing::OperationName::Egress, + {{":method", requestHeaderCustomTag(":method")}}, + zero_percent, + zero_percent, + zero_percent, + false, + false, + 256}); + + std::shared_ptr filter(new NiceMock()); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { + auto factory = createDecoderFilterFactoryCb(filter); + manager.applyFilterFactoryCb({}, factory); + return true; + })); + + // Treat request as internal, otherwise x-request-id header will be overwritten. + use_remote_address_ = false; + EXPECT_CALL(random_, uuid()).Times(0); + + EXPECT_CALL(*codec_, dispatch(_)) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status { + decoder_ = &conn_manager_->newStream(response_encoder_); + + RequestHeaderMapPtr headers{ + new TestRequestHeaderMapImpl{{":method", "GET"}, + {":authority", "host"}, + {":path", "/"}, + {"x-request-id", "125a4afb-6f55-a4ba-ad80-413f09f48a28"}}}; + decoder_->decodeHeaders(std::move(headers), true); + + filter->callbacks_->streamInfo().setResponseCodeDetails(""); + ResponseHeaderMapPtr response_headers{new TestResponseHeaderMapImpl{ + {":status", "200"}, {"x-envoy-decorator-operation", "testOp"}}}; + filter->callbacks_->encodeHeaders(std::move(response_headers), true, "details"); + + // Active span always be null span when there is no HCM tracing config. + EXPECT_EQ(&Tracing::NullSpan::instance(), &filter->callbacks_->activeSpan()); + // Child span should also be null span. + Tracing::SpanPtr child_span = filter->callbacks_->activeSpan().spawnChild( + Tracing::EgressConfig::get(), "null_child", test_time_.systemTime()); + Tracing::SpanPtr null_span = std::make_unique(); + auto& child_span_ref = *child_span; + auto& null_span_ref = *null_span; + EXPECT_EQ(typeid(child_span_ref).name(), typeid(null_span_ref).name()); + + data.drain(4); + return Http::okStatus(); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); +} + TEST_F(HttpConnectionManagerImplTest, TestAccessLog) { static constexpr char remote_address[] = "0.0.0.0"; static constexpr char xff_address[] = "1.2.3.4"; diff --git a/test/common/http/conn_manager_impl_test_base.cc b/test/common/http/conn_manager_impl_test_base.cc index 4ffe97b60cd8..b5cd63662b5a 100644 --- a/test/common/http/conn_manager_impl_test_base.cc +++ b/test/common/http/conn_manager_impl_test_base.cc @@ -227,6 +227,7 @@ void HttpConnectionManagerImplMixin::setup(const SetupOpts& opts) { percent1, percent2, percent1, + true, false, 256}); } diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index e118cf188c07..33e75e34728d 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -108,7 +108,7 @@ class ConnectionManagerUtilityTest : public testing::Test { percent2.set_numerator(10000); percent2.set_denominator(envoy::type::v3::FractionalPercent::TEN_THOUSAND); tracing_config_ = { - Tracing::OperationName::Ingress, {}, percent1, percent2, percent1, false, 256}; + Tracing::OperationName::Ingress, {}, percent1, percent2, percent1, true, false, 256}; ON_CALL(config_, tracingConfig()).WillByDefault(Return(&tracing_config_)); ON_CALL(config_, localReply()).WillByDefault(ReturnRef(*local_reply_)); diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index f471b6664c9d..d090865e9cd9 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -550,6 +550,7 @@ TEST_F(HttpConnectionManagerConfigTest, SamplingDefault) { EXPECT_EQ(10000, config.tracingConfig()->overall_sampling_.numerator()); EXPECT_EQ(envoy::type::v3::FractionalPercent::TEN_THOUSAND, config.tracingConfig()->overall_sampling_.denominator()); + EXPECT_TRUE(config.tracingConfig()->propagate_unsampled_); } TEST_F(HttpConnectionManagerConfigTest, SamplingConfigured) { @@ -566,6 +567,7 @@ TEST_F(HttpConnectionManagerConfigTest, SamplingConfigured) { value: 2 overall_sampling: value: 3 + propagate_unsampled: false http_filters: - name: envoy.filters.http.router typed_config: @@ -587,6 +589,7 @@ TEST_F(HttpConnectionManagerConfigTest, SamplingConfigured) { EXPECT_EQ(300, config.tracingConfig()->overall_sampling_.numerator()); EXPECT_EQ(envoy::type::v3::FractionalPercent::TEN_THOUSAND, config.tracingConfig()->overall_sampling_.denominator()); + EXPECT_FALSE(config.tracingConfig()->propagate_unsampled_); } TEST_F(HttpConnectionManagerConfigTest, FractionalSamplingConfigured) { @@ -624,6 +627,7 @@ TEST_F(HttpConnectionManagerConfigTest, FractionalSamplingConfigured) { EXPECT_EQ(30, config.tracingConfig()->overall_sampling_.numerator()); EXPECT_EQ(envoy::type::v3::FractionalPercent::TEN_THOUSAND, config.tracingConfig()->overall_sampling_.denominator()); + EXPECT_TRUE(config.tracingConfig()->propagate_unsampled_); } TEST_F(HttpConnectionManagerConfigTest, OverallSampling) { @@ -682,6 +686,40 @@ TEST_F(HttpConnectionManagerConfigTest, OverallSampling) { EXPECT_GE(1200, sampled_count); } +TEST_F(HttpConnectionManagerConfigTest, PropagateUnsampledExplicitTrue) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + internal_address_config: + unix_sockets: true + route_config: + name: local_route + tracing: + propagate_unsampled: true + http_filters: + - name: envoy.filters.http.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromYaml(yaml_string), context_, + date_provider_, route_config_provider_manager_, + &scoped_routes_config_provider_manager_, tracer_manager_, + filter_config_provider_manager_, creation_status_); + ASSERT_TRUE(creation_status_.ok()); + + EXPECT_EQ(100, config.tracingConfig()->client_sampling_.numerator()); + EXPECT_EQ(Tracing::DefaultMaxPathTagLength, config.tracingConfig()->max_path_tag_length_); + EXPECT_EQ(envoy::type::v3::FractionalPercent::HUNDRED, + config.tracingConfig()->client_sampling_.denominator()); + EXPECT_EQ(10000, config.tracingConfig()->random_sampling_.numerator()); + EXPECT_EQ(envoy::type::v3::FractionalPercent::TEN_THOUSAND, + config.tracingConfig()->random_sampling_.denominator()); + EXPECT_EQ(10000, config.tracingConfig()->overall_sampling_.numerator()); + EXPECT_EQ(envoy::type::v3::FractionalPercent::TEN_THOUSAND, + config.tracingConfig()->overall_sampling_.denominator()); + EXPECT_TRUE(config.tracingConfig()->propagate_unsampled_); +} + TEST_F(HttpConnectionManagerConfigTest, UnixSocketInternalAddress) { const std::string yaml_string = R"EOF( stat_prefix: ingress_http