Fix 'sa' annotation encoding and remove the ':service_port' configur…

…ation (#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

@@ -1,3 +1,7 @@
+# 0.32.4
+* Remove the ':service_port' configuration.
+* Fix 'sa' annotation encoding.
+
 # 0.32.3
 * Fix bug using trace generator.
 

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

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
     }

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

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
 
 

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

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
       )

lib/zipkin-tracer/version.rb

@@ -1,3 +1,3 @@
 module ZipkinTracer
-  VERSION = '0.32.3'.freeze
+  VERSION = '0.32.4'.freeze
 end

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

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

spec/lib/sidekiq/middleware_spec.rb

@@ -12,7 +12,6 @@
       let(:config) do
         {
           service_name: 'test_service',
-          service_port: 3000,
           traceable_workers: [ :String ]
         }
       end

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

spec/lib/tracer_factory_spec.rb

@@ -87,15 +87,15 @@ 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
 
       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

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'
 }