From f5e545a8aa9f7828142332b4d224d981786a6e8c Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Sat, 4 Jan 2025 12:25:46 +0100 Subject: [PATCH] router: make shadow requests inherit parent sampling decision by default Signed-off-by: Rohit Agrawal --- .../config/route/v3/route_components.proto | 5 ++- changelogs/current.yaml | 5 +++ envoy/router/router.h | 5 +++ source/common/router/config_impl.cc | 1 + source/common/router/config_impl.h | 2 + source/common/router/router.cc | 13 ++++-- test/common/router/config_impl_test.cc | 44 +++++++++++++++++++ 7 files changed, 71 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index a3d4d009f0eb..c3cf19e150aa 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -815,7 +815,10 @@ message RouteAction { // value, the request will be mirrored. core.v3.RuntimeFractionalPercent runtime_fraction = 3; - // Determines if the trace span should be sampled. Defaults to true. + // Specifies whether the trace span for the shadow request should be sampled. If this field is not explicitly set, + // the shadow request will inherit the sampling decision of its parent span. This ensures consistency with the trace + // sampling policy of the original request and prevents oversampling, especially in scenarios where runtime sampling + // is disabled. google.protobuf.BoolValue trace_sampled = 4; // Disables appending the ``-shadow`` suffix to the shadowed ``Host`` header. Defaults to ``false``. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 909c0053f23d..b6031865d82d 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -117,6 +117,11 @@ minor_behavior_changes: config because existing provider already has up to date routes config. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.normalize_rds_provider_config`` to false. +- area: router + change: | + Changed the behavior of shadow request sampling so that if trace sampling is not explicitly configured in the shadow + policy, the shadow request will inherit the parent's sampling decision. This means sampling will follow the trace + sampling policy of the original request, which prevents oversampling when runtime sampling is disabled. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/envoy/router/router.h b/envoy/router/router.h index 2efc01474346..39b92b28037e 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -540,6 +540,11 @@ class ShadowPolicy { */ virtual bool traceSampled() const PURE; + /** + * @return true if the config has `trace_sampled` set. + */ + virtual bool hasTraceSampledConf() const PURE; + /** * @return true if host name should be suffixed with "-shadow". */ diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index f158bca8b07c..0b8502db9374 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -490,6 +490,7 @@ ShadowPolicyImpl::ShadowPolicyImpl(const RequestMirrorPolicy& config, absl::Stat default_value_.set_denominator(envoy::type::v3::FractionalPercent::HUNDRED); } trace_sampled_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, trace_sampled, true); + has_trace_sampled_ = config.has_trace_sampled(); } DecoratorImpl::DecoratorImpl(const envoy::config::route::v3::Decorator& decorator) diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 344a31326ad5..bd44200b7b1c 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -510,6 +510,7 @@ class ShadowPolicyImpl : public ShadowPolicy { const std::string& runtimeKey() const override { return runtime_key_; } const envoy::type::v3::FractionalPercent& defaultValue() const override { return default_value_; } bool traceSampled() const override { return trace_sampled_; } + bool hasTraceSampledConf() const override { return has_trace_sampled_; } bool disableShadowHostSuffixAppend() const override { return disable_shadow_host_suffix_append_; } private: @@ -520,6 +521,7 @@ class ShadowPolicyImpl : public ShadowPolicy { std::string runtime_key_; envoy::type::v3::FractionalPercent default_value_; bool trace_sampled_; + bool has_trace_sampled_; const bool disable_shadow_host_suffix_append_; }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 7be5ba569780..bf881769658c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -444,6 +444,14 @@ void Filter::chargeUpstreamCode(Http::Code code, chargeUpstreamCode(response_status_code, *fake_response_headers, upstream_host, dropped); } +absl::optional getSampleValueFromShadowPolicy(const ShadowPolicy& shadow_policy) { + // If trace sampling is not explicitly configured in shadow_policy, we pass null optional to + // inherit the parent's sampling decision. This prevents oversampling when runtime sampling is + // disabled. + return shadow_policy.hasTraceSampledConf() ? absl::make_optional(shadow_policy.traceSampled()) + : absl::nullopt; +} + Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) { downstream_headers_ = &headers; @@ -802,7 +810,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, .setTimeout(timeout_.global_timeout_) .setParentSpan(callbacks_->activeSpan()) .setChildSpanName("mirror") - .setSampled(shadow_policy.traceSampled()) + .setSampled(getSampleValueFromShadowPolicy(shadow_policy)) .setIsShadow(true) .setIsShadowSuffixDisabled(shadow_policy.disableShadowHostSuffixAppend()) .setBufferAccount(callbacks_->account()) @@ -1066,12 +1074,11 @@ void Filter::maybeDoShadowing() { if (shadow_trailers_) { request->trailers(Http::createHeaderMap(*shadow_trailers_)); } - auto options = Http::AsyncClient::RequestOptions() .setTimeout(timeout_.global_timeout_) .setParentSpan(callbacks_->activeSpan()) .setChildSpanName("mirror") - .setSampled(shadow_policy.traceSampled()) + .setSampled(getSampleValueFromShadowPolicy(shadow_policy)) .setIsShadow(true) .setIsShadowSuffixDisabled(shadow_policy.disableShadowHostSuffixAppend()); options.setFilterConfig(config_); diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 8a25c063734f..744080a2b2ff 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -11542,6 +11542,50 @@ TEST_F(CommonConfigImplTest, TestCommonConfig) { shared_config.ignorePathParametersInPathMatching()); } +TEST_F(RouteMatcherTest, RequestMirrorPoliciesWithTraceSampled) { + const std::string yaml = R"EOF( +virtual_hosts: +- name: www2 + domains: ["*"] + routes: + - match: + prefix: "/foo" + route: + request_mirror_policies: + # Policy with explicit trace_sampled set to false + - cluster: some_cluster + trace_sampled: false + # Policy with explicit trace_sampled set to true + - cluster: some_cluster2 + trace_sampled: true + # Policy with no trace_sampled specified (should default to true) + - cluster: some_cluster3 + cluster: www2 + )EOF"; + + factory_context_.cluster_manager_.initializeClusters( + {"www2", "some_cluster", "some_cluster2", "some_cluster3"}, {}); + TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true, + creation_status_); + + { + Http::TestRequestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET"); + const auto& shadow_policies = config.route(headers, 0)->routeEntry()->shadowPolicies(); + + // First policy should have trace sampled = false + EXPECT_TRUE(shadow_policies[0]->hasTraceSampledConf()); + EXPECT_FALSE(shadow_policies[0]->traceSampled()); + + // Second policy should have trace sampled = true + EXPECT_TRUE(shadow_policies[1]->hasTraceSampledConf()); + EXPECT_TRUE(shadow_policies[1]->traceSampled()); + + // Since the config is unspecified for the third policy, trace sampled should default to true + EXPECT_FALSE(shadow_policies[2]->hasTraceSampledConf()); + EXPECT_TRUE(shadow_policies[2]->traceSampled()); + } +} + } // namespace } // namespace Router } // namespace Envoy