WIP: Up limits on payload lengths, more focussed trimming #431

Cawllec · 2018-02-21T14:47:27Z

Changes how payload trimming is implemented to favour keeping longer stacktraces in place rather than limiting them to 40 items.

Namely:

Ups max payload length to 512000 bytes (~0.5 Mb)
Ups max metadata array length to 80
Ups max other array length (namely stacktraces) to 160
Changes order of payload trimming to:
1. Trimming strings to max length (3072)
2. Trimming metadata arrays to max length (80)
3. Trimming all other arrays (including stacktraces) to max length (160)
4. Removing metadata from the payload
5. Remove :code from stacktrace items, oldest first, until within limits
6. Remove stacktrace items, oldest first, until within limits

WIP:

I need to write performance tests to compare with the older version to see if extra performance time is within acceptable limits
Review of how iteration is performed over payload to ensure performing optimally
Review of tests to ensure all cases are covered
Refactoring of more complex functions to be more readable will probably be required

kattrali

This is a fairly complex solution to the problem and expensive for what it does. Serializing the payload is relatively costly and shouldn't be done for every stacktrace line, for example. Added more inline.

kattrali · 2018-02-22T14:48:04Z

lib/bugsnag/stacktrace.rb

@@ -1,3 +1,5 @@
+require 'pp'


Is this from debugging?

Yes, I'll remove that.

kattrali · 2018-02-22T14:48:48Z

spec/helper_spec.rb

@@ -171,6 +172,111 @@ def to_s
          expect(trimmed_value[:events].first[:metaData]).to be_nil
        end
      end
+
+      context "and new trimming priorities are in place" do


This shouldn't be a new context because no conditions are being changed.

It's mostly a readability context for me at the moment before the tests are concreted.

kattrali · 2018-02-22T14:50:27Z

lib/bugsnag/helpers.rb

+      return payload unless payload.is_a?(Hash) and payload[:events].respond_to?(:map)
+      payload[:events].map do |event|
+        event[:exceptions].map do |exception|
+          initial_size = get_payload_length(exception[:stacktrace])


just do exception[:stacktrace].each and pop the code from each entry. No need for all of these checks and breaks or using downto.

kattrali · 2018-02-22T14:50:59Z

lib/bugsnag/helpers.rb

+          initial_size = get_payload_length(exception[:stacktrace])
+          (exception[:stacktrace].length - 1).downto(0).each do |i|
+            break unless (initial_size - get_payload_length(exception[:stacktrace])) < threshold
+            exception[:stacktrace][i].delete(:code) if exception[:stacktrace][i].include?(:code)


No need to check for inclusion before deletion.

kattrali · 2018-02-22T14:51:13Z

lib/bugsnag/helpers.rb

+    def self.trim_stacktrace_functions(payload, threshold)
+      return payload unless payload.is_a?(Hash) and payload[:events].respond_to?(:map)
+      payload[:events].map do |event|
+        event[:exceptions].map do |exception|


Same changes as above ☝️

snmaynard · 2018-02-24T19:27:52Z

Can you ping me on this when it's ready for a second review.

…ion data - Remove unnecessary trimming and reduce number of Stringifications

…ion data - Remove unnecessary tests, ensure tests are setup correctly

Cawllec · 2018-05-02T10:21:02Z

I've updated this to more closely match the original spec for trimming payloads.
Logic is as stated in the ticket:

Bump the payload limit to 512kb
in the case where the payload is too large try to shorten metadata
if that doesnt work enough strip the code out from the oldest called functions.
If that doesnt work strip out all the code
then shorten the stacktrace.
There are a number of places where efficiencies could be made, such as block deletion of older stacktrace entries if it's too long, but I think it's worth another review.
Would you mind taking another look @snmaynard @kattrali ?

…ion data - Refactor exception unpacking into helper function - Use each instead of map as result isn't utilised

kattrali · 2018-05-08T23:09:37Z

lib/bugsnag/helpers.rb

@@ -60,21 +70,74 @@ def self.deep_merge!(l_hash, r_hash)

    TRUNCATION_INFO = '[TRUNCATED]'

+    ##
+    # Trim stacktrace code out if they're too large, oldest functions first


Rather than checking the length for every line, just delete the code from every frame.

- # Trim stacktrace code out if they're too large, oldest functions first + # Remove all code from stacktraces

kattrali · 2018-05-15T23:52:32Z

lib/bugsnag/helpers.rb

    # Shorten array until it fits within the payload size limit when serialized
-    def self.truncate_array(array)
+    def self.truncate_array(array, limit = MAX_ARRAY_LENGTH)


If limit is just a constant value everywhere and never modified, just use it directly instead of passing it from one method to the next.

Cawllec added 7 commits February 8, 2018 09:16

Added premliminary of metadata trimming

bf06ead

More efficient and effective trimming

4b65733

Fixed issue with arguments

2435786

Tuned trace trimming

d31a56f

Merge branch 'master' into cawllec/payload-trimmer-redux

64299c6

Rubocop fixes

ff3de4f

Correct break statements

72b6fb6

Cawllec requested review from kattrali and snmaynard February 21, 2018 14:47

kattrali suggested changes Feb 22, 2018

View reviewed changes

Cawllec and others added 7 commits February 27, 2018 15:45

Simplified loops somewhat, realigned tests

e595c3c

Simplified stacktrace trimming to require less conversions

33d60bb

Turned 'each' loops into 'while' loops

c758ccd

Merge branch 'master' into cawllec/payload-trimmer-redux

932794e

feature(Trimming): Change trimming logic to intelligently trim except…

72296b3

…ion data - Remove unnecessary trimming and reduce number of Stringifications

feature(Trimming): Change trimming logic to intelligently trim except…

19a21d8

…ion data - Remove unnecessary tests, ensure tests are setup correctly

Merge branch 'master' into cawllec/payload-trimmer-redux

1bc2d63

Cawllec and others added 4 commits May 2, 2018 14:25

feature(Trimming): Change trimming logic to intelligently trim except…

457d8b3

…ion data - Refactor exception unpacking into helper function - Use each instead of map as result isn't utilised

tests(Rack): Fix mocked rack constants

c43785f

Merge branch 'master' into cawllec/payload-trimmer-redux

4030ba1

Merge branch 'master' into next

53f8443

Cawllec changed the base branch from master to next May 11, 2018 14:23

Merge branch 'next' into cawllec/payload-trimmer-redux

46e0dac

kattrali suggested changes May 15, 2018

View reviewed changes

kattrali force-pushed the next branch from 9480165 to 8bf6647 Compare May 17, 2018 21:07

kattrali added 3 commits June 1, 2018 17:19

Simplify trimming, set limits up front instead of repeated checks

1717eae

Trim unused parameters

2cfc47c

Drop metadata if needed

8662014

kattrali force-pushed the cawllec/payload-trimmer-redux branch from 34e9c35 to 8662014 Compare June 2, 2018 00:20

kattrali approved these changes Jun 2, 2018

View reviewed changes

Merge branch 'next' into cawllec/payload-trimmer-redux

90b517b

kattrali merged commit ae02477 into next Jun 2, 2018

kattrali deleted the cawllec/payload-trimmer-redux branch June 2, 2018 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Up limits on payload lengths, more focussed trimming #431

WIP: Up limits on payload lengths, more focussed trimming #431

Cawllec commented Feb 21, 2018 •

edited

Loading

kattrali left a comment

kattrali Feb 22, 2018

Cawllec Feb 22, 2018

kattrali Feb 22, 2018

Cawllec Feb 22, 2018

kattrali Feb 22, 2018

Cawllec Feb 22, 2018

kattrali Feb 22, 2018

kattrali Feb 22, 2018

snmaynard commented Feb 24, 2018

Cawllec commented May 2, 2018 •

edited

Loading

kattrali May 8, 2018

kattrali May 15, 2018

WIP: Up limits on payload lengths, more focussed trimming #431

WIP: Up limits on payload lengths, more focussed trimming #431

Conversation

Cawllec commented Feb 21, 2018 • edited Loading

kattrali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snmaynard commented Feb 24, 2018

Cawllec commented May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cawllec commented Feb 21, 2018 •

edited

Loading

Cawllec commented May 2, 2018 •

edited

Loading