Sentry has RuntimeError when not enabled in current environment

[henrahmagix]

Issue Description

When running Capybara tests, and an app request raises an exception when my system hits its ulimit from using SecureRandom, Sentry also causes the same error when capturing an app exception and wrapping it, here:

sentry-ruby/sentry-ruby/lib/sentry/event.rb

Line 48 in 6d312f8

@event_id = SecureRandom.uuid.delete("-")

What happens then is my test failure trace is for the Sentry capture_exception trace, not the original app exception.

It was very confusing because test is not in my config.enabled_environments array.

Reproduction Steps

• Set Sentry config.enabled_environments to an array of some environment names
• Make sure SecureRandom raises an error – I don't know how to cause this, maybe it can be mocked? Or the underlying system call can be mocked somehow? I can 75% reliably reproduce this on my current system (on which ulimit -n retunrs 256) by running 15 feature tests that contain ~20 app requests each.
  • or make something in Sentry's code raise an error, and make your rack app raise any error, so this line captures an exception:

    sentry-ruby/sentry-ruby/lib/sentry/rack/capture_exceptions.rb

    Line 16 in 6d312f8

    Sentry.with_scope do |scope|

• Run your app in an environment that's not in config.enabled_environments, e.g. in a Rails app set config.enabled_environments = ['production'] and run the app in RAILS_ENV=test
• Make an app request to hit the app code that will raise an error
• Check where app request exceptions are logged
• See that the trace is for the error raised in Sentry, not the app error

Expected Behavior

Sentry code does not try to capture exceptions for an app who's environment is not in config.enabled_environments.

Actual Behavior

Sentry runs capture_exception and tries to create an event for it, and raises an error of its own, replacing the app exception trace with its own.

Ruby Version

2.6.9

SDK Version

4.7.3

Integration and Its Version

Rails 6.0.4.1

Sentry Config

# config/initializers/sentry.rb
Sentry.init do |config|
  config.dsn = Rails.configuration.sentry.fetch(:sentry_dsn)
  config.enabled_environments = %w[production staging]
end

[henrahmagix]

Sorry I raised the pull-request first because I've been playing around with the code locally, then I reported this issue =) See #1682

[henrahmagix]

Also just to be clear on this, my issue isn't with the SecureRandom error (that's extensively covered in https://bugs.ruby-lang.org/issues/14716 and I don't intend to ask Sentry to solve it for me).

It's just that that's my example here for an error that, when raised in the app, also occurs in Sentry when trying to wrap the exception.

Ideally, as #1682 does, Sentry doesn't do anything when it's not enabled for the environment =)

[st0012]

@henrahmagix Hey sorry for the issue. But bypassing the entire middleware isn't the right fix for the issue and can cause other issues.

The middleware is also responsible for providing a clean scope for the rest of the request (like controller actions). And as long as the SDK is initialized, that needs to be guaranteed as users may have other code depends on it.

I think the actual problem is that the SDK's judgement for event reporting happens a bit late. Currently it's like this:

1. Sentry.capture_* is called.
2. Sentry::Client#event_from_* generates event objects.
3. Sentry::Client#capture_event checks if the event should be reported.

Although step 2 isn't necessary, it didn't cause any issue before. But unfortunately that's not the case for your app 😓

So I think a better solution is to move the check from step 3 to step 1.

[henrahmagix]

Amazing, thanks so much for finding a better fix! Much appreciated ☺️