Breadcrumbs: Update reports class, add Breadcrumbs middleware #512
Conversation
lib/bugsnag/report.rb
Outdated
| filter_cleaner = Bugsnag::Cleaner.new(configuration.meta_data_filters) | ||
| payload_event[:metaData] = filter_cleaner.clean_object(meta_data) | ||
| payload_event[:breadcrumbs] = breadcrumbs.map do |raw_crumb| | ||
| breadcrumb = raw_crumb.to_h |
There was a problem hiding this comment.
if breadcrumbs is an Array of Breadcrumb objects, should one of these be breadcrumb (instead of raw_crumb)? It seems like the variable names should be reversed, as raw implies something more primitive.
Alternatively, the block variable can be breadcrumb but the converted one can be named something like breadcrumb_hash.
spec/report_spec.rb
Outdated
| it "includes left breadcrumbs" do | ||
| Bugsnag.leave_breadcrumb("Test breadcrumb") | ||
| notify_test_exception | ||
| expect(Bugsnag).to have_sent_notification{ |payload, headers| |
There was a problem hiding this comment.
Use do + end, although I suspect there could be some Ruby precedence issue that prevents this from working; if that's the case, add a space before the { (same comment applies to a few other lines below)
There was a problem hiding this comment.
Currently the have_sent_notification method doesn't handled do end blocks, only {} blocks. This may be something to look at in a longer term ticket.
spec/report_spec.rb
Outdated
| expect(report.name).to eq("ZeroDivisionError") | ||
| expect(report.message).to eq("divided by 0") | ||
|
|
||
| expect(report.summary).to match({ |
There was a problem hiding this comment.
This should probably use eq instead of match (same with line 1185)
lib/bugsnag/report.rb
Outdated
| self.meta_data = {} | ||
| self.name = exception.class.to_s |
There was a problem hiding this comment.
If exception is a string (which is likely true if exception doesn't respond to message) instead of an exception, would it make more sense to default to RuntimeError instead (because code later on will convert it to that)?
There was a problem hiding this comment.
I think the breadcrumb name should reflect what was notified explicitly. In this case the result would be String which on a breadcrumb with limited information I think is more telling than RuntimeError would be.
There was a problem hiding this comment.
Users will use this information to try and find the error in the UI so switching between class and message will be confusing.
There was a problem hiding this comment.
That's fair, I hadn't considered it in that context.
lib/bugsnag/report.rb
Outdated
| self.meta_data = {} | ||
|
|
||
| # Notified strings display as RuntimeErrors in the dashboard | ||
| self.name = exception.is_a?(Exception) ? exception.class.to_s : RuntimeError.to_s |
There was a problem hiding this comment.
Should this check match the check in #generate_raw_exceptions? (so that it calls #to_exception or #exception if applicable, and also check that it isn't a Java::JavaLang::Throwable for jruby; this might require a little refactoring)
lib/bugsnag/report.rb
Outdated
| # @return [Hash] a Hash containing the report's name, message, and severity | ||
| def summary | ||
| { | ||
| :name => name, |
There was a problem hiding this comment.
I think the key name here should be class or error_class instead to align with the Data Access API filters and field names (slightly leaning towards error_class).
lib/bugsnag/report.rb
Outdated
| attr_accessor :meta_data | ||
| attr_accessor :name |
There was a problem hiding this comment.
After digging through the existing code some more, it looks like it would be easier to define a plain name method (instead of attr_accessor :name) that returns exceptions[0][:errorClass] as that would be consistent (because the exceptions attribute also makes the exception go through the #error_class method to find its error class or "name", which your code further below doesn't do now).
There was a problem hiding this comment.
Keeping the message and name as it is now allows the user to override how the follow-up breadcrumb will be displayed. If they want to assign a custom name/message that will allow them to navigate breadcrumbs more easily they should be able to.
lib/bugsnag/report.rb
Outdated
| attr_accessor :configuration | ||
| attr_accessor :context | ||
| attr_accessor :delivery_method | ||
| attr_accessor :exceptions | ||
| attr_accessor :hostname | ||
| attr_accessor :grouping_hash | ||
| attr_accessor :message |
There was a problem hiding this comment.
Similar to the comment a few lines after this, I think you can remove this and change your #summary method to use exceptions[0][:message]
Breadcrumbs/Report: Make summary more accurate
lib/bugsnag/report.rb
Outdated
| # @return [Hash] a Hash containing the report's error class, error message, and severity | ||
| def summary | ||
| # Guard against the exceptions array being removed/changed or emptied here | ||
| if !exceptions.is_a?(Array) || exceptions.first.nil? |
There was a problem hiding this comment.
I think this would be easier to read if you changed the condition to exceptions.respond_to?(:first) && exceptions.first and put the known error class case first.
Goal
Adds breadcrumbs middleware and enables the report to receive them.