router: make shadow requests inherit parent sampling decision by default

Signed-off-by: Rohit Agrawal <[email protected]>

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``.

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*

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".
    */

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)

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_;
 };
 

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<bool> 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<Http::RequestTrailerMapImpl>(*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_);

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