From 76ea4940d019f03d32ec036f6f6a4e114bc79669 Mon Sep 17 00:00:00 2001 From: Yohei Kitamura Date: Fri, 1 Mar 2019 16:26:33 +0900 Subject: [PATCH] Fix 'sa' annotation encoding and remove the ':service_port' configuration (#139) * Update the ':service_port' configuration to be optional * Fix 'sa' annotation encoding * Not convert boolean values to string * Remove the ':service_port' configuration --- CHANGELOG.md | 4 +++ README.md | 2 -- lib/zipkin-tracer/config.rb | 5 +-- lib/zipkin-tracer/excon/zipkin-tracer.rb | 2 +- lib/zipkin-tracer/faraday/zipkin-tracer.rb | 2 +- lib/zipkin-tracer/trace.rb | 12 ++++--- lib/zipkin-tracer/tracer_factory.rb | 1 - lib/zipkin-tracer/version.rb | 2 +- spec/lib/config_spec.rb | 4 +-- spec/lib/middleware_shared_examples.rb | 2 +- spec/lib/sidekiq/middleware_spec.rb | 1 - spec/lib/trace_spec.rb | 37 +++++++++++++++++----- spec/lib/tracer_factory_spec.rb | 6 ++-- spec/support/test_app_config.ru | 1 - 14 files changed, 50 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e2aead..d7c3c03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 0.32.4 +* Remove the ':service_port' configuration. +* Fix 'sa' annotation encoding. + # 0.32.3 * Fix bug using trace generator. diff --git a/README.md b/README.md index 812b900..3031e8d 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,6 @@ use ZipkinTracer::RackHandler, config # config is optional where `Rails.config.zipkin_tracer` or `config` is a hash that can contain the following keys: * `:service_name` **REQUIRED** - the name of the service being traced. There are two ways to configure this value. Either write the service name in the config file or set the "DOMAIN" environment variable (e.g. 'test-service.example.com' or 'test-service'). The environment variable takes precedence over the config file value. -* `:service_port` **REQUIRED** - the port of the service being traced (e.g. 80 or 443) * `:sample_rate` (default: 0.1) - the ratio of requests to sample, from 0 to 1 * `:json_api_host` - hostname with protocol of a zipkin api instance (e.g. `https://zipkin.example.com`) to use the JSON tracer * `:zookeeper` - the address of the zookeeper server to use by the Kafka tracer @@ -64,7 +63,6 @@ Sidekiq tracing can be turned on by adding ZipkinTracer::Sidekiq::Middleware to ```ruby zipkin_tracer_config = { service_name: 'service', - service_port: 3000, json_api_host: 'http://zipkin.io', traceable_workers: [:MyWorker, :MyWorker2], sample_rate: 0.5 diff --git a/lib/zipkin-tracer/config.rb b/lib/zipkin-tracer/config.rb index fbaaae6..9c0b918 100644 --- a/lib/zipkin-tracer/config.rb +++ b/lib/zipkin-tracer/config.rb @@ -5,7 +5,7 @@ module ZipkinTracer # Configuration of this gem. It reads the configuration and provides default values class Config - attr_reader :service_name, :service_port, :json_api_host, + 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, @@ -15,8 +15,6 @@ def initialize(app, config_hash) config = config_hash || Application.config(app) # The name of the current service @service_name = config[:service_name] - # The port where the current service is running - @service_port = config[:service_port] || DEFAULTS[:service_port] # The address of the Zipkin server which we will send traces to @json_api_host = config[:json_api_host] # Zookeeper information @@ -74,7 +72,6 @@ def adapter DEFAULTS = { sample_rate: 0.1, - service_port: 80, sampled_as_boolean: true, trace_id_128bit: false } diff --git a/lib/zipkin-tracer/excon/zipkin-tracer.rb b/lib/zipkin-tracer/excon/zipkin-tracer.rb index 9757a61..57c8499 100644 --- a/lib/zipkin-tracer/excon/zipkin-tracer.rb +++ b/lib/zipkin-tracer/excon/zipkin-tracer.rb @@ -40,7 +40,7 @@ def response_call(datum) private - SERVER_ADDRESS_SPECIAL_VALUE = '1'.freeze + SERVER_ADDRESS_SPECIAL_VALUE = true STATUS_ERROR_REGEXP = /\A(4.*|5.*)\z/.freeze def b3_headers diff --git a/lib/zipkin-tracer/faraday/zipkin-tracer.rb b/lib/zipkin-tracer/faraday/zipkin-tracer.rb index 784f518..e1a4b46 100644 --- a/lib/zipkin-tracer/faraday/zipkin-tracer.rb +++ b/lib/zipkin-tracer/faraday/zipkin-tracer.rb @@ -25,7 +25,7 @@ def call(env) private - SERVER_ADDRESS_SPECIAL_VALUE = '1'.freeze + SERVER_ADDRESS_SPECIAL_VALUE = true STATUS_ERROR_REGEXP = /\A(4.*|5.*)\z/.freeze diff --git a/lib/zipkin-tracer/trace.rb b/lib/zipkin-tracer/trace.rb index 91526c0..5d54e34 100644 --- a/lib/zipkin-tracer/trace.rb +++ b/lib/zipkin-tracer/trace.rb @@ -235,7 +235,8 @@ def record(value, endpoint = Trace.default_endpoint) end def record_tag(key, value, type = Trace::BinaryAnnotation::Type::STRING, endpoint = Trace.default_endpoint) - binary_annotations << Trace::BinaryAnnotation.new(key, value.to_s, type, endpoint) + value = value.to_s if type == Trace::BinaryAnnotation::Type::STRING + binary_annotations << Trace::BinaryAnnotation.new(key, value, type, endpoint) end def record_local_component(value) @@ -274,9 +275,9 @@ def self.host_to_i32(host) signed_i32 end - def self.local_endpoint(service_port, service_name, ip_format) + def self.local_endpoint(service_name, ip_format) hostname = Socket.gethostname - Endpoint.new(hostname, service_port, service_name, ip_format) + Endpoint.new(hostname, nil, service_name, ip_format) end def self.remote_endpoint(url, remote_service_name, ip_format) @@ -285,11 +286,12 @@ def self.remote_endpoint(url, remote_service_name, ip_format) end def to_h - { + hsh = { ipv4: ipv4, - port: port, serviceName: service_name } + hsh[:port] = port if port + hsh end end diff --git a/lib/zipkin-tracer/tracer_factory.rb b/lib/zipkin-tracer/tracer_factory.rb index f3357c5..8fd35b9 100644 --- a/lib/zipkin-tracer/tracer_factory.rb +++ b/lib/zipkin-tracer/tracer_factory.rb @@ -28,7 +28,6 @@ def tracer(config) # TODO: move this to the TracerBase and kill scribe tracer ip_format = [:kafka, :kafka_producer].include?(config.adapter) ? :i32 : :string Trace.default_endpoint = Trace::Endpoint.local_endpoint( - config.service_port, service_name(config.service_name), ip_format ) diff --git a/lib/zipkin-tracer/version.rb b/lib/zipkin-tracer/version.rb index f3fd7da..83df7be 100644 --- a/lib/zipkin-tracer/version.rb +++ b/lib/zipkin-tracer/version.rb @@ -1,3 +1,3 @@ module ZipkinTracer - VERSION = '0.32.3'.freeze + VERSION = '0.32.4'.freeze end diff --git a/spec/lib/config_spec.rb b/spec/lib/config_spec.rb index 7f1f00e..916bca5 100644 --- a/spec/lib/config_spec.rb +++ b/spec/lib/config_spec.rb @@ -5,7 +5,7 @@ module ZipkinTracer before do allow(Application).to receive(:logger).and_return(Logger.new(nil)) end - [:service_name, :service_port, :json_api_host, + [:service_name, :json_api_host, :zookeeper, :log_tracing, :annotate_plugin, :filter_plugin, :whitelist_plugin].each do |method| it "can set and read configuration values for #{method}" do @@ -21,7 +21,7 @@ module ZipkinTracer it 'sets defaults' do config = Config.new(nil, {}) - [:sample_rate, :service_port, :sampled_as_boolean, :trace_id_128bit].each do |key| + [:sample_rate, :sampled_as_boolean, :trace_id_128bit].each do |key| expect(config.send(key)).to_not eq(nil) end end diff --git a/spec/lib/middleware_shared_examples.rb b/spec/lib/middleware_shared_examples.rb index 0de0724..ac7bc4f 100644 --- a/spec/lib/middleware_shared_examples.rb +++ b/spec/lib/middleware_shared_examples.rb @@ -67,7 +67,7 @@ def expect_tracing expect_any_instance_of(Trace::Span).to receive(:record_tag) do |_, key, value, type, host| expect(key).to eq('sa') - expect(value).to eq('1') + expect(value).to eq(true) expect(type).to eq('BOOL') expect_host(host, hostname, service_name) end diff --git a/spec/lib/sidekiq/middleware_spec.rb b/spec/lib/sidekiq/middleware_spec.rb index f7d260b..2a15e43 100644 --- a/spec/lib/sidekiq/middleware_spec.rb +++ b/spec/lib/sidekiq/middleware_spec.rb @@ -12,7 +12,6 @@ let(:config) do { service_name: 'test_service', - service_port: 3000, traceable_workers: [ :String ] } end diff --git a/spec/lib/trace_spec.rb b/spec/lib/trace_spec.rb index 9d649bc..35aba40 100644 --- a/spec/lib/trace_spec.rb +++ b/spec/lib/trace_spec.rb @@ -133,6 +133,7 @@ let(:key) { 'key' } let(:value) { 'value' } let(:numeric_value) { 123 } + let(:boolean_value) { true } before do Timecop.freeze(Time.utc(2016, 1, 16, 23, 45)) @@ -190,6 +191,13 @@ ann = span_with_parent.binary_annotations[-1] expect(ann.value).to eq('123') end + + it 'does not convert the boolean value to string' do + span_with_parent.record_tag(key, boolean_value, Trace::BinaryAnnotation::Type::BOOL) + + ann = span_with_parent.binary_annotations[-1] + expect(ann.value).to eq(true) + end end describe '#record_local_component' do @@ -241,8 +249,8 @@ describe '.local_endpoint' do it 'auto detects the hostname' do allow(Socket).to receive(:gethostname).and_return('z1.example.com') - expect(Trace::Endpoint).to receive(:new).with('z1.example.com', 80, service_name, :string) - Trace::Endpoint.local_endpoint(80, service_name, :string) + expect(Trace::Endpoint).to receive(:new).with('z1.example.com', nil, service_name, :string) + Trace::Endpoint.local_endpoint(service_name, :string) end end @@ -266,12 +274,25 @@ end describe '#to_h' do - it 'returns a hash representation of an endpoint' do - expect(dummy_endpoint.to_h).to eq( - ipv4: '127.0.0.1', - port: 9411, - serviceName: 'DummyService' - ) + context 'with service_port' do + it 'returns a hash representation of an endpoint' do + expect(dummy_endpoint.to_h).to eq( + ipv4: '127.0.0.1', + port: 9411, + serviceName: 'DummyService' + ) + end + end + + context 'without service_port' do + let(:dummy_endpoint) { Trace::Endpoint.new('127.0.0.1', nil, 'DummyService') } + + it 'returns a hash representation of an endpoint witout "port"' do + expect(dummy_endpoint.to_h).to eq( + ipv4: '127.0.0.1', + serviceName: 'DummyService' + ) + end end end end diff --git a/spec/lib/tracer_factory_spec.rb b/spec/lib/tracer_factory_spec.rb index 7ad2eff..5a06e10 100644 --- a/spec/lib/tracer_factory_spec.rb +++ b/spec/lib/tracer_factory_spec.rb @@ -87,7 +87,7 @@ def configuration(options) end it 'sets the trace endpoint service name to the default configuration file value' do - expect(Trace::Endpoint).to receive(:local_endpoint).with(80, 'zipkin-tester', :string) { 'endpoint' } + expect(Trace::Endpoint).to receive(:local_endpoint).with('zipkin-tester', :string) { 'endpoint' } expect(Trace).to receive(:default_endpoint=).with('endpoint') described_class.new.tracer(config) end @@ -95,7 +95,7 @@ def configuration(options) context 'json adapter' do let(:config) { configuration(service_name: 'zipkin-tester', json_api_host: 'host') } it 'calls with string ip format' do - expect(Trace::Endpoint).to receive(:local_endpoint).with(80, 'zipkin-tester', :string) { 'endpoint' } + expect(Trace::Endpoint).to receive(:local_endpoint).with('zipkin-tester', :string) { 'endpoint' } expect(Trace).to receive(:default_endpoint=).with('endpoint') described_class.new.tracer(config) end @@ -109,7 +109,7 @@ def configuration(options) end it 'sets the trace endpoint service name to the environment variable value' do - expect(Trace::Endpoint).to receive(:local_endpoint).with(anything, 'zipkin-env-var-tester', :string) { 'endpoint' } + expect(Trace::Endpoint).to receive(:local_endpoint).with('zipkin-env-var-tester', :string) { 'endpoint' } expect(Trace).to receive(:default_endpoint=).with('endpoint') described_class.new.tracer(config) end diff --git a/spec/support/test_app_config.ru b/spec/support/test_app_config.ru index e2ea527..f162971 100644 --- a/spec/support/test_app_config.ru +++ b/spec/support/test_app_config.ru @@ -4,7 +4,6 @@ require File.join(`pwd`.chomp, 'spec', 'support', 'test_app') zipkin_tracer_config = { service_name: 'your service name here', - service_port: 9410, sample_rate: 1, json_api_host: '127.0.0.1:9410' }