envoyproxy / envoyproxy/envoy

Shadowed requests override parent sampling decisions

Open
#37,766 3 comments 2 reactions 1 assignee Claimed by @agrawroh View on GitHub
area/mirroring area/tracing enhancement help wanted
Dominant language
C++
Stars
28.9k
Forks
5.6k
Avg merge
1d 22h
Merged PRs (30d)
430

Description

*Title*: Shadowed requests override parent sampling decisions

*Description*:
Similar to #17085, shadowed requests override the parents sampling status [here](https://github.com/envoyproxy/envoy/blob/99bbfe841b4696367c33de682e272ba33db252f1/source/common/router/router.cc#L805). I see that #19343 added support to make this optional, but this was only updated for ext_authz. I believe it should be safe to do the same for shadowed requests, though there's some considerations on how this should be implemented since we do have sampling as part of the config. One proposal is that we pass `absl::nullopt` if `shadow_policy.traceSampled()` is true, and otherwise pass false, which would allow disabling the tracing with the option, and then enabling would as "as expected"

The impact here is that even when we have the runtime_percent for sampling set to 0%, the mirrored requests are always sampled, leading to excessive oversampling. As far as I can tell this isn't specific to the Datadog trace extension but it is possible this isn't an issue with others as I'm surprised this hasn't been flagged yet.

*Repro steps*:
I haven't reduced this to a minimal repro, but given the existence of #17085 and the code linked above it feels sufficient to me, but please let me know if you feel a repro is necessary.

cc @nulltrope

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.