Enable priority sampling by default #654

delner · 2018-12-14T21:15:04Z

This pull request enables priority sampling by default.

delner · 2018-12-14T21:17:19Z

lib/ddtrace/tracer.rb

      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)


Added this check because if you configured the tracer twice, it would wrap the previous PrioritySampler with another PrioritySampler, and that seems problematic...

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.

it might be good to add a comment here about why we have this check here.

Cool, I'll add that.

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?

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 a RateSampler?

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.

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?

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.

delner · 2018-12-14T21:18:05Z

test/context_flush_test.rb

@@ -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


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.

It might be good to add a comment here about that

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.

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()

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.

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

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.

test/sampler_test.rb

delner · 2018-12-14T21:54:40Z

Looks like simply changing the option from false to true breaks the test suite in all kinds of ways. This is going to require more work than expected.

brettlangdon · 2018-12-14T22:16:54Z

docs/GettingStarted.md

@@ -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:


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.

Good point.

brettlangdon · 2018-12-14T22:19:23Z

lib/ddtrace/tracer.rb

      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)


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.

brettlangdon · 2018-12-14T22:19:44Z

test/context_flush_test.rb

@@ -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


It might be good to add a comment here about that

brettlangdon · 2018-12-14T22:19:58Z

lib/ddtrace/tracer.rb

      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)


it might be good to add a comment here about why we have this check here.

test/sampler_test.rb

brettlangdon

looking good, just a note about how we are handling #configure but otherwise, looks good

brettlangdon · 2018-12-18T19:33:25Z

lib/ddtrace/tracer.rb

      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)


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?

brettlangdon · 2018-12-18T19:35:39Z

spec/ddtrace/contrib/active_support/notifications/subscription_spec.rb

@@ -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 }


nice, 👍 for encapsulating this somewhere.

brettlangdon · 2018-12-18T19:37:17Z

spec/support/tracer_helpers.rb

+      # 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.


👍 agree with this comment, this is great, especially if it helps us get by, but a little hacky/nasty.

test/helper.rb

brettlangdon

looks good

delner added the core Involves Datadog core libraries label Dec 14, 2018

delner added this to the 0.19.0 milestone Dec 14, 2018

delner self-assigned this Dec 14, 2018

delner requested review from pawelchcki and brettlangdon December 14, 2018 21:15

delner commented Dec 14, 2018

View reviewed changes

test/sampler_test.rb Show resolved Hide resolved

delner added the do-not-merge/WIP Not ready for merge label Dec 14, 2018

brettlangdon reviewed Dec 14, 2018

View reviewed changes

delner force-pushed the feature/priority_sampling_by_default branch from f8c2f89 to c33dc35 Compare December 17, 2018 17:35

delner removed the do-not-merge/WIP Not ready for merge label Dec 17, 2018

brettlangdon reviewed Dec 18, 2018

View reviewed changes

delner force-pushed the feature/priority_sampling_by_default branch from c33dc35 to df01e39 Compare December 18, 2018 19:40

delner added 3 commits December 18, 2018 15:38

Changed: Enable priority_sampling by default.

0cef479

Fixed: Priority sampling with minitests.

a8236ef

Fixed: Priority sampling with specs.

956577f

delner force-pushed the feature/priority_sampling_by_default branch from df01e39 to 956577f Compare December 18, 2018 20:38

brettlangdon approved these changes Dec 18, 2018

View reviewed changes

delner changed the base branch from 0.18-dev to 0.19-dev December 18, 2018 21:05

delner merged commit a57074f into 0.19-dev Dec 18, 2018

delner deleted the feature/priority_sampling_by_default branch December 18, 2018 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable priority sampling by default #654

Enable priority sampling by default #654

delner commented Dec 14, 2018

delner Dec 14, 2018

brettlangdon Dec 14, 2018

brettlangdon Dec 14, 2018

delner Dec 17, 2018

brettlangdon Dec 18, 2018

delner Dec 18, 2018 •

edited

Loading

brettlangdon Dec 18, 2018

delner Dec 18, 2018

delner Dec 14, 2018

brettlangdon Dec 14, 2018

delner Dec 17, 2018

brettlangdon Dec 17, 2018

delner Dec 17, 2018

brettlangdon Dec 17, 2018

delner Dec 17, 2018

delner commented Dec 14, 2018

brettlangdon Dec 14, 2018

delner Dec 17, 2018

brettlangdon Dec 14, 2018

brettlangdon Dec 14, 2018

brettlangdon Dec 14, 2018

brettlangdon left a comment

brettlangdon Dec 18, 2018

brettlangdon Dec 18, 2018

brettlangdon Dec 18, 2018

brettlangdon left a comment

Enable priority sampling by default #654

Enable priority sampling by default #654

Conversation

delner commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettlangdon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettlangdon left a comment

Choose a reason for hiding this comment

delner Dec 18, 2018 •

edited

Loading