Don't enable sampling by default #482
Conversation
| @@ -64,7 +58,7 @@ def __init__( | |||
| trace_id = generate_trace_id() | |||
|
|
|||
| if trace_options is None: | |||
| trace_options = DEFAULT | |||
| trace_options = trace_options_module.TraceOptions() | |||
There was a problem hiding this comment.
Re-using the default trace options here meant that tracers would set each others' contexts' sampling bit.
I suspect there are other bugs like this lurking in the module.
There was a problem hiding this comment.
TODO: add a test for tracer isolation.
| ) | ||
|
|
||
| # TODO: deprecate, replace with is_sampled() |
| def __init__(self, trace_options_byte=DEFAULT): | ||
| if not isinstance(trace_options_byte, int): | ||
| logging.warning("trace_options_byte must be an int, this will be " | ||
| "an error in future versions") |
There was a problem hiding this comment.
This is technically an API change, so I figure the right approach is to log a warning here and make this an error in the next minor version release. We would benefit from some convention for deprecation, compare to java's @deprecated annotation.
| @@ -308,7 +308,7 @@ def test_translate_to_jaeger(self): | |||
| operationName='test1', | |||
| startTime=1502820146071158, | |||
| duration=10000000, | |||
| flags=1, | |||
| flags=0, | |||
There was a problem hiding this comment.
TODO: check that this is the behavior we want.
| @@ -73,7 +73,7 @@ def should_sample(self): | |||
| :rtype: bool | |||
| :returns: Whether to trace the request or not. | |||
| """ | |||
| return self.span_context.trace_options.enabled \ | |||
| return self.span_context.trace_options.get_enabled() \ | |||
| or self.sampler.should_sample(self.span_context.trace_id) | |||
There was a problem hiding this comment.
Related to this PR, should_sample needs to take the trace options as well. For example, the way the probability sampler should work is that if trace options is true then return true, else check against the configured probability. This way if a remote parent is sampled the children will be too, but still allowing for a sampler to override that.
c24t
requested review from
aabmass,
hectorhdzg,
lzchen,
songy23 and
a team
as code owners
This is a WIP PR to disable sampling using default trace options. This started as a drive-by fix to replace a string-valued bit field with an int and expanded as I compared
TraceOptionsandSpanContextto their counterparts in java.@liyanhui1228 I'd appreciate some comments here since it's not obvious that sampling shouldn't be enabled by default, and there are a few design choices here I don't understand.
Addresses #481.