-
Notifications
You must be signed in to change notification settings - Fork 2
Add sentry middleware #89
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.
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
Conversation
|
Will it interfere with https://github.com/AirHelp/eventboss/blob/master/lib/eventboss/error_handlers/sentry.rb? |
c1c87df to
ff83f00
Compare
lib/eventboss/long_poller.rb
Outdated
| max_number_of_messages: 10, | ||
| wait_time_seconds: TIME_WAIT | ||
| wait_time_seconds: TIME_WAIT, | ||
| attribute_names: ['SentTimestamp', 'ApproximateReceiveCount'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to make the SQS return the SentTimestamp and ApproximateReceiveCount
@seban yes. That one will needed to be deleted. I see no point in keeping it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR integrates Sentry monitoring and error tracking into the Eventboss gem to provide better observability for message queue processing. The implementation includes distributed tracing support, automatic error capture, and performance monitoring.
Key changes:
- Adds Sentry server middleware for automatic transaction tracking and error handling
- Implements distributed tracing by propagating Sentry trace headers through message attributes
- Updates message polling to fetch necessary attributes for latency and retry count metrics
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/eventboss/sentry/server_middleware.rb | Core middleware that handles Sentry transaction creation, error tracking, and span management for message processing |
| lib/eventboss/sentry/integration.rb | Sentry integration registration module |
| lib/eventboss/sentry/error_handler.rb | Custom error handler for capturing exceptions in Sentry |
| lib/eventboss/sentry/configure.rb | Auto-configuration for adding Sentry middleware and error handlers |
| lib/eventboss/publisher.rb | Adds trace propagation headers to published messages for distributed tracing |
| lib/eventboss/long_poller.rb | Updates message fetching to include Sentry trace attributes and timing metadata |
| spec/eventboss/long_poller_spec.rb | Updates test expectations to include new message attributes |
| eventboss.gemspec | Adds sentry-ruby as development dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| Eventboss::Sentry::Integration.capture_exception( | ||
| exception, | ||
| contexts: { eventboss: { } }, |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty hash in the eventboss context provides no useful information. Consider adding relevant context data like queue name, message ID, or listener information to help with debugging.
| contexts: { eventboss: { } }, | |
| eventboss_context = {} | |
| eventboss_context[:queue_name] = _context[:queue_name] if _context.key?(:queue_name) | |
| eventboss_context[:message_id] = _context[:message_id] if _context.key?(:message_id) | |
| eventboss_context[:listener] = _context[:listener] if _context.key?(:listener) | |
| Eventboss::Sentry::Integration.capture_exception( | |
| exception, | |
| contexts: { eventboss: eventboss_context }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have that in the middleware
|
|
||
| def extract_sentry_user(work) | ||
| if (value = work.message.message_attributes["sentry_user"]&.string_value) | ||
| JSON.parse(value) |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON parsing can raise JSON::ParserError for malformed data. Consider wrapping this in a rescue block to handle invalid JSON gracefully and prevent crashes during message processing.
| JSON.parse(value) | |
| begin | |
| JSON.parse(value) | |
| rescue JSON::ParserError | |
| nil | |
| end |
lib/eventboss/publisher.rb
Outdated
| { string_value: header_value, data_type: 'String' } | ||
| end | ||
| user = ::Sentry.get_current_scope.user | ||
| if user && !user.empty? |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty? method may not be available on all user objects returned by Sentry. Consider using a more specific check like user.respond_to?(:empty?) && !user.empty? or checking for specific user attributes.
| if user && !user.empty? | |
| if user && user.respond_to?(:empty?) && !user.empty? |
eventboss.gemspec
Outdated
| spec.add_development_dependency "bundler", ">= 1" | ||
| spec.add_development_dependency 'rake', '>= 10.0' | ||
| spec.add_development_dependency "rspec", "~> 3.0" | ||
| spec.add_development_dependency "sentry-ruby" |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Development dependency should specify a version constraint to ensure consistent builds. Consider adding a version requirement like "sentry-ruby", "~> 5.0" or similar.
| spec.add_development_dependency "sentry-ruby" | |
| spec.add_development_dependency "sentry-ruby", "~> 5.0" |
| else | ||
| publish_without_sentry(payload) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to implement client middleware to support it cleanly. But that was never needed and never requested. That can be refactored then to support the current and old needs. Right now i don't want to crystal ball a future use-case
lib/eventboss/publisher.rb
Outdated
| if sentry_enabled? | ||
| with_sentry_span(&publisher_action) | ||
| else | ||
| publisher_action.call | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but maybe with_sentry_span could just yield return if sentry is not enabled, so there won't be a need for if condition, and would be easier later on to wrap into multiple procs
lib/eventboss/publisher.rb
Outdated
| end | ||
|
|
||
| # Constructs SNS message attributes for Sentry trace propagation. | ||
| def build_sentry_message_attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a good candidate to be moved to a separate class
lib/eventboss/publisher.rb
Outdated
| end | ||
|
|
||
| user = ::Sentry.get_current_scope&.user | ||
| if user && !user.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the scenario that the user could be empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example a cron job can publish a eventboss event. The cronjob doesnt have to be per user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have a similar for #empty? in the sidekiq middleware, so i kept it https://github.com/getsentry/sentry-ruby/blob/master/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb#L112
| SPAN_ORIGIN = 'auto.queue.eventboss' | ||
|
|
||
| # since sentry has env selector, we can remove it from queue names | ||
| QUEUES_WITHOUT_ENV = Hash.new do |hash, key| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to just populate it with proper keys, instead of setting the default value? This is prone to an error, where someone would provide an non existing queue to this hash, and still get the value back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the publisher: send name of events are defined in the publisher class and are fist time seen when we they try to publish the event. I don't think it's possible to do that without changing the interface of eventboss itself
This is prone to an error, where someone would provide an non existing queue to this hash, and still get the value back
I think that's the error of the user of eventboss. This constant only replaces the environment with placeholder ENV in the queue name for visualisation purpose. If the user wants to publish to not-existing queue, then i don't think the creation of Span metadata should be the one reporting that, but maybe eventboss itself
rapsad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nice work!
This PR introduces Sentry for Eventboss with performance monitoring, distributed tracing, and enhanced error reporting.
The new
Eventboss::Sentry::ServerMiddlewarecreates transactions for message processing witg context (queue names, message IDs, latency, retry counts)Simple one-line setup via
require 'eventboss/sentry/configure'automatically configures both client and server components. The old Eventboss::ErrorHandlers::Sentry is deprecatedTo see it working please see: https://airhelp.sentry.io/insights/backend/queues/?environment=staging&project=4506716776693760&statsPeriod=1h
This branch is currently on staging & production. The staging has newest version deployed ~1h ago
This middleware is based on sidekiq integration
Later i saw this we have in Airhelp already a custome one :D But it is lacking features