Merge pull request #358 from airbrake/notify-request

notifier: rename #inc_request to #notify_request

README.md

@@ -404,7 +404,7 @@ end
 
 #### route_stats_flush_period
 
-By default, it's set to `15`. When `Airbrake.inc_request` is invoked, then
+By default, it's set to `15`. When `Airbrake.notify_request` is invoked, then
 Airbrake Ruby waits 15 seconds trying to collect route stats. After this delay
 it sends all route stats in a batch using only one web request. Setting this
 value allows speeding up or slowing down this process. Zero value means no

lib/airbrake-ruby.rb

@@ -115,7 +115,7 @@ def configured?
     def merge_context(_context); end
 
     # @macro see_public_api_method
-    def inc_request(method, route, status_code, dur, time); end
+    def notify_request(request_info); end
   end
 
   # A Hash that holds all notifiers. The keys of the Hash are notifier
@@ -345,26 +345,32 @@ def merge_context(context)
       @notifiers[:default].merge_context(context)
     end
 
-    # Increments request count of a certain +route+ that was invoked with
-    # +method+, and returned +status_code+ at +time+ and took +dur+
-    # milliseconds.
+    # Increments request statistics of a certain +route+ that was invoked on
+    # +start_time+ and ended on +end_time+ with +method+, and returned
+    # +status_code+.
     #
     # After a certain amount of time (n seconds) the aggregated route
     # information will be sent to Airbrake.
     #
     # @example
-    #   Airbrake.inc_request('POST', '/thing/:id/create', 200, 123, Time.now)
+    #   Airbrake.notify_request(
+    #     method: 'POST',
+    #     route: '/thing/:id/create',
+    #     status_code: 200,
+    #     start_time: timestamp,
+    #     end_time: Time.now
+    #   )
     #
-    # @param [String] method The HTTP method that was invoked
-    # @param [String] route The route that was invoked
-    # @param [Integer] status_code The respose code that the route returned
-    # @param [Float] dur How much time the processing of the request took in
-    #   milliseconds
-    # @param [Time] time When the request happened
+    # @param [Hash{Symbol=>Object}] request_info
+    # @option request_info [String] :method The HTTP method that was invoked
+    # @option request_info [String] :route The route that was invoked
+    # @option request_info [Integer] :status_code The respose code that the route returned
+    # @option request_info [Date] :start_time When the request started
+    # @option request_info [Time] :end_time When the request ended (optional)
     # @return [void]
     # @since v3.0.0
-    def inc_request(method, route, status_code, dur, time)
-      @notifiers[:default].inc_request(method, route, status_code, dur, time)
+    def notify_request(request_info)
+      @notifiers[:default].notify_request(request_info)
     end
   end
 end

lib/airbrake-ruby/notifier.rb

@@ -100,8 +100,8 @@ def merge_context(context)
     end
 
     # @macro see_public_api_method
-    def inc_request(*args)
-      @route_sender.inc_request(*args)
+    def notify_request(request_info)
+      @route_sender.notify_request(request_info)
     end
 
     # @return [String] customized inspect to lessen the amount of clutter

lib/airbrake-ruby/route_sender.rb

@@ -87,14 +87,18 @@ def initialize(config)
     end
 
     # @macro see_public_api_method
-    def inc_request(method, route, status_code, dur, tm)
-      route = create_route_key(method, route, status_code, tm)
-
+    def notify_request(request_info)
+      route = create_route_key(
+        request_info[:method],
+        request_info[:route],
+        request_info[:status_code],
+        utc_truncate_minutes(request_info[:start_time])
+      )
       promise = Airbrake::Promise.new
 
       @mutex.synchronize do
         @routes[route] ||= RouteStat.new
-        increment_stats(@routes[route], dur)
+        increment_stats(request_info, @routes[route])
 
         if @flush_period > 0
           schedule_flush(promise)
@@ -117,10 +121,10 @@ def create_route_key(method, route, status_code, tm)
       RouteKey.new(method, route, status_code, time.rfc3339)
     end
 
-    def increment_stats(stat, dur)
+    def increment_stats(request_info, stat)
       stat.count += 1
 
-      ms = dur.to_f
+      ms = (request_info[:end_time] || Time.now) - request_info[:start_time]
       stat.sum += ms
       stat.sumsq += ms * ms
 
@@ -159,5 +163,11 @@ def send(routes, promise)
         URI.join(@config.host, "api/v5/projects/#{@config.project_id}/routes-stats")
       )
     end
+
+    def utc_truncate_minutes(time)
+      time_array = time.to_a
+      time_array[0] = 0
+      Time.utc(*time_array)
+    end
   end
 end

spec/airbrake_spec.rb

@@ -111,13 +111,17 @@
     end
   end
 
-  describe ".inc_request" do
-    it "forwards 'inc_request' to the notifier" do
-      t = Time.now
-      expect(default_notifier).to receive(:inc_request).with(
-        'GET', '/foo', 200, 1000, t
-      )
-      described_class.inc_request('GET', '/foo', 200, 1000, t)
+  describe ".notify_request" do
+    it "forwards 'notify_request' to the notifier" do
+      params = {
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 20, 0, 0),
+        end_time: Time.new(2018, 1, 1, 0, 19, 0, 0)
+      }
+      expect(default_notifier).to receive(:notify_request).with(params)
+      described_class.notify_request(params)
     end
   end
 end

spec/notifier_spec.rb

@@ -450,13 +450,18 @@
     end
   end
 
-  describe "#inc_request" do
-    it "forwards 'inc_request' to RouteSender" do
-      t = Time.now
-      expect_any_instance_of(Airbrake::RouteSender).to receive(:inc_request).with(
-        'GET', '/foo', 200, 1000, t
-      )
-      subject.inc_request('GET', '/foo', 200, 1000, t)
+  describe "#notify_request" do
+    it "forwards 'notify_request' to RouteSender" do
+      params = {
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 20, 0, 0),
+        end_time: Time.new(2018, 1, 1, 0, 19, 0, 0)
+      }
+      expect_any_instance_of(Airbrake::RouteSender)
+        .to receive(:notify_request).with(params)
+      subject.notify_request(params)
     end
   end
 

spec/route_sender_spec.rb

@@ -13,7 +13,7 @@
 
   subject { described_class.new(config) }
 
-  describe "#inc_request" do
+  describe "#notify_request" do
     before do
       stub_request(:put, endpoint).to_return(status: 200, body: '')
     end
@@ -22,62 +22,106 @@
     after { sleep 0.2 }
 
     it "rounds time to the floor minute" do
-      subject.inc_request('GET', '/foo', 200, 24, Time.new(2018, 1, 1, 0, 0, 20, 0))
+      subject.notify_request(
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 0, 20, 0)
+      )
       sleep 0.2
       expect(
         a_request(:put, endpoint).with(body: /"time":"2018-01-01T00:00:00\+00:00"/)
       ).to have_been_made
     end
 
     it "increments routes with the same key" do
-      subject.inc_request('GET', '/foo', 200, 24, Time.new(2018, 1, 1, 0, 0, 20, 0))
-      subject.inc_request('GET', '/foo', 200, 24, Time.new(2018, 1, 1, 0, 0, 50, 0))
+      subject.notify_request(
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 0, 20, 0)
+      )
+      subject.notify_request(
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 0, 50, 0)
+      )
       sleep 0.2
       expect(
         a_request(:put, endpoint).with(body: /"count":2/)
       ).to have_been_made
     end
 
     it "groups routes by time" do
-      subject.inc_request('GET', '/foo', 200, 24, Time.new(2018, 1, 1, 0, 0, 20, 0))
-      subject.inc_request('GET', '/foo', 200, 10, Time.new(2018, 1, 1, 0, 1, 20, 0))
+      subject.notify_request(
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 0, 49, 0),
+        end_time: Time.new(2018, 1, 1, 0, 0, 50, 0)
+      )
+      subject.notify_request(
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 1, 49, 0),
+        end_time: Time.new(2018, 1, 1, 0, 1, 55, 0)
+      )
       sleep 0.2
       expect(
         a_request(:put, endpoint).with(
           body: %r|\A
             {"routes":\[
               {"method":"GET","route":"/foo","status_code":200,
-               "time":"2018-01-01T00:00:00\+00:00","count":1,"sum":24.0,
-               "sumsq":576.0,"tdigest":"AAAAAkA0AAAAAAAAAAAAAUHAAAAB"},
+               "time":"2018-01-01T00:00:00\+00:00","count":1,"sum":1.0,
+               "sumsq":1.0,"tdigest":"AAAAAkA0AAAAAAAAAAAAAT\+AAAAB"},
               {"method":"GET","route":"/foo","status_code":200,
-               "time":"2018-01-01T00:01:00\+00:00","count":1,"sum":10.0,
-               "sumsq":100.0,"tdigest":"AAAAAkA0AAAAAAAAAAAAAUEgAAAB"}\]}
+               "time":"2018-01-01T00:01:00\+00:00","count":1,"sum":6.0,
+               "sumsq":36.0,"tdigest":"AAAAAkA0AAAAAAAAAAAAAUDAAAAB"}\]}
           \z|x
         )
       ).to have_been_made
     end
 
     it "groups routes by route key" do
-      subject.inc_request('GET', '/foo', 200, 24, Time.new(2018, 1, 1, 0, 0, 20, 0))
-      subject.inc_request('POST', '/foo', 200, 10, Time.new(2018, 1, 1, 0, 0, 20, 0))
+      subject.notify_request(
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 49, 0, 0),
+        end_time: Time.new(2018, 1, 1, 0, 50, 0, 0)
+      )
+      subject.notify_request(
+        method: 'POST',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 49, 0, 0),
+        end_time: Time.new(2018, 1, 1, 0, 50, 0, 0)
+      )
       sleep 0.2
       expect(
         a_request(:put, endpoint).with(
           body: %r|\A
             {"routes":\[
               {"method":"GET","route":"/foo","status_code":200,
-               "time":"2018-01-01T00:00:00\+00:00","count":1,"sum":24.0,
-               "sumsq":576.0,"tdigest":"AAAAAkA0AAAAAAAAAAAAAUHAAAAB"},
+               "time":"2018-01-01T00:49:00\+00:00","count":1,"sum":60.0,
+               "sumsq":3600.0,"tdigest":"AAAAAkA0AAAAAAAAAAAAAUJwAAAB"},
               {"method":"POST","route":"/foo","status_code":200,
-               "time":"2018-01-01T00:00:00\+00:00","count":1,"sum":10.0,
-               "sumsq":100.0,"tdigest":"AAAAAkA0AAAAAAAAAAAAAUEgAAAB"}\]}
+               "time":"2018-01-01T00:49:00\+00:00","count":1,"sum":60.0,
+               "sumsq":3600.0,"tdigest":"AAAAAkA0AAAAAAAAAAAAAUJwAAAB"}\]}
           \z|x
         )
       ).to have_been_made
     end
 
     it "returns a promise" do
-      promise = subject.inc_request('GET', '/foo', 200, 24, Time.new)
+      promise = subject.notify_request(
+        method: 'GET',
+        route: '/foo',
+        status_code: 200,
+        start_time: Time.new(2018, 1, 1, 0, 49, 0, 0)
+      )
       sleep 0.2
       expect(promise).to be_an(Airbrake::Promise)
       expect(promise.value).to eq('' => nil)