simplify conditionals for checks on request. add more testing for pre…

…cedence

lib/scout_apm/sampling.rb

@@ -19,29 +19,27 @@ def initialize(config)
     end
 
     def drop_request?(transaction)
-      # global sample check
-      if global_sample_rate
-        return true if sample?(global_sample_rate)
-      end
-
       # job or endpoint?
-      # check if ignored _then_ sampled
+      # check if request should be sampled first
+      # Individual sample rate always takes precedence over global sample rate
       if transaction.job?
         job_name = transaction.layer_finder.job.name
+        rate = job_sample_rate(job_name)
+        return sample?(rate) unless rate.nil?
         return true if ignore_job?(job_name)
-        if sample_job?(job_name)
-          return true if sample?(sample_jobs[job_name])
-        end
       elsif transaction.web?
         uri = transaction.annotations[:uri]
+        rate = web_sample_rate(uri)
+        return sample?(rate) unless rate.nil?
         return true if ignore_uri?(uri)
-        do_sample, rate = sample_uri?(uri)
-        if do_sample
-          return true if sample?(rate)
-        end
       end
 
-      false # not ignored
+      # global sample check
+      if @global_sample_rate
+        return sample?(@global_sample_rate)
+      end
+
+      false # don't drop the request
     end
 
     def individual_sample_to_hash(sampling_config)
@@ -63,22 +61,22 @@ def ignore_uri?(uri)
       false
     end
 
-    def sample_uri?(uri)
-      return false if @sample_endpoints.blank?
+    def web_sample_rate(uri)
+      return nil if @sample_endpoints.blank?
       @sample_endpoints.each do |prefix, rate|
-        return true, rate if uri.start_with?(prefix)
+        return rate if uri.start_with?(prefix)
       end
-      return false, nil
+      nil
     end
 
     def ignore_job?(job_name)
-      return false if ignore_jobs.blank?
-      ignore_jobs.include?(job_name)
+      return false if @ignore_jobs.blank?
+      @ignore_jobs.include?(job_name)
     end
 
-    def sample_job?(job_name)
-      return false if sample_jobs.blank?
-      sample_jobs.has_key?(job_name)
+    def job_sample_rate(job_name)
+      return nil if @sample_jobs.blank?
+      @sample_jobs.fetch(job_name, nil)
     end
 
     def sample?(rate)

test/test_helper.rb

@@ -38,6 +38,10 @@ def value(key)
     @values[key]
   end
 
+  def values
+    @values
+  end
+
   def has_key?(key)
     @values.has_key?(key)
   end

test/unit/sampling_test.rb

@@ -7,13 +7,13 @@ class SamplingTest < Minitest::Test
     def setup
       @global_sample_config = FakeConfigOverlay.new(
         {
-          'sample_rate' => 50,
+          'sample_rate' => 80,
         }
       )
 
       @individual_config = FakeConfigOverlay.new(
         {
-          'sample_endpoints' => ['/foo:50', '/bar/zap:80'],
+          'sample_endpoints' => ['/foo/bar:100', '/foo:50', '/bar/zap:80'],
           'ignore_endpoints' => ['/baz'],
           'sample_jobs' => ['joba:50'],
           'ignore_jobs' => 'jobb,jobc',
@@ -23,7 +23,10 @@ def setup
 
     def test_individual_sample_to_hash
       sampling = ScoutApm::Sampling.new(@individual_config)
-      assert_equal({'/foo' => 50, '/bar/zap' => 80}, sampling.individual_sample_to_hash(@individual_config.value('sample_endpoints')))
+      assert_equal({'/foo/bar' => 100, '/foo' => 50, '/bar/zap' => 80}, sampling.individual_sample_to_hash(@individual_config.value('sample_endpoints')))
+
+      sampling = ScoutApm::Sampling.new(@global_sample_config)
+      assert_equal nil, sampling.individual_sample_to_hash(@global_sample_config.value('sample_endpoints'))
     end
 
     def test_uri_ignore
@@ -34,15 +37,17 @@ def test_uri_ignore
 
     def test_uri_sample
       sampling = ScoutApm::Sampling.new(@individual_config)
-      do_sample, rate = sampling.sample_uri?('/foo/far')
-      assert_equal true, do_sample
+      rate = sampling.web_sample_rate('/foo/far')
       assert_equal 50, rate
 
-      do_sample, rate = sampling.sample_uri?('/bar')
-      assert_equal false, do_sample
+      rate = sampling.web_sample_rate('/bar')
+      assert_equal nil, rate
+
+      rate = sampling.web_sample_rate('/baz/bap')
+      assert_equal nil, rate
 
-      do_sample, rate = sampling.sample_uri?('/baz/bap')
-      assert_equal false, do_sample
+      rate = sampling.web_sample_rate('/foo/bar/baz')
+      assert_equal 100, rate
     end
 
     def test_job_ignore
@@ -53,8 +58,8 @@ def test_job_ignore
 
     def test_job_sample
       sampling = ScoutApm::Sampling.new(@individual_config)
-      assert_equal true, sampling.sample_job?('joba')
-      assert_equal false, sampling.sample_job?('jobb')
+      assert_equal 50, sampling.job_sample_rate('joba')
+      assert_equal nil, sampling.job_sample_rate('jobb')
     end
 
     def test_sample
@@ -75,8 +80,9 @@ def test_old_ignore
       assert_equal false, sampling.ignore_uri?('/baz')
     end
 
-    def test_web_request
+    def test_web_request_individual_sampling
       sampling = ScoutApm::Sampling.new(@individual_config)
+
       # should be ignored
       transaction = FakeTrackedRequest.new_web_request('/baz/bap')
       assert_equal true, sampling.drop_request?(transaction)
@@ -85,19 +91,43 @@ def test_web_request
       transaction = FakeTrackedRequest.new_web_request('/faz/bap')
       assert_equal false, sampling.drop_request?(transaction)
 
-      # should be sampled if rand > 50
+
       transaction = FakeTrackedRequest.new_web_request('/foo/far')
       sampling.stub(:rand, 0.01) do
         assert_equal false, sampling.drop_request?(transaction)
       end
 
+      # passes individual sample but caught by global rate
       sampling.stub(:rand, 0.99) do
         assert_equal true, sampling.drop_request?(transaction)
       end
     end
 
-    def test_job_request
+    def test_web_request_with_global_sampling
+      config = FakeConfigOverlay.new(@individual_config.values.merge({'sample_rate' => 20}))
+      sampling = ScoutApm::Sampling.new(config)
+
+      # caught by individual rate
+      transaction = FakeTrackedRequest.new_web_request('/foo/far')
+      sampling.stub(:rand, 0.01) do
+        assert_equal false, sampling.drop_request?(transaction)
+      end
+
+      # passes individual rate (50) but caught by global rate (20)
+      sampling.stub(:rand, 0.30) do
+        assert_equal false, sampling.drop_request?(transaction)
+      end
+
+      # passes individual rate
+      sampling.stub(:rand, 0.99) do
+        assert_equal true, sampling.drop_request?(transaction)
+      end
+
+    end
+
+    def test_job_request_individual_sampling
       sampling = ScoutApm::Sampling.new(@individual_config)
+
       # should be ignored
       transaction = FakeTrackedRequest.new_job_request('jobb')
       assert_equal true, sampling.drop_request?(transaction)
@@ -116,4 +146,25 @@ def test_job_request
         assert_equal true, sampling.drop_request?(transaction)
       end
     end
+
+    def test_job_request_global_sampling
+      config = FakeConfigOverlay.new(@individual_config.values.merge({'sample_rate' => 20}))
+      sampling = ScoutApm::Sampling.new(config)
+
+      # caught by individual rate
+      transaction = FakeTrackedRequest.new_job_request('joba')
+      sampling.stub(:rand, 0.01) do
+        assert_equal false, sampling.drop_request?(transaction)
+      end
+
+      # passes individual rate (50) but caught by global rate (20)
+      sampling.stub(:rand, 0.30) do
+        assert_equal false, sampling.drop_request?(transaction)
+      end
+
+      # passes individual rate
+      sampling.stub(:rand, 0.99) do
+        assert_equal true, sampling.drop_request?(transaction)
+      end
+    end
 end