From f4030f9954558ec759820b855795da9ce04ea736 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Fri, 20 Aug 2021 13:11:14 +0100 Subject: [PATCH 1/6] Add OnBreadcrumbCallbackList class This manages and calls any registered on_breadcrumb callbacks --- .../on_breadcrumb_callback_list.rb | 44 ++++ .../on_breadcrumb_callback_list_spec.rb | 231 ++++++++++++++++++ 2 files changed, 275 insertions(+) create mode 100644 lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb create mode 100644 spec/breadcrumbs/on_breadcrumb_callback_list_spec.rb diff --git a/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb b/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb new file mode 100644 index 00000000..5f345fff --- /dev/null +++ b/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb @@ -0,0 +1,44 @@ +require "set" + +module Bugsnag::Breadcrumbs + class OnBreadcrumbCallbackList + def initialize + @callbacks = Set.new + @mutex = Mutex.new + end + + ## + # @param callback [Proc, Method, #call] + # @return [void] + def add(callback) + @mutex.synchronize do + @callbacks.add(callback) + end + end + + ## + # @param callback [Proc, Method, #call] + # @return [void] + def remove(callback) + @mutex.synchronize do + @callbacks.delete(callback) + end + end + + ## + # @param breadcrumb [Breadcrumb] + # @return [void] + def call(breadcrumb) + @callbacks.each do |callback| + should_continue = callback.call(breadcrumb) + + # only stop if should_continue is explicity 'false' to allow callbacks + # to return 'nil' + if should_continue == false + breadcrumb.ignore! + break + end + end + end + end +end diff --git a/spec/breadcrumbs/on_breadcrumb_callback_list_spec.rb b/spec/breadcrumbs/on_breadcrumb_callback_list_spec.rb new file mode 100644 index 00000000..87ab1f33 --- /dev/null +++ b/spec/breadcrumbs/on_breadcrumb_callback_list_spec.rb @@ -0,0 +1,231 @@ +require "spec_helper" + +require "bugsnag/breadcrumbs/on_breadcrumb_callback_list" + +RSpec.describe Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList do + it "can add callbacks to its list" do + callback = spy('callback') + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list.add(callback) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(callback).to have_received(:call).with(breadcrumb) + expect(breadcrumb.ignore?).to be(false) + end + + it "can remove callbacks to its list" do + callback = spy('callback') + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list.add(callback) + list.remove(callback) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(callback).not_to have_received(:call) + expect(breadcrumb.ignore?).to be(false) + end + + it "won't remove a callback that isn't the same instance" do + callback1 = spy('callback1') + callback2 = spy('callback2') + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + # note: adding callback1 but removing callback2 + list.add(callback1) + list.remove(callback2) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(callback1).to have_received(:call).with(breadcrumb) + expect(callback2).not_to have_received(:call) + expect(breadcrumb.ignore?).to be(false) + end + + it "calls callbacks in the order they were added" do + calls = [] + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + list.add(proc { calls << 1 }) + list.add(proc { calls << 2 }) + list.add(proc { calls << 3 }) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(calls).to eq([1, 2, 3]) + expect(breadcrumb.ignore?).to be(false) + end + + it "ignores the breadcrumb if a callback returns false" do + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + list.add(proc { false }) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(breadcrumb.ignore?).to be(true) + end + + it "does not ignore the breadcrumb if a callback returns nil" do + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + list.add(proc { nil }) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(breadcrumb.ignore?).to be(false) + end + + it "supports Method objects as callbacks" do + class ArbitraryClassMethod + def self.arbitrary_name(breadcrumb) + breadcrumb.metadata[:abc] = 123 + end + end + + class ArbitraryInstanceMethod + def arbitrary_name(breadcrumb) + breadcrumb.metadata[:xyz] = 789 + end + end + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + list.add(ArbitraryClassMethod.method(:arbitrary_name)) + + xyz = ArbitraryInstanceMethod.new + list.add(xyz.method(:arbitrary_name)) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(breadcrumb.metadata).to eq({ abc: 123, xyz: 789 }) + end + + it "allows removing Method objects as callbacks" do + class ArbitraryClassMethod + def self.arbitrary_name(breadcrumb) + breadcrumb.metadata[:abc] = 123 + end + end + + class ArbitraryInstanceMethod + def arbitrary_name(breadcrumb) + breadcrumb.metadata[:xyz] = 789 + end + end + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + list.add(ArbitraryClassMethod.method(:arbitrary_name)) + list.remove(ArbitraryClassMethod.method(:arbitrary_name)) + + xyz = ArbitraryInstanceMethod.new + list.add(xyz.method(:arbitrary_name)) + list.remove(xyz.method(:arbitrary_name)) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(breadcrumb.metadata).to eq({}) + end + + it "supports arbitrary objects that respond to #call as callbacks" do + class RespondsToCallAsClassMethod + def self.call(breadcrumb) + breadcrumb.metadata[:abc] = 123 + end + end + + class RespondsToCallAsInstanceMethod + def call(breadcrumb) + breadcrumb.metadata[:xyz] = 789 + end + end + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + list.add(RespondsToCallAsClassMethod) + list.add(RespondsToCallAsInstanceMethod.new) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(breadcrumb.metadata).to eq({ abc: 123, xyz: 789 }) + end + + it "supports removing arbitrary objects that respond to #call as callbacks" do + class RespondsToCallAsClassMethod + def self.call(breadcrumb) + breadcrumb.metadata[:abc] = 123 + end + end + + class RespondsToCallAsInstanceMethod + def call(breadcrumb) + breadcrumb.metadata[:xyz] = 789 + end + end + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + list.add(RespondsToCallAsClassMethod) + list.remove(RespondsToCallAsClassMethod) + + instance = RespondsToCallAsInstanceMethod.new + list.add(instance) + list.remove(instance) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(breadcrumb.metadata).to eq({}) + end + + it "works when accessed concurrently" do + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + + list.add(proc do |breadcrumb| + breadcrumb.metadata[:numbers] = [] + end) + + NUMBER_OF_THREADS = 20 + + threads = NUMBER_OF_THREADS.times.map do |i| + Thread.new do + list.add(proc do |breadcrumb| + breadcrumb.metadata[:numbers].push(i) + end) + end + end + + threads.shuffle.each(&:join) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + list.call(breadcrumb) + + # sort the numbers as they will be out of order but that doesn't matter as + # long as every number is present + expect(breadcrumb.metadata[:numbers].sort).to eq((0...NUMBER_OF_THREADS).to_a) + end +end From c4de39f6a56b0f690c8adff1eee9a1159ae7bb86 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Fri, 20 Aug 2021 13:19:18 +0100 Subject: [PATCH 2/6] Handle errors raised in on_breadcrumb callbacks --- .../on_breadcrumb_callback_list.rb | 10 ++- .../on_breadcrumb_callback_list_spec.rb | 67 ++++++++++++++++--- 2 files changed, 64 insertions(+), 13 deletions(-) diff --git a/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb b/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb index 5f345fff..df7cc5ad 100644 --- a/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb +++ b/lib/bugsnag/breadcrumbs/on_breadcrumb_callback_list.rb @@ -2,9 +2,10 @@ module Bugsnag::Breadcrumbs class OnBreadcrumbCallbackList - def initialize + def initialize(configuration) @callbacks = Set.new @mutex = Mutex.new + @configuration = configuration end ## @@ -30,7 +31,12 @@ def remove(callback) # @return [void] def call(breadcrumb) @callbacks.each do |callback| - should_continue = callback.call(breadcrumb) + begin + should_continue = callback.call(breadcrumb) + rescue StandardError => e + @configuration.warn("Error occurred in on_breadcrumb callback: '#{e}'") + @configuration.warn("on_breadcrumb callback stacktrace: #{e.backtrace.inspect}") + end # only stop if should_continue is explicity 'false' to allow callbacks # to return 'nil' diff --git a/spec/breadcrumbs/on_breadcrumb_callback_list_spec.rb b/spec/breadcrumbs/on_breadcrumb_callback_list_spec.rb index 87ab1f33..6011124a 100644 --- a/spec/breadcrumbs/on_breadcrumb_callback_list_spec.rb +++ b/spec/breadcrumbs/on_breadcrumb_callback_list_spec.rb @@ -6,7 +6,7 @@ it "can add callbacks to its list" do callback = spy('callback') - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(callback) breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) @@ -20,7 +20,7 @@ it "can remove callbacks to its list" do callback = spy('callback') - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(callback) list.remove(callback) @@ -36,7 +36,7 @@ callback1 = spy('callback1') callback2 = spy('callback2') - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) # note: adding callback1 but removing callback2 list.add(callback1) @@ -54,7 +54,7 @@ it "calls callbacks in the order they were added" do calls = [] - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(proc { calls << 1 }) list.add(proc { calls << 2 }) @@ -69,7 +69,7 @@ end it "ignores the breadcrumb if a callback returns false" do - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(proc { false }) @@ -81,7 +81,7 @@ end it "does not ignore the breadcrumb if a callback returns nil" do - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(proc { nil }) @@ -105,7 +105,7 @@ def arbitrary_name(breadcrumb) end end - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(ArbitraryClassMethod.method(:arbitrary_name)) @@ -132,7 +132,7 @@ def arbitrary_name(breadcrumb) end end - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(ArbitraryClassMethod.method(:arbitrary_name)) list.remove(ArbitraryClassMethod.method(:arbitrary_name)) @@ -161,7 +161,7 @@ def call(breadcrumb) end end - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(RespondsToCallAsClassMethod) list.add(RespondsToCallAsInstanceMethod.new) @@ -186,7 +186,7 @@ def call(breadcrumb) end end - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(RespondsToCallAsClassMethod) list.remove(RespondsToCallAsClassMethod) @@ -203,7 +203,7 @@ def call(breadcrumb) end it "works when accessed concurrently" do - list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) list.add(proc do |breadcrumb| breadcrumb.metadata[:numbers] = [] @@ -228,4 +228,49 @@ def call(breadcrumb) # long as every number is present expect(breadcrumb.metadata[:numbers].sort).to eq((0...NUMBER_OF_THREADS).to_a) end + + it "logs errors thrown in callbacks" do + logger = spy('logger') + Bugsnag.configuration.logger = logger + + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) + error = RuntimeError.new('Oh no!') + + list.add(proc { raise error }) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(breadcrumb.ignore?).to be(false) + + message_index = 0 + expected_messages = [ + /^Error occurred in on_breadcrumb callback: 'Oh no!'$/, + /^on_breadcrumb callback stacktrace:/ + ] + + expect(logger).to have_received(:warn).with("[Bugsnag]").twice do |&block| + expect(block.call).to match(expected_messages[message_index]) + message_index += 1 + end + end + + it "calls subsequent callbacks after an error is raised" do + list = Bugsnag::Breadcrumbs::OnBreadcrumbCallbackList.new(Bugsnag.configuration) + calls = [] + + list.add(proc { calls << 1 }) + list.add(proc { calls << 2 }) + list.add(proc { raise 'ab' }) + list.add(proc { calls << 4 }) + list.add(proc { calls << 5 }) + + breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new('name', 'type', {}, nil) + + list.call(breadcrumb) + + expect(calls).to eq([1, 2, 4, 5]) + expect(breadcrumb.ignore?).to be(false) + end end From 21f459bae6240340853979d191f97fd0cd9dc182 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Fri, 20 Aug 2021 13:43:17 +0100 Subject: [PATCH 3/6] Move before_breadcrumb tests to a new describe --- spec/bugsnag_spec.rb | 156 ++++++++++++++++++++++--------------------- 1 file changed, 79 insertions(+), 77 deletions(-) diff --git a/spec/bugsnag_spec.rb b/spec/bugsnag_spec.rb index 15a16065..dcd373c0 100644 --- a/spec/bugsnag_spec.rb +++ b/spec/bugsnag_spec.rb @@ -323,97 +323,99 @@ module Kernel }) end - it "runs callbacks before leaving" do - Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| - breadcrumb.meta_data = { - :callback => true - } - end - Bugsnag.leave_breadcrumb("TestName") - expect(breadcrumbs.to_a.size).to eq(1) - expect(breadcrumbs.first.to_h).to match({ - :name => "TestName", - :type => Bugsnag::Breadcrumbs::MANUAL_BREADCRUMB_TYPE, - :metaData => { - :callback => true - }, - :timestamp => match(timestamp_regex) - }) - end - - it "validates after callbacks" do - Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| - breadcrumb.meta_data = { - :int => 1, - :array => [1, 2, 3], - :hash => { - :a => 1, - :b => 2 + describe "before_breadcrumb_callbacks" do + it "runs callbacks before leaving" do + Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| + breadcrumb.meta_data = { + :callback => true } - } - breadcrumb.type = "Not a real type" - breadcrumb.name = "123123123123123123123123123123456456456456456" + end + Bugsnag.leave_breadcrumb("TestName") + expect(breadcrumbs.to_a.size).to eq(1) + expect(breadcrumbs.first.to_h).to match({ + :name => "TestName", + :type => Bugsnag::Breadcrumbs::MANUAL_BREADCRUMB_TYPE, + :metaData => { + :callback => true + }, + :timestamp => match(timestamp_regex) + }) end - Bugsnag.leave_breadcrumb("TestName") - - expect(breadcrumbs.to_a.size).to eq(1) - expect(breadcrumbs.first.to_h).to match({ - :name => "123123123123123123123123123123456456456456456", - :type => Bugsnag::Breadcrumbs::MANUAL_BREADCRUMB_TYPE, - :metaData => { - :int => 1, - :array => [1, 2, 3], - :hash => { - :a => 1, - :b => 2 + it "validates after callbacks" do + Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| + breadcrumb.meta_data = { + :int => 1, + :array => [1, 2, 3], + :hash => { + :a => 1, + :b => 2 + } } - }, - :timestamp => match(timestamp_regex) - }) - end + breadcrumb.type = "Not a real type" + breadcrumb.name = "123123123123123123123123123123456456456456456" + end - it "doesn't add when ignored by the validator" do - Bugsnag.configuration.enabled_automatic_breadcrumb_types = [] - Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE, :auto) - expect(breadcrumbs.to_a.size).to eq(0) - end + Bugsnag.leave_breadcrumb("TestName") + + expect(breadcrumbs.to_a.size).to eq(1) + expect(breadcrumbs.first.to_h).to match({ + :name => "123123123123123123123123123123456456456456456", + :type => Bugsnag::Breadcrumbs::MANUAL_BREADCRUMB_TYPE, + :metaData => { + :int => 1, + :array => [1, 2, 3], + :hash => { + :a => 1, + :b => 2 + } + }, + :timestamp => match(timestamp_regex) + }) + end - it "doesn't add if ignored in a callback" do - Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| - breadcrumb.ignore! + it "doesn't add when ignored by the validator" do + Bugsnag.configuration.enabled_automatic_breadcrumb_types = [] + Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE, :auto) + expect(breadcrumbs.to_a.size).to eq(0) end - Bugsnag.leave_breadcrumb("TestName") - expect(breadcrumbs.to_a.size).to eq(0) - end - it "doesn't add when ignored after the callbacks" do - Bugsnag.configuration.enabled_automatic_breadcrumb_types = [ - Bugsnag::Breadcrumbs::MANUAL_BREADCRUMB_TYPE - ] - Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| - breadcrumb.type = Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE + it "doesn't add if ignored in a callback" do + Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| + breadcrumb.ignore! + end + Bugsnag.leave_breadcrumb("TestName") + expect(breadcrumbs.to_a.size).to eq(0) end - Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::Breadcrumbs::MANUAL_BREADCRUMB_TYPE, :auto) - expect(breadcrumbs.to_a.size).to eq(0) - end - it "doesn't call callbacks if ignored early" do - Bugsnag.configuration.enabled_automatic_breadcrumb_types = [] - Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| - fail "This shouldn't be called" + it "doesn't add when ignored after the callbacks" do + Bugsnag.configuration.enabled_automatic_breadcrumb_types = [ + Bugsnag::Breadcrumbs::MANUAL_BREADCRUMB_TYPE + ] + Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| + breadcrumb.type = Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE + end + Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::Breadcrumbs::MANUAL_BREADCRUMB_TYPE, :auto) + expect(breadcrumbs.to_a.size).to eq(0) end - Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE, :auto) - end - it "doesn't continue to call callbacks if ignored in them" do - Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| - breadcrumb.ignore! + it "doesn't call callbacks if ignored early" do + Bugsnag.configuration.enabled_automatic_breadcrumb_types = [] + Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| + fail "This shouldn't be called" + end + Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE, :auto) end - Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| - fail "This shouldn't be called" + + it "doesn't continue to call callbacks if ignored in them" do + Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| + breadcrumb.ignore! + end + Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new do |breadcrumb| + fail "This shouldn't be called" + end + Bugsnag.leave_breadcrumb("TestName") end - Bugsnag.leave_breadcrumb("TestName") end end From 99d7729fd82ea94fa8b57eb2b747afb5bae8475e Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Fri, 20 Aug 2021 13:52:39 +0100 Subject: [PATCH 4/6] Add add and remove on_breadcrumb methods --- lib/bugsnag.rb | 27 +++++++++++++++++++++++++++ lib/bugsnag/configuration.rb | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index f4b1121a..e8d7ff1a 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -295,6 +295,33 @@ def remove_on_error(callback) configuration.remove_on_error(callback) end + ## + # Add the given callback to the list of on_breadcrumb callbacks + # + # The on_breadcrumb callbacks will be called when a breadcrumb is left and + # are passed the {Breadcrumbs::Breadcrumb Breadcrumb} object + # + # Returning false from an on_breadcrumb callback will cause the breadcrumb + # to be ignored and will prevent any remaining callbacks from being called + # + # @param callback [Proc, Method, #call] + # @return [void] + def add_on_breadcrumb(callback) + configuration.add_on_breadcrumb(callback) + end + + ## + # Remove the given callback from the list of on_breadcrumb callbacks + # + # Note that this must be the same instance that was passed to + # {add_on_breadcrumb}, otherwise it will not be removed + # + # @param callback [Proc, Method, #call] + # @return [void] + def remove_on_breadcrumb(callback) + configuration.remove_on_breadcrumb(callback) + end + ## # Returns the client's Cleaner object, or creates one if not yet created. # diff --git a/lib/bugsnag/configuration.rb b/lib/bugsnag/configuration.rb index 9594f75a..438f20b6 100644 --- a/lib/bugsnag/configuration.rb +++ b/lib/bugsnag/configuration.rb @@ -12,6 +12,7 @@ require "bugsnag/middleware/breadcrumbs" require "bugsnag/utility/circular_buffer" require "bugsnag/breadcrumbs/breadcrumbs" +require "bugsnag/breadcrumbs/on_breadcrumb_callback_list" module Bugsnag class Configuration @@ -148,6 +149,11 @@ class Configuration # @return [Array] attr_reader :scopes_to_filter + # Expose on_breadcrumb_callbacks internally for Bugsnag.leave_breadcrumb + # @api private + # @return [Breadcrumbs::OnBreadcrumbCallbackList] + attr_reader :on_breadcrumb_callbacks + API_KEY_REGEX = /[0-9a-f]{32}/i THREAD_LOCAL_NAME = "bugsnag_req_data" @@ -196,6 +202,7 @@ def initialize # All valid breadcrumb types should be allowable initially self.enabled_automatic_breadcrumb_types = Bugsnag::Breadcrumbs::VALID_BREADCRUMB_TYPES.dup self.before_breadcrumb_callbacks = [] + @on_breadcrumb_callbacks = Breadcrumbs::OnBreadcrumbCallbackList.new(self) # Store max_breadcrumbs here instead of outputting breadcrumbs.max_items # to avoid infinite recursion when creating breadcrumb buffer @@ -506,6 +513,33 @@ def remove_on_error(callback) middleware.remove(callback) end + ## + # Add the given callback to the list of on_breadcrumb callbacks + # + # The on_breadcrumb callbacks will be called when a breadcrumb is left and + # are passed the {Breadcrumbs::Breadcrumb Breadcrumb} object + # + # Returning false from an on_breadcrumb callback will cause the breadcrumb + # to be ignored and will prevent any remaining callbacks from being called + # + # @param callback [Proc, Method, #call] + # @return [void] + def add_on_breadcrumb(callback) + @on_breadcrumb_callbacks.add(callback) + end + + ## + # Remove the given callback from the list of on_breadcrumb callbacks + # + # Note that this must be the same instance that was passed to + # {add_on_breadcrumb}, otherwise it will not be removed + # + # @param callback [Proc, Method, #call] + # @return [void] + def remove_on_breadcrumb(callback) + @on_breadcrumb_callbacks.remove(callback) + end + # TODO: These methods can be a simple attr_accessor when they replace the # methods they are aliasing # NOTE: they are not aliases as YARD doesn't allow documenting the non-alias From 056d110009a89812ad573dedb9c31f072b0a4499 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Fri, 20 Aug 2021 13:53:19 +0100 Subject: [PATCH 5/6] Call on_breadcrumb callbacks in leave_breadcrumb --- lib/bugsnag.rb | 6 ++- spec/bugsnag_spec.rb | 89 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index e8d7ff1a..23ba1029 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -252,7 +252,7 @@ def leave_breadcrumb(name, meta_data={}, type=Bugsnag::Breadcrumbs::MANUAL_BREAD # Skip if it's already invalid return if breadcrumb.ignore? - # Run callbacks + # Run before_breadcrumb_callbacks configuration.before_breadcrumb_callbacks.each do |c| c.arity > 0 ? c.call(breadcrumb) : c.call break if breadcrumb.ignore? @@ -261,6 +261,10 @@ def leave_breadcrumb(name, meta_data={}, type=Bugsnag::Breadcrumbs::MANUAL_BREAD # Return early if ignored return if breadcrumb.ignore? + # Run on_breadcrumb callbacks + configuration.on_breadcrumb_callbacks.call(breadcrumb) + return if breadcrumb.ignore? + # Validate again in case of callback alteration validator.validate(breadcrumb) diff --git a/spec/bugsnag_spec.rb b/spec/bugsnag_spec.rb index dcd373c0..a09520c8 100644 --- a/spec/bugsnag_spec.rb +++ b/spec/bugsnag_spec.rb @@ -417,6 +417,95 @@ module Kernel Bugsnag.leave_breadcrumb("TestName") end end + + describe "on_breadcrumb callbacks" do + it "runs callbacks when a breadcrumb is left" do + Bugsnag.add_on_breadcrumb(proc do |breadcrumb| + breadcrumb.metadata = { callback: true } + end) + + Bugsnag.leave_breadcrumb("TestName") + + expect(breadcrumbs.to_a.size).to eq(1) + expect(breadcrumbs.first.to_h).to match({ + name: "TestName", + type: Bugsnag::BreadcrumbType::MANUAL, + metaData: { callback: true }, + timestamp: match(timestamp_regex) + }) + end + + it "validates any changes made in a callback" do + Bugsnag.add_on_breadcrumb(proc do |breadcrumb| + breadcrumb.metadata = { abc: 123, xyz: { a: 1, b: 2 } } + + breadcrumb.type = "Not a real type" + breadcrumb.name = "123123123123123123123123123123456456456456456" + end) + + Bugsnag.leave_breadcrumb("TestName") + + expect(breadcrumbs.to_a.size).to eq(1) + expect(breadcrumbs.first.to_h).to match({ + name: "123123123123123123123123123123456456456456456", + type: Bugsnag::BreadcrumbType::MANUAL, + metaData: { abc: 123, xyz: { a: 1, b: 2 } }, + timestamp: match(timestamp_regex) + }) + end + + it "doesn't add the breadcrumb when ignored due to enabled_breadcrumb_types" do + Bugsnag.configure do |config| + config.enabled_breadcrumb_types = [Bugsnag::BreadcrumbType::MANUAL] + + config.add_on_breadcrumb(proc do |breadcrumb| + breadcrumb.type = Bugsnag::BreadcrumbType::ERROR + end) + end + + Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::BreadcrumbType::MANUAL, :auto) + + expect(breadcrumbs.to_a).to be_empty + end + + it "stops calling callbacks if the breadcrumb is ignored in them" do + callback1 = spy('callback1') + callback2 = spy('callback2') + + Bugsnag.configure do |config| + config.add_on_breadcrumb(callback1) + config.add_on_breadcrumb(proc { false }) + config.add_on_breadcrumb(callback2) + end + + Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::BreadcrumbType::ERROR, :auto) + + expect(callback1).to have_received(:call) + expect(callback2).not_to have_received(:call) + end + + it "continues calling callbacks after a callback raises" do + callback1 = spy('callback1') + callback2 = spy('callback2') + + Bugsnag.configure do |config| + config.add_on_breadcrumb(callback1) + config.add_on_breadcrumb(proc { raise 'uh oh' }) + config.add_on_breadcrumb(callback2) + end + + Bugsnag.leave_breadcrumb("TestName", {}, Bugsnag::BreadcrumbType::ERROR, :auto) + + expect(callback1).to have_received(:call) + expect(callback2).to have_received(:call) + expect(breadcrumbs.to_a.first.to_h).to match({ + name: "TestName", + type: Bugsnag::BreadcrumbType::ERROR, + metaData: {}, + timestamp: match(timestamp_regex) + }) + end + end end describe "request headers" do From 7545b012fa835200d412120e8350c982cc98d27c Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Mon, 23 Aug 2021 11:20:17 +0100 Subject: [PATCH 6/6] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f138728..30618f79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ Changelog * Sessions will now be delivered every 10 seconds, instead of every 30 seconds | [#680](https://github.com/bugsnag/bugsnag-ruby/pull/680) +* Add `on_breadcrumb` callbacks to replace `before_breadcrumb_callbacks` + | [#686](https://github.com/bugsnag/bugsnag-ruby/pull/686) + ### Fixes * Avoid starting session delivery thread when the current release stage is not enabled @@ -15,6 +18,7 @@ Changelog ### Deprecated +* `before_breadcrumb_callbacks` have been deprecated in favour of `on_breadcrumb` callbacks and will be removed in the next major release * For consistency with Bugsnag notifiers for other languages, a number of methods have been deprecated in this release. The old options will be removed in the next major version | [#676](https://github.com/bugsnag/bugsnag-ruby/pull/676) * The `notify_release_stages` configuration option has been deprecated in favour of `enabled_release_stages` * The `auto_capture_sessions` and `track_sessions` configuration options have been deprecated in favour of `auto_track_sessions`