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

Added unhandled functionality, updated tests and integrations #368

Merged
merged 17 commits into from
Oct 2, 2017
Merged
Show file tree
Hide file tree
Changes from 12 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
30 changes: 27 additions & 3 deletions lib/bugsnag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ def configure

# Explicitly notify of an exception
def notify(exception, auto_notify=false, &block)
if auto_notify && !configuration.auto_notify
report = Report.new(exception, configuration, auto_notify)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create the report before the validation blocks? This is extra processing when a user is running in a testing/debug mode.


if !configuration.auto_notify && auto_notify
configuration.debug("Not notifying because auto_notify is disabled")
return
end
Expand All @@ -49,8 +51,6 @@ def notify(exception, auto_notify=false, &block)
return
end

report = Report.new(exception, configuration)

# If this is an auto_notify we yield the block before the any middleware is run
yield(report) if block_given? && auto_notify
if report.ignore?
Expand All @@ -65,13 +65,27 @@ def notify(exception, auto_notify=false, &block)
return
end

# Store before_middleware severity reason for future reference
before_middleware_severity = report.severity
before_middleware_reason = report.severity_reason

# Run users middleware
configuration.middleware.run(report) do
if report.ignore?
configuration.debug("Not notifying #{report.exceptions.last[:errorClass]} due to ignore being signified in user provided middleware")
return
end

# Update severity reason if necessary
if report.severity != before_middleware_severity
report.severity_reason = {
:type => "userSpecifiedSeverity"
Copy link
Contributor

Choose a reason for hiding this comment

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

The values for severity_reason should be defined as constants to avoid typos and to aggregate all possible values. Currently these values are scattered between different files.

}
end

# Store severity before user callbacks
before_callback_severity = report.severity

# If this is not an auto_notify then the block was provided by the user. This should be the last
# block that is run as it is the users "most specific" block.
yield(report) if block_given? && !auto_notify
Expand All @@ -80,6 +94,16 @@ def notify(exception, auto_notify=false, &block)
return
end

# Test whether severity has been changed and ensure severity_reason is consistant in auto_notify case
if auto_notify
report.severity = before_middleware_severity
report.severity_reason = before_middleware_reason
elsif report.severity != before_callback_severity
report.severity_reason = {
:type => "userCallbackSeverity"
}
end

# Deliver
configuration.info("Notifying #{configuration.endpoint} of #{report.exceptions.last[:errorClass]}")
payload_string = ::JSON.dump(Bugsnag::Helpers.trim_if_needed(report.as_json))
Expand Down
2 changes: 2 additions & 0 deletions lib/bugsnag/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
require "bugsnag/middleware/callbacks"
require "bugsnag/middleware/exception_meta_data"
require "bugsnag/middleware/ignore_error_class"
require "bugsnag/middleware/classify_error"

module Bugsnag
class Configuration
Expand Down Expand Up @@ -73,6 +74,7 @@ def initialize
self.internal_middleware = Bugsnag::MiddlewareStack.new
self.internal_middleware.use Bugsnag::Middleware::ExceptionMetaData
self.internal_middleware.use Bugsnag::Middleware::IgnoreErrorClass
self.internal_middleware.use Bugsnag::Middleware::ClassifyError

self.middleware = Bugsnag::MiddlewareStack.new
self.middleware.use Bugsnag::Middleware::Callbacks
Expand Down
6 changes: 6 additions & 0 deletions lib/bugsnag/integrations/delayed_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ def error(job, error)

::Bugsnag.notify(error, true) do |report|
report.severity = "error"
report.set_handled_state({
Copy link
Contributor

Choose a reason for hiding this comment

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

set_handled_state doesn't seem to be defined anywhere.

:type => "unhandledExceptionMiddleware",
:attributes => {
:framework => "DelayedJob"
}
})
report.meta_data.merge! overrides
end

Expand Down
6 changes: 6 additions & 0 deletions lib/bugsnag/integrations/mailman.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ def call(mail)
raise ex if [Interrupt, SystemExit, SignalException].include? ex.class
Bugsnag.notify(ex, true) do |report|
report.severity = "error"
report.set_handled_state({
:type => "unhandledExceptionMiddleware",
:attributes => {
:framework => "Mailman"
}
})
end
raise
ensure
Expand Down
12 changes: 12 additions & 0 deletions lib/bugsnag/integrations/rack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ def call(env)
# Notify bugsnag of rack exceptions
Bugsnag.notify(raised, true) do |report|
report.severity = "error"
report.set_handled_state({
:type => "unhandledExceptionMiddleware",
:attirbutes => {
Copy link
Contributor

Choose a reason for hiding this comment

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

"attirbutes"

:framework => "Rack"
}
})
end

# Re-raise the exception
Expand All @@ -45,6 +51,12 @@ def call(env)
if env["rack.exception"]
Bugsnag.notify(env["rack.exception"], true) do |report|
report.severity = "error"
report.set_handled_state({
:type => "middleware_handler",
:attributes => {
:name => "rack"
Copy link
Contributor

Choose a reason for hiding this comment

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

Rack above is capitalized, while this is not. Integration names are another candidate for a constant enumeration.

}
})
end
end

Expand Down
6 changes: 6 additions & 0 deletions lib/bugsnag/integrations/rails/active_record_rescue.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ def run_callbacks(kind, *args, &block)
# This exception will NOT be escalated, so notify it here.
Bugsnag.notify(exception, true) do |report|
report.severity = "error"
report.set_handled_state({
:type => "unhandledExceptionMiddleware",
:attributes => {
:framework => "Rails"
}
})
end
raise
end
Expand Down
6 changes: 6 additions & 0 deletions lib/bugsnag/integrations/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ class Railtie < Rails::Railtie
if $!
Bugsnag.notify($!, true) do |report|
report.severity = "error"
report.set_handled_state({
:type => "unhandledExceptionMiddleware",
:attributes => {
:framework => "Rails"
}
})
end
end
end
Expand Down
6 changes: 6 additions & 0 deletions lib/bugsnag/integrations/rake.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ def execute_with_bugsnag(args=nil)
rescue Exception => ex
Bugsnag.notify(ex, true) do |report|
report.severity = "error"
report.set_handled_state({
:type => "unhandledExceptionMiddleware",
:attributes => {
:framework => "Rake"
}
})
end
raise
ensure
Expand Down
6 changes: 6 additions & 0 deletions lib/bugsnag/integrations/resque.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ def self.add_failure_backend
def save
Bugsnag.notify(exception, true) do |report|
report.severity = "error"
report.set_handled_state({
:type => "unhandledExceptionMiddleware",
:attributes => {
:framework => "Resque"
}
})
report.meta_data.merge!({:context => "#{payload['class']}@#{queue}", :payload => payload, :delivery_method => :synchronous})
end
end
Expand Down
6 changes: 6 additions & 0 deletions lib/bugsnag/integrations/sidekiq.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ def call(worker, msg, queue)
raise ex if [Interrupt, SystemExit, SignalException].include? ex.class
Bugsnag.notify(ex, true) do |report|
report.severity = "error"
report.set_handled_states({
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above, set_handled_states (trailing 's') doesn't seem to be defined anywhere.

:type => 'unhandledExceptionMiddleware',
:attributes => {
:framework => "Sidekiq"
}
})
end
raise
ensure
Expand Down
45 changes: 45 additions & 0 deletions lib/bugsnag/middleware/classify_error.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
module Bugsnag::Middleware
class ClassifyError
INFO_CLASSES = [
"AbstractController::ActionNotFound",
"ActionController::InvalidAuthenticityToken",
"ActionController::ParameterMissing",
"ActionController::UnknownAction",
"ActionController::UnknownFormat",
"ActionController::UnknownHttpMethod",
"ActiveRecord::RecordNotFound",
"CGI::Session::CookieStore::TamperedWithCookie",
"Mongoid::Errors::DocumentNotFound",
"SignalException",
"SystemExit"
]

def initialize(bugsnag)
@bugsnag = bugsnag
end

def call(report)
report.raw_exceptions.each do |ex|
ancestor_chain = ex.class.ancestors.select { |ancestor| ancestor.is_a?(Class) }.map { |ancestor| ancestor.to_s }
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking the blocks onto separate lines would make this expression more readable.


INFO_CLASSES.each do |info_class|
if ancestor_chain.include?(info_class)
report.set_handled_state(
{
:type => "error_class",
:attributes => {
:errorClass => info_class
}
}
)
report.severity = 'info'
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded semicolon

end
end
end

@bugsnag.call(report)
end
end
end

9 changes: 8 additions & 1 deletion lib/bugsnag/report.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ class Report
attr_accessor :raw_exceptions
attr_accessor :release_stage
attr_accessor :severity
attr_accessor :severity_reason
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be declared as attr_reader, since its only being set internally by a method (I think that was the intent of set_handled_states). It would prevent accidental overrides by other code/callback writers.

attr_accessor :user

def initialize(exception, passed_configuration)
def initialize(exception, passed_configuration, auto_notify=false)
@should_ignore = false
@unhandled = auto_notify

self.configuration = passed_configuration

Expand All @@ -43,6 +45,9 @@ def initialize(exception, passed_configuration)
self.meta_data = {}
self.release_stage = configuration.release_stage
self.severity = "warning"
self.severity_reason = {
:type => "handledException"
}
self.user = {}
end

Expand Down Expand Up @@ -84,6 +89,8 @@ def as_json
groupingHash: grouping_hash,
payloadVersion: CURRENT_PAYLOAD_VERSION,
severity: severity,
severityReason: severity_reason,
unhandled: @unhandled,
user: user
}

Expand Down
4 changes: 3 additions & 1 deletion lib/bugsnag/tasks/bugsnag.rake
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ namespace :bugsnag do
begin
raise RuntimeError.new("Bugsnag test exception")
rescue => e
Bugsnag.notify(e, {:context => "rake#test_exception"})
Bugsnag.notify(e) do |report|
report.context = "rake#test_exception"
end
end
end
end
Expand Down
64 changes: 64 additions & 0 deletions spec/middleware_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,68 @@ def call(report)
}
end

it "doesn't allow handledState properties to be changed in middleware" do
HandledStateChanger = Class.new do
def initialize(bugsnag)
@bugsnag = bugsnag
end

def call(report)
report.set_handled_state({
:test => "test"
})
@bugsnag.call(report)
end
end

Bugsnag.configure do |c|
c.middleware.use HandledStateChanger
end

Bugsnag.notify(BugsnagTestException.new("It crashed"), true) do |report|
report.set_handled_state({
:type => "middleware_handler",
:attributes => {
:name => "middleware_test"
}
})
end

expect(Bugsnag).to have_sent_notification{ |payload|
event = get_event_from_payload(payload)
expect(event["unhandled"]).to be true
expect(event["severityReason"]).to eq({
"type" => "middleware_handler",
"attributes" => {
"name" => "middleware_test"
}
})
}
end

it "sets defaultSeverity to false if changed in middleware" do
SeverityChanger = Class.new do
def initialize(bugsnag)
@bugsnag = bugsnag
end

def call(report)
report.severity = "info"
@bugsnag.call(report)
end
end

Bugsnag.configure do |c|
c.middleware.use SeverityChanger
end

Bugsnag.notify(BugsnagTestException.new("It crashed"))

expect(Bugsnag).to have_sent_notification{ |payload|
event = get_event_from_payload(payload)
expect(event["severity"]).to eq("info")
expect(event["defaultSeverity"]).to be false
}
end

end
Loading