Record tags inconditionally at server receive (openzipkin#144)

* Record tags inconditionally at server receive

* update changelog and version

* All tracers set error tagg when error happens

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

@@ -8,7 +8,7 @@ class Config
     attr_reader :service_name, :json_api_host,
       :zookeeper, :sample_rate, :logger, :log_tracing,
       :annotate_plugin, :filter_plugin, :whitelist_plugin,
-      :sampled_as_boolean, :record_on_server_receive,
+      :sampled_as_boolean,
       :kafka_producer, :kafka_topic, :trace_id_128bit
 
     def initialize(app, config_hash)
@@ -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/excon/zipkin-tracer.rb

@@ -27,8 +27,7 @@ def request_call(datum)
 
     def response_call(datum)
       if span = datum[:span]
-        status = response_status(datum)
-        record_response_tags(span, status) if status
+        span.record_status(response_status(datum))
         Trace.tracer.end_span(span)
       end
 
@@ -37,8 +36,6 @@ def response_call(datum)
 
     private
 
-    STATUS_ERROR_REGEXP = /\A(4.*|5.*)\z/.freeze
-
     def b3_headers
       {
         trace_id: 'X-B3-TraceId',
@@ -61,11 +58,6 @@ def response_status(datum)
       datum[:response] && datum[:response][:status] && datum[:response][:status].to_s
     end
 
-    def record_response_tags(span, status)
-      span.record_tag(Trace::Span::Tag::STATUS, status)
-      span.record_tag(Trace::Span::Tag::ERROR, status) if STATUS_ERROR_REGEXP.match(status)
-    end
-
     def trace!(datum, trace_id)
       method = datum[:method].to_s
       url_string = Excon::Utils::request_uri(datum)

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

@@ -25,8 +25,6 @@ def call(env)
 
     private
 
-    STATUS_ERROR_REGEXP = /\A(4.*|5.*)\z/.freeze
-
     def b3_headers
       {
         trace_id: 'X-B3-TraceId',
@@ -51,7 +49,7 @@ def trace!(env, trace_id)
         span.record_tag(Trace::Span::Tag::METHOD, method.upcase)
         span.record_tag(Trace::Span::Tag::PATH, url.path)
         response = @app.call(env).on_complete do |renv|
-          record_response_tags(span, renv[:status].to_s)
+          span.record_status(renv[:status])
         end
       end
       response
@@ -70,10 +68,5 @@ def record_error(span, msg)
       span.record_tag(Trace::Span::Tag::ERROR, msg)
     end
 
-    def record_response_tags(span, status)
-      span.record_tag(Trace::Span::Tag::STATUS, status)
-      span.record_tag(Trace::Span::Tag::ERROR, status) if STATUS_ERROR_REGEXP.match(status)
-    end
-
   end
 end

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 !@config.record_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_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/trace.rb

@@ -221,7 +221,7 @@ def record(value)
     end
 
     def record_tag(key, value)
-      @tags[key] = value
+      @tags[key] = value.to_s
     end
 
     def record_local_component(value)
@@ -232,6 +232,15 @@ def has_parent_span?
       !@span_id.parent_id.nil?
     end
 
+    STATUS_ERROR_REGEXP = /\A(4.*|5.*)\z/.freeze
+
+    def record_status(status)
+      return if status.nil?
+      status = status.to_s
+      record_tag(Tag::STATUS, status)
+      record_tag(Tag::ERROR, status) if STATUS_ERROR_REGEXP.match(status)
+    end
+
     private
 
     UNKNOWN_DURATION = 0 # mark duration was not set

lib/zipkin-tracer/version.rb

@@ -1,3 +1,3 @@
 module ZipkinTracer
-  VERSION = '0.34.0'.freeze
+  VERSION = '0.35.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'].include?(traces[0]['sampled'])).to be_truthy
+    expect(['1'].include?(traces[0]['sampled'])).to eq(true)
   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'].include?(traces[1]['sampled'])).to be_truthy
+    expect(['1'].include?(traces[1]['sampled'])).to eq(true)
   end
 end

spec/lib/excon/zipkin-tracer_spec.rb

@@ -221,10 +221,9 @@ def process(body, url, headers = {})
         stub_request(:get, url)
           .to_return(status: 200, body: '', headers: {})
 
-        span = spy('Trace::Span')
-        allow(Trace::Span).to receive(:new).and_return(span)
-
-        expect(span).to receive(:record_tag).with(Trace::Span::Tag::STATUS, "200").once
+        expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.path', "/some/path/here")
+        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')
 
         ZipkinTracer::TraceContainer.with_trace_id(trace_id) do
           middlewares = [ZipkinTracer::ExconHandler] + Excon.defaults[:middlewares]

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

spec/lib/trace_spec.rb

@@ -139,7 +139,7 @@
     let(:duration) { 0 }
     let(:key) { 'key' }
     let(:value) { 'value' }
-    let(:numeric_value) { 123 }
+    let(:numeric_value) { '123' }
     let(:span_without_parent) do
       Trace::Span.new('get', Trace::TraceId.new(span_id, nil, span_id, true, Trace::Flags::EMPTY))
     end
@@ -239,7 +239,7 @@
         span_with_parent.record_tag(key, numeric_value)
 
         tags = span_with_parent.tags
-        expect(tags[key]).to eq(123)
+        expect(tags[key]).to eq('123')
       end
     end
 

spec/support/test_app_config.ru

@@ -5,7 +5,8 @@ require File.join(`pwd`.chomp, 'spec', 'support', 'test_app')
 zipkin_tracer_config = {
   service_name: 'your service name here',
   sample_rate: 1,
-  json_api_host: '127.0.0.1:9410'
+  json_api_host: '127.0.0.1:9410',
+  sampled_as_boolean: false
 }
 
 use ZipkinTracer::RackHandler, zipkin_tracer_config