bugsnag / bugsnag-ruby

BugSnag error monitoring & reporting software for rails, sinatra, rack and ruby
https://docs.bugsnag.com/platforms/ruby
MIT License
246 stars 174 forks source link

[feature request] Can we support disable breadcrumb when `enabled_breadcrumb_types` config is empty? #794

Closed liijunwei closed 11 months ago

liijunwei commented 11 months ago

didn't find related issue / discussion

Description

Given

  1. our application uses bugsnag(v6.24.0), I've checked master branch, it's the same logic
  2. our application uses ActiveSupport::Cache::MemoryStore, which will emit many instrumentation events
  3. we set bugsnag enabled_breadcrumb_types configuration to empty array as we don't really depend on it
  4. bugsnag subscribed a few events instrumented by MemoryStore

Expected

when enabled_breadcrumb_types is empty:

  1. we don't want any breadcrumbs collected or sent
  2. breadcrumbs related computation should be saved

Actual

  1. we don't want any breadcrumbs collected or sent(✅)
  2. breadcrumbs related computation should be saved(❌)

our profiling result shows that Bugsnag.leave_breadcrumb accounts for a big portion of api response time, mainly from: https://github.com/bugsnag/bugsnag-ruby/blob/bdfbf3972f2137b3b9b95194a6bedf9c8edf6555/lib/bugsnag.rb#L282-L287

Describe the solution you'd like

when configuration.enabled_breadcrumb_types is empty, this part of initialization/validation is a waste, better get rid of it

either one of below works for us

  1. check configuration.enabled_breadcrumb_types.empty? before Bugsnag::Breadcrumbs::Breadcrumb.new
  2. add a configuration option to disable breadcrumb at all
  3. make Bugsnag::Rails::DEFAULT_RAILS_BREADCRUMBS configurable

Describe alternatives you've considered

NA

Additional context

We're heavily relying on ActiveSupport::Cache::MemoryStore, and this will give us a significant performance improvement

if the request sounds reasonable to you, I'm glad to prepare a pull request, thank you

profiling example

# 1. run with rails + bugsnag integration
# 2. open /tmp/
# 3. drag profiling result to https://www.speedscope.app/

require 'stackprof'
require 'active_support'
require 'json'

def profile(filename, warmup_cycles: 0, &block)
  warmup_cycles.times { block.call }

  result = StackProf.run(mode: :wall, aggregate: false, raw: true, &block)

  path = "/tmp/#{filename}"
  File.open("/tmp/#{filename}", 'w') { |f| f << JSON.generate(result) }
  puts "store to file: path :#{path}"
end

store = ActiveSupport::Cache::MemoryStore.new(compress: false)

output_file = "bugsnag-breadcrumbs-#{Time.now.strftime("%m-%d-%Y.%H.%M.%S")}.json"
profile(output_file, warmup_cycles: 1) do
  1_000.times do
    store.fetch(:foo, expires_in: rand(10)) { :bar }
  end
end
liijunwei commented 11 months ago

looks like false analysis after we released our patch to production, closing this for now