Merge pull request #609 from airbrake/601-start-end-time-removal

performance_notifier: drop support for :{start,end}_time params

CHANGELOG.md

@@ -5,8 +5,13 @@ Airbrake Ruby Changelog
 
 * Fixed unwanted mutation of nested hashes passed through `Notice#[]=`
   ([#597](https://github.com/airbrake/airbrake-ruby/pull/597))
-* Dropped support for Ruby 2.1 ([#603](https://github.com/airbrake/airbrake-ruby/pull/603))
-* Dropped support for Ruby 2.2 ([#604](https://github.com/airbrake/airbrake-ruby/pull/604))
+* Dropped support for Ruby 2.1
+  ([#603](https://github.com/airbrake/airbrake-ruby/pull/603))
+* Dropped support for Ruby 2.2
+  ([#604](https://github.com/airbrake/airbrake-ruby/pull/604))
+* Dropped support for `:start_time` & `:end_time` params for APM methods such as
+  `Airbrake.notify_request`
+  ([#609](https://github.com/airbrake/airbrake-ruby/pull/609))
 
 ### [v5.0.0.rc.2][v5.0.0.rc.2] (July 29, 2020)
 

benchmarks/performance.rb

@@ -15,24 +15,22 @@
   func: 'foo',
   file: 'foo.rb',
   line: 123,
-  start_time: Time.new - 200,
-  end_time: Time.new,
+  timing: 200,
 }
 
 request = {
   method: 'GET',
   route: '/things/1',
   status_code: 200,
-  start_time: Time.new - 200,
-  end_time: Time.new,
+  timing: 200,
 }
 
 breakdown = {
   method: 'GET',
   route: '/things/1',
   response_type: 'json',
   groups: { db: 24.0, view: 0.4 },
-  start_time: Time.new,
+  timing: 200,
 }
 
 Benchmark.ips do |ips|

lib/airbrake-ruby/performance_breakdown.rb

@@ -12,16 +12,13 @@ class PerformanceBreakdown
     include Stashable
     include Mergeable
 
-    attr_accessor :method, :route, :response_type, :groups, :start_time,
-                  :end_time, :timing, :time
+    attr_accessor :method, :route, :response_type, :groups, :timing, :time
 
     def initialize(
       method:,
       route:,
       response_type:,
       groups:,
-      start_time: Time.now,
-      end_time: start_time + 1,
       timing: nil,
       time: Time.now
     )
@@ -30,8 +27,6 @@ def initialize(
       @route = route
       @response_type = response_type
       @groups = groups
-      @start_time = start_time
-      @end_time = end_time
       @timing = timing
       @time = time
     end

lib/airbrake-ruby/performance_notifier.rb

@@ -104,27 +104,14 @@ def update_payload(resource)
         @payload[resource] = { total: Airbrake::Stat.new }
       end
 
-      update_total(resource, @payload[resource][:total])
+      @payload[resource][:total].increment_ms(resource.timing)
 
       resource.groups.each do |name, ms|
         @payload[resource][name] ||= Airbrake::Stat.new
         @payload[resource][name].increment_ms(ms)
       end
     end
 
-    def update_total(resource, total)
-      if resource.timing
-        total.increment_ms(resource.timing)
-      else
-        loc = caller_locations(6..6).first
-        Kernel.warn(
-          "#{loc.path}:#{loc.lineno}: warning: :start_time and :end_time are " \
-          "deprecated. Use :timing & :time instead",
-        )
-        total.increment(resource.start_time, resource.end_time)
-      end
-    end
-
     def check_configuration(resource)
       promise = @config.check_configuration
       return promise if promise.rejected?

lib/airbrake-ruby/query.rb

@@ -12,8 +12,7 @@ class Query
     include Mergeable
     include Grouppable
 
-    attr_accessor :method, :route, :query, :func, :file, :line, :start_time,
-                  :end_time, :timing, :time
+    attr_accessor :method, :route, :query, :func, :file, :line, :timing, :time
 
     def initialize(
       method:,
@@ -22,8 +21,6 @@ def initialize(
       func: nil,
       file: nil,
       line: nil,
-      start_time: Time.now,
-      end_time: start_time + 1,
       timing: nil,
       time: Time.now
     )
@@ -34,8 +31,6 @@ def initialize(
       @func = func
       @file = file
       @line = line
-      @start_time = start_time
-      @end_time = end_time
       @timing = timing
       @time = time
     end

lib/airbrake-ruby/queue.rb

@@ -4,30 +4,24 @@ module Airbrake
   # @see Airbrake.notify_queue
   # @api public
   # @since v4.9.0
-  # rubocop:disable Metrics/ParameterLists
   class Queue
     include HashKeyable
     include Ignorable
     include Stashable
 
-    attr_accessor :queue, :error_count, :groups, :start_time, :end_time,
-                  :timing, :time
+    attr_accessor :queue, :error_count, :groups, :timing, :time
 
     def initialize(
       queue:,
       error_count:,
       groups: {},
-      start_time: Time.now,
-      end_time: start_time + 1,
       timing: nil,
       time: Time.now
     )
       @time_utc = TimeTruncate.utc_truncate_minutes(time)
       @queue = queue
       @error_count = error_count
       @groups = groups
-      @start_time = start_time
-      @end_time = end_time
       @timing = timing
       @time = time
     end
@@ -68,5 +62,4 @@ def route
       ''
     end
   end
-  # rubocop:enable Metrics/ParameterLists
 end

lib/airbrake-ruby/request.rb

@@ -4,32 +4,26 @@ module Airbrake
   # @see Airbrake.notify_request
   # @api public
   # @since v3.2.0
-  # rubocop:disable Metrics/ParameterLists
   class Request
     include HashKeyable
     include Ignorable
     include Stashable
     include Mergeable
     include Grouppable
 
-    attr_accessor :method, :route, :status_code, :start_time, :end_time,
-                  :timing, :time
+    attr_accessor :method, :route, :status_code, :timing, :time
 
     def initialize(
       method:,
       route:,
       status_code:,
-      start_time: Time.now,
-      end_time: start_time + 1,
       timing: nil,
       time: Time.now
     )
       @time_utc = TimeTruncate.utc_truncate_minutes(time)
       @method = method
       @route = route
       @status_code = status_code
-      @start_time = start_time
-      @end_time = end_time
       @timing = timing
       @time = time
     end
@@ -51,5 +45,4 @@ def to_h
       }.delete_if { |_key, val| val.nil? }
     end
   end
-  # rubocop:enable Metrics/ParameterLists
 end

lib/airbrake-ruby/stat.rb

@@ -9,7 +9,7 @@ module Airbrake
   #
   # @example
   #   stat = Airbrake::Stat.new
-  #   stat.increment(Time.now - 200)
+  #   stat.increment_ms(2000)
   #   stat.to_h # Pack and serialize data so it can be transmitted.
   #
   # @since v3.2.0
@@ -41,17 +41,6 @@ def to_h
       end
     end
 
-    # Increments tdigest timings and updates tdigest with the difference between
-    # +end_time+ and +start_time+.
-    #
-    # @param [Date] start_time
-    # @param [Date] end_time
-    # @return [void]
-    def increment(start_time, end_time = nil)
-      end_time ||= Time.new
-      increment_ms((end_time - start_time) * 1000)
-    end
-
     # Increments tdigest timings and updates tdigest with given +ms+ value.
     #
     # @param [Float] ms

spec/performance_breakdown_spec.rb

@@ -3,21 +3,9 @@
     subject do
       described_class.new(
         method: 'GET', route: '/', response_type: '', groups: {},
-        start_time: Time.now
       )
     end
 
     it { is_expected.to respond_to(:stash) }
   end
-
-  describe "#end_time" do
-    it "is always equal to start_time + 1 second by default" do
-      time = Time.now
-      performance_breakdown = described_class.new(
-        method: 'GET', route: '/', response_type: '', groups: {},
-        start_time: time
-      )
-      expect(performance_breakdown.end_time).to eq(time + 1)
-    end
-  end
 end

spec/performance_notifier_spec.rb

@@ -541,31 +541,6 @@
       end
     end
 
-    context "when :start_time is specified (deprecated)" do
-      before do
-        allow(Kernel).to receive(:warn)
-      end
-
-      it "uses the value of :start_time to update stat" do
-        subject.notify(
-          Airbrake::Query.new(
-            method: 'POST',
-            route: '/foo',
-            query: 'SELECT * FROM things',
-            start_time: Time.new(2018, 1, 1, 0, 49, 0, 0),
-            end_time: Time.new(2018, 1, 1, 0, 50, 0, 0),
-          ),
-        )
-        subject.close
-
-        expect(
-          a_request(:put, queries).with(
-            body: /"count":1,"sum":60000.0,"sumsq":3600000000.0/,
-          ),
-        ).to have_been_made
-      end
-    end
-
     context "when provided :timing is zero" do
       it "doesn't notify" do
         queue = Airbrake::Queue.new(queue: 'bananas', error_count: 0, timing: 0)

spec/query_spec.rb

@@ -2,20 +2,10 @@
   describe "#stash" do
     subject do
       described_class.new(
-        method: 'GET', route: '/', query: '', start_time: Time.now,
+        method: 'GET', route: '/', query: '',
       )
     end
 
     it { is_expected.to respond_to(:stash) }
   end
-
-  describe "#end_time" do
-    it "is always equal to start_time + 1 second by default" do
-      time = Time.now
-      query = described_class.new(
-        method: 'GET', route: '/', query: '', start_time: time,
-      )
-      expect(query.end_time).to eq(time + 1)
-    end
-  end
 end

spec/queue_spec.rb

@@ -9,21 +9,9 @@
     it { is_expected.to respond_to(:stash) }
   end
 
-  describe "#end_time" do
-    it "is always equal to start_time + 1 second by default" do
-      time = Time.now
-      queue = described_class.new(
-        queue: 'bananas', error_count: 0, start_time: time,
-      )
-      expect(queue.end_time).to eq(time + 1)
-    end
-  end
-
   describe "#route" do
     it "always returns an empty route" do
-      queue = described_class.new(
-        queue: 'a', error_count: 0, start_time: Time.now,
-      )
+      queue = described_class.new(queue: 'a', error_count: 0)
       expect(queue.route).to be_empty
     end
   end

spec/request_spec.rb

@@ -1,21 +1,9 @@
 RSpec.describe Airbrake::Request do
   describe "#stash" do
     subject do
-      described_class.new(
-        method: 'GET', route: '/', status_code: 200, start_time: Time.now,
-      )
+      described_class.new(method: 'GET', route: '/', status_code: 200)
     end
 
     it { is_expected.to respond_to(:stash) }
   end
-
-  describe "#end_time" do
-    it "is always equal to start_time + 1 second by default" do
-      time = Time.now
-      request = described_class.new(
-        method: 'GET', route: '/', status_code: 200, start_time: time,
-      )
-      expect(request.end_time).to eq(time + 1)
-    end
-  end
 end

spec/stat_spec.rb

@@ -10,15 +10,6 @@
     end
   end
 
-  describe "#increment" do
-    let(:start_time) { Time.new(2018, 1, 1, 0, 0, 20, 0) }
-    let(:end_time) { Time.new(2018, 1, 1, 0, 0, 22, 0) }
-
-    before { subject.increment(start_time, end_time) }
-
-    its(:sum) { is_expected.to eq(2000) }
-  end
-
   describe "#increment_ms" do
     before { subject.increment_ms(1000) }