From 9fe7ee2947555d94bb9c5f8cef0d6b774d791c6b Mon Sep 17 00:00:00 2001 From: Yohei Kitamura Date: Tue, 19 Mar 2019 16:26:00 +0900 Subject: [PATCH] Add the ':additional_paths' configuration option to enable tracing on non-Rails-routable paths --- .travis.yml | 6 +- CHANGELOG.md | 3 + README.md | 1 + lib/zipkin-tracer/config.rb | 7 ++- lib/zipkin-tracer/rack/zipkin-tracer.rb | 24 ++++---- lib/zipkin-tracer/version.rb | 2 +- spec/lib/rack/zipkin-tracer_spec.rb | 76 +++++++++++++++---------- 7 files changed, 70 insertions(+), 49 deletions(-) diff --git a/.travis.yml b/.travis.yml index 315e39a..e5a98f1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,8 +2,8 @@ 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 @@ -15,7 +15,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: diff --git a/CHANGELOG.md b/CHANGELOG.md index b84e878..84eb41d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,6 @@ +# 0.34.0 +* Add the ':additional_paths' configuration option to enable tracing on non-Rails-routable paths + # 0.33.0 * Switch to Zipkin v2 span format diff --git a/README.md b/README.md index 218cf3e..04eb719 100644 --- a/README.md +++ b/README.md @@ -28,6 +28,7 @@ where `Rails.config.zipkin_tracer` or `config` is a hash that can contain the fo * `: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) +* `:additional_paths` - A comma separated list of path prefixes (e.g. `/v1, /v2`) you want to trace in non-Rails applications. ### Sending traces on outgoing requests with Faraday diff --git a/lib/zipkin-tracer/config.rb b/lib/zipkin-tracer/config.rb index 9c0b918..ce3d9c7 100644 --- a/lib/zipkin-tracer/config.rb +++ b/lib/zipkin-tracer/config.rb @@ -9,7 +9,7 @@ class Config :zookeeper, :sample_rate, :logger, :log_tracing, :annotate_plugin, :filter_plugin, :whitelist_plugin, :sampled_as_boolean, :record_on_server_receive, - :kafka_producer, :kafka_topic, :trace_id_128bit + :kafka_producer, :kafka_topic, :trace_id_128bit, :additional_paths def initialize(app, config_hash) config = config_hash || Application.config(app) @@ -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 @@ -50,6 +48,9 @@ def initialize(app, config_hash) # (See http://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-request-tracing.html) @trace_id_128bit = config[:trace_id_128bit].nil? ? DEFAULTS[:trace_id_128bit] : config[:trace_id_128bit] + # A comma separated list of path prefixes you want to trace in non-Rails applications. + @additional_paths = present?(config[:additional_paths]) ? config[:additional_paths].split(",").map(&:strip) : nil + Trace.sample_rate = @sample_rate Trace.trace_id_128bit = @trace_id_128bit end diff --git a/lib/zipkin-tracer/rack/zipkin-tracer.rb b/lib/zipkin-tracer/rack/zipkin-tracer.rb index 3ac5d5b..8331c73 100644 --- a/lib/zipkin-tracer/rack/zipkin-tracer.rb +++ b/lib/zipkin-tracer/rack/zipkin-tracer.rb @@ -12,7 +12,7 @@ class RackHandler REQUEST_METHOD = Rack::REQUEST_METHOD rescue 'REQUEST_METHOD'.freeze DEFAULT_SERVER_RECV_TAGS = { - Trace::Span::Tag::PATH => PATH_INFO + Trace::Span::Tag::PATH => PATH_INFO }.freeze def initialize(app, config = nil) @@ -25,12 +25,13 @@ 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) - @app.call(env) - else - @tracer.with_new_span(trace_id, span_name(env)) do |span| - trace!(span, zipkin_env) { @app.call(env) } - end + return @app.call(env) unless trace_id.sampled? + + routable = routable_request?(env) + return @app.call(env) unless (routable || additional_path?(env)) + + @tracer.with_new_span(trace_id, span_name(env, routable)) do |span| + trace!(span, zipkin_env) { @app.call(env) } end end end @@ -41,12 +42,13 @@ def routable_request?(env) Application.routable_request?(env[PATH_INFO], env[REQUEST_METHOD]) end - def route(env) - Application.get_route(env) + def additional_path?(env) + @config.additional_paths && env[PATH_INFO].start_with?(*@config.additional_paths) end - def span_name(env) - "#{env[REQUEST_METHOD].to_s.downcase} #{route(env)}".strip + def span_name(env, routable) + route = Application.get_route(env) if routable + "#{env[REQUEST_METHOD].to_s.downcase} #{route}".strip end def annotate_plugin(span, env, status, response_headers, response_body) diff --git a/lib/zipkin-tracer/version.rb b/lib/zipkin-tracer/version.rb index ae6f8ca..1164149 100644 --- a/lib/zipkin-tracer/version.rb +++ b/lib/zipkin-tracer/version.rb @@ -1,3 +1,3 @@ module ZipkinTracer - VERSION = '0.33.0'.freeze + VERSION = '0.34.0'.freeze end diff --git a/spec/lib/rack/zipkin-tracer_spec.rb b/spec/lib/rack/zipkin-tracer_spec.rb index 4c483ce..5cef7b6 100644 --- a/spec/lib/rack/zipkin-tracer_spec.rb +++ b/spec/lib/rack/zipkin-tracer_spec.rb @@ -23,7 +23,8 @@ def expect_host(host) let(:app_status) { 200 } let(:app_headers) { { 'Content-Type' => 'text/plain' } } let(:app_body) { path } - let(:path) { '/'} + let(:path) { '/' } + let(:route) { nil } let(:tracer) { Trace.tracer } let(:app) { @@ -44,17 +45,30 @@ def expect_host(host) shared_examples_for 'traces the request' do 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(tracer).to receive(:with_new_span).ordered.with(anything, "get #{route}".strip).and_call_original + expect_any_instance_of(Trace::Span).to receive(:record_tag).with('http.path', path) expect_any_instance_of(Trace::Span).to receive(:kind=).with(Trace::Span::Kind::SERVER) - status, headers, body = subject.call(mock_env) + status, headers, body = subject.call(mock_env(path)) expect(status).to eq(app_status) expect(headers).to eq(app_headers) expect { |b| body.each &b }.to yield_with_args(app_body) end end + shared_examples_for 'does not trace the request' do + it 'calls the app and does not trace the request' do + expect(ZipkinTracer::TraceContainer).to receive(:with_trace_id).and_call_original + expect(tracer).not_to receive(:with_new_span) + expect_any_instance_of(Trace::Span).not_to receive(:kind=) + + status, _, body = subject.call(mock_env(path)) + # return expected status + expect(status).to eq(200) + expect { |b| body.each &b }.to yield_with_args(app_body) + 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)) } @@ -78,32 +92,20 @@ def expect_host(host) context 'Using Rails' do subject { middleware(app) } - context 'accessing a valid URL "/" 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(nil) - end + before do + allow(ZipkinTracer::Application).to receive(:routable_request?).and_return(true) + allow(ZipkinTracer::Application).to receive(:get_route).and_return(route) + end + context 'accessing a valid URL "/" of our service' do it_should_behave_like 'traces the request' end 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") - end - - 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) + let(:path) { "/thing/123" } + let(:route) { "/thing/:id" } - status, headers, body = subject.call(mock_env_route) - expect(status).to eq(app_status) - expect(headers).to eq(app_headers) - expect { |b| body.each &b }.to yield_with_args("/thing/123") - end + it_should_behave_like 'traces the request' end context 'accessing an invalid URL of our service' do @@ -111,14 +113,26 @@ def expect_host(host) allow(ZipkinTracer::Application).to receive(:routable_request?).and_return(false) end - it 'calls the app and does not trace the request' do - expect_any_instance_of(Trace::Span).not_to receive(:record) + it_should_behave_like 'does not trace the request' + end + end - status, _, body = subject.call(mock_env) - # return expected status - expect(status).to eq(200) - expect { |b| body.each &b }.to yield_with_args(app_body) - end + context 'non-Rails app' do + subject { middleware(app, additional_paths: '/some, /thing') } + before do + allow(ZipkinTracer::Application).to receive(:routable_request?).and_return(false) + end + + context 'accessing a valid URL "/thing/123" of our service' do + let(:path) { "/thing/123" } + + it_should_behave_like 'traces the request' + end + + context 'accessing an invalid URL of our service' do + let(:path) { '/unconfigured_path'} + + it_should_behave_like 'does not trace the request' end end