Record tags inconditionally at server receive

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 0.35.0
+* removes record_on_server_receive option
+* records status_code and method on all server receive events.
+
 # 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.

README.md

@@ -26,7 +26,6 @@ where `Rails.config.zipkin_tracer` or `config` is a hash that can contain the fo
 * `:filter_plugin` - plugin function which receives the Rack env and will skip tracing if it returns false
 * `:whitelist_plugin` - plugin function which receives the Rack env and will force sampling if it returns true
 * `:sampled_as_boolean` - When set to true (default but deprecrated), it uses true/false for the `X-B3-Sampled` header. When set to false uses 1/0 which is preferred.
-* `:record_on_server_receive` - a CSV style list of tags to record on server receive, even if the zipkin headers were present in the incoming request. Currently only supports the value `http.path`, others being discarded.
 * `:trace_id_128bit` - When set to true, high 8-bytes will be prepended to trace_id. The upper 4-bytes are epoch seconds and the lower 4-bytes are random. This makes it convertible to Amazon X-Ray trace ID format v1. (See http://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-request-tracing.html)
 
 ### Sending traces on outgoing requests with Faraday

lib/zipkin-tracer/config.rb

@@ -39,8 +39,6 @@ def initialize(app, config_hash)
       if @sampled_as_boolean
         @logger && @logger.warn("Using a boolean in the Sampled header is deprecated. Consider setting sampled_as_boolean to false")
       end
-      # Record the given tags on server receive, even if the zipkin headers were present in the incoming request?
-      @record_on_server_receive = parse_tags(config[:record_on_server_receive])
 
       # When set to true, high 8-bytes will be prepended to trace_id.
       # The upper 4-bytes are epoch seconds and the lower 4-bytes are random.
@@ -74,14 +72,6 @@ def adapter
       trace_id_128bit: false
     }
 
-    def parse_tags(tag_names)
-      return {} unless present?(tag_names)
-      names = tag_names.split(",").map(&:strip)
-      (ZipkinTracer::RackHandler::DEFAULT_SERVER_RECV_TAGS.keys & names).each_with_object({}) do |name, tags|
-        tags[name] = ZipkinTracer::RackHandler::DEFAULT_SERVER_RECV_TAGS[name]
-      end
-    end
-
     def present?(str)
       return false if str.nil?
       !!(/\A[[:space:]]*\z/ !~ str)

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

@@ -11,10 +11,6 @@ class RackHandler
     PATH_INFO = Rack::PATH_INFO rescue 'PATH_INFO'.freeze
     REQUEST_METHOD = Rack::REQUEST_METHOD rescue 'REQUEST_METHOD'.freeze
 
-    DEFAULT_SERVER_RECV_TAGS = {
-     Trace::Span::Tag::PATH => PATH_INFO
-    }.freeze
-
     def initialize(app, config = nil)
       @app = app
       @config = Config.new(app, config).freeze
@@ -37,6 +33,11 @@ def call(env)
 
     private
 
+    SERVER_RECV_TAGS = {
+      Trace::Span::Tag::PATH => PATH_INFO,
+      Trace::Span::Tag::METHOD => REQUEST_METHOD
+    }.freeze
+
     def span_name(env)
       "#{env[REQUEST_METHOD].to_s.downcase} #{Application.route(env)}".strip
     end
@@ -46,23 +47,17 @@ def annotate_plugin(span, env, status, response_headers, response_body)
     end
 
     def trace!(span, zipkin_env, &block)
-      trace_request_information(span, zipkin_env)
-      span.kind = Trace::Span::Kind::SERVER
       status, headers, body = yield
     ensure
+      trace_server_information(span, zipkin_env, status)
+
       annotate_plugin(span, zipkin_env.env, status, headers, body)
     end
 
-    def trace_request_information(span, zipkin_env)
-      tags = if [email protected]_on_server_receive.empty?
-        # if the user specified tags to record on server receive, use these no matter what
-        @config.record_on_server_receive
-      elsif !zipkin_env.called_with_zipkin_headers?
-        # if the request comes from a non zipkin-enabled source record the default tags
-        DEFAULT_SERVER_RECV_TAGS
-      end
-      return if tags.nil?
-      tags.each { |annotation_key, env_key| span.record_tag(annotation_key, zipkin_env.env[env_key]) }
+    def trace_server_information(span, zipkin_env, status)
+      span.kind = Trace::Span::Kind::SERVER
+      span.record_tag(Trace::Span::Tag::STATUS, status)
+      SERVER_RECV_TAGS.each { |annotation_key, env_key| span.record_tag(annotation_key, zipkin_env.env[env_key]) }
     end
   end
 end

lib/zipkin-tracer/version.rb

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

spec/lib/rack/zipkin-tracer_spec.rb

@@ -20,6 +20,13 @@ def expect_host(host)
     expect(host.ipv4).to eq(host_ip)
   end
 
+  def expect_tags(path = '/')
+    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', path)
+    expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.status_code', 200)
+    expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.method', 'GET')
+  end
+
   let(:app_status) { 200 }
   let(:app_headers) { { 'Content-Type' => 'text/plain' } }
   let(:app_body) { path }
@@ -45,8 +52,7 @@ def expect_host(host)
     it 'traces the request' do
       expect(ZipkinTracer::TraceContainer).to receive(:with_trace_id).and_call_original
       expect(tracer).to receive(:with_new_span).ordered.with(anything, 'get').and_call_original
-      expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.path', '/')
-      expect_any_instance_of(Trace::Span).to receive(:kind=).with(Trace::Span::Kind::SERVER)
+      expect_tags
 
       status, headers, body = subject.call(mock_env)
       expect(status).to eq(app_status)
@@ -55,26 +61,6 @@ def expect_host(host)
     end
   end
 
-  context 'Zipkin headers are passed to the middleware' do
-    subject { middleware(app) }
-    let(:env) { mock_env(',', ZipkinTracer::ZipkinEnv::B3_REQUIRED_HEADERS.map {|a| Hash[a, 1] }.inject(:merge)) }
-
-    it 'does not set the RPC method' do
-      expect(::Trace).not_to receive(:set_rpc_name)
-      subject.call(env)
-    end
-
-    it 'does not set the path info' do
-      expect_any_instance_of(Trace::Span).not_to receive(:record_tag)
-      subject.call(env)
-    end
-
-    it 'force-sets the path info, excluding unknown keys' do
-      expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.path', '/,')
-      middleware(app, record_on_server_receive: 'whatever, http.path , unknown,keys ').call(env)
-    end
-  end
-
   context 'Using Rails' do
     subject { middleware(app) }
 
@@ -96,8 +82,7 @@ def expect_host(host)
       it 'traces the request' do
         expect(ZipkinTracer::TraceContainer).to receive(:with_trace_id).and_call_original
         expect(tracer).to receive(:with_new_span).ordered.with(anything, 'get /thing/:id').and_call_original
-        expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.path', '/thing/123')
-        expect_any_instance_of(Trace::Span).to receive(:kind=).with(Trace::Span::Kind::SERVER)
+        expect_tags('/thing/123')
 
         status, headers, body = subject.call(mock_env_route)
         expect(status).to eq(app_status)
@@ -183,9 +168,10 @@ def expect_host(host)
     it 'traces a request with additional annotations' do
       expect(ZipkinTracer::TraceContainer).to receive(:with_trace_id).and_call_original
       expect(tracer).to receive(:with_new_span).and_call_original.ordered
+      expect_tags
+      expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.status_code', 200)
+      expect_any_instance_of(Trace::Span).to receive(:record_tag).with('foo', 'FOO')
 
-      expect_any_instance_of(Trace::Span).to receive(:record_tag).exactly(3).times
-      expect_any_instance_of(Trace::Span).to receive(:kind=).with(Trace::Span::Kind::SERVER)
       status, _, _ = subject.call(mock_env)
 
       # return expected status
@@ -216,8 +202,7 @@ def expect_host(host)
       subject { middleware(app, whitelist_plugin: lambda { |env| true }, sample_rate: 0) }
 
       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_tags
         status, _, _ = subject.call(mock_env)
         expect(status).to eq(200)
       end