-
Notifications
You must be signed in to change notification settings - Fork 47
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
Handle when zipkin_attrs are passed in and sampled correctly #10
Handle when zipkin_attrs are passed in and sampled correctly #10
Conversation
…d, but the zipkin_span also has a sample_rate argument
if self.sample_rate is not None: | ||
self.is_root = True | ||
|
||
# This clause allows for sampling this service independently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to clarify for my own understanding, this means that sample_rate is the sampling rate of all requests that are not already sampled by a parent caller? so the actual tracing rate of the service is potentially higher than the specified sample_rate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if so i think it'll be worth mentioning this in the docstring for the sample_rate param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is true, but it was true before this PR. I will update the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah 😣 thanks for adding documentation that i should have added! 😅
|
When sampled zipkin_attrs are passed into
zipkin_span
along with a sample_rate param, new zipkin_attrs are generated, even when they shouldn't be. This fixes that.I also renamed the 'is_root' to 'perform_logging' and moved the logic around a bit. I can't recall if we've discussed this before, but I still find the 'is_root' variable name to be confusing. I'm open to changing that part back if anyone feels strongly.
Finally, added a regression test for the correct zipkin_attrs + sampling logic.