-
Notifications
You must be signed in to change notification settings - Fork 375
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
Enable priority sampling by default #654
Conversation
lib/ddtrace/tracer.rb
Outdated
max_spans_before_partial_flush = options.fetch(:max_spans_before_partial_flush, nil) | ||
min_spans_before_partial_flush = options.fetch(:min_spans_before_partial_flush, nil) | ||
partial_flush_timeout = options.fetch(:partial_flush_timeout, nil) | ||
|
||
@enabled = enabled unless enabled.nil? | ||
@sampler = sampler unless sampler.nil? | ||
|
||
if priority_sampling | ||
if priority_sampling && !@sampler.is_a?(PrioritySampler) |
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.
Added this check because if you configured the tracer twice, it would wrap the previous PrioritySampler
with another PrioritySampler
, and that seems problematic...
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.
yeah, we have a weird thing in dd-trace-py
where we have priority_sampling
default to nil
so that we only try to update the sampler if the option has been provided, rather than defaulting to True
.
I think this basically does the same thing.
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.
it might be good to add a comment here about why we have this check here.
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.
Cool, I'll add that.
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.
I see we updated options.fetch(:priority_sampling, true)
, does that mean we should have this be if !priority_sampling.nil?
(sorry for my bad ruby)
Since nil
is falsey, we probably only want to modify what sampler/writer we are using if we are setting priority sampling.
Also, do we need to handle :priority_smapling => false
? if we reconfigure and set to false
do we need to update the sampler/writer again to use a RateSampler
?
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.
Since
nil
is falsey, we probably only want to modify what sampler/writer we are using if we are setting priority sampling.
Consider the following: {}.fetch(:a, true) #=> true
. Users would have to explicitly specify the key with a nil
value in order to pass nil
.
Given the above, I think your original assertion is already the case here. The if
block wouldn't run if given nil
, but it should never receive nil
unless given nil
explicitly.
Also, do we need to handle
:priority_smapling => false
? if we reconfigure and set to false do we need to update the sampler/writer again to use aRateSampler
?
I think if we support this, then we need to add an elsif
:
elsif !priority_sampling
@sampler = sampler || Datadog::AllSampler.new if @sampler.is_a?(PrioritySampler)
@writer = Writer.new
end
If you want nil
to be a no-op, then you'd have to change !priority_sampling
to priority_sampling == false
.
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.
yeah, sorry I misread the code. In dd-trace-py
we default to None
(nil
), which means the user did not supply the option so we then default to True
.
How do we instruct people to use #configure
?
In dd-trace-py
we allow people to supply only the arguments they want to modify.
An example:
tracer.configure(priority_sampling=False)
tracer.configure(host='127.0.0.1')
Which will configure the tracer disabling priority sampling, and then configure the host to be localhost while keeping priority sampling disabled.
This might just be how we have .configure()
setup for dd-trace-py
, curious if this is (or should be) the same case here?
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.
Right, first call would deactivate, then second would reactivate. That's no good.
Changing the logic such that it defaults to nil
, activates if != false
, and deactivates only if == false
.
test/context_flush_test.rb
Outdated
@@ -366,6 +366,7 @@ def test_tracer_hard_limit_overrides_soft_limit | |||
tracer.configure(min_spans_before_partial_flush: context.max_length, | |||
max_spans_before_partial_flush: context.max_length, | |||
partial_flush_timeout: 3600) | |||
tracer.writer = FauxWriter.new |
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.
Nasty side effect of this is that everytime we reconfigure the tracer, it automatically overrides the previous writer with a new one, which puts test tracers in a bad state.
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.
It might be good to add a comment here about that
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.
Unfortunately this comment will apply just about everywhere we use tracer.configure
in tests so I don't think it'll help. I'd rather try to find some way to get rid of this, because this kind of practice in tests is not sustainable.
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.
Something dd-trace-py
did for tests in create a DummyTracer
to use in all tests that will always overwrite the writer to be a DummyWriter
, even after calling .configure()
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.
Yeah, I was looking at that, too. Problem is configure
applies a bunch of mutations to Writer
, so if you have to re-initialize with a FauxWriter
, then you have to copy over all the settings, otherwise you'll end up with a misconfigured Writer
.
I think ideally, the code should either construct immutable Writer
objects that have to be replaced, or mutable ones that are never replaced; right now we're in a kind of unhappy middle ground.
Nonetheless I'm going to try to avoid such refactoring right now, maybe there's something else I can do to work around this in the mean time.
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.
yeah, we have that same issue. since we isolated it into one place in a subclass of Tracer
it wasn't too bad... enough to get by for now
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.
For sure, I'll try one of these tricks to make something work for now, but it would be nice if we refactored things to be more testable. A goal for the future, perhaps.
Looks like simply changing the option from |
docs/GettingStarted.md
Outdated
@@ -1386,6 +1378,14 @@ span.context.sampling_priority = Datadog::Ext::Priority::USER_REJECT | |||
span.context.sampling_priority = Datadog::Ext::Priority::USER_KEEP | |||
``` | |||
|
|||
You can disable priority sampling via: |
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.
In dd-trace-py
we removed this part from our docs, since disabling priority sampling has a chance of breaking distributed tracing, better to just leave this out.
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.
Good point.
lib/ddtrace/tracer.rb
Outdated
max_spans_before_partial_flush = options.fetch(:max_spans_before_partial_flush, nil) | ||
min_spans_before_partial_flush = options.fetch(:min_spans_before_partial_flush, nil) | ||
partial_flush_timeout = options.fetch(:partial_flush_timeout, nil) | ||
|
||
@enabled = enabled unless enabled.nil? | ||
@sampler = sampler unless sampler.nil? | ||
|
||
if priority_sampling | ||
if priority_sampling && !@sampler.is_a?(PrioritySampler) |
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.
yeah, we have a weird thing in dd-trace-py
where we have priority_sampling
default to nil
so that we only try to update the sampler if the option has been provided, rather than defaulting to True
.
I think this basically does the same thing.
test/context_flush_test.rb
Outdated
@@ -366,6 +366,7 @@ def test_tracer_hard_limit_overrides_soft_limit | |||
tracer.configure(min_spans_before_partial_flush: context.max_length, | |||
max_spans_before_partial_flush: context.max_length, | |||
partial_flush_timeout: 3600) | |||
tracer.writer = FauxWriter.new |
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.
It might be good to add a comment here about that
lib/ddtrace/tracer.rb
Outdated
max_spans_before_partial_flush = options.fetch(:max_spans_before_partial_flush, nil) | ||
min_spans_before_partial_flush = options.fetch(:min_spans_before_partial_flush, nil) | ||
partial_flush_timeout = options.fetch(:partial_flush_timeout, nil) | ||
|
||
@enabled = enabled unless enabled.nil? | ||
@sampler = sampler unless sampler.nil? | ||
|
||
if priority_sampling | ||
if priority_sampling && !@sampler.is_a?(PrioritySampler) |
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.
it might be good to add a comment here about why we have this check here.
f8c2f89
to
c33dc35
Compare
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.
looking good, just a note about how we are handling #configure
but otherwise, looks good
lib/ddtrace/tracer.rb
Outdated
max_spans_before_partial_flush = options.fetch(:max_spans_before_partial_flush, nil) | ||
min_spans_before_partial_flush = options.fetch(:min_spans_before_partial_flush, nil) | ||
partial_flush_timeout = options.fetch(:partial_flush_timeout, nil) | ||
|
||
@enabled = enabled unless enabled.nil? | ||
@sampler = sampler unless sampler.nil? | ||
|
||
if priority_sampling | ||
if priority_sampling && !@sampler.is_a?(PrioritySampler) |
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.
I see we updated options.fetch(:priority_sampling, true)
, does that mean we should have this be if !priority_sampling.nil?
(sorry for my bad ruby)
Since nil
is falsey, we probably only want to modify what sampler/writer we are using if we are setting priority sampling.
Also, do we need to handle :priority_smapling => false
? if we reconfigure and set to false
do we need to update the sampler/writer again to use a RateSampler
?
@@ -7,7 +7,7 @@ | |||
RSpec.describe Datadog::Contrib::ActiveSupport::Notifications::Subscription do | |||
describe 'instance' do | |||
subject(:subscription) { described_class.new(tracer, span_name, options, &block) } | |||
let(:tracer) { ::Datadog::Tracer.new(writer: FauxWriter.new) } | |||
let(:tracer) { get_test_tracer } |
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.
nice, 👍 for encapsulating this somewhere.
# TODO: Let's try to get rid of this override, which has too much | ||
# knowledge about the internal workings of the tracer. | ||
# It is done to prevent the activation of priority sampling | ||
# from wiping out the configured test writer, by replacing it. |
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.
👍 agree with this comment, this is great, especially if it helps us get by, but a little hacky/nasty.
c33dc35
to
df01e39
Compare
df01e39
to
956577f
Compare
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.
looks good
This pull request enables priority sampling by default.