Skip to content

Conversation

stympy
Copy link
Member

@stympy stympy commented Sep 9, 2025

Send events recorded by calling Rails.event.notify to Insights.

See rails/rails#55334 for more info

Copy link
Contributor

@Copilot Copilot AI left a 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 adds support for subscribing to Rails.event notifications to send structured events to Honeybadger Insights. It introduces a new Rails event subscriber that captures events emitted via Rails.event.notify and forwards them to Honeybadger's event system.

  • Implements a new RailsEventSubscriber class to handle Rails.event notifications
  • Integrates the subscriber into the Rails plugin with conditional loading for Rails 8.1+ compatibility
  • Adds comprehensive test coverage for both available and unavailable Rails.event scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
lib/honeybadger/plugins/rails.rb Adds conditional subscription to Rails.event when available
lib/honeybadger/notification_subscriber.rb Implements RailsEventSubscriber class to process Rails events
spec/integration/rails/event_subscriber_spec.rb Adds test coverage for Rails event subscription functionality
gemfiles/rails.gemfile Simplifies Rails gem dependencies to use main branch

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@joshuap joshuap left a comment

Choose a reason for hiding this comment

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

Love this Rails feature

This takes a different approach than d3d2767 to get the latest rails gem versions.
Copy link
Contributor

@rabidpraxis rabidpraxis left a comment

Choose a reason for hiding this comment

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

Yeah, this is very cool.

The activerecord-jdbcsqlite3-adapter gem doesn't seem to be quite ready for Rails 8 yet.
Rails 8 requires Ruby 3.2 but JRuby 9 provides Ruby 3.1
@stympy stympy mentioned this pull request Sep 10, 2025
@stympy
Copy link
Member Author

stympy commented Sep 10, 2025

I'm having second thoughts about the check for Honeybadger.config.load_plugin_insights_events?(:rails) ... Perhaps that check shouldn't be there, as the intent of that configuration option is to suppress/enable automatic instrumentation of Rails events (e.g., SQL queries reported via ActiveSupport::Notifications), whereas Rails.event is designed to be used intentionally by a developer to log specific events. Thoughts?

@rabidpraxis
Copy link
Contributor

That config name is not screaming it, but I think that makes sense if we want to keep that just for "framework" events.

It seems like we should provide a way to disable this though.

@joshuap
Copy link
Member

joshuap commented Sep 16, 2025

I'm having second thoughts about the check for Honeybadger.config.load_plugin_insights_events?(:rails) ... Perhaps that check shouldn't be there, as the intent of that configuration option is to suppress/enable automatic instrumentation of Rails events (e.g., SQL queries reported via ActiveSupport::Notifications), whereas Rails.event is designed to be used intentionally by a developer to log specific events. Thoughts?

Agreed, I was thinking this too. I could see cases where people might want to disable the automatic instrumentation, but still get Rails.event in Honeybadger. I think it's similar to calling Honeybadger.event, actually. Also agree w/ Kevin about a way to disable the event integration. I would say rails.insights.events, but that config option already exists for the automatic events. 😅

@stympy
Copy link
Member Author

stympy commented Oct 17, 2025

@joshuap and @rabidpraxis what do you think about the new rails.insights.custom_events option in 7857ff0?

@rabidpraxis
Copy link
Contributor

I like it, but I am wondering if rails.insights.rails_events would be a more descriptive name for the flag.

@joshuap
Copy link
Member

joshuap commented Oct 23, 2025

I like the idea, but I agree that the insights config is getting pretty confusing with all the similarly named options. With this addition, these are the current insights options that would be relevant for Rails, right?

{
      "insights.enabled": {
        description: "Enable/Disable Honeybadger Insights built-in instrumentation.",
        default: true,
        type: Boolean
      },
      "rails.insights.enabled": {
        description: "Enable automatic data collection for Ruby on Rails.",
        default: true,
        type: Boolean
      },
      "rails.insights.events": {
        description: "Enable automatic event capturing for Ruby on Rails.",
        default: true,
        type: Boolean
      },
      "rails.insights.metrics": {
        description: "Enable automatic metric data collection for Ruby on Rails.",
        default: false,
        type: Boolean
      },
      "rails.insights.custom_events": {
        description: "Enable capturing of custom Rails.event events in Rails 8.1 and later.",
        default: true,
        type: Boolean
      }
}

rails.insights.enabled is a local plugin override for the global insights.enabled option, so that makes sense to me. With the addition of a second events-related option, I kind of wish they were both more specific. Consider this example:

---
rails:
  insights:
    events: false

If you are new to Honeybadger, would you expect that option to disable Rails.event, our custom instrumentation events, or both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants