Merge pull request #196 from rollbar/fix-frames-truncation-strategy

Don't try to truncate payloads with 'trace' if that key doesn't exist.

README.md

@@ -410,11 +410,11 @@ You can supply your own handler using ```config.async_handler```. The object to
 ```ruby
 config.use_async
 config.async_handler = Proc.new { |payload|
-  Thread.new { Rollbar.process_payload(payload) }
+  Thread.new { Rollbar.process_payload_safely(payload) }
 }
 ```
 
-Make sure you pass ```payload``` to ```Rollbar.process_payload``` in your own implementation.
+Make sure you pass ```payload``` to ```Rollbar.process_payload_safely``` in your own implementation.
 
 ## Failover handlers
 

lib/generators/rollbar/templates/initializer.rb

@@ -46,7 +46,7 @@
   # config.use_async = true
   # Supply your own async handler:
   # config.async_handler = Proc.new { |payload|
-  #  Thread.new { Rollbar.process_payload(payload) }
+  #  Thread.new { Rollbar.process_payload_safely(payload) }
   # }
 
   # Enable asynchronous reporting (using sucker_punch)

lib/rollbar.rb

@@ -31,7 +31,7 @@ module Rollbar
     Rack::Multipart::UploadedFile
   ].freeze
   PUBLIC_NOTIFIER_METHODS = %w(debug info warn warning error critical log logger
-                               process_payload scope send_failsafe log_info log_debug
+                               process_payload process_payload_safely scope send_failsafe log_info log_debug
                                log_warning log_error silenced)
 
   class Notifier
@@ -180,6 +180,15 @@ def process_payload(payload)
       else
         send_payload(payload)
       end
+    rescue => e
+      log_error("[Rollbar] Error processing the payload: #{e.class}, #{e.message}. Payload: #{payload.inspect}")
+      raise e
+    end
+
+    def process_payload_safely(payload)
+      process_payload(payload)
+    rescue => e
+      report_internal_error(e)
     end
 
     private

lib/rollbar/delay/girl_friday.rb

@@ -16,7 +16,7 @@ def queue_class
 
       def call(payload)
         self.class.queue = queue_class.new(nil, :size => 5) do |payload|
-          Rollbar.process_payload(payload)
+          Rollbar.process_payload_safely(payload)
         end
 
         self.class.queue.push(payload)

lib/rollbar/delay/resque.rb

@@ -23,7 +23,7 @@ def self.perform(payload)
         end
 
         def perform(payload)
-          Rollbar.process_payload(payload)
+          Rollbar.process_payload_safely(payload)
         end
       end
     end

lib/rollbar/delay/sidekiq.rb

@@ -16,7 +16,7 @@ def call(payload)
       include ::Sidekiq::Worker
 
       def perform(*args)
-        Rollbar.process_payload(*args)
+        Rollbar.process_payload_safely(*args)
       end
     end
   end

lib/rollbar/delay/sucker_punch.rb

@@ -11,7 +11,7 @@ def self.call(payload)
       end
 
       def perform(*args)
-        Rollbar.process_payload(*args)
+        Rollbar.process_payload_safely(*args)
       end
     end
   end

lib/rollbar/delay/thread.rb

@@ -6,7 +6,7 @@ def self.call(payload)
       end
 
       def call(payload)
-        ::Thread.new { Rollbar.process_payload(payload) }
+        ::Thread.new { Rollbar.process_payload_safely(payload) }
       end
     end
   end

lib/rollbar/truncation/frames_strategy.rb

@@ -15,7 +15,7 @@ def call(payload)
 
         if body['trace_chain']
           truncate_trace_chain(body)
-        else
+        elsif body['trace']
           truncate_trace(body)
         end
 

spec/delay/sidekiq_spec.rb

@@ -17,7 +17,7 @@ class Sidekiq
 
   describe "#perform" do
     it "performs payload" do
-      Rollbar.should_receive(:process_payload).with(payload)
+      Rollbar.should_receive(:process_payload_safely).with(payload)
       subject.perform payload
     end
   end
@@ -26,7 +26,7 @@ class Sidekiq
     shared_examples "a Rollbar processor" do
 
       it "processes payload" do
-        Rollbar.should_receive(:process_payload).with(payload)
+        Rollbar.should_receive(:process_payload_safely).with(payload)
 
         subject.call payload
         described_class.drain

spec/delay/sucker_punch_spec.rb

@@ -17,7 +17,7 @@ class SuckerPunch
     let(:payload) { "anything" }
 
     it "performs the task asynchronously" do
-      Rollbar.should_receive(:process_payload)
+      Rollbar.should_receive(:process_payload_safely)
 
       Rollbar::Delay::SuckerPunch.call payload
     end

spec/rollbar/delay/resque_spec.rb

@@ -14,7 +14,7 @@
     it 'process the payload' do
       loaded_hash = MultiJson.load(MultiJson.dump(payload))
 
-      expect(Rollbar).to receive(:process_payload).with(loaded_hash)
+      expect(Rollbar).to receive(:process_payload_safely).with(loaded_hash)
       described_class.call(payload)
     end
   end

spec/rollbar/delay/thread_spec.rb

@@ -5,7 +5,7 @@
     let(:payload) { { :key => 'value' } }
 
     it 'process the payload in a new thread' do
-      expect(Rollbar).to receive(:process_payload).with(payload)
+      expect(Rollbar).to receive(:process_payload_safely).with(payload)
 
       th = described_class.call(payload)
       th.join

spec/rollbar/truncation/frames_strategy_spec.rb

@@ -52,5 +52,19 @@ def expand_frames(frames)
         expect(new_frames2.last).to be_eql(frames2.last)
       end
     end
+
+    context 'without trace or trace_chain', :fixture => :payload do
+      let(:payload_fixture) { 'payloads/sample.trace.json' }
+
+      before do
+        payload['data']['body'].delete('trace')
+      end
+
+      it 'returns the original payload' do
+        result = MultiJson.load(described_class.call(payload))
+
+        expect(result).to be_eql(payload)
+      end
+    end
   end
 end

spec/rollbar_spec.rb

@@ -1547,6 +1547,33 @@ class DummyClass
     end
   end
 
+  describe '.process_payload' do
+    context 'if there is an exception sending the payload' do
+      let(:exception) { StandardError.new('error message') }
+      let(:payload) { { :foo => :bar } }
+
+      it 'logs the error and the payload' do
+        allow(Rollbar.notifier).to receive(:send_payload).and_raise(exception)
+        expect(Rollbar.notifier).to receive(:log_error)
+
+        expect { Rollbar.notifier.process_payload(payload) }.to raise_error(exception)
+      end
+    end
+  end
+
+  describe '.process_payload_safely' do
+    context 'with errors' do
+      let(:exception) { StandardError.new('the error') }
+
+      it 'doesnt raise anything and sends internal error' do
+        allow(Rollbar.notifier).to receive(:process_payload).and_raise(exception)
+        expect(Rollbar.notifier).to receive(:report_internal_error).with(exception)
+
+        Rollbar.notifier.process_payload_safely({})
+      end
+    end
+  end
+
   # configure with some basic params
   def configure
     reconfigure_notifier