Breadcrumbs: Rails Breadcrumbs #514

Cawllec · 2018-12-12T17:20:17Z

Goal

Adds ActiveSupport::Notifications subscriber for creating automatic breadcrumbs from Rails events.

…s-breadcrumbs

tobyhs · 2018-12-12T20:39:41Z

lib/bugsnag/integrations/rails/rails_breadcrumbs.rb

@@ -0,0 +1,134 @@
+require "bugsnag/breadcrumbs/breadcrumbs"
+
+module Bugsnag::RailsBreadcrumbs


This should probably be under the Bugsnag::Rails module/namespace to match the other 2 integrations in lib/bugsnag/integrations/rails/

This is fine, but I've had to move the require statement for the file due to namespace clashes. (As having a Bugsnag::Rails namespace makes it search for Rails::Railtie at Bugsnag::Rails::Railtie)

In that case, it would be preferable to use ::Rails::Railtie instead of Rails::Railtie in lib/bugsnag/integrations/railtie.rb to avoid possible issues.

tobyhs · 2018-12-12T20:44:07Z

lib/bugsnag/integrations/railtie.rb

+    # @param event [Hash] details of the event to subscribe to
+    def event_subscription(event)
+      ActiveSupport::Notifications.subscribe(event[:id]) do |*, data|
+        filtered_data = data.select{ |k, v| event[:allowed_data].include?(k) }


Add a space before {; also, I think you can use data.slice(*event[:allowed_data]) instead (because ActiveSupport provides Hash#slice)

tobyhs · 2018-12-12T20:53:09Z

lib/bugsnag/integrations/railtie.rb

+    def event_subscription(event)
+      ActiveSupport::Notifications.subscribe(event[:id]) do |*, data|
+        filtered_data = data.select{ |k, v| event[:allowed_data].include?(k) }
+        filtered_data[:event_id] = event[:id]


I'm not sure if event_id is a good name for the key in the metadata (as seeing the key itself could imply the ID of an actual instrumented/emitted occurrence of an event as opposed to the event type). Unfortunately, I don't see a good name for this in Rails documentation; the best I can come up with is event_name or event_type.

tobyhs · 2018-12-12T21:00:10Z

lib/bugsnag/integrations/rails/rails_breadcrumbs.rb

+      :message => "Perform ActionCable",
+      :type => Bugsnag::Breadcrumbs::PROCESS_BREADCRUMB_TYPE,
+      :allowed_data => [
+        :event_id,


I don't think you need to include event_id (or whatever you might rename it to) in all the breadcrumb types because you add it on after you select the appropriate fields from the notification payload.

…nge name to event_name

… to avoid namespace clashes

…dcrumb definitions

tobyhs · 2018-12-17T21:05:50Z

features/rails_features/breadcrumbs.feature

+    | 2.3          | 3             |
+    | 2.3          | 4             |
+    | 2.3          | 5             |
+    | 2.4          | 3             |


I might be missing context, but why is Rails 4 missing from Ruby versions 2.4 and 2.5 from this example table and the one on line 45?

The Rails 4 fixture doesn't work with Ruby 2.4+, so it was dropped when the maze fixtures were originally written.

tobyhs · 2018-12-17T21:28:55Z

features/rails_features/breadcrumbs.feature

+    | 2.4          | 3             |
+    | 2.4          | 5             |
+    | 2.5          | 3             |
+    | 2.5          | 5             |


Were you also planning on writing additional scenario outlines for the other automatic breadcrumb types? This might help in determining if some of the breadcrumb types are not really useful or are near impossible to trigger.

In addition, it might be helpful to also check some of the metadata values (although I realize some of them such as connection_id for SQL breadcrumbs are not deterministic).

…e possible

tobyhs · 2018-12-18T23:19:45Z

features/fixtures/rails3/app/app/controllers/breadcrumbs_controller.rb

@@ -0,0 +1,23 @@
+class BreadcrumbsController < ActionController::Base


Inherit from ApplicationController instead

tobyhs · 2018-12-18T23:25:41Z

features/fixtures/rails3/app/app/controllers/breadcrumbs_controller.rb

@@ -0,0 +1,23 @@
+class BreadcrumbsController < ActionController::Base
+  def initialize


I don't normally see overriding of #initialize for a controller class. You should be able to use Rails.cache in your #cache_read method instead.

tobyhs · 2018-12-18T23:31:22Z

features/fixtures/rails4/app/app/jobs/application_job.rb

@@ -0,0 +1,5 @@
+class ApplicationJob < ActiveJob::Base
+  def perform


ApplicationJob is meant to be an abstract class for other job classes to inherit from, so you should create a separate job class that inherits from ApplicationJob.

tobyhs · 2018-12-18T23:36:11Z

features/fixtures/rails5/app/app/controllers/breadcrumbs_controller.rb

+    render json: {}
+  end
+
+  def action_mailer


It looks like there is no route or maze-runner test that visits this. Did you forget to add something?

tobyhs · 2018-12-19T00:04:26Z

features/rails_features/breadcrumbs.feature

+
+  Examples:
+    | ruby_version | rails_version |
+    | 2.2          | 4             |


Lines 81 and 83 are failing for me with:

2.2 | 4 | <"perform_start.active_job"> expected but was <"start_processing.action_controller">. (Test::Unit::AssertionFailedError) features/rails_features/breadcrumbs.feature:81:in `And the event "breadcrumbs.0.metaData.event_name" equals "perform_start.active_job"' features/rails_features/breadcrumbs.feature:77:in `And the event "breadcrumbs.0.metaData.event_name" equals "perform_start.active_job"' Failing Scenarios: cucumber features/rails_features/breadcrumbs.feature:81 # Scenario Outline: Active job breadcrumb, Examples (#1)

Just checked after the latest update, and these tests are now passing ✅

…es more rails-y

tobyhs

lgtm; I guess we can tweak some of the captured breadcrumbs and metadata as we test do more testing

* Breadcrumbs/Circular buffer: Added circular buffer class * Breadcrumbs/Circular buffer: Added initial buffer unit tests * Breadcrumbs/Circular buffer: Ensure test file is named correctly * Breadcrumbs/Circular buffer: Code tidy up * Breadcrumbs / Breadcrumb class: Add first pass breadcrumb class with unit tests * Breadcrumbs/Circular buffer rubocop fixes * Breadcrumbs / Breadcrumb class: Rubocop fixes * Breadcrumbs/Cricular buffer: Improve test readability * Breadcrumbs/Breadcrumb: Simplified timestamp, improved tests * Breadcrumbs/Breadcrumb: Rubocop requirements * Breadcrumbs/Breadcrumb: Added doc comments * Breadcrumbs/Circular buffer: Fixed RSpec/Rspec typo * Breadcrumbs/Breadcrumb: Using explicit RSpec.describe call * Breadcrumbs/Validator: Added validator and tests * Breadcrumbs/Validator: Rubocop fixes * Breadcrumbs/Validator: Tidying up from initial review * Breadcrumbs/Validator: Re-add renamed types file * Breadcrumbs/Breadcrumb: Change message => name, add yard comments * Breadcrumbs/Circular buffer: Added yard docs * Breadcrumbs/Breadcrumb: Improve tests, convert to utc sooner * Breadcrumbs/Configuration: Initial configuration changes * Breadcrumbs/Report: Initial report changes to support breadcrumbs * Breadcrumbs/Breadcrumb: Test timestamps match up * Breadcrumbs/Validator: Change 'message' to 'name', use name and meta_data clones * Breadcrumbs/Validator: Refactor toward instance_double & let * Breadcrumbs/Configuration: Add in missing import statement * Breadcrumbs/Circular buffer docs: Improve readability * Breadcrumbs/Configuration: Add configuration tests * Breadcrumbs/Configuration: Added yardoc comments * Breadcrumbs/Configuration: Simplify before_breadcrumb_callbacks, allow automatic_breadcrumb_types editting * Breadcrumbs/Configuration: Add automatic breadcrumbs types editablilty test * Breadcrumbs/Configuration: Add types to Yardoc arrays * Breadcrumbs/Validator: Use an instance_double to reflect Configuration better * Breadcrumbs/Validator: Add Yardoc comments * Breadcrumbs/Validator: Use select over each when filtering * Breadcrumbs/Validator: Clean up tests * Breadcrumbs/Validator: Use double for configuration * Breadcrumbs/Validator: Improved Yardoc * Breadcrumbs/Leave breadcrumb: Add implementation and tests * Breadcrumbs/Middleware: Add breadcrumb middleware * Breadcrumbs/Report: Rubify get_summary method name * Breadcrumbs/Report: Ensure message is set correctly * Breadcrumbs/Report: Change default breadcrumbs from Hash to Array * Breadcrumbs/Report: Ensure breadcrumb meta_data is filtered correctly * Breadcrumbs/Report: Fixed typo * Bugsnag/Report: Added tests * Breadcrumbs/Leave breadcrumb: Update rubocop to allow increased Bugsnag module length * Breadcrumbs/middleware: Rubocop fixes * Breadcrumbs/Leave-breadcrumb: Rubocop improvements/early return * Breadcrumbs/Leave-breadcrumb: Better block styling * Breadcrumbs/Report: Ensure report summary is consistent with dashboard * Breadcrumbs/Report: Better Rubocop report exclusions * Breadcrumbs/Report: Rspec improvements * Breadcrumbs/Notify-breadcrumb: Initial implementation and tests * Breadcrumbs/Notify breadcrumb: Add test against notifying a string * Breadcrumbs/Report: Better checking against Java error * Breadcrumbs/Report: Make summary more accurate * Breadcrumbs/Report update: make summary guard more readable * Breadcrumbs/Update automatic_breadcrumb_types to enabled_automatic_breadcrumb_types * Breadcrumbs: Rails Breadcrumbs (#514) * Breadcrumbs/Rails integration: Initial integration implemenetation * Breadcrumbs/Rails: Minor fixes * Breadcrumbs/Rails: Add initial maze tests * Breadcrumbs/Rails breadcrumbs: Remove plural event data * Breadcrumbs/Rails breadcrumbs: Ammend breadcrumb namespace * Breadcrumbs/Rails breadcrumbs: Remove event_id from allowed keys, change name to event_name * Breadcrumbs/Rails breadcrumbs: Require breadcrumb file when necessary to avoid namespace clashes * Breadcrumbs/Rails breadcrumbs: Fix rubocop issues * Breadcrumbs/Rails breadcrumbs: Clarify namespaces better * Breadcrumbs/Rails breadcrumbs: Remove invalid data from captured breadcrumb definitions * Breadcrumbs/Rails breadcrumbs: Added ActiveJob breadcrumb test * Breadcrumbs/Rails breadcrumbs: Ammended active_job event id * Breadcrumbs/Ruby breadcrumbs: Removed complex breadcrumb metadata * Breadcrumbs/Ruby: Added cache breadcrumb * Breadcrumbs/Ruby breadcrumbs: Fleshed out maze-test expectations where possible * Breadcrumbs/Rails breadcrumbs: Fixed broken test fixture, made fixtures more rails-y * Breadcrumbs: Add Mongo integration (#516) * Breadcrumbs/Rails integration: Initial integration implemenetation * Breadcrumbs/Rails: Minor fixes * Breadcrumbs/Rails: Add initial maze tests * Breadcrumbs/Mongo: Initial subscriber implementation * Breadcrumbs/Rails breadcrumbs: Remove plural event data * Breadcrumbs/Rails breadcrumbs: Ammend breadcrumb namespace * Breadcrumbs/Rails breadcrumbs: Remove event_id from allowed keys, change name to event_name * Breadcrumbs/Rails breadcrumbs: Require breadcrumb file when necessary to avoid namespace clashes * Breadcrumbs/Mongo integration: Fix module name issue * Breadcrumbs/Mongo integration: Maintain consistency with ActiveSupport arguments * Breadcrumbs/Rails breadcrumbs: Fix rubocop issues * Breadcrumbs/Rails breadcrumbs: Clarify namespaces better * Breadcrumbs/Mongo integration: Add integration tests * Breadcrumbs/Mongo integration: Remove example commits * Breadcrumbs/Rails breadcrumbs: Remove invalid data from captured breadcrumb definitions * Breadcrumbs/Mongo integration: Rubocop fixes * Breadcrumbs/Mongo integration: Better doc comments * Breadcrumbs/Mongo integration: Improve tests, add 'failure' test * Breadcrumbs/Mongo integration: Re-add 'success' test * Breadcrumbs/Rails breadcrumbs: Added ActiveJob breadcrumb test * Breadcrumbs/Rails breadcrumbs: Ammended active_job event id * Breadcrumbs/Ruby breadcrumbs: Removed complex breadcrumb metadata * Breadcrumbs/Ruby: Added cache breadcrumb * Breadcrumbs/Ruby breadcrumbs: Fleshed out maze-test expectations where possible * Breadcrumbs/Mongo integration: Rubocop fixes * Breadcrumbs/Rails breadcrumbs: Fixed broken test fixture, made fixtures more rails-y * Breadcrumbs/Mongo integration: Maze fixes * Breadcrumbs/Validator: Allow nil as an acceptable meta_data type (#520) * Breadcrumbs/Mongo: Remove 'started' breadcrumb, add collection detail (#519) * Breadcrumbs/Mongo integration: Remove 'started' event, add 'collection' data * Breadcrumbs/Mongo: Add filter keys * Breadcrumbs/Mongo integration: Made filters consistent with other breadcrumb implementations * Breadcrumbs/Mongo integration: Rubocop fixes * Breadcrumbs/Mongo integration: Address feedback * Breadcrumbs: Additional Rails ActiveRecord data (#521) * Breadcrumbs/Rails breadcrumbs: Capture event_id * Breadcrumbs/Rails breadcrumbs: Capture 'sql' string and redacted bindings * Breadcrumbs/Rails integration: Redact SQL string values * Breadcrumbs/Rails integration: Remove SQL string acquisition * Breadcrumbs/Rails improvements: Remove unnecessary require * Breadcrumbs/Rails integration: Add integration tests for bindings * Breadcrumbs/Ruby integration: lock nokogiri version * Breadcrumbs/Ruby integration: Better meta_data naming, Ruby < 2.1 compatability * Breadcrumbs/Mongo integration: Handle 'any_of' cases (#522) * Breadcrumbs/Mongo integration: Handle 'any_of' cases * Breadcrumbs/Mongo Integration: Improve filter sanitization process and add unit tests * Breadcrumbs/Mongo integration: Ensure tests are notated correctly * Breadcrumbs/Mongo filters: Add 1.9.3 & 2.0.0 compatability * Breadcrumbs/Mongo integration: Appease rubocop * Breadcrumbs/Mongo integration: Minor test and efficieny improvements * Breadcrumbs/Mongo integration: Test improvements * Breadcrumbs/Mongo integrations: Test spelling * Breadcrumbs: Fix rubocop issues (excluding configuration from ClassLength until refactor)

Cawllec added 5 commits December 11, 2018 18:10

Breadcrumbs/Rails integration: Initial integration implemenetation

fde94dd

Merge branch 'breadcrumbs/base' into breadcrumbs/rails-breadcrumbs

d778468

Breadcrumbs/Rails: Minor fixes

a30612f

Merge branch 'breadcrumbs/update-reports-class' into breadcrumbs/rail…

f3b5110

…s-breadcrumbs

Breadcrumbs/Rails: Add initial maze tests

82dcd48

Cawllec requested review from tobyhs, kellymason and a team December 12, 2018 17:20

Breadcrumbs/Rails breadcrumbs: Remove plural event data

23b7cfb

tobyhs reviewed Dec 12, 2018

View reviewed changes

Cawllec added 5 commits December 13, 2018 11:26

Breadcrumbs/Rails breadcrumbs: Ammend breadcrumb namespace

0aae277

Breadcrumbs/Rails breadcrumbs: Remove event_id from allowed keys, cha…

ed5e05b

…nge name to event_name

Breadcrumbs/Rails breadcrumbs: Require breadcrumb file when necessary…

909d3df

… to avoid namespace clashes

Breadcrumbs/Rails breadcrumbs: Fix rubocop issues

61532e9

Breadcrumbs/Rails breadcrumbs: Clarify namespaces better

537c3f1

Cawllec changed the base branch from breadcrumbs/update-reports-class to breadcrumbs/base December 17, 2018 14:33

Breadcrumbs/Rails breadcrumbs: Remove invalid data from captured brea…

4dd868f

…dcrumb definitions

Cawllec changed the title ~~WIP: Breadcrumbs: Rails Breadcrumbs~~ Breadcrumbs: Rails Breadcrumbs Dec 17, 2018

Merge branch 'breadcrumbs/base' into breadcrumbs/rails-breadcrumbs

6a5484f

tobyhs reviewed Dec 17, 2018

View reviewed changes

Cawllec added 5 commits December 18, 2018 17:25

Breadcrumbs/Rails breadcrumbs: Added ActiveJob breadcrumb test

064ecc8

Breadcrumbs/Rails breadcrumbs: Ammended active_job event id

df05870

Breadcrumbs/Ruby breadcrumbs: Removed complex breadcrumb metadata

ddd6e87

Breadcrumbs/Ruby: Added cache breadcrumb

2228391

Breadcrumbs/Ruby breadcrumbs: Fleshed out maze-test expectations wher…

59d1d58

…e possible

tobyhs reviewed Dec 18, 2018

View reviewed changes

tobyhs reviewed Dec 19, 2018

View reviewed changes

Breadcrumbs/Rails breadcrumbs: Fixed broken test fixture, made fixtur…

b9b13da

…es more rails-y

tobyhs approved these changes Dec 19, 2018

View reviewed changes

Cawllec merged commit d45e766 into breadcrumbs/base Dec 19, 2018

imjoehaines deleted the breadcrumbs/rails-breadcrumbs branch June 16, 2021 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breadcrumbs: Rails Breadcrumbs #514

Breadcrumbs: Rails Breadcrumbs #514

Cawllec commented Dec 12, 2018 •

edited

Loading

tobyhs Dec 12, 2018

Cawllec Dec 13, 2018

tobyhs Dec 13, 2018

tobyhs Dec 12, 2018

tobyhs Dec 12, 2018

tobyhs Dec 12, 2018

tobyhs Dec 17, 2018

Cawllec Dec 18, 2018

tobyhs Dec 17, 2018

tobyhs Dec 18, 2018

tobyhs Dec 18, 2018

tobyhs Dec 18, 2018

tobyhs Dec 18, 2018

tobyhs Dec 19, 2018

kattrali Dec 19, 2018

tobyhs left a comment

		@@ -0,0 +1,134 @@
		require "bugsnag/breadcrumbs/breadcrumbs"

		module Bugsnag::RailsBreadcrumbs

		@@ -0,0 +1,23 @@
		class BreadcrumbsController < ActionController::Base

		@@ -0,0 +1,23 @@
		class BreadcrumbsController < ActionController::Base
		def initialize

		@@ -0,0 +1,5 @@
		class ApplicationJob < ActiveJob::Base
		def perform

Breadcrumbs: Rails Breadcrumbs #514

Breadcrumbs: Rails Breadcrumbs #514

Conversation

Cawllec commented Dec 12, 2018 • edited Loading

Goal

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobyhs left a comment

Choose a reason for hiding this comment

Cawllec commented Dec 12, 2018 •

edited

Loading