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

span context: set the default recorded flag to 0, and fix default trace options bug #474

Merged

Conversation

wsabransky
Copy link
Contributor

@wsabransky wsabransky commented Jan 30, 2019

Closes #298 .

Currently, the default trace option for a new trace context is to set enabled to true. This causes all samplers applied to be ignored here. We should keep the parent sampling decision if it is true, but ignore it if false. A parent shouldn't be able to force a downstream service to not sample.

Also fixes bug described here.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@wsabransky
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@songy23 songy23 requested a review from c24t February 4, 2019 17:48
@songy23
Copy link
Contributor

songy23 commented Feb 4, 2019

Please rebase and fix lint.

@benley
Copy link

benley commented Feb 4, 2019

Please rebase and fix lint.

The lint error appears to be wholly unrelated to this PR, in a file not touched. Do you want it fixed in this change?

@songy23
Copy link
Contributor

songy23 commented Feb 4, 2019

The lint error appears to be wholly unrelated to this PR, in a file not touched. Do you want it fixed in this change?

I see, For some reason lint didn't fail on my local machine. For now please ignore the error and just rebase against master.

@c24t
Copy link
Member

c24t commented Feb 5, 2019

I looked into the same issue in #482. There's another problem here: multiple Tracers can set the sampling ("enabled") bit on the shared default TraceOptions. See #482 (review).

Note that we still set the tracing bit in get_tracer based on the sampler, and the default sampler always samples:

def get_tracer(self):
"""Return a tracer according to the sampling decision."""
sampled = self.should_sample()
if sampled:
self.span_context.trace_options.set_enabled(True)
return context_tracer.ContextTracer(
exporter=self.exporter,
span_context=self.span_context)
else:
return noop_tracer.NoopTracer()
)

This might be the behavior you want, but to merge this PR we also need a test to show that the old behavior is broken. E.g. that the tracer used to sample even while using the AlwaysOffSampler.

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

If you add the test to show that the old behavior was wrong we can merge this.

@bogdandrutu
Copy link

@wsabransky any chance we can get some tests :)

@wsabransky
Copy link
Contributor Author

@bogdandrutu @c24t Sorry about the delay, I'll add some tests today.

@wsabransky wsabransky requested review from reyang, songy23 and a team as code owners March 5, 2019 20:26
@wsabransky
Copy link
Contributor Author

@c24t @bogdandrutu Okay, apparently there were already tests that cover this case, but they weren't included by the test runner because of how they were named.

Additionally, a scoping problem was causing the default trace options to be overwritten at runtime in some cases.

@wsabransky wsabransky changed the title span context: set the default recorded flag to 0 span context: set the default recorded flag to 0, and fix default trace options bug Mar 6, 2019
@wsabransky
Copy link
Contributor Author

@c24t any chance we could get this merged?

@c24t
Copy link
Member

c24t commented Apr 1, 2019

@wsabransky it looks like this change breaks a jaeger integration test. I may have time to check it out later, but it can't merge as-is.

Will Sabransky added 2 commits April 1, 2019 16:12
@wsabransky
Copy link
Contributor Author

@c24t looks like it was just a fixture that needed to be updated to reflect the new behavior in this PR. I've made the change.

@c24t
Copy link
Member

c24t commented Apr 2, 2019

Thanks @wsabransky!

@c24t c24t merged commit 0aa5525 into census-instrumentation:master Apr 2, 2019
@wsabransky wsabransky deleted the change-default-traceflags branch April 2, 2019 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All samplers are ignored as the default span context is force enabled
6 participants