Support capturing low-level errors propagated to Puma

[st0012]

This PR implements Puma's error reporting by patching Puma::Server#lowlevel_error.

I thought about using the lowlevel_error_handler API to add a Sentry error handler, but it's not possible to do that after puma.rb is loaded (which is extremely early in a normal app setup, way before the SDK is loaded). There is also no good ways to fetch and update the server's lowlevel_error_handler option. So the only option left is to patch the method.

Notes:

• The lowlevel_error patch is compatible with Puma 3.0+ (current latest is 6.1). So it's pretty stable and I'm not sure if it'd be worth introducing another test matrix for Puma versions.
• I think for patches like this it's better to NOT require the related library in spec_helper as the nonappearance of the library should also be tested. This is why I added the spec under isolated folder.
  • I plan to apply this test pattern to the redis patch in another PR.
• I also want to improve the Sentry.register_patch API as we now have 3 patches using it and I've spotted some common boilerplate code.

Closes #1511

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -15.40 ⚠️

Comparison is base (7fd714e) 98.59% compared to head (fc66bc9) 83.20%.

❗ Current head fc66bc9 differs from pull request most recent head c26bd01. Consider uploading reports for the commit c26bd01 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2026       +/-   ##
===========================================
- Coverage   98.59%   83.20%   -15.40%     
===========================================
  Files         159      119       -40     
  Lines       10499     5638     -4861     
===========================================
- Hits        10352     4691     -5661     
- Misses        147      947      +800     

Impacted Files	Coverage Δ
sentry-ruby/lib/sentry-ruby.rb	69.31% <100.00%> (-26.12%)	⬇️
sentry-ruby/lib/sentry/puma.rb	100.00% <100.00%> (ø)

... and 82 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[dentarg]

@st0012 @sl0thentr0py Is it possible to opt-out from this?

This makes Sentry by default catch the generally non-actionable Puma::HttpParserError exception :/

[dentarg]

How does this behave if you already use Sentry in the lowlevel_error_handler?

[st0012]

How does this behave if you already use Sentry in the lowlevel_error_handler?

I think it'll send 2 events in this case. So I'd advise removing the custom lowlevel_error_handler. Or do you have some custom context captured there too?

This makes Sentry by default catch the generally non-actionable Puma::HttpParserError exception

Sorry that I wasn't aware such exceptions would be captured too. Does adding it to config.excluded_exceptions help? If it does, I will add it to the default exclusion list.

[dentarg]

So I'd advise removing the custom lowlevel_error_handler

that's is not a great solution, that handler is also used to define the HTTP response you want you application to have

Does adding it to config.excluded_exceptions help?

I assume so (we have done that now) but I haven't verified it

[dentarg]

Can you consider raising a feature request on the Puma repository to facilitate doing this in a less intrusive way? (You could contribute the feature if we reach some good idea how to do it)

[st0012]

that's is not a great solution, that handler is also used to define the HTTP response you want you application to have

Sorry I'm not saying not using that handler, just not sending events to Sentry from it now that we have this integration.

[dentarg]

There's also HttpParserError501 and MiniSSL::SSLError: https://github.com/puma/puma/blob/fd259b7f1a64a505b694cd7cdef605a7de731611/lib/puma/server.rb#L494-L513

[shouichi]

We started to see Puma::HttpParserError after puma v6.2x. Can you consider a proper solution without monkey patching?

[st0012]

Apologies for the delayed response.

I believe the core issue here is that I failed to implement proper exception filtering along with the feature. I will address this in a subsequent patch.

facilitate doing this in a less intrusive way?

I'm open to creating a feature proposal for this, but I'm unclear about how a less intrusive API would function. Based on the comments in this thread, the problem isn't that the patch causes an infinite loop or interferes with other projects' patches, is it?

As for exception captures, wouldn't Puma::HttpParserError, HttpParserError501, and MiniSSL::SSLError show up in the users' lowlevel_error_handler proc if they configure one? In other words, if a user also invokes Sentry.capture_exception within their lowlevel_error_handler, won't they encounter a similar issue until they manually filter out those exceptions? According to the README, that seems to be a primary use case for the configuration?

[dentarg]

I'm open to creating a feature proposal for this, but I'm unclear about how a less intrusive API would function. Based on the comments in this thread, the problem isn't that the patch causes an infinite loop or interferes with other projects' patches, is it?

It is not easy to opt-out of the monkey patch (even impossible perhaps?)

As for exception captures, wouldn't Puma::HttpParserError, HttpParserError501, and MiniSSL::SSLError show up in the users' lowlevel_error_handler proc if they configure one? In other words, if a user also invokes Sentry.capture_exception within their lowlevel_error_handler, won't they encounter a similar issue until they manually filter out those exceptions? According to the README, that seems to be a primary use case for the configuration?

Yes, but if the user have a low level handler invoking Sentry.capture_exception they have probably already added the exceptions they want to exclude, to the exclude list.

Yeah, the problem was rolling this out as opt-in by default without any exclusions. You could have made it opt-in but I guess you thought the adoption of the feature would have been slow then.

[st0012]

Yeah so if the problem is exception filtering, I'm sorry for not including it in the first place.

However, given that

• Introducing new APIs in puma won't replace the ignoring list in Ignore low-level Puma exceptions by default #2055
• We'll still need to keep the monkey-patch for older puma versions

I don't plan to initiate a new API for this feature. But if you do plan to propose one, I'd love to participate the discussion and become early adopters 😄