From ce9383573805eb3964ba8ba9d6cd1e6b8a93ef49 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 3 Oct 2016 18:09:41 +0200 Subject: [PATCH 1/5] [trace] add rubocop code analyzer with relaxed rules --- .rubocop.yml | 22 ++++++++++++++++++++++ Gemfile.lock | 14 ++++++++++++++ Rakefile | 5 +++++ ddtrace.gemspec | 1 + 4 files changed, 42 insertions(+) create mode 100644 .rubocop.yml diff --git a/.rubocop.yml b/.rubocop.yml new file mode 100644 index 00000000000..4eec168c3d9 --- /dev/null +++ b/.rubocop.yml @@ -0,0 +1,22 @@ +# 80 characters is a nice goal, but not worth currently changing in existing +# code for the sake of changing it to conform to a length set in 1928 (IBM). +Metrics/LineLength: + Max: 100 + +# These exceptions are good goals to attain, and probably will over time, +# so periodic disabling and re-running to inspect values is suggested. + +Metrics/AbcSize: + Max: 40 + +# TODO: As refactors continue, this should drop. However, the goal of +# 10 lines in a method may be a little lofty. +Metrics/MethodLength: + Max: 36 + +# TODO: this is not compliant with the Ruby community style guide. We +# should enable again this rule but it will change the public API because +# we're using set_ methods. We should work on that because also Rails +# honors this rule. +Style/AccessorMethodName: + Enabled: false diff --git a/Gemfile.lock b/Gemfile.lock index eb902cfc4c2..3da257dd018 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -6,8 +6,21 @@ PATH GEM remote: https://rubygems.org/ specs: + ast (2.3.0) minitest (5.9.1) + parser (2.3.1.4) + ast (~> 2.2) + powerpack (0.1.1) + rainbow (2.1.0) rake (10.5.0) + rubocop (0.43.0) + parser (>= 2.3.1.1, < 3.0) + powerpack (~> 0.1) + rainbow (>= 1.99.1, < 3.0) + ruby-progressbar (~> 1.7) + unicode-display_width (~> 1.0, >= 1.0.1) + ruby-progressbar (1.8.1) + unicode-display_width (1.1.1) PLATFORMS ruby @@ -17,6 +30,7 @@ DEPENDENCIES ddtrace! minitest (~> 5.0) rake (~> 10.0) + rubocop (~> 0.43) BUNDLED WITH 1.12.5 diff --git a/Rakefile b/Rakefile index 9f12ffad5d1..6ba7e2ff4f4 100644 --- a/Rakefile +++ b/Rakefile @@ -1,4 +1,5 @@ require "bundler/gem_tasks" +require 'rubocop/rake_task' require 'rake/testtask' Rake::TestTask.new(:test) do |task| @@ -6,4 +7,8 @@ Rake::TestTask.new(:test) do |task| task.test_files = FileList['test/**/*_test.rb'] end +RuboCop::RakeTask.new(:rubocop) do |t| + t.patterns = ['lib/**/*.rb', 'test/**/*.rb', 'Gemfile', 'Rakefile'] +end + task :default => :test diff --git a/ddtrace.gemspec b/ddtrace.gemspec index a2791430f1d..ca4c263d2f4 100644 --- a/ddtrace.gemspec +++ b/ddtrace.gemspec @@ -35,5 +35,6 @@ EOS spec.add_development_dependency "bundler", "~> 1.12" spec.add_development_dependency "rake", "~> 10.0" + spec.add_development_dependency "rubocop", "~> 0.43" spec.add_development_dependency "minitest", "~> 5.0" end From 44f8a54c257809ff805e7a06aa9259778a942b43 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 4 Oct 2016 09:28:09 +0200 Subject: [PATCH 2/5] [trace] linting tests --- test/helper.rb | 18 +++++++--------- test/span_test.rb | 34 +++++++++++++++--------------- test/tracer_test.rb | 50 ++++++++++++++++++++------------------------- test/writer_test.rb | 23 +++++++++------------ 4 files changed, 55 insertions(+), 70 deletions(-) diff --git a/test/helper.rb b/test/helper.rb index 617af5c9e8f..bbb23d3e91b 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -4,33 +4,29 @@ require 'ddtrace/tracer' require 'ddtrace/span' - # Return a test tracer instance with a faux writer. -def get_test_tracer() - return Datadog::Tracer.new({:writer => FauxWriter.new}) +def get_test_tracer + Datadog::Tracer.new(writer: FauxWriter.new) end - # FauxWriter is a dummy writer that buffers spans locally. class FauxWriter - - def initialize() + def initialize @buff = [] end def write(spans) # Ensure all of our test spans can be encoded to catch weird errors. - Datadog::encode_spans(spans) + Datadog.encode_spans(spans) @buff.concat(spans) end - def spans() + def spans buff = @buff @buff = [] - return buff + buff end - end # Return a hash mapping the given spans by name. @@ -39,5 +35,5 @@ def spans_by_name(spans) spans.each do |s| n[s.name] = s end - return n + n end diff --git a/test/span_test.rb b/test/span_test.rb index c4e1ffdafda..3e1a748c0d4 100644 --- a/test/span_test.rb +++ b/test/span_test.rb @@ -2,35 +2,34 @@ require 'ddtrace/span' class SpanTest < Minitest::Test - - def test_span_finish() + def test_span_finish tracer = nil span = Datadog::Span.new(tracer, 'my.op') assert span.start_time < Time.now.utc assert_equal(span.end_time, nil) - span.finish() + span.finish assert span.end_time < Time.now.utc end - def test_span_ids() + def test_span_ids span = Datadog::Span.new(nil, 'my.op') assert span.span_id - assert span.parent_id == 0 + assert span.parent_id.zero? assert span.trace_id == span.span_id assert_equal(span.name, 'my.op') - assert span.span_id != 0 - assert span.trace_id != 0 + assert span.span_id.nonzero? + assert span.trace_id.nonzero? end - def test_span_with_parent() - span = Datadog::Span.new(nil, 'my.op', {:parent_id=>12, :trace_id=>13}) + def test_span_with_parent + span = Datadog::Span.new(nil, 'my.op', parent_id: 12, trace_id: 13) assert span.span_id assert_equal(span.parent_id, 12) assert_equal(span.trace_id, 13) assert_equal(span.name, 'my.op') end - def test_span_block() + def test_span_block start = Time.now.utc span = nil Datadog::Span.new(nil, 'my.op').trace do |s| @@ -39,28 +38,27 @@ def test_span_block() span = s end - assert span.end_time != nil + assert !span.end_time.nil? assert span.end_time < Time.now.utc assert span.end_time > start end - def test_span_block_error() + def test_span_block_error start = Time.now.utc span = Datadog::Span.new(nil, 'my.op') assert_raises ZeroDivisionError do span.trace do - 1/0 + 1 / 0 end end assert_equal(span.status, 1) - assert span.end_time != nil + assert !span.end_time.nil? assert span.end_time < Time.now.utc assert span.end_time > start - assert_equal(span.get_tag("error.msg"), "divided by 0") - assert_equal(span.get_tag("error.type"), "ZeroDivisionError") - assert span.get_tag("error.stack").include?("dd-trace-rb") + assert_equal(span.get_tag('error.msg'), 'divided by 0') + assert_equal(span.get_tag('error.type'), 'ZeroDivisionError') + assert span.get_tag('error.stack').include?('dd-trace-rb') end - end diff --git a/test/tracer_test.rb b/test/tracer_test.rb index 6c1750b64c5..193bd311240 100644 --- a/test/tracer_test.rb +++ b/test/tracer_test.rb @@ -1,15 +1,11 @@ - - require 'helper' require 'ddtrace/tracer' - class TracerTest < Minitest::Test + def test_trace + tracer = get_test_tracer - def test_trace() - tracer = get_test_tracer() - - tracer.trace("something") do |s| + tracer.trace('something') do |s| assert_equal(s.end_time, nil) sleep 0.1 end @@ -17,42 +13,42 @@ def test_trace() spans = tracer.writer.spans() assert_equal(spans.length, 1) span = spans[0] - assert_equal(span.name, "something") + assert_equal(span.name, 'something') end - def test_trace_no_block() - tracer = get_test_tracer() - tracer.trace("something") + def test_trace_no_block + tracer = get_test_tracer + tracer.trace('something') end - def test_trace_error() - tracer = get_test_tracer() + def test_trace_error + tracer = get_test_tracer assert_raises ZeroDivisionError do - tracer.trace("something") do |s| + tracer.trace('something') do |s| assert_equal(s.end_time, nil) - 1/0 + 1 / 0 end end spans = tracer.writer.spans() assert_equal(spans.length, 1) span = spans[0] - assert_equal(span.name, "something") - assert_equal(span.get_tag("error.type"), "ZeroDivisionError") + assert_equal(span.name, 'something') + assert_equal(span.get_tag('error.type'), 'ZeroDivisionError') end - def test_trace_child() - tracer = get_test_tracer() + def test_trace_child + tracer = get_test_tracer # test a parent with two children - tracer.trace("a", :service=>"parent") do - tracer.trace("b") {|s| s.set_tag("a", "a")} - tracer.trace("c", :service=>"other") {|s| s.set_tag("b", "b")} + tracer.trace('a', service: 'parent') do + tracer.trace('b') { |s| s.set_tag('a', 'a') } + tracer.trace('c', service: 'other') { |s| s.set_tag('b', 'b') } end spans = tracer.writer.spans() - spans.sort! {|a,b| a.name <=> b.name} + spans.sort! { |a, b| a.name <=> b.name } assert_equal(spans.length, 3) a, b, c = spans assert_equal(a.name, 'a') @@ -63,10 +59,8 @@ def test_trace_child() assert_equal(a.span_id, b.parent_id) assert_equal(a.span_id, c.parent_id) - assert_equal(a.service, "parent") - assert_equal(b.service, "parent") - assert_equal(c.service, "other") + assert_equal(a.service, 'parent') + assert_equal(b.service, 'parent') + assert_equal(c.service, 'other') end - - end diff --git a/test/writer_test.rb b/test/writer_test.rb index ffb490f746a..0656bf582da 100644 --- a/test/writer_test.rb +++ b/test/writer_test.rb @@ -2,35 +2,32 @@ require 'minitest/autorun' class TraceBufferTest < Minitest::Test - - def test_trace_buffer_max_size() + def test_trace_buffer_max_size buffer = Datadog::TraceBuffer.new(3) buffer.push(1) buffer.push(2) buffer.push(3) buffer.push(4) - out = buffer.pop() + out = buffer.pop assert_equal(out.length, 3) assert out.include?(4) end - def test_trace_buffer() + def test_trace_buffer buffer = Datadog::TraceBuffer.new(500) thread_count = 100 - threads = thread_count.times.map do |i| - Thread.new { - sleep(rand()/1000) + threads = Array.new(thread_count) do |i| + Thread.new do + sleep(rand / 1000) buffer.push(i) - } + end end - threads.each{|t| t.join()} - out = buffer.pop() + threads.each(&:join) + out = buffer.pop assert !out.nil? - expected = (0..thread_count-1).to_a + expected = (0..thread_count - 1).to_a assert_equal(out.sort, expected) end - end - From a1670c33dd339fe4bf201cb4b7ec610cff3aeeb9 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 4 Oct 2016 09:28:27 +0200 Subject: [PATCH 3/5] [trace] linting examples --- examples/loop.rb | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/examples/loop.rb b/examples/loop.rb index ae4f05877fa..749f3884593 100644 --- a/examples/loop.rb +++ b/examples/loop.rb @@ -1,43 +1,37 @@ - -require "ddtrace/tracer" +require 'ddtrace/tracer' # Generate a fake trace with the given tracer. def trace(tracer) - urls = ["/home", "/login", "/logout"] - resource = urls.sample() + urls = ['/home', '/login', '/logout'] + resource = urls.sample # rake web request. - tracer.trace("web.request", :service=>"web", :resource=>resource) do + tracer.trace('web.request', service: 'web', resource: resource) do sleep rand(0..0.1) # fake query. - tracer.trace("db.query", :service=>"db") do + tracer.trace('db.query', service: 'db') do sleep rand(0..0.1) end # fake template. - tracer.trace("web.template") do + tracer.trace('web.template') do r = rand(0..1.0) - if r < 0.25 - 1/0 - end + 1 / 0 if r < 0.25 end end rescue ZeroDivisionError => e - puts "error #{e}" + puts "error #{e}" end # Generate fake traces. -def run() - tracer = Datadog::Tracer.new() +def run + tracer = Datadog::Tracer.new loop do trace(tracer) sleep 0.0001 - puts "traced #{tracer.writer.stats()}" + puts "traced #{tracer.writer.stats}" end end - -if __FILE__ == $0 - run() -end +run if __FILE__ == $PROGRAM_NAME From 035f7d6d75a518a489b0dc386187852cd5a2ef78 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 4 Oct 2016 09:28:53 +0200 Subject: [PATCH 4/5] [trace] linting lib --- lib/ddtrace/ext/errors.rb | 9 ++-- lib/ddtrace/local.rb | 18 +++----- lib/ddtrace/span.rb | 94 +++++++++++++++++---------------------- lib/ddtrace/tracer.rb | 20 +++------ lib/ddtrace/transport.rb | 10 +---- lib/ddtrace/version.rb | 2 +- lib/ddtrace/writer.rb | 54 ++++++++++------------ 7 files changed, 82 insertions(+), 125 deletions(-) diff --git a/lib/ddtrace/ext/errors.rb b/lib/ddtrace/ext/errors.rb index 8b5a4b847c3..c581778a71b 100644 --- a/lib/ddtrace/ext/errors.rb +++ b/lib/ddtrace/ext/errors.rb @@ -1,10 +1,7 @@ - module Datadog - module Errors - MSG = "error.msg" - TYPE = "error.type" - STACK = "error.stack" + MSG = 'error.msg'.freeze + TYPE = 'error.type'.freeze + STACK = 'error.stack'.freeze end - end diff --git a/lib/ddtrace/local.rb b/lib/ddtrace/local.rb index 7931f90f5ff..42f53787b6a 100644 --- a/lib/ddtrace/local.rb +++ b/lib/ddtrace/local.rb @@ -1,28 +1,22 @@ - - require 'thread' module Datadog - class SpanBuffer - # Set the current active span. def set(span) Thread.current[:datadog_span] = span end # Return the current active span or nil. - def get() - return Thread.current[:datadog_span] + def get + Thread.current[:datadog_span] end # Pop the current active span. - def pop() - s = self.get() - self.set(nil) - return s + def pop + s = get + set(nil) + s end - end - end diff --git a/lib/ddtrace/span.rb b/lib/ddtrace/span.rb index f8b3d4cecf7..63553dcc2e3 100644 --- a/lib/ddtrace/span.rb +++ b/lib/ddtrace/span.rb @@ -1,27 +1,22 @@ - require 'json' require 'time' - - module Datadog - class Span - attr_accessor :name, :service, :resource, :start_time, :end_time, :span_id, :trace_id, :parent_id, :meta, :status, :parent # Create a new span linked to the given tracer. - def initialize(tracer, name, options={}) + def initialize(tracer, name, options = {}) @tracer = tracer @name = name @service = options[:service] @resource = options[:resource] || name - @span_id = Datadog::next_id() + @span_id = Datadog.next_id @parent_id = options[:parent_id] || 0 @trace_id = options[:trace_id] || @span_id @@ -34,104 +29,95 @@ def initialize(tracer, name, options={}) @end_time = nil end - def trace() - begin - if block_given? - yield(self) - end - rescue StandardError => e - self.set_error(e) - raise - ensure - self.finish() - end + def trace + yield(self) if block_given? + rescue StandardError => e + set_error(e) + raise + ensure + finish end def set_tag(key, value) - return @meta[key] = value + @meta[key] = value end # Return the tag wth the given key, nil if it doesn't exist. def get_tag(key) - return @meta[key] + @meta[key] end # Mark the span with the given error. def set_error(e) - if e != nil + unless e.nil? @status = 1 - @meta["error.msg"] = e.message - @meta["error.type"] = e.class.to_s - @meta["error.stack"] = e.backtrace.join("\n") + @meta['error.msg'] = e.message + @meta['error.type'] = e.class.to_s + @meta['error.stack'] = e.backtrace.join("\n") end end # Mark the span finished at the current time and submit it. - def finish() - return self.finish_at(Time.now.utc) + def finish + finish_at(Time.now.utc) end # Mark the span finished at the given time and submit it. def finish_at(end_time) @end_time = end_time - if !@tracer.nil? - @tracer.record(self) - end + @tracer.record(self) unless @tracer.nil? - return self + self end # Return a string representation of the span. - def to_s() - return "Span(name:#{@name},sid:#{@span_id},tid:#{@trace_id},pid:#{@parent_id})" + def to_s + "Span(name:#{@name},sid:#{@span_id},tid:#{@trace_id},pid:#{@parent_id})" end # Set this span's parent, inheriting any properties not explicitly set. def set_parent(parent) @parent = parent - if parent != nil + unless parent.nil? @trace_id = parent.trace_id @parent_id = parent.span_id - @service = @service || parent.service + @service ||= parent.service end end - def to_hash() + def to_hash h = { - :span_id => @span_id, - :parent_id => @parent_id, - :trace_id => @trace_id, - :name => @name, - :service => @service, - :resource => @resource, - :type => "FIXME", - :meta => @meta, - :error => @status, + span_id: @span_id, + parent_id: @parent_id, + trace_id: @trace_id, + name: @name, + service: @service, + resource: @resource, + type: 'FIXME', + meta: @meta, + error: @status } - if @start_time != nil && @end_time != nil + if !@start_time.nil? && !@end_time.nil? h[:start] = (@start_time.to_f * 1e9).to_i h[:duration] = ((@end_time - @start_time) * 1e9).to_i end - return h + h end - end - @@max_id = 2**64-1 + @@max_id = 2**64 - 1 # Return a span id. - def self.next_id() - return rand(@@max_id) + def self.next_id + rand(@@max_id) end # Encode the given set of spans. def self.encode_spans(spans) - hashes = spans.map{|s| s.to_hash()} - return JSON.dump(hashes) + hashes = spans.map(&:to_hash) + JSON.dump(hashes) end - - end diff --git a/lib/ddtrace/tracer.rb b/lib/ddtrace/tracer.rb index e82dbfb887a..f352b23e23a 100644 --- a/lib/ddtrace/tracer.rb +++ b/lib/ddtrace/tracer.rb @@ -1,30 +1,25 @@ - require 'ddtrace/span' require 'ddtrace/local' require 'ddtrace/writer' - module Datadog - class Tracer - attr_reader :writer - def initialize(options={}) - + def initialize(options = {}) # buffers and sends completed traces. - @writer = options[:writer] || Datadog::Writer.new() + @writer = options[:writer] || Datadog::Writer.new # store thes the active thread in the current span. - @buffer = Datadog::SpanBuffer.new() + @buffer = Datadog::SpanBuffer.new @spans = [] end - def trace(name, options={}) + def trace(name, options = {}) span = Span.new(self, name, options) # set up inheritance - parent = @buffer.get() + parent = @buffer.get span.set_parent(parent) @buffer.set(span) @@ -32,7 +27,7 @@ def trace(name, options={}) if block_given? return span.trace(&Proc.new) else - return span.trace() + return span.trace end end @@ -44,13 +39,12 @@ def record(span) if parent.nil? spans = @spans @spans = [] - self.write(spans) + write(spans) end end def write(spans) @writer.write(spans) unless @writer.nil? end - end end diff --git a/lib/ddtrace/transport.rb b/lib/ddtrace/transport.rb index 38bf36f9a53..f74b8f71a8a 100644 --- a/lib/ddtrace/transport.rb +++ b/lib/ddtrace/transport.rb @@ -1,26 +1,20 @@ - require 'net/http' - module Datadog - class Transport - def initialize(host, port) @http = Net::HTTP.new(host, port) - @headers = {'Content-Type' => 'text/json'} + @headers = { 'Content-Type' => 'text/json' } end def write(spans) out = Datadog.encode_spans(spans) - request = Net::HTTP::Post.new("/spans", @headers) + request = Net::HTTP::Post.new('/spans', @headers) request.body = out response = @http.request(request) puts response - end - end end diff --git a/lib/ddtrace/version.rb b/lib/ddtrace/version.rb index 87fe5f7a99b..2bf294e34a3 100644 --- a/lib/ddtrace/version.rb +++ b/lib/ddtrace/version.rb @@ -4,6 +4,6 @@ module VERSION MINOR = 1 PATCH = 0 - STRING = [MAJOR, MINOR, PATCH].compact.join(".") + STRING = [MAJOR, MINOR, PATCH].compact.join('.') end end diff --git a/lib/ddtrace/writer.rb b/lib/ddtrace/writer.rb index 23fe55af6c8..b90e6ab486c 100644 --- a/lib/ddtrace/writer.rb +++ b/lib/ddtrace/writer.rb @@ -1,37 +1,33 @@ - - require 'ddtrace/transport' module Datadog - # Writer buffer and periodically sends traces to the server. class Writer - - def initialize(options={}) - @transport = options[:transport] || Datadog::Transport.new("localhost", "7777") + def initialize(options = {}) + @transport = options[:transport] || Datadog::Transport.new('localhost', '7777') @trace_buffer = TraceBuffer.new(100) @flush_interval = 1 @traces_flushed = 0 - self.spawn(@flush_interval) + spawn(@flush_interval) end # spawn will spawn a thread that will periodically flush to the server. def spawn(interval) - Thread.new { + Thread.new do loop do sleep(interval) - self.flush() + flush end - } + end end # flush will trigger a flush to the server. - def flush() - traces = @trace_buffer.pop() - if 0 < traces.length - spans = traces.flatten() + def flush + traces = @trace_buffer.pop + unless traces.empty? + spans = traces.flatten # FIXME[matt] submit as an array of traces or a flat array of spans? # @transport.write(spans) # FIXME matt: if there's an error, requeue @@ -45,19 +41,17 @@ def write(trace) end # stats returns a dictionary of stats about the writer. - def stats() - return { - :traces_flushed => @traces_flushed, - :traces_buffered => @trace_buffer.length() + def stats + { + traces_flushed: @traces_flushed, + traces_buffered: @trace_buffer.length } end - end # TraceBuffer buffers a maximum number of traces waiting to be sent to the # app. class TraceBuffer - def initialize(max_size) @mutex = Mutex.new @traces = [] @@ -65,31 +59,29 @@ def initialize(max_size) end def push(trace) - @mutex.synchronize { + @mutex.synchronize do len = @traces.length if len < @max_size @traces << trace else - @traces[rand(len)] = trace # drop a random one + @traces[rand(len)] = trace # drop a random one end - } + end end - def length() - @mutex.synchronize { + def length + @mutex.synchronize do return @traces.length - } + end end - def pop() - @mutex.synchronize { + def pop + @mutex.synchronize do # FIXME: reuse array? t = @traces @traces = [] return t - } + end end - end - end From 235bef0617c2da68515df5aa5338721768bde99a Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 4 Oct 2016 09:49:35 +0200 Subject: [PATCH 5/5] [trace] generic linting --- Gemfile | 1 - Rakefile | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index ce484410ebb..fa75df15632 100644 --- a/Gemfile +++ b/Gemfile @@ -1,4 +1,3 @@ source 'https://rubygems.org' - gemspec diff --git a/Rakefile b/Rakefile index 6ba7e2ff4f4..6f19df72998 100644 --- a/Rakefile +++ b/Rakefile @@ -1,4 +1,4 @@ -require "bundler/gem_tasks" +require 'bundler/gem_tasks' require 'rubocop/rake_task' require 'rake/testtask' @@ -11,4 +11,4 @@ RuboCop::RakeTask.new(:rubocop) do |t| t.patterns = ['lib/**/*.rb', 'test/**/*.rb', 'Gemfile', 'Rakefile'] end -task :default => :test +task default: :test