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

AdamMarkowski · 2024-02-07T08:10:02Z

Jira: https://airhelp.atlassian.net/browse/ECHO-1987

To make the Sentry integration easier for applications leveraging Eventboss.

nnnande · 2024-02-15T11:47:37Z

lib/eventboss/error_handlers/sentry.rb

+
+        default_options = { }
+
+        ::Sentry.with_scope do |scope|


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

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

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

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.

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.

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

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.

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 👍

nnnande · 2024-02-15T11:48:50Z

lib/eventboss/error_handlers/sentry.rb

+
+        ::Sentry.with_scope do |scope|
+          scope.set_tags(
+            context.merge(eventboss_context, default_options)


I would remove default_options as it adds no value to merge an empty Hash :)

True, I would rather do this:

Suggested change

context.merge(eventboss_context, default_options)

eventboss_context.merge(context)

But let's do like this to have the same way as it was in rollbar:

Suggested change

context.merge(eventboss_context, default_options)

context.merge(eventboss_context)

Applied 👍

nnnande · 2024-02-15T11:58:39Z

lib/eventboss/error_handlers/sentry.rb

+          scope.set_tags(
+            context.merge(eventboss_context, default_options)
+          )


@AdamMarkowski What do you think about segregating the event data this way:

Suggested change

scope.set_tags(

context.merge(eventboss_context, default_options)

)

scope.set_tags(eventboss_context)

scope.set_context('context', context)

)

Tags - As they are both indexed we will be able to filter and search Sentry issues by the tags.
Context - We cannot search them, but they are viewable on the Sentry issue page.

I think that tags fits here, as they are :worker_id, :pooler_id and :processor

@nnnande I chose set_tags here because we have the ability to search through them, as opposed to context where we cannot - is it ok for you?

Sure, sounds reasonable - being able to filter stuff by tags will help with no doubts @AdamMarkowski

ahpawel · 2024-02-15T11:57:07Z

lib/eventboss/error_handlers/sentry.rb

+
+        default_options = { }
+
+        ::Sentry.with_scope do |scope|


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

ahpawel · 2024-02-15T11:57:59Z

lib/eventboss/error_handlers/sentry.rb

+    class Sentry
+      def call(exception, context = {})
+        eventboss_context = { component: 'eventboss' }
+        eventboss_context[:action] = context[:processor].class.to_s if context[:processor]


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

ahpawel · 2024-02-15T11:58:10Z

lib/eventboss/error_handlers/sentry.rb

+        eventboss_context = { component: 'eventboss' }
+        eventboss_context[:action] = context[:processor].class.to_s if context[:processor]
+
+        default_options = { }


copy pasta :)

ahpawel · 2024-02-15T12:01:56Z

lib/eventboss/error_handlers/sentry.rb

+
+        ::Sentry.with_scope do |scope|
+          scope.set_tags(
+            context.merge(eventboss_context, default_options)


True, I would rather do this:

Suggested change

context.merge(eventboss_context, default_options)

eventboss_context.merge(context)

But let's do like this to have the same way as it was in rollbar:

Suggested change

context.merge(eventboss_context, default_options)

context.merge(eventboss_context)

nglx · 2024-02-28T16:30:27Z

@AdamMarkowski @wojtih @rapsad it's released:
https://rubygems.org/gems/eventboss/versions/1.8.1

bawcie sie i cieszcie razem ze mna

444

AdamMarkowski added 3 commits February 7, 2024 09:07

[ECHO-1987] Add Sentry error handler

89e4e20

[ECHO-1987] Remove leftover

f355983

[ECHO-1987] Polish code

cd502c2

AdamMarkowski marked this pull request as ready for review February 14, 2024 14:54

AdamMarkowski requested review from a team, rapsad and air-forkbomb and removed request for a team February 14, 2024 14:54

nnnande reviewed Feb 15, 2024

View reviewed changes

ahpawel previously approved these changes Feb 15, 2024

View reviewed changes

[ECHO-1987] CR remarks

358a076

AdamMarkowski dismissed ahpawel’s stale review via 358a076 February 19, 2024 12:07

AdamMarkowski requested review from nnnande and ahpawel February 19, 2024 12:09

nnnande previously approved these changes Feb 19, 2024

View reviewed changes

carlagomesah previously approved these changes Feb 20, 2024

View reviewed changes

rapsad previously approved these changes Feb 22, 2024

View reviewed changes

[ECHO-1987] Bump up gem version

4a79b05

AdamMarkowski dismissed stale reviews from rapsad, carlagomesah, and nnnande via 4a79b05 February 22, 2024 10:28

AdamMarkowski requested review from carlagomesah, nnnande, rapsad and seban February 22, 2024 10:32

[ECHO-1987] Update Gemfile.lock

cd9938b

rapsad approved these changes Feb 23, 2024

View reviewed changes

michallurban approved these changes Feb 23, 2024

View reviewed changes

wojtih approved these changes Feb 23, 2024

View reviewed changes

rapsad merged commit d8a5927 into master Feb 23, 2024
2 checks passed

rapsad deleted the ECHO-1987 branch February 23, 2024 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AdamMarkowski commented Feb 7, 2024

nnnande Feb 15, 2024

ahpawel Feb 15, 2024

nnnande Feb 15, 2024

ahpawel Feb 15, 2024

nnnande Feb 15, 2024

rapsad Feb 19, 2024 •

edited

Loading

seban Feb 20, 2024

rapsad Feb 22, 2024 •

edited

Loading

nnnande Feb 15, 2024

ahpawel Feb 15, 2024

AdamMarkowski Feb 19, 2024

nnnande Feb 15, 2024

ahpawel Feb 15, 2024

AdamMarkowski Feb 19, 2024

nnnande Feb 19, 2024

ahpawel Feb 15, 2024

ahpawel Feb 15, 2024

ahpawel Feb 15, 2024

ahpawel Feb 15, 2024

nglx commented Feb 28, 2024 •

edited

Loading

	context.merge(eventboss_context, default_options)
	eventboss_context.merge(context)

	context.merge(eventboss_context, default_options)
	context.merge(eventboss_context)

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

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

Conversation

AdamMarkowski commented Feb 7, 2024

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

rapsad Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rapsad Feb 22, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nglx commented Feb 28, 2024 • edited Loading

rapsad Feb 19, 2024 •

edited

Loading

rapsad Feb 22, 2024 •

edited

Loading

nglx commented Feb 28, 2024 •

edited

Loading