Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router: make shadow requests inherit parent sampling decision by default #37874

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion api/envoy/config/route/v3/route_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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``.
Expand Down
5 changes: 5 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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*
Expand Down
5 changes: 5 additions & 0 deletions envoy/router/router.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

agrawroh marked this conversation as resolved.
Show resolved Hide resolved
/**
* @return true if host name should be suffixed with "-shadow".
*/
Expand Down
1 change: 1 addition & 0 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions source/common/router/config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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_;
};

Expand Down
13 changes: 10 additions & 3 deletions source/common/router/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
agrawroh marked this conversation as resolved.
Show resolved Hide resolved

Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) {
downstream_headers_ = &headers;

Expand Down Expand Up @@ -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))
agrawroh marked this conversation as resolved.
Show resolved Hide resolved
.setIsShadow(true)
.setIsShadowSuffixDisabled(shadow_policy.disableShadowHostSuffixAppend())
.setBufferAccount(callbacks_->account())
Expand Down Expand Up @@ -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))
agrawroh marked this conversation as resolved.
Show resolved Hide resolved
.setIsShadow(true)
.setIsShadowSuffixDisabled(shadow_policy.disableShadowHostSuffixAppend());
options.setFilterConfig(config_);
Expand Down
44 changes: 44 additions & 0 deletions test/common/router/config_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading