Fix: Handle exception with large stacktrace without dropping entire item - by keeping N frames

[PikachuEXE]

Description

This should fix #1799

Certain exception type such as SystemStackError has long backtrace (thus the stack error)
The whole envelope item was dropped due to payload size limit logic

This ensures it tries to remove most of the stacktrace frames (except first 10) when payload is too large, so that the envelope item won't be dropped = exception still reported

This is an alternative to #1806

Note: 10 is a random number I picked so feel free to suggest other numbers

[st0012]

As mentioned in #1806 (comment), we should update the log message as well (see line 106).

[PikachuEXE]

What should it be updated to since this PR remove some frames only (or keeping some frames)

[PikachuEXE]

Is 10 a good enough number as no. of stacktrace frames to keep?
Edit: Let me know if I should setup a new config option about this
Not sure about the naming though

[PikachuEXE]

In case a new config option is desired
I have a draft implementation (See latest commit(s))

[PikachuEXE]

Please let me know early if you got any comment
I think we are in different time zones so far away that we can only communicate once per day

[st0012]

@PikachuEXE It's mainly because I don't work on this full-time 🙂
I don't plan to accept the new config for limiting frame size. It's a bit too far for the problem of #1799
And don't worry about the message I mentioned. I'll likely work on it later and if the current code looks good I'll merge it first.

[st0012]

@sl0thentr0py Do you think this will fit into enhancement or bug fix? I think it's kinda an enhancement but you can also argue that we should've handled the oversized frames.

[sl0thentr0py]

@st0012 I'd say improvements to truncation count as enhancement

[PikachuEXE]

OK no problem
Just let me know if any change needed
Any improvement is better than none (coz my project is not capturing that kind of error right now)

[sl0thentr0py]

@PikachuEXE I think 10 is way too conservative, taking an average stacktrace line like

  from activesupport (7.0.2.3) lib/active_support/notifications/instrumenter.rb:24:in `instrument'

the length is approximately 100 bytes, so even with an upper bound of 200 bytes, we can fit 1024 such lines into Event::MAX_SERIALIZED_PAYLOAD_SIZE = 1024 * 200.

I'd therefore pick something like 100 or even 500 for truncating the number of frames.
Also can you move this value into a constant variable?

[PikachuEXE]

Updated
I have also updated the frame truncating logic to keep first & last N/2 frames instead of just last N frames
This is due to having only last N frames would make debugging difficult for SystemStackError

First N/2 frames example (in real world exception should contain scripts / files that cause the SystemStackError

But in case of other errors or there are many frames at the beginning, last N/2 frames should be kept as well.

With this update the stacktrace would look like this on Sentry Event View

[PikachuEXE]

Here is my script for reproducing a SystemStackError

require "sentry-ruby"

# thingy.rb
class Thingy
  def thingy
    puts "thingy"
  end
end

# thingy_with_foo.rb
module ThingyWithFoo
  def thingy
    # puts "thingy with foo"
    super
  end
end

Thingy.prepend(ThingyWithFoo)

# thingy_with_bar.rb
class Thingy
  alias_method :thingy_without_bar, :thingy # Wont't alias create an alias for Thingy#thingy but ThingyWithFoo#thingy instead

  def thingy_with_bar
    # puts "thingy with bar"
    thingy_without_bar # Expected to call original Thingy#thingy method but will call prepended method instead
  end

  alias_method :thingy, :thingy_with_bar
end


::Sentry.init do |config|
  config.dsn = ""

  config.debug = true
  # config.logger = Logger.new(STDOUT, level: :debug)
end


begin
  Thingy.new.thingy # raises: stack level too deep (SystemStackError))
rescue SystemStackError => exception
  Sentry.capture_exception(exception)
  # puts exception.backtrace.size #=> 11911
  # puts exception.backtrace[-100..-1]
end

[sl0thentr0py]

thx @PikachuEXE, also cool observation with taking N/2 frames on both sides!
lgtm now

[st0012]

Let me cut a patch release for #1812 first then I'll start merging feature PRs

[PikachuEXE]

Please let me know when you area ready to merge this
I am aware of the CHANGELOG.md conflict but I don't want to fix it multiple times

[PikachuEXE]

Resolved conflict on CHANGELOG.md and also marked it as a feature instead of a fix

[st0012]

Thank you. I’ll give it another pass this week 🙂

[st0012]

Thanks for the great work 👍