Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ECHO-1987] Add a Sentry error handler #68

Merged
merged 6 commits into from
Feb 23, 2024
Merged

[ECHO-1987] Add a Sentry error handler #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89e4e20
[ECHO-1987] Add Sentry error handler
AdamMarkowski Feb 7, 2024
f355983
[ECHO-1987] Remove leftover
AdamMarkowski Feb 7, 2024
cd502c2
[ECHO-1987] Polish code
AdamMarkowski Feb 7, 2024
358a076
[ECHO-1987] CR remarks
AdamMarkowski Feb 19, 2024
4a79b05
[ECHO-1987] Bump up gem version
AdamMarkowski Feb 22, 2024
cd9938b
[ECHO-1987] Update Gemfile.lock
AdamMarkowski Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
eventboss (1.8.0)
eventboss (1.8.1)
aws-sdk-sns (>= 1.1.0)
aws-sdk-sqs (>= 1.3.0)
dotenv (~> 2.1, >= 2.1.1)
Expand All @@ -10,26 +10,26 @@ PATH
GEM
remote: https://rubygems.org/
specs:
aws-eventstream (1.2.0)
aws-partitions (1.568.0)
aws-sdk-core (3.130.0)
aws-eventstream (~> 1, >= 1.0.2)
aws-partitions (~> 1, >= 1.525.0)
aws-sigv4 (~> 1.1)
jmespath (~> 1.0)
aws-sdk-sns (1.53.0)
aws-sdk-core (~> 3, >= 3.127.0)
aws-eventstream (1.3.0)
aws-partitions (1.894.0)
aws-sdk-core (3.191.3)
aws-eventstream (~> 1, >= 1.3.0)
aws-partitions (~> 1, >= 1.651.0)
aws-sigv4 (~> 1.8)
jmespath (~> 1, >= 1.6.1)
aws-sdk-sns (1.72.0)
aws-sdk-core (~> 3, >= 3.191.0)
aws-sigv4 (~> 1.1)
aws-sdk-sqs (1.51.0)
aws-sdk-core (~> 3, >= 3.127.0)
aws-sdk-sqs (1.70.0)
aws-sdk-core (~> 3, >= 3.191.0)
aws-sigv4 (~> 1.1)
aws-sigv4 (1.4.0)
aws-sigv4 (1.8.0)
aws-eventstream (~> 1, >= 1.0.2)
diff-lcs (1.5.0)
dotenv (2.7.6)
jmespath (1.6.1)
dotenv (2.8.1)
jmespath (1.6.2)
rake (13.0.6)
rexml (3.2.5)
rexml (3.2.6)
rspec (3.11.0)
rspec-core (~> 3.11.0)
rspec-expectations (~> 3.11.0)
Expand Down
17 changes: 17 additions & 0 deletions lib/eventboss/error_handlers/sentry.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
module Eventboss
module ErrorHandlers
class Sentry
def call(exception, context = {})
eventboss_context = { component: 'eventboss' }
eventboss_context[:action] = context[:processor].class.to_s if context[:processor]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's copied since airbrake. It's not beneficial as the information is in processor param.


::Sentry.with_scope do |scope|
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use Sentry in our gem then we should probably mark this as a project's dependency (eventboss.gemspec)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, it's optional plugin, which makes Sentry will be required only when it's added in configuration.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still is a dependency on the gem - it uses Sentry API that is valid for the specific range of Sentry versions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but what if I use Rollbar, not a Sentry? My whole project would have Sentry downloaded and required when then. As this is plugin, we don't force you to that feature. Unfortunately, as gem creators, we don't have options for checking the dependency, so you should take care about it by yourself. In that case you probably have it in your decencies, because you have to have it configured. Only version validation is still missing.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this that way - Sentry is clearly dependency here. Its compatibility needs to be ensured somewhere - either in the client app or the gem side.

I see no harm that user needs to download all the gems related with our plugins if we decided to keep them within the repo - it's still better that leave things unsecured in hope that nothing will blow up :)

We can still create some other gems like eventboss-extensions where all the plugins would be downloaded if the user really wants to. We could provide event greater granularity by exposing things likeeventboss-rollbar etc.

The third option would be to inform users that there's a hook for custom error handlers so that you can attach one if you want - in respect to your Gemfile.

Copy link
Contributor

@rapsad rapsad Feb 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not a place for this Eventboss::ErrorHandlers::Sentry:

  • This class introduces external dependancy Sentry and it's unnecessary:
    What if I'm using rollbar? Then I wouldn't need to load additional gem.
  • Scenario when I'm having different implementation and I don't want to use this one - unnecessary require.

What I would suggest is to just simply pass lambda to error_handlers in Eventboss initializer:

Eventboss.configure do |config|
  config.eventboss_app_name = ['EVENTBUS_APP_NAME']
  config.logger = Rails.logger
  config.error_handlers << lambda do |exception, context = {}|
    # code
  end
  config.log_level = Rails.configuration.log_level
end

And dependency to Sentry only lives in your application

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that anyway by default these files from error_handlers are not loaded. So far nor Rollbar neither AirBrake gems were not marked as eventboss dependency.

As eventboss user I would like to delegate anything to this lib. With lambda approach every client app would need to get dig into what can be in context which basically requires digging deep into eventboss, sqs etc.

Copy link
Contributor

@rapsad rapsad Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As eventboss user I would like to delegate anything to this lib. With lambda approach every client app would need to get dig into what can be in context which basically requires digging deep into eventboss, sqs etc.

That's fair point! I think we can proceed with class like this 👍

scope.set_tags(
context.merge(eventboss_context)
)
::Sentry.capture_exception(exception)
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/eventboss/extensions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'eventboss/error_handlers/logger'
require 'eventboss/error_handlers/airbrake'
require 'eventboss/error_handlers/rollbar'
require 'eventboss/error_handlers/sentry'
require 'eventboss/error_handlers/db_connection_drop_handler'
require 'eventboss/error_handlers/db_connection_not_established_handler'
require 'eventboss/error_handlers/non_existent_queue_handler'
2 changes: 1 addition & 1 deletion lib/eventboss/version.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module Eventboss
VERSION = "1.8.0"
VERSION = "1.8.1"
end
22 changes: 22 additions & 0 deletions spec/eventboss/error_handlers/sentry_handler_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
require 'spec_helper'

describe Eventboss::ErrorHandlers::Sentry do
module Sentry; end

subject { described_class.new }

let(:some_error) { double(class: StandardError) }
let(:sentry_with_scope_mock) { double(class: ::Sentry) }

context 'when receiving some exception' do

it 'calls Sentry.capture_exception' do
expect(::Sentry).to receive(:with_scope).and_yield(sentry_with_scope_mock)

expect(sentry_with_scope_mock).to receive(:set_tags).with(hash_including({ component: 'eventboss' }))
expect(::Sentry).to receive(:capture_exception).with(some_error)

subject.call(some_error)
end
end
end
Loading