Mark Bugsnag frames as out of project #497

Cawllec · 2018-11-19T16:06:18Z

Goal

To enable easier examination of the Bugsnag reporting process, and why certain events appear as they do.

Design

Rather than dropping frames from lib/bugsnag they are now included, but will automatically be marked out of project.

Tests

Unit test added to ensure Bugsnag frames are added correctly and inProject is not set.

Will require #496 to be merged first so that Maze tests can be added (in progress)

Review

For the submitter, initial self-review:

Commented on code changes inline explain the reasoning behind the approach
Reviewed the test cases added for completeness and possible points for discussion
A changelog entry was added for the goal of this pull request
Check the scope of the changeset - is everything in the diff required for the pull request?
This pull request is ready for:
- Initial review of the intended approach, not yet feature complete
- Structural review of the classes, functions, and properties modified
- Final review

For the pull request reviewer(s), this changeset has been reviewed for:

Consistency across platforms for structures or concepts added or modified
Consistency between the changeset and the goal stated above
Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
Usage friction - is the proposed change in usage cumbersome or complicated?
Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
Concurrency concerns - if components are accessed asynchronously, what issues will arise
Thoroughness of added tests and any missing edge cases
Idiomatic use of the language

spec/report_spec.rb

spec/stacktrace_spec.rb

spec/spec_helper.rb

lib/bugsnag/stacktrace.rb

Cawllec · 2018-11-23T12:38:08Z

I've fixed the maze tests that this change has broken.

snmaynard · 2018-11-23T17:18:59Z

features/plain_features/report_stack_frames.feature

@@ -14,8 +14,8 @@ Scenario Outline: Stack frames can be removed
  And the request used the "Ruby Bugsnag Notifier" notifier
  And the request used payload v4 headers
  And the request contained the api key "a35a2a72bd230ac0aa0f52715bbdc6aa"
-  And the "file" of stack frame 0 equals "/usr/src/app/stack_frame_modification/initiators/<initiator>.rb"


You should instead update the features to raise the exception so that bugsnag doesnt appear in the stacktrace

This comment is still valid. The tests that dont need it should be set back to stack frame 0

I've updated the scenarios to separate the tests that require this change and those that don't.

Cawllec · 2018-11-27T15:37:50Z

Additional maze changes have been made to ensure stacks from different types of handled errors (a string vs a caught exception) are both tested.

snmaynard · 2018-11-27T21:22:56Z

spec/report_spec.rb

+          expect(frame["inProject"]).to be_nil
+        end
+      end
+      expect(bugsnag_count).to be > 0


This should be a specific number rather than > 0

snmaynard · 2018-11-27T21:25:40Z

features/plain_features/report_stack_frames.feature

@@ -14,8 +14,8 @@ Scenario Outline: Stack frames can be removed
  And the request used the "Ruby Bugsnag Notifier" notifier
  And the request used payload v4 headers
  And the request contained the api key "a35a2a72bd230ac0aa0f52715bbdc6aa"
-  And the "file" of stack frame 0 equals "/usr/src/app/stack_frame_modification/initiators/<initiator>.rb"


This comment is still valid. The tests that dont need it should be set back to stack frame 0

tobyhs · 2018-11-27T21:34:24Z

features/steps/ruby_notifier_steps.rb

+end
+Then(/^the "(.+)" of the top project stackframe equals "(.+)"(?: for request (\d+))?$/) do |element, value, request_index|
+  stacktrace = read_key_path(find_request(request_index)[:body], 'events.0.exceptions.0.stacktrace')
+  frame_index = 0


I think you can use Array#find_index to make this more concise (probably something like frame_index = stacktrace.find_index { |frame| frame['file'] !~ %r{lib/bugsnag.*\.rb\z} })

tobyhs · 2018-11-27T21:37:26Z

features/steps/ruby_notifier_steps.rb

@@ -12,4 +12,30 @@
  steps %Q{
    When I set environment variable "#{env_var}" to "#{credentials}@#{current_ip}:#{MOCK_API_PORT}"
  }
+end
+Then(/^the "(.+)" of the top project stackframe equals "(.+)"(?: for request (\d+))?$/) do |element, value, request_index|


Can we have a blank line between step definitions for organization/easier editor navigation? (between lines 15-16, 28-29)

tobyhs · 2018-11-27T21:45:00Z

features/steps/ruby_notifier_steps.rb

+  stacktrace = read_key_path(find_request(request_index)[:body], 'events.0.exceptions.0.stacktrace')
+  frame_index = 0
+  stacktrace.each do |frame, index|
+    unless /.*lib\/bugsnag.*\.rb/.match(frame["file"])


The step definition pattern has "of the top project stackframe", so should this actually check the inProject value instead?

Otherwise, if checking against this regular expression is correct, should the step definition pattern have something like "of the top non-Bugsnag stackframe" instead?

tobyhs · 2018-11-27T21:49:35Z

features/steps/ruby_notifier_steps.rb

+    the "#{element}" of stack frame #{frame_index} equals "#{value}"
+  }
+end
+Then(/^the "(.+)" of the top project stackframe equals (\d+)(?: for request (\d+))?$/) do |element, value, request_index|


I don't think you need 2 very similar step definitions; you can probably get away with using (\d+|(".+")) (so the double quotes are part of the capture for the string case and you can just have equals #{value})

…rame ordering

…in tests

tobyhs · 2018-11-29T01:41:58Z

spec/stacktrace_spec.rb

+        (starting_line + 0).to_s => "      _a = 1",
+        (starting_line + 1).to_s => "      _b = 2",
+        (starting_line + 2).to_s => "      _c = 3",
+        (starting_line + 3).to_s => "      \"Test\".prepnd \"T\"",


This might be easier to read if you used single quotes on the outside so you don't have to escape the double quotes inside.

tobyhs · 2018-11-29T01:58:54Z

features/plain_features/report_stack_frames.feature

+  And the "lineNumber" of the top non-bugsnag stackframe equals <lineNumber>
+
+  Examples:
+  | ruby version | lineNumber |


They are all the same line number because you're using the running the same file, so this lineNumber column isn't needed

tobyhs · 2018-11-29T02:24:15Z

features/plain_features/handled_errors.feature

@@ -67,8 +67,8 @@ Scenario Outline: A handled error can attach metadata in a block
  And the event "severity" equals "warning"
  And the event "severityReason.type" equals "handledException"
  And the exception "errorClass" equals "RuntimeError"
-  And the "file" of stack frame 0 equals "/usr/src/app/handled/block_metadata.rb"


It seems like lines 70-71 can remain as is (with "stack frame 0" instead of "the top non-bugsnag stackframe") (similar comment may apply to couple other places)

Removed, and broken the top most scenario out into two separate scenarios for the same reason.

tobyhs

lgtm

tobyhs · 2018-11-29T18:24:27Z

features/plain_features/report_stack_frames.feature

  | 2.5          | unhandled_before_notify | 21         |

+Scenario Outline: Stack frames can be removed


Perhaps this Scenario Outline string should be adjusted as it is identical to the previous scenario outline's string

Comments have been addressed

Cawllec added 5 commits November 16, 2018 17:20

Bugsnag frames InProject: Remove nilification of Bugsnag lib frames

1360276

Bugsnag frames InProject: Update report spec frames

60a8729

Bugsnag frames InProject: Ensure correct frame is passed back

179f3be

Bugsnag frame InProject: Allow non-top frame to be selected

a99e452

Bugsnag frames InProject: Add Bugsnag frames tests

02bf1c9

snmaynard suggested changes Nov 19, 2018

View reviewed changes

tobyhs reviewed Nov 20, 2018

View reviewed changes

lib/bugsnag/stacktrace.rb Outdated Show resolved Hide resolved

Bugsnag frames InProject: Cleared up comments and tests

eb8ae62

snmaynard previously approved these changes Nov 20, 2018

View reviewed changes

Cawllec added 2 commits November 22, 2018 16:05

Merge branch 'master' into cawllec/in-project-bugsnag

59af113

Bugsnag frames inproject: Updated maze tests

9e14ecf

Cawllec dismissed snmaynard’s stale review via 9e14ecf November 23, 2018 12:37

snmaynard reviewed Nov 23, 2018

View reviewed changes

Cawllec added 2 commits November 26, 2018 14:27

Bugsnag frame inproject: Updates rails tests

90594cf

Bugsnag frames inproject: Updates to stackframe maze tests

07ea1bf

snmaynard previously requested changes Nov 27, 2018

View reviewed changes

tobyhs reviewed Nov 27, 2018

View reviewed changes

Cawllec added 2 commits November 28, 2018 16:38

Bugsnag frames in-project: Updated tests to better reflect new stackf…

25b5b94

…rame ordering

Bugsnag frames in-project: Accounted for JRUBY stackframes differing …

0449c44

…in tests

tobyhs reviewed Nov 29, 2018

View reviewed changes

Bugsnag frames in-project: Separated notified strings tests out

4c5ea1a

tobyhs approved these changes Nov 29, 2018

View reviewed changes

Bugsnag frames in-project: Make scenario names more explicit

3256f77

Cawllec merged commit c8938c9 into master Nov 30, 2018

Cawllec deleted the cawllec/in-project-bugsnag branch November 30, 2018 08:32

Cawllec mentioned this pull request Nov 30, 2018

v6.10.0 Release #502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark Bugsnag frames as out of project #497

Mark Bugsnag frames as out of project #497

Cawllec commented Nov 19, 2018

Cawllec commented Nov 23, 2018

snmaynard Nov 23, 2018

snmaynard Nov 27, 2018

Cawllec Nov 29, 2018

Cawllec commented Nov 27, 2018

snmaynard Nov 27, 2018

snmaynard Nov 27, 2018

tobyhs Nov 27, 2018

tobyhs Nov 27, 2018

tobyhs Nov 27, 2018

tobyhs Nov 27, 2018

tobyhs Nov 29, 2018

tobyhs Nov 29, 2018

Cawllec Nov 29, 2018

tobyhs Nov 29, 2018

Cawllec Nov 29, 2018

tobyhs left a comment

tobyhs Nov 29, 2018

		\| 2.5 \| unhandled_before_notify \| 21 \|

		Scenario Outline: Stack frames can be removed

Mark Bugsnag frames as out of project #497

Mark Bugsnag frames as out of project #497

Conversation

Cawllec commented Nov 19, 2018

Goal

Design

Tests

Review

Cawllec commented Nov 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cawllec commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobyhs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment