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

Shadowed requests override parent sampling decisions #37766

Open
samschlegel opened this issue Dec 19, 2024 · 2 comments · May be fixed by #37768
Open

Shadowed requests override parent sampling decisions #37766

samschlegel opened this issue Dec 19, 2024 · 2 comments · May be fixed by #37768
Labels
area/mirroring area/tracing enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@samschlegel
Copy link

samschlegel commented Dec 19, 2024

Title: Shadowed requests override parent sampling decisions

Description:
Similar to #17085, shadowed requests override the parents sampling status here. 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

@samschlegel samschlegel added bug triage Issue requires triage labels Dec 19, 2024
@adisuissa adisuissa added area/mirroring and removed triage Issue requires triage labels Dec 20, 2024
@adisuissa
Copy link
Contributor

Thanks for notifying us about this issue.
cc @alyssawilk @paul-r-gall @wbpcode as this may intersect with your work on mirroring and sampling.

@wbpcode
Copy link
Member

wbpcode commented Dec 23, 2024

SGTM to change the default behavior to keep the parent trace decisions if the trace_sampled is not set explicitly.

@wbpcode wbpcode added enhancement Feature requests. Not bugs or questions. area/tracing help wanted Needs help! and removed bug labels Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/mirroring area/tracing enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants