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

ext_proc: change default sampling to false inheriting from parent decision #37794

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

cainelli
Copy link
Contributor

@cainelli cainelli commented Dec 23, 2024

Commit Message: ext_proc: change default sampling decision to false
Fixes: #37783
Additional Description: Default to using the parent span's sampled status instead of true (default in StreamOptions), so that we don't emit orphan spans for all ext_proc streams where the parent span is not sampled. That's the same issue seen in Lua httpCall (#33200) and ext_authz (#19343).

Risk Level: Low
Testing: changed the tracing integration test behavior and validated in production.

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #37794 was opened by cainelli.

see: more, trace.

@cainelli cainelli changed the title ext_proc: change default sampling decision to false ext_proc: change default sampling to false inheriting from parent decision Dec 23, 2024
@cainelli
Copy link
Contributor Author

/retest

@cainelli cainelli marked this pull request as ready for review December 23, 2024 16:32
Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you

@cainelli
Copy link
Contributor Author

cainelli commented Jan 8, 2025

@tyxia would you need an extra reviewer before merging?

@tyxia tyxia merged commit f213d16 into envoyproxy:main Jan 8, 2025
24 checks passed
@cainelli cainelli deleted the extproc-tracing-sampling branch January 8, 2025 15:55
Yueren-Wang pushed a commit to Yueren-Wang/envoy that referenced this pull request Jan 9, 2025
…ision (envoyproxy#37794)

Commit Message: ext_proc: change default sampling decision to false
Fixes: envoyproxy#37783
Additional Description: Default to using the parent span's sampled
status instead of true (default in
[StreamOptions](https://github.com/envoyproxy/envoy/blob/df5260957fc9446b57114a74ffbdc5d9b5831bfd/envoy/http/async_client.h#L315-L316)),
so that we don't emit orphan spans for all ext_proc streams where the
parent span is not sampled. That's the same issue seen in Lua httpCall
(envoyproxy#33200) and ext_authz
(envoyproxy#19343).

Risk Level: Low
Testing: changed the tracing integration test behavior and validated in
production.

---------

Signed-off-by: Fernando Cainelli <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ext_proc: GRPC tracing requests are always sampled regardless of the parent decision
2 participants