Add centralized error reporting via Rage::Errors#275
Conversation
|
Hey @Digvijay-x1 , I usually don't review in-progress PRs, so I haven't looked at this one yet. Anything I can help with? Do you want me to give it a look? |
Hey @rsamoilov, Thanks for offering to help :) Before requesting a review, I wanted to test whether all cases behave as expected. This took a bit longer since I had some college assignments last week. From my testing so far, the core error reporting API appears to be working well. I set up a small Rage app with Sentry integration and verified that reporter registration, error forwarding, context propagation, duplicate prevention, and backtrace generation all behave correctly. I’ve tested 5 out of the 9 call sites so far — including unhandled controller exceptions (covering both application.rb and fiber_wrapper.rb), deferred task failures, and direct Rage::Errors.report calls. The remaining areas (events/subscriber, SSE, Cable, Redis adapter, telemetry) all route through the same Rage::Errors.report method, which is already working as expected. I think the PR is in a good state for review now. Would appreciate your feedback whenever you get a chance! |
rsamoilov
left a comment
There was a problem hiding this comment.
Hey @Digvijay-x1 , the code looks good. I'll test the implementation more thoroughly and get back to you early next week.
There was a problem hiding this comment.
Hi @Digvijay-x1 ,
This is an amazing implementation. I love how minimal it is - this is exactly what I had in mind.
My main comment is that you're mixing two different concerns (configuration and reporting) inside Rage::Errors.
report should be part of the module - that's OK. But configuration (<<) belongs to Rage::Configuration - you should have a separate namespace there that allows to add and remove error reporters. For example:
Rage.configure do
config.error_handlers << MyErrorHandler.new
endThen, once a reporter is added, the configuration code should call a @private method on Rage::Errors to actually register the reporter.
| @reporters.length.times do |i| | ||
| __send__(:"__report_#{i}", exception, context) | ||
| rescue => e | ||
| Rage.logger.error("Error reporter #{@reporters[i].class} failed: #{e.class} (#{e.message})") |
There was a problem hiding this comment.
| Rage.logger.error("Error reporter #{@reporters[i].class} failed: #{e.class} (#{e.message})") | |
| Rage.logger.error("Error reporter #{@reporters[i].class} failed while reporting #{exception.class}: #{e.class} (#{e.message})") |
|
Let's also add the reporter hook into |
Allow users to register error reporters that receive all
framework-caught exceptions, so they can forward them to services
like Sentry or Bugsnag without manually rescuing in every component.
Rage.configure do
config.errors << SentryReporter.new
end
Reporters can accept just the exception or an optional context hash.
Signature detection uses Rage::Internal.build_arguments at registration
time to keep the reporting path fast. Each reporter is called in its
own rescue block so a broken reporter doesn't affect the rest.
Duplicate prevention uses an instance variable on the exception
(similar to Rails). Manually-created exceptions get a real backtrace
via re-raise rather than set_backtrace(caller).
Hooked into all existing framework rescue sites: controllers,
fiber wrapper, Cable connections/channels, Redis adapter, deferred
tasks, event subscribers, SSE streams, and telemetry tracers.
Signed-off-by: Digvijay Singh Rawat <hackerearthx1@gmail.com>
5455e7a to
5fb887a
Compare
…eporting # Conflicts: # lib/rage/cable/adapters/redis.rb # spec/cable/adapters/redis_spec.rb
rsamoilov
left a comment
There was a problem hiding this comment.
Looks good! Can't wait to include this into the next release!
Allow users to register error reporters that receive all framework-caught exceptions, so they can forward them to services like Sentry or Bugsnag without manually rescuing in every component.
Reporters can accept just the exception or an optional context hash. Signature detection uses Rage::Internal.build_arguments at registration time to keep the reporting path fast. Each reporter is called in its own rescue block so a broken reporter doesn't affect the rest. Duplicate prevention uses an instance variable on the exception (similar to Rails). Manually-created exceptions get a real backtrace via re-raise rather than set_backtrace(caller).
Hooked into all existing framework rescue sites: controllers, fiber wrapper, Cable connections/channels, Redis adapter, deferred tasks, event subscribers, SSE streams, and telemetry tracers.
Fixes #232