Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give the configuration context precedence over context in integrations #688

Merged
merged 5 commits into from
Aug 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ Changelog
| [#681](https://github.com/bugsnag/bugsnag-ruby/pull/681)
* Add `on_breadcrumb` callbacks to replace `before_breadcrumb_callbacks`
| [#686](https://github.com/bugsnag/bugsnag-ruby/pull/686)
* Add `context` attribute to configuration, which will be used as the default context for events
* Add `context` attribute to configuration, which will be used as the default context for events. Using this option will disable automatic context setting
| [#687](https://github.com/bugsnag/bugsnag-ruby/pull/687)
| [#688](https://github.com/bugsnag/bugsnag-ruby/pull/688)

### Fixes

Expand Down
13 changes: 13 additions & 0 deletions lib/bugsnag/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ class Configuration
attr_accessor :vendor_path

# The default context for all future events
# Setting this will disable automatic context setting
# @return [String, nil]
attr_accessor :context

Expand Down Expand Up @@ -552,6 +553,18 @@ def remove_on_breadcrumb(callback)
@on_breadcrumb_callbacks.remove(callback)
end

##
# Has the context been explicitly set?
#
# This is necessary to differentiate between the context not being set and
# the context being set to 'nil' explicitly
#
# @api private
# @return [Boolean]
def context_set?
defined?(@context) != nil
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
Expand Down
2 changes: 1 addition & 1 deletion lib/bugsnag/integrations/resque.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def save

context = "#{class_name}@#{queue}"
report.meta_data.merge!({ context: context, payload: metadata })
report.context = context
report.automatic_context = context
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/bugsnag/middleware/active_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def call(report)

if data
report.add_tab(:active_job, data)
report.context = "#{data[:job_name]}@#{data[:queue]}"
report.automatic_context = "#{data[:job_name]}@#{data[:queue]}"
end

@bugsnag.call(report)
Expand Down
2 changes: 1 addition & 1 deletion lib/bugsnag/middleware/delayed_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def call(report)
payload_data = construct_job_payload(job.payload_object)

context = get_context(payload_data, job_data[:active_job])
report.context = context unless context.nil?
report.automatic_context = context unless context.nil?

job_data[:payload] = payload_data
end
Expand Down
2 changes: 2 additions & 0 deletions lib/bugsnag/middleware/exception_meta_data.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ def call(report)

if exception.respond_to?(:bugsnag_context)
context = exception.bugsnag_context
# note: this should set 'context' not 'automatic_context' as it's a
# user-supplied value
report.context = context if context.is_a?(String)
end

Expand Down
4 changes: 2 additions & 2 deletions lib/bugsnag/middleware/rack_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ def call(report)
client_ip = request.ip.to_s rescue SPOOF
session = env["rack.session"]

# Set the context
report.context = "#{request.request_method} #{request.path}"
# Set the automatic context
report.automatic_context = "#{request.request_method} #{request.path}"

# Set a sensible default for user_id
report.user["id"] = request.ip
Expand Down
4 changes: 2 additions & 2 deletions lib/bugsnag/middleware/rails3_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ def call(report)
client_ip = env["action_dispatch.remote_ip"].to_s rescue SPOOF

if params
# Set the context
report.context = "#{params[:controller]}##{params[:action]}"
# Set the automatic context
report.automatic_context = "#{params[:controller]}##{params[:action]}"

# Augment the request tab
report.add_tab(:request, {
Expand Down
2 changes: 1 addition & 1 deletion lib/bugsnag/middleware/rake.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def call(report)
:arguments => task.arg_description
})

report.context ||= task.name
report.automatic_context ||= task.name
end

@bugsnag.call(report)
Expand Down
2 changes: 1 addition & 1 deletion lib/bugsnag/middleware/sidekiq.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def call(report)
sidekiq = report.request_data[:sidekiq]
if sidekiq
report.add_tab(:sidekiq, sidekiq)
report.context ||= "#{sidekiq[:msg]['wrapped'] || sidekiq[:msg]['class']}@#{sidekiq[:msg]['queue']}"
report.automatic_context ||= "#{sidekiq[:msg]['wrapped'] || sidekiq[:msg]['class']}@#{sidekiq[:msg]['queue']}"
end
@bugsnag.call(report)
end
Expand Down
25 changes: 20 additions & 5 deletions lib/bugsnag/report.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@ class Report
# @return [Configuration]
attr_accessor :configuration

# Additional context for this report
# @return [String, nil]
attr_accessor :context

# The delivery method that will be used for this report
# @see Configuration#delivery_method
# @return [Symbol]
Expand Down Expand Up @@ -118,7 +114,7 @@ def initialize(exception, passed_configuration, auto_notify=false)
self.app_type = configuration.app_type
self.app_version = configuration.app_version
self.breadcrumbs = []
self.context = configuration.context
self.context = configuration.context if configuration.context_set?
self.delivery_method = configuration.delivery_method
self.hostname = configuration.hostname
self.runtime_versions = configuration.runtime_versions.dup
Expand All @@ -129,6 +125,25 @@ def initialize(exception, passed_configuration, auto_notify=false)
self.user = {}
end

##
# Additional context for this report
# @!attribute context
# @return [String, nil]
def context
return @context if defined?(@context)

@automatic_context
end

attr_writer :context

##
# Context set automatically by Bugsnag uses this attribute, which prevents
# it from overwriting the user-supplied context
# @api private
# @return [String, nil]
attr_accessor :automatic_context

##
# Add a new metadata tab to this notification.
#
Expand Down
2 changes: 1 addition & 1 deletion lib/bugsnag/tasks/bugsnag.rake
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace :bugsnag do
raise RuntimeError.new("Bugsnag test exception")
rescue => e
Bugsnag.notify(e) do |report|
report.context = "rake#test_exception"
report.automatic_context = "rake#test_exception"
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions spec/integrations/rack_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class Request
allow(report).to receive(:request_data).and_return({
:rack_env => rack_env
})
expect(report).to receive(:context=).with("TEST /TEST_PATH")
expect(report).to receive(:automatic_context=).with("TEST /TEST_PATH")
expect(report).to receive(:user).and_return({})

config = Bugsnag.configuration
Expand Down Expand Up @@ -185,7 +185,7 @@ class Request
allow(report).to receive(:request_data).and_return({
:rack_env => rack_env
})
expect(report).to receive(:context=).with("TEST /TEST_PATH")
expect(report).to receive(:automatic_context=).with("TEST /TEST_PATH")
expect(report).to receive(:user).and_return({})

config = Bugsnag.configuration
Expand Down
4 changes: 2 additions & 2 deletions spec/integrations/rake_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
:description => "TEST_COMMENT",
:arguments => "TEST_ARGS"
})
expect(report).to receive(:context).with(no_args)
expect(report).to receive(:context=).with("TEST_NAME")
expect(report).to receive(:automatic_context).with(no_args)
expect(report).to receive(:automatic_context=).with("TEST_NAME")

expect(callback).to receive(:call).with(report)

Expand Down
2 changes: 1 addition & 1 deletion spec/integrations/resque_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def require(path)
"class" => "class"
}
})
expect(report).to receive(:context=).with(expected_context)
expect(report).to receive(:automatic_context=).with(expected_context)
expect(Bugsnag).to receive(:notify).with(exception, true).and_yield(report)
resque.save
end
Expand Down
57 changes: 57 additions & 0 deletions spec/report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,63 @@ def gloops
}
end

it "uses automatic context if no other context has been set" do
Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report|
report.automatic_context = "automatic context"
end

expect(Bugsnag).to(have_sent_notification { |payload, _headers|
event = get_event_from_payload(payload)
expect(event["context"]).to eq("automatic context")
})
end

it "uses Configuration context even if the automatic context has been set" do
Bugsnag.configure do |config|
config.context = "configuration context"
end

Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report|
report.automatic_context = "automatic context"
end

expect(Bugsnag).to(have_sent_notification { |payload, _headers|
event = get_event_from_payload(payload)
expect(event["context"]).to eq("configuration context")
})
end

it "uses overridden context even if the automatic context has been set" do
Bugsnag.configure do |config|
config.context = "configuration context"
end

Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report|
report.context = "overridden context"
report.automatic_context = "automatic context"
end

expect(Bugsnag).to(have_sent_notification { |payload, _headers|
event = get_event_from_payload(payload)
expect(event["context"]).to eq("overridden context")
})
end

it "uses overridden context even it is set to 'nil'" do
Bugsnag.configure do |config|
config.context = nil
end

Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report|
report.automatic_context = "automatic context"
end

expect(Bugsnag).to(have_sent_notification { |payload, _headers|
event = get_event_from_payload(payload)
expect(event["context"]).to be_nil
})
end

it "uses the context from Configuration, if set" do
Bugsnag.configure do |config|
config.context = "example context"
Expand Down