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

fix(tracing/core): sampled flags and propagation #10655

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

samugi
Copy link
Member

@samugi samugi commented Apr 11, 2023

Summary

fix an issue where incoming propagation headers with disabled sampling (sampled flag = 0) produced invalid propagation of noop spans and broken traces

This fix:

  • ensures propagation is not attempted on noop spans
  • ensures no span is sampled when sampling is disabled in the incoming headers
  • propagates the correct header when sampling is disabled in the incoming headers

Checklist

Full changelog

  • [Implement ...]

Issue reference

https://konghq.atlassian.net/browse/KAG-1184
https://konghq.atlassian.net/browse/KAG-1176

@samugi samugi marked this pull request as draft April 11, 2023 21:46
@samugi samugi marked this pull request as ready for review April 11, 2023 22:06
kong/pdk/tracing.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@StarlightIbuki StarlightIbuki left a comment

Choose a reason for hiding this comment

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

I have a question after reviewing the code (though not related to the fix). If every node always inherits "should_sample", and none of the upstream nodes will ever send the trace, what's the point we propagate the tracing context via headers?

kong/pdk/tracing.lua Outdated Show resolved Hide resolved
@samugi
Copy link
Member Author

samugi commented Apr 12, 2023

I have a question after reviewing the code (though not related to the fix). If every node always inherits "should_sample", and none of the upstream nodes will ever send the trace, what's the point we propagate the tracing context via headers?

I think the answer is in here, for example: A component may also fall back to probability sampling and set the sampled flag to 1 for the subset of requests.. In other words we don't know whether upstream will use Deferred sampling or some other strategy so the right thing to do would be propagating the context.

fix an issue where incoming propagation headers with disabled sampling
produced invalid propagation of noop spans and broken traces

This fix:
* ensures propagation is not attempted on noop spans
* ensures no span is sampled when sampling is disabled in the incoming
  headers
* propagates the correct header in when sampling is
  disabled in the incoming headers
@samugi samugi changed the title fix(tracing): sampled flags and propagation fix(tracing/core): sampled flags and propagation Apr 13, 2023
@samugi samugi merged commit 56bb1f2 into master Apr 13, 2023
@samugi samugi deleted the fix/tracing-span-sampling branch April 13, 2023 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants