Better locals data, and truncation improvements

lib/rollbar/item/frame.rb

@@ -97,6 +97,8 @@ def context_data(file_lines, lineno)
       end
 
       def locals_data(filename, lineno)
+        return unless configuration.locals[:enabled]
+
         ::Rollbar::Item::Locals.locals_for_location(filename, lineno)
       end
 

lib/rollbar/item/locals.rb

@@ -1,5 +1,6 @@
 require 'rollbar/notifier'
 require 'rollbar/scrubbers/params'
+require 'rollbar/util'
 
 module Rollbar
   class Item
@@ -39,8 +40,51 @@ def locals_for(frame)
           end
         end
 
+        # Prepare objects to be handled by the payload serializer.
+        #
+        # Hashes and Arrays are traversed. Then all types execpt strings and
+        # immediates are exported using #inspect. Sending the object itself to the
+        # serializer can result in large recursive expansions, especially in Rails
+        # environments with ActiveRecord, ActiveSupport, etc. on the stack.
+        # Other export options could be #to_s, #to_h, and #as_json. Several of these
+        # will omit the class name, or are not implemented for many types.
+        #
+        # #inspect has the advantage that it is specifically intended for debugging
+        # output. If the user wants more or different information in the payload
+        # about a specific type, #inspect is the correct place to implement it.
+        # Likewise the default implementation should be oriented toward usefulness
+        # in debugging.
+        #
+        # Because #inspect outputs a string, it can be handled well by the string
+        # truncation strategy for large payloads.
+        #
         def prepare_value(value)
-          value.to_s
+          return simple_value?(value) ? value : value.inspect unless value.is_a?(Hash) || value.is_a?(Array)
+
+          cloned_value = ::Rollbar::Util.deep_copy(value)
+          ::Rollbar::Util.iterate_and_update_with_block(cloned_value) do |v|
+            simple_value?(v) ? v : v.inspect
+          end
+
+          cloned_value
+        end
+
+        def simple_classes
+          if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.4.0')
+            [String, Symbol, Integer, Float, TrueClass, FalseClass, NilClass]
+          else
+            [String, Symbol, Fixnum, Bignum, Float, TrueClass, FalseClass, NilClass] # rubocop:disable Lint/UnifiedInteger
+          end
+        end
+
+        def simple_value?(value)
+          simple_classes.each do |klass|
+            # Use instance_of? so that subclasses and module containers will
+            # be treated like complex object types, not simple values.
+            return true if value.instance_of?(klass)
+          end
+
+          false
         end
 
         def scrub(hash)

lib/rollbar/truncation/frames_strategy.rb

@@ -11,7 +11,7 @@ def self.call(payload)
       end
 
       def call(payload)
-        new_payload = Rollbar::Util.deep_copy(payload)
+        new_payload = payload
         body = new_payload['data']['body']
 
         if body['trace_chain']

lib/rollbar/truncation/mixin.rb

@@ -9,7 +9,7 @@ def truncate?(result)
         result.bytesize > MAX_PAYLOAD_SIZE
       end
 
-      def select_frames(frames, range = 150)
+      def select_frames(frames, range = 50)
         return frames unless frames.count > range * 2
 
         frames[0, range] + frames[-range, range]

lib/rollbar/truncation/strings_strategy.rb

@@ -6,7 +6,7 @@ module Truncation
     class StringsStrategy
       include ::Rollbar::Truncation::Mixin
 
-      STRING_THRESHOLDS = [1024, 512, 256].freeze
+      STRING_THRESHOLDS = [1024, 512, 256, 128].freeze
 
       def self.call(payload)
         new.call(payload)
@@ -29,7 +29,9 @@ def call(payload)
 
       def truncate_strings_proc(threshold)
         proc do |value|
-          if value.is_a?(String) && value.bytesize > threshold
+          # Rollbar::Util.truncate will operate on characters, not bytes,
+          # so use value.length, not bytesize.
+          if value.is_a?(String) && value.length > threshold
             Rollbar::Util.truncate(value, threshold)
           else
             value

lib/rollbar/util.rb

@@ -2,6 +2,10 @@
 
 module Rollbar
   module Util # :nodoc:
+    def self.iterate_and_update_with_block(obj, &block)
+      iterate_and_update(obj, block)
+    end
+
     def self.iterate_and_update(obj, block, seen = {})
       return if obj.frozen? || seen[obj.object_id]
 

spec/controllers/home_controller_spec.rb

@@ -301,18 +301,18 @@ def wrap_process_args(*args)
           {
             :obj => 'Post',
             :password => '******',
-            :hash => "{:foo=>Post, :bar=>\"bar\"}", # rubocop:disable Style/StringLiterals
+            :hash => {:foo => 'Post', :bar => 'bar'},
             :foo => 'Post',
-            :_index => '0'
+            :_index => 0
           },
           {
-            :foo => 'Post', :_index => '0'
+            :foo => 'Post', :_index => 0
           },
           {
-            :foo => 'Post', :_index => '0'
+            :foo => 'Post', :_index => 0
           },
           {
-            :foo => 'Post', :index => '0'
+            :foo => 'Post', :index => 0
           },
           {
             :foo => 'Post'
@@ -348,7 +348,7 @@ def wrap_process_args(*args)
         logger_mock.should_receive(:info).with('[Rollbar] Success').once
 
         expect { get '/cause_exception_with_locals' }.to raise_exception(NoMethodError)
-        expect(Rollbar.last_report[:body][:trace][:frames][-1][:locals]).to be_eql({})
+        expect(Rollbar.last_report[:body][:trace][:frames][-1][:locals]).to be_eql(nil)
       end
     end
   end

spec/rollbar/item/frame_spec.rb

@@ -61,6 +61,7 @@
         let(:configuration) do
           double('configuration',
                  :send_extra_frame_data => :none,
+                 :locals => {},
                  :root => '/var/www')
         end
 
@@ -79,6 +80,7 @@
         let(:configuration) do
           double('configuration',
                  :send_extra_frame_data => :all,
+                 :locals => {},
                  :root => '/var/www')
         end
 
@@ -92,7 +94,7 @@
               :pre => %w[foo3 foo4 foo5 foo6],
               :post => %w[foo8 foo9 foo10 foo11]
             },
-            :locals => {}
+            :locals => nil
           }
 
           expect(subject.to_h).to be_eql(expected_result)
@@ -135,6 +137,7 @@
           let(:configuration) do
             double('configuration',
                    :send_extra_frame_data => :app,
+                   :locals => {},
                    :root => '/outside/project',
                    :project_gem_paths => [])
           end
@@ -154,6 +157,7 @@
           let(:configuration) do
             double('configuration',
                    :send_extra_frame_data => :app,
+                   :locals => {},
                    :root => '/var/outside/',
                    :project_gem_paths => ['/var/www/'])
           end
@@ -168,7 +172,7 @@
                 :pre => %w[foo3 foo4 foo5 foo6],
                 :post => %w[foo8 foo9 foo10 foo11]
               },
-              :locals => {}
+              :locals => nil
             }
 
             expect(subject.to_h).to be_eql(expected_result)
@@ -179,6 +183,7 @@
           let(:configuration) do
             double('configuration',
                    :send_extra_frame_data => :app,
+                   :locals => {},
                    :root => '/var/www',
                    :project_gem_paths => [])
           end
@@ -193,7 +198,7 @@
                 :pre => %w[foo3 foo4 foo5 foo6],
                 :post => %w[foo8 foo9 foo10 foo11]
               },
-              :locals => {}
+              :locals => nil
             }
 
             expect(subject.to_h).to be_eql(expected_result)
@@ -203,6 +208,7 @@
             let(:configuration) do
               double('configuration',
                      :send_extra_frame_data => :app,
+                     :locals => {},
                      :root => '/var/www/',
                      :project_gem_paths => [])
             end
@@ -237,7 +243,7 @@
                   :pre => %w[foo1 foo2],
                   :post => %w[foo4 foo5 foo6 foo7]
                 },
-                :locals => {}
+                :locals => nil
               }
 
               expect(subject.to_h).to be_eql(expected_result)
@@ -259,7 +265,7 @@
                   :pre => %w[foo7 foo8 foo9 foo10],
                   :post => %w[foo12 foo13]
                 },
-                :locals => {}
+                :locals => nil
               }
 
               expect(subject.to_h).to be_eql(expected_result)

spec/rollbar/middleware/sinatra_spec.rb

@@ -245,18 +245,18 @@ def app
             {
               :obj => 'Post',
               :bar => 'bar',
-              :hash => "{:foo=>Post, :bar=>\"bar\"}", # rubocop:disable Style/StringLiterals
+              :hash => {:foo => 'Post', :bar => 'bar'},
               :foo => 'Post',
-              :_index => '0'
+              :_index => 0
             },
             {
-              :foo => 'Post', :_index => '0'
+              :foo => 'Post', :_index => 0
             },
             {
-              :foo => 'Post', :_index => '0'
+              :foo => 'Post', :_index => 0
             },
             {
-              :foo => 'Post', :index => '0'
+              :foo => 'Post', :index => 0
             },
             {
               :foo => 'Post'
@@ -292,7 +292,7 @@ def app
           logger_mock.should_receive(:info).with('[Rollbar] Success').once
 
           expect { get '/cause_exception_with_locals' }.to raise_exception(NoMethodError)
-          expect(Rollbar.last_report[:body][:trace][:frames][-1][:locals]).to be_eql({})
+          expect(Rollbar.last_report[:body][:trace][:frames][-1][:locals]).to be_eql(nil)
         end
       end
     end

spec/rollbar/truncation/frames_strategy_spec.rb

@@ -15,12 +15,12 @@ def expand_frames(frames)
         payload['data']['body']['trace']['frames'] = expand_frames(frames)
       end
 
-      it 'returns a new payload with 300 frames' do
+      it 'returns a new payload with 100 frames' do
         result = Rollbar::JSON.load(described_class.call(payload))
 
         new_frames = result['data']['body']['trace']['frames']
 
-        expect(new_frames.count).to be_eql(300)
+        expect(new_frames.count).to be_eql(100)
         expect(new_frames.first).to be_eql(frames.first)
         expect(new_frames.last).to be_eql(frames.last)
       end
@@ -37,17 +37,17 @@ def expand_frames(frames)
         payload['data']['body']['trace_chain'][1]['frames'] = expand_frames(frames2)
       end
 
-      it 'returns a new payload with 300 frames for each chain item' do
+      it 'returns a new payload with 100 frames for each chain item' do
         result = Rollbar::JSON.load(described_class.call(payload))
 
         new_frames1 = result['data']['body']['trace_chain'][0]['frames']
         new_frames2 = result['data']['body']['trace_chain'][1]['frames']
 
-        expect(new_frames1.count).to be_eql(300)
+        expect(new_frames1.count).to be_eql(100)
         expect(new_frames1.first).to be_eql(frames1.first)
         expect(new_frames1.last).to be_eql(frames1.last)
 
-        expect(new_frames2.count).to be_eql(300)
+        expect(new_frames2.count).to be_eql(100)
         expect(new_frames2.first).to be_eql(frames2.first)
         expect(new_frames2.last).to be_eql(frames2.last)
       end