Allow vendor_path to be configured #544

jnraine · 2019-06-10T15:13:09Z

Goal

By default, lines within vendor/ or .bundle/ are consider “out of project”. This is a good default for most Rails projects but there are other directories we’d also like to ignore. For example, patches to gems or the framework. If left as “in project”, these lines cause multiple issues to be grouped together because the first line in the stacktrace always points to the same patch line.

Right now, we’ve side stepped this by reaching into the gem and redefining Bugsnag::Stacktrace::VENDOR_PATH but it would be wonderful if this was a configurable option like any other.

Changeset

Added

vendor_path can now be configured.

This allows developers to set the vendor path regular expression, used to determine whether or not a line in the stacktrace appears in the project stacktrace or the full trace.

Bugsnag.configure do |config|
  config.vendor_path = /(^(vendor\/|\.bundle\/|extensions\/))/
end

Tests

Test for the configuration setter and getter were added.
Tests to ensure the new configuration option was respected was added for Bugsnag::Stacktrace.

Discussion

Outstanding Questions

Should the name of the configuration option be changed? Instead of vendor_path, perhaps project_file_pattern or similar.

Review

For the submitter, initial self-review:

~~Commented on code changes inline explain the reasoning behind the approach~~ n/a
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

This allows developers to set the vendor path regular expression, used to determine whether or not a line in the stacktrace appears in the project stacktrace or the full trace. ```ruby Bugsnag.configure do |config| config.vendor_path = /(^(vendor\/|\.bundle\/|extensions\/|private_gems\/))/ end ``` By default, lines within `vendor/` or `.bundle/` are consider “out of project”. This is a good default for most Rails projects but there are other directories we’d also like to ignore. For example, patches to gems or the framework. If left as “in project”, these lines cause multiple issues to be grouped together because the first line in the stacktrace always points to the same patch line. Right now, we’ve side stepped this by reaching into the gem and redefining `Bugsnag::Stacktrace::VENDOR_PATH` but it would be wonderful if this was a configurable option like any other.

jnraine · 2019-06-10T22:46:42Z

CI failure seems unrelated to this changeset:

$ bundle _${BUNDLE_VERSION}_ install --with "$GEMSETS" --binstubs
Fetching https://github.com/bugsnag/maze-runner
Fetching gem metadata from https://rubygems.org/..........
Fetching version metadata from https://rubygems.org/...
Fetching dependency metadata from https://rubygems.org/..
Resolving dependencies....
Installing rake 12.3.2
Installing concurrent-ruby 1.1.5
Installing minitest 5.11.3
Installing thread_safe 0.3.6
Installing addressable 2.3.8
Installing coderay 1.1.2
Installing connection_pool 2.2.2
Installing safe_yaml 1.0.5
Installing diff-lcs 1.3
Installing hashdiff 0.4.0
Installing method_source 0.9.2
Installing rack 2.0.7
Installing rdoc 5.1.0
Installing redis 4.1.2                                    -----
Gem::InstallError: redis requires Ruby version >= 2.3.0. <----- redis 4.1.2 doesn't work with Ruby 2.2
Installing rspec-support 3.8.0                            -----
Using bundler 1.12.0
Installing i18n 1.4.0
Using bugsnag 6.11.1 from source at `.`
Installing tzinfo 1.2.5
Installing crack 0.4.3
Installing pry 0.12.2
Installing rack-protection 2.0.5
An error occurred while installing redis (4.1.2), and Bundler cannot continue.
Make sure that `gem install redis -v '4.1.2'` succeeds before bundling.
The command "bundle _${BUNDLE_VERSION}_ install --with "$GEMSETS" --binstubs" failed and exited with 5 during .

The last time master was built, it used redis 4.1.1. Since then redis 4.1.2 has been released, which explictly drops support for Ruby 2.2.

tobyhs · 2019-06-11T23:45:42Z

I fixed the Ruby 2.2 + redis gem issue in #546 so you can merge master into your branch to fix it if you want. However, new features go into the next branch, so you might want to wait until that branch is brought up to date and then we'll try switching the base branch of this pull request to next.

tobyhs · 2019-06-11T23:58:39Z

lib/bugsnag/configuration.rb

@@ -63,6 +63,10 @@ class Configuration
    # @return [Integer] the maximum allowable amount of breadcrumbs per thread
    attr_reader :max_breadcrumbs

+    ##
+    # @return [Regexp] matching file paths within the project


I'm not sure if an @return comment makes sense for an attr_writer.

However, you can change the next line to attr_accessor (so an @return comment would be fine), and in #initialize set @vendor_path to the default regular expression.

tobyhs · 2019-06-12T00:01:22Z

lib/bugsnag/configuration.rb

+    ##
+    # Regex used to mark stacktrace lines as within the project. Lines marked
+    # as "out of project" will only appear in the full trace.
+    def vendor_path


Add an @return comment (or not if you choose to use attr_accessor instead and set it to the default in initialize)

tobyhs · 2019-06-12T00:05:40Z

lib/bugsnag/configuration.rb

+    # Regex used to mark stacktrace lines as within the project. Lines marked
+    # as "out of project" will only appear in the full trace.
+    def vendor_path
+      @vendor_path || Bugsnag::Stacktrace::VENDOR_PATH


Consider moving/renaming Bugsnag::Stacktrace::VENDOR_PATH to Bugsnag::Configuration::DEFAULT_VENDOR_PATH, as it is now a default, and it might be easier for users to find/discover this constant in Bugsnag::Configuration in case they need to use this regular expression to add to it for a new vendor path regular expression (maybe using something like Regexp.union).

tobyhs · 2019-06-12T00:06:52Z

spec/configuration_spec.rb

@@ -396,4 +396,15 @@ def debug(name, &block)
      expect(second_array).to eq([1, 2])
    end
  end
+
+  describe "vendor_path" do


#vendor_path (with the # since it is an instance method)

tobyhs · 2019-06-12T00:09:43Z

spec/stacktrace_spec.rb

@@ -87,4 +87,43 @@
      })
    }
  end
+
+  describe "#vendor_cache" do


I don't understand this doc string, as there isn't a vendor_cache instance method, and the tests don't appear to be related to caching.

tobyhs · 2019-06-12T00:11:46Z

spec/configuration_spec.rb

@@ -396,4 +396,15 @@ def debug(name, &block)
      expect(second_array).to eq([1, 2])
    end
  end
+
+  describe "vendor_path" do
+    it "should have the default vendor path" do


I prefer to avoid "should" in doc strings (https://github.com/rubocop-hq/rspec-style-guide#should-in-example-docstrings), mainly to avoid repeating it a lot (similar comment for line 405)

tobyhs · 2019-06-12T00:26:36Z

CHANGELOG.md

+
+### Enhancements
+
+* Add option to configure what file paths are included in the project stacktrace (`vendor_path`)


This doesn't seem clear, as all the file paths are usually included in the stacktrace. I would reword this to something like "Add option (vendor_path) to adjust filename matching for marking stackframes as in-project".

tobyhs · 2019-06-17T17:39:51Z

I updated the next branch and changed the base branch of this pull request to next. @jnraine Can you update this when you get the chance?

rafaelfranca · 2019-08-09T15:28:16Z

This option was present in 5.5 and was removed without giving any reason. #253. It used to allow arrays, and I believe it still should since you can have more than 1 vendor path.

jnraine · 2019-08-09T15:55:27Z

@tedyangx / @mctaylorpants, can you take this to completion? It'll let you drop a monkey patch from the codebase. 😄

mctaylorpants · 2019-08-16T14:09:53Z

@jnraine yep we can take a look! Are you able to give us write access to your fork?

jnraine · 2019-08-16T19:56:19Z

@mctaylorpants Done! Thanks for taking a look.

…uration (and rename it to DEFAULT_VENDOR_PATH). 2. Add `attr_accessor :vendor_path` to Bugsnag::Configuration and initialize it with DEFAULT_VENDOR_PATH.

…onfiguration::DEFAULT_VENDOR_PATH

tedyangx · 2020-01-24T23:34:09Z

@tobyhs @tomlongridge I have addressed the review comments and updated the pull request. Much appreciated if you can review this. Thanks!

tobyhs · 2020-01-26T07:52:50Z

@tedyangx : I no longer work at Bugsnag, so I won't be able to merge this.

tomlongridge · 2020-01-28T12:42:16Z

Thank @tedyangx - we'll take a look.

tomlongridge · 2020-01-28T15:12:42Z

Raised separate PR with these changes - #583 - to enable CI to run.

tomlongridge · 2020-01-31T08:40:05Z

@tedyangx / @jnraine - thanks for your contributions on this 🙏 The feature has now been released on 6.13.0.

jnraine force-pushed the configurable-vendor-path branch from 7659168 to b7fae15 Compare June 10, 2019 15:18

jnraine marked this pull request as ready for review June 10, 2019 15:19

tobyhs changed the base branch from master to next June 11, 2019 23:41

tobyhs changed the base branch from next to master June 11, 2019 23:41

tobyhs reviewed Jun 12, 2019

View reviewed changes

tobyhs changed the base branch from master to next June 17, 2019 17:10

tomlongridge added the on hold label Nov 21, 2019

tedyangx and others added 4 commits January 24, 2020 14:54

remove TBD Changelog (will add back later)

ac66e41

Merge branch 'next' into configurable-vendor-path

fd161b2

1. Move the VENDOR_PATH from Bugsnag::Stacktrace to Bugsnag::Config…

592d926

…uration (and rename it to DEFAULT_VENDOR_PATH). 2. Add `attr_accessor :vendor_path` to Bugsnag::Configuration and initialize it with DEFAULT_VENDOR_PATH.

Update spec since Bugsnag::Stacktrace::VENDOR_PATH becomes Bugsnag::C…

a1a9fde

…onfiguration::DEFAULT_VENDOR_PATH

tedyangx force-pushed the configurable-vendor-path branch from 25534d3 to a1a9fde Compare January 24, 2020 23:13

tedyangx added 2 commits January 24, 2020 15:24

Update specs to be more descriptive

02adbb7

Add Changelog message

379db59

tomlongridge removed the on hold label Jan 28, 2020

tomlongridge mentioned this pull request Jan 28, 2020

Configurable vendor path #583

Merged

15 tasks

tomlongridge closed this Jan 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow vendor_path to be configured #544

Allow vendor_path to be configured #544

jnraine commented Jun 10, 2019 •

edited

Loading

jnraine commented Jun 10, 2019 •

edited

Loading

tobyhs commented Jun 11, 2019

tobyhs Jun 11, 2019

tobyhs Jun 12, 2019

tobyhs Jun 12, 2019

tobyhs Jun 12, 2019

tobyhs Jun 12, 2019

tobyhs Jun 12, 2019

tobyhs Jun 12, 2019

tobyhs commented Jun 17, 2019

rafaelfranca commented Aug 9, 2019

jnraine commented Aug 9, 2019 •

edited

Loading

mctaylorpants commented Aug 16, 2019

jnraine commented Aug 16, 2019

tedyangx commented Jan 24, 2020

tobyhs commented Jan 26, 2020

tomlongridge commented Jan 28, 2020

tomlongridge commented Jan 28, 2020

tomlongridge commented Jan 31, 2020


		### Enhancements

		* Add option to configure what file paths are included in the project stacktrace (`vendor_path`)

Allow vendor_path to be configured #544

Allow vendor_path to be configured #544

Conversation

jnraine commented Jun 10, 2019 • edited Loading

Goal

Changeset

Added

Tests

Discussion

Outstanding Questions

Review

jnraine commented Jun 10, 2019 • edited Loading

tobyhs commented Jun 11, 2019

tobyhs Jun 11, 2019

Choose a reason for hiding this comment

tobyhs Jun 12, 2019

Choose a reason for hiding this comment

tobyhs Jun 12, 2019

Choose a reason for hiding this comment

tobyhs Jun 12, 2019

Choose a reason for hiding this comment

tobyhs Jun 12, 2019

Choose a reason for hiding this comment

tobyhs Jun 12, 2019

Choose a reason for hiding this comment

tobyhs Jun 12, 2019

Choose a reason for hiding this comment

tobyhs commented Jun 17, 2019

rafaelfranca commented Aug 9, 2019

jnraine commented Aug 9, 2019 • edited Loading

mctaylorpants commented Aug 16, 2019

jnraine commented Aug 16, 2019

tedyangx commented Jan 24, 2020

tobyhs commented Jan 26, 2020

tomlongridge commented Jan 28, 2020

tomlongridge commented Jan 28, 2020

tomlongridge commented Jan 31, 2020

jnraine commented Jun 10, 2019 •

edited

Loading

jnraine commented Jun 10, 2019 •

edited

Loading

jnraine commented Aug 9, 2019 •

edited

Loading