Whitelist would have precedence to routing (openzipkin#143)

* Whitelist would have precedence to routing

* Better comments of the code

* Add more specs around sampling code

* Update ruby versions on the CI

.travis.yml

@@ -2,11 +2,12 @@ language: ruby
 jdk:
   - oraclejdk8
 rvm:
-  - 2.6.0
-  - 2.5.3
+  - 2.6.2
+  - 2.5.5
   - 2.4.5
   - 2.3.8
-  - jruby-9.1.6.0
+  - jruby-9.1.17.0
+  - jruby-9.2.6.0
 deploy:
   provider: rubygems
   api_key:
@@ -15,7 +16,7 @@ deploy:
   on:
     tags: true
     repo: openzipkin/zipkin-ruby
-    condition: "$TRAVIS_RUBY_VERSION == 2.6.0"
+    condition: "$TRAVIS_RUBY_VERSION == 2.6.2"
 notifications:
   webhooks:
     urls:

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 0.34.0
+* Whitelist plugin ensure a request is traced even if it is not routable
+* Gem requires Ruby > 2.3.0. In practice this was true already.
+
 # 0.33.0
 * Switch to Zipkin v2 span format
 

lib/zipkin-tracer/application.rb

@@ -1,17 +1,18 @@
 module ZipkinTracer
-
   # Useful methods on the Application we are instrumenting
   class Application
-    # If the request is not valid for this service, we do not what to trace it.
-    def self.routable_request?(path_info, http_method)
+    # Determines if our framework knows whether the request will be routed to a controller
+    def self.routable_request?(env)
       return true unless defined?(Rails) # If not running on a Rails app, we can't verify if it is invalid
+      path_info = env[ZipkinTracer::RackHandler::PATH_INFO]
+      http_method = env[ZipkinTracer::RackHandler::REQUEST_METHOD]
       Rails.application.routes.recognize_path(path_info, method: http_method)
       true
     rescue ActionController::RoutingError
       false
     end
 
-    def self.get_route(env)
+    def self.route(env)
       return nil unless defined?(Rails)
       stub_env = {
         "PATH_INFO" => env[ZipkinTracer::RackHandler::PATH_INFO],
@@ -27,7 +28,7 @@ def self.get_route(env)
     end
 
     def self.logger
-      if defined?(Rails.logger) # If we happen to be inside a Rails app, use its logger
+      if defined?(Rails.logger)
         Rails.logger
       else
         Logger.new(STDOUT)

lib/zipkin-tracer/config.rb

@@ -26,8 +26,6 @@ def initialize(app, config_hash)
       @sample_rate       = config[:sample_rate]       || DEFAULTS[:sample_rate]
       # A block of code which can be called to do extra annotations of traces
       @annotate_plugin   = config[:annotate_plugin]   # call for trace annotation
-      @filter_plugin     = config[:filter_plugin]     # skip tracing if returns false
-      @whitelist_plugin  = config[:whitelist_plugin]  # force sampling if returns true
       # A block of code which can be called to skip traces. Skip tracing if returns false
       @filter_plugin     = config[:filter_plugin]
       # A block of code which can be called to force sampling. Forces sampling if returns true

lib/zipkin-tracer/excon/zipkin-tracer.rb

@@ -37,7 +37,6 @@ def response_call(datum)
 
     private
 
-    SERVER_ADDRESS_SPECIAL_VALUE = true
     STATUS_ERROR_REGEXP = /\A(4.*|5.*)\z/.freeze
 
     def b3_headers

lib/zipkin-tracer/faraday/zipkin-tracer.rb

@@ -25,10 +25,8 @@ def call(env)
 
     private
 
-    SERVER_ADDRESS_SPECIAL_VALUE = true
     STATUS_ERROR_REGEXP = /\A(4.*|5.*)\z/.freeze
 
-
     def b3_headers
       {
         trace_id: 'X-B3-TraceId',

lib/zipkin-tracer/rack/zipkin-tracer.rb

@@ -25,7 +25,7 @@ def call(env)
       zipkin_env = ZipkinEnv.new(env, @config)
       trace_id = zipkin_env.trace_id
       TraceContainer.with_trace_id(trace_id) do
-        if !trace_id.sampled? || !routable_request?(env)
+        if !trace_id.sampled?
           @app.call(env)
         else
           @tracer.with_new_span(trace_id, span_name(env)) do |span|
@@ -37,16 +37,8 @@ def call(env)
 
     private
 
-    def routable_request?(env)
-      Application.routable_request?(env[PATH_INFO], env[REQUEST_METHOD])
-    end
-
-    def route(env)
-      Application.get_route(env)
-    end
-
     def span_name(env)
-      "#{env[REQUEST_METHOD].to_s.downcase} #{route(env)}".strip
+      "#{env[REQUEST_METHOD].to_s.downcase} #{Application.route(env)}".strip
     end
 
     def annotate_plugin(span, env, status, response_headers, response_body)
@@ -56,7 +48,6 @@ def annotate_plugin(span, env, status, response_headers, response_body)
     def trace!(span, zipkin_env, &block)
       trace_request_information(span, zipkin_env)
       span.kind = Trace::Span::Kind::SERVER
-      span.record('whitelisted') if zipkin_env.force_sample?
       status, headers, body = yield
     ensure
       annotate_plugin(span, zipkin_env.env, status, headers, body)

lib/zipkin-tracer/rack/zipkin_env.rb

@@ -20,10 +20,6 @@ def called_with_zipkin_headers?
       @called_with_zipkin_headers ||= B3_REQUIRED_HEADERS.all? { |key| @env.key?(key) }
     end
 
-    def force_sample?
-      @force_sample ||= @config.whitelist_plugin && @config.whitelist_plugin.call(@env)
-    end
-
     private
 
     B3_REQUIRED_HEADERS = %w(HTTP_X_B3_TRACEID HTTP_X_B3_SPANID).freeze
@@ -61,15 +57,24 @@ def current_trace_sampled?
     end
 
     def sampled_header_value(parent_trace_sampled)
-      if parent_trace_sampled  # A service upstream decided this goes in all the way
+      if parent_trace_sampled # A service upstream decided this goes in all the way
         parent_trace_sampled
       else
-        new_sampled_header_value(force_sample? || current_trace_sampled? && !filtered?)
+        new_sampled_header_value(force_sample? || current_trace_sampled? && !filtered? && routable_request?)
       end
     end
 
+    def force_sample?
+      @config.whitelist_plugin && @config.whitelist_plugin.call(@env)
+    end
+
     def filtered?
       @config.filter_plugin && !@config.filter_plugin.call(@env)
     end
+
+    def routable_request?
+      Application.routable_request?(@env)
+    end
+
   end
 end

lib/zipkin-tracer/version.rb

@@ -1,3 +1,3 @@
 module ZipkinTracer
-  VERSION = '0.33.0'.freeze
+  VERSION = '0.34.0'.freeze
 end

spec/integration/integration_spec.rb

@@ -57,7 +57,7 @@ def assert_level_0_trace_correct(traces)
     expect(traces[0]['trace_id']).not_to be_empty
     expect(traces[0]['parent_span_id']).to be_empty
     expect(traces[0]['span_id']).not_to be_empty
-    expect(['true', 'false'].include?(traces[0]['sampled'])).to be_truthy
+    expect(['true'].include?(traces[0]['sampled'])).to be_truthy
   end
 
   # Assert that the second level of trace data is correct (or not!).
@@ -68,6 +68,6 @@ def assert_level_1_trace_correct(traces)
     expect(traces[1]['parent_span_id']).to eq(traces[0]['span_id'])
     expect(traces[1]['span_id']).not_to be_empty
     expect([traces[1]['trace_id'], traces[1]['parent_span_id']].include?(traces[1]['span_id'])).to be_falsey
-    expect(['true', 'false'].include?(traces[1]['sampled'])).to be_truthy
+    expect(['true'].include?(traces[1]['sampled'])).to be_truthy
   end
 end

spec/lib/application_spec.rb

@@ -1,8 +1,9 @@
 require 'spec_helper'
+
 module ZipkinTracer
   RSpec.describe Application do
     describe '.routable_request?' do
-      subject { Application.routable_request?("path", "METHOD") }
+      subject { Application.routable_request?({}) }
 
       context 'Rails available' do
         before do
@@ -12,6 +13,7 @@ module ZipkinTracer
         context 'route is found' do
           before do
             allow(Rails).to receive_message_chain('application.routes.recognize_path') { nil }
+            stub_const('ActionController::RoutingError', StandardError)
           end
 
           it 'is true' do
@@ -38,9 +40,9 @@ module ZipkinTracer
       end
     end
 
-    describe '.get_route' do
+    describe '.route' do
       let(:env) { { "PATH_INFO" => "path", "REQUEST_METHOD" => "METHOD" } }
-      subject { Application.get_route(env) }
+      subject { Application.route(env) }
 
       context 'Rails available' do
         before do

spec/lib/rack/zipkin-tracer_spec.rb

@@ -90,7 +90,7 @@ def expect_host(host)
     context 'accessing a valid URL "/thing/123" of our service' do
       before do
         allow(ZipkinTracer::Application).to receive(:routable_request?).and_return(true)
-        allow(ZipkinTracer::Application).to receive(:get_route).and_return("/thing/:id")
+        allow(ZipkinTracer::Application).to receive(:route).and_return("/thing/:id")
       end
 
       it 'traces the request' do
@@ -218,7 +218,6 @@ def expect_host(host)
       it 'samples the request' do
         expect_any_instance_of(Trace::Span).to receive(:kind=).with(Trace::Span::Kind::SERVER)
         expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.path', '/')
-        expect_any_instance_of(Trace::Span).to receive(:record).with('whitelisted')
         status, _, _ = subject.call(mock_env)
         expect(status).to eq(200)
       end

spec/lib/rack/zipkin_env_spec.rb

@@ -35,8 +35,8 @@ def mock_env(params = {}, path = '/')
       )
     end
 
-    it 'sampling is not forced' do
-      expect(zipkin_env.force_sample?).to eq(false)
+    it 'trace is not sampled' do
+      expect(zipkin_env.trace_id.sampled?).to eq(false)
     end
   end
 
@@ -51,8 +51,24 @@ def mock_env(params = {}, path = '/')
       )
     end
 
-    it 'sampling is forced' do
-      expect(zipkin_env.force_sample?).to eq(true)
+    it 'trace is sampled' do
+      expect(zipkin_env.trace_id.sampled?).to eq(true)
+    end
+  end
+
+  context 'Not sampling in this node and not tracing information in the environment' do
+    let(:config) do
+      instance_double(
+        ZipkinTracer::Config,
+        filter_plugin: proc { false },
+        whitelist_plugin: proc { false },
+        sample_rate: 0,
+        sampled_as_boolean: sampled_as_boolean
+      )
+    end
+
+    it 'trace is not sampled' do
+      expect(zipkin_env.trace_id.sampled?).to eq(false)
     end
   end
 
@@ -79,7 +95,7 @@ def mock_env(params = {}, path = '/')
     end
 
     it 'sampling information is set' do
-      # In this spec we are forcing sampling with the whitelist_plugin
+      # Because sample rate == 1
       expect(zipkin_env.trace_id.sampled?).to eq(true)
     end
 
@@ -111,6 +127,11 @@ def mock_env(params = {}, path = '/')
       expect(zipkin_env.trace_id.shared).to eq(true)
     end
 
+    it 'sampling information is set' do
+      # Because sample rate == 1
+      expect(zipkin_env.trace_id.sampled?).to eq(true)
+    end
+
     context 'parent_id is not provided' do
       it 'uses the trace_id and span_id' do
         trace_id = zipkin_env.trace_id
@@ -165,5 +186,31 @@ def mock_env(params = {}, path = '/')
         expect(zipkin_env.trace_id.flags.to_i).to eq(0)
       end
     end
+
+    context 'Sampled admits using 1 as well as true' do
+      let(:parent_id) { rand(131) }
+      let(:zipkin_headers) do
+        { 'HTTP_X_B3_TRACEID' => id, 'HTTP_X_B3_SPANID' => id, 'HTTP_X_B3_PARENTSPANID' => parent_id,
+          'HTTP_X_B3_SAMPLED' => '1', 'HTTP_X_B3_FLAGS' => 0 }
+      end
+
+      it 'uses the trace_id and span_id' do
+        trace_id = zipkin_env.trace_id
+        expect(trace_id.trace_id.to_i).to eq(id)
+        expect(trace_id.span_id.to_i).to eq(id)
+      end
+
+      it 'uses the parent_id' do
+        expect(zipkin_env.trace_id.parent_id.to_i).to eq(parent_id)
+      end
+
+      it 'uses the sampling information' do
+        expect(zipkin_env.trace_id.sampled?).to eq(true)
+      end
+
+      it 'uses the flags' do
+        expect(zipkin_env.trace_id.flags.to_i).to eq(0)
+      end
+    end
   end
 end

zipkin-tracer.gemspec

@@ -15,7 +15,7 @@ Gem::Specification.new do |s|
   s.license                   = 'Apache-2.0'
 
   s.required_rubygems_version = '>= 1.3.5'
-  s.required_ruby_version     = '>= 2.0.0'
+  s.required_ruby_version     = '>= 2.3.0'
 
   s.files                     = Dir.glob('{bin,lib}/**/*')
   s.require_path              = 'lib'