diff --git a/CHANGELOG.md b/CHANGELOG.md index eccf0e0..768b9b6 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/README.md b/README.md index 218cf3e..9265183 100644 --- a/README.md +++ b/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 diff --git a/lib/zipkin-tracer/config.rb b/lib/zipkin-tracer/config.rb index dee3f7b..2cbd9af 100644 --- a/lib/zipkin-tracer/config.rb +++ b/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) diff --git a/lib/zipkin-tracer/excon/zipkin-tracer.rb b/lib/zipkin-tracer/excon/zipkin-tracer.rb index 9413996..ae45f3d 100644 --- a/lib/zipkin-tracer/excon/zipkin-tracer.rb +++ b/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) diff --git a/lib/zipkin-tracer/faraday/zipkin-tracer.rb b/lib/zipkin-tracer/faraday/zipkin-tracer.rb index 279d238..2e10781 100644 --- a/lib/zipkin-tracer/faraday/zipkin-tracer.rb +++ b/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 diff --git a/lib/zipkin-tracer/rack/zipkin-tracer.rb b/lib/zipkin-tracer/rack/zipkin-tracer.rb index f9e6d3e..f49e8b8 100644 --- a/lib/zipkin-tracer/rack/zipkin-tracer.rb +++ b/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 diff --git a/lib/zipkin-tracer/trace.rb b/lib/zipkin-tracer/trace.rb index def4679..1dee016 100644 --- a/lib/zipkin-tracer/trace.rb +++ b/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 diff --git a/lib/zipkin-tracer/version.rb b/lib/zipkin-tracer/version.rb index 1164149..6145509 100644 --- a/lib/zipkin-tracer/version.rb +++ b/lib/zipkin-tracer/version.rb @@ -1,3 +1,3 @@ module ZipkinTracer - VERSION = '0.34.0'.freeze + VERSION = '0.35.0'.freeze end diff --git a/spec/integration/integration_spec.rb b/spec/integration/integration_spec.rb index f0428d2..f4317c4 100644 --- a/spec/integration/integration_spec.rb +++ b/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 diff --git a/spec/lib/excon/zipkin-tracer_spec.rb b/spec/lib/excon/zipkin-tracer_spec.rb index 2a768f3..a041f33 100644 --- a/spec/lib/excon/zipkin-tracer_spec.rb +++ b/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] diff --git a/spec/lib/rack/zipkin-tracer_spec.rb b/spec/lib/rack/zipkin-tracer_spec.rb index d92c6ca..bfc1674 100644 --- a/spec/lib/rack/zipkin-tracer_spec.rb +++ b/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 diff --git a/spec/lib/trace_spec.rb b/spec/lib/trace_spec.rb index df811cc..d8507e2 100644 --- a/spec/lib/trace_spec.rb +++ b/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 diff --git a/spec/support/test_app_config.ru b/spec/support/test_app_config.ru index f162971..a911647 100644 --- a/spec/support/test_app_config.ru +++ b/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